From 7e8f0dc967cb9764238f1937c57228e1322ed679 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Sun, 26 Feb 2017 17:09:57 -0500 Subject: [PATCH] FIX: attempt to handle ios edge case where token is seen but unsaved This relaxes our security in the following way - prev auth token is always accepted as long as rotation date is within our window of SiteSetting.maximum_session_age.hours (previously old token expired within a minute of new one being seen) - new auth token is marked unseen if we are presented with an old token after we already saw new one This attempts to fix an issue where ios webkit is not committing new cookies --- app/models/user_auth_token.rb | 29 ++++++++++++++++--------- spec/models/user_auth_token_spec.rb | 33 ++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/app/models/user_auth_token.rb b/app/models/user_auth_token.rb index ee6ae647588..6e9d4e9d577 100644 --- a/app/models/user_auth_token.rb +++ b/app/models/user_auth_token.rb @@ -46,18 +46,10 @@ class UserAuthToken < ActiveRecord::Base user_token = find_by("(auth_token = :token OR prev_auth_token = :token OR - (auth_token = :unhashed_token AND legacy)) AND created_at > :expire_before", + (auth_token = :unhashed_token AND legacy)) AND rotated_at > :expire_before", token: token, unhashed_token: unhashed_token, expire_before: expire_before) - token_expired = - user_token && - user_token.seen_at && - user_token.auth_token_seen && - user_token.prev_auth_token == token && - user_token.prev_auth_token != user_token.auth_token && - user_token.seen_at < 1.minute.ago - - if token_expired || !user_token + if !user_token if SiteSetting.verbose_auth_token_logging UserAuthTokenLog.create( @@ -72,6 +64,23 @@ class UserAuthToken < ActiveRecord::Base return nil end + if user_token.prev_auth_token == token && user_token.auth_token_seen + changed_rows = UserAuthToken + .where(id: user_token.id, prev_auth_token: token) + .update_all(auth_token_seen: false) + + # not updating AR model cause we want to give it one more req + # with wrong cookie + UserAuthTokenLog.create( + action: changed_rows == 0 ? "prev seen token unchanged" : "prev seen token", + user_auth_token_id: user_token.id, + user_id: user_token.user_id, + auth_token: user_token.auth_token, + user_agent: opts && opts[:user_agent], + client_ip: opts && opts[:client_ip] + ) + end + if mark_seen && user_token && !user_token.auth_token_seen && user_token.auth_token == token # we must protect against concurrency issues here changed_rows = UserAuthToken diff --git a/spec/models/user_auth_token_spec.rb b/spec/models/user_auth_token_spec.rb index b57c79d9ac7..b7961d1d444 100644 --- a/spec/models/user_auth_token_spec.rb +++ b/spec/models/user_auth_token_spec.rb @@ -85,6 +85,34 @@ describe UserAuthToken do expect(user_token.user_agent).to eq("some user agent 2") end + it "expires correctly" do + + user = Fabricate(:user) + + user_token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true) + + freeze_time (SiteSetting.maximum_session_age.hours - 1).from_now + + user_token.reload + + user_token.rotate! + UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true) + + freeze_time (SiteSetting.maximum_session_age.hours - 1).from_now + + still_good = UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true) + expect(still_good).not_to eq(nil) + + freeze_time 2.hours.from_now + + not_good = UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true) + expect(not_good).to eq(nil) + end + it "can properly rotate tokens" do user = Fabricate(:user) @@ -127,7 +155,10 @@ describe UserAuthToken do freeze_time(2.minute.from_now) looked_up = UserAuthToken.lookup(unhashed_prev) - expect(looked_up).to eq(nil) + expect(looked_up).not_to eq(nil) + + looked_up.reload + expect(looked_up.auth_token_seen).to eq(false) rotated = user_token.rotate!(user_agent: "a new user agent", client_ip: "1.1.2.4") expect(rotated).to eq(true)