Disallow confirmation token reuse

app/controllers/concerns/authentication.rb

@@ -53,6 +53,6 @@ def user_signed_in?
   end
 
   def store_location
-    session[:user_return_to] = request.original_url if request.get? && request.local?
+    session[:user_return_to] = request.original_url if request.get?
   end
 end

app/controllers/confirmations_controller.rb

@@ -13,7 +13,16 @@ def create
   end
 
   def edit
-    @user = User.find_signed(params[:confirmation_token], purpose: :confirm_email)
+    expected_email = params[:email].strip.downcase
+
+    # It's possible that the email address that we're confirming is the 
+    # primary "email" field or the secondary "unconfirmed_email" field.
+    # We need to check both fields to find the user, but will only check
+    # the primary field if the secondary field is null.
+    @user = (User.where(unconfirmed_email: expected_email).
+               or(User.where(email: expected_email, unconfirmed_email: nil))).
+        find_signed(params[:confirmation_token],
+                    purpose: User.email_confirmation_purpose_for(expected_email))
     if @user.present? && @user.unconfirmed_or_reconfirming?
       if @user.confirm!
         login @user

app/mailers/user_mailer.rb

@@ -6,9 +6,10 @@ class UserMailer < ApplicationMailer
   #
   #   en.user_mailer.confirmation.subject
   #
-  def confirmation(user, confirmation_token)
+  def confirmation(user, confirmation_token, confirmable_email)
     @user = user
     @confirmation_token = confirmation_token
+    @confirmable_email = confirmable_email
 
     mail to: @user.confirmable_email, subject: "Confirmation Instructions"
   end

app/models/user.rb

@@ -53,8 +53,17 @@ def confirmable_email
     end
   end
 
+  def self.email_confirmation_purpose_for(email)
+    "confirm_email: #{email}"
+  end
+
+  def email_confirmation_purpose
+    self.class.email_confirmation_purpose_for(confirmable_email)
+  end
+
   def generate_confirmation_token
-    signed_id expires_in: CONFIRMATION_TOKEN_EXPIRATION, purpose: :confirm_email
+    signed_id expires_in: CONFIRMATION_TOKEN_EXPIRATION,
+      purpose: email_confirmation_purpose
   end
 
   def generate_password_reset_token
@@ -63,7 +72,7 @@ def generate_password_reset_token
 
   def send_confirmation_email!
     confirmation_token = generate_confirmation_token
-    UserMailer.confirmation(self, confirmation_token).deliver_now
+    UserMailer.confirmation(self, confirmation_token, confirmable_email).deliver_now
   end
 
   def send_password_reset_email!

app/views/user_mailer/confirmation.html.erb

@@ -1,3 +1,3 @@
 <h1>Confirmation Instructions</h1>
 
-<%= link_to "Click here to confirm your email.", edit_confirmation_url(@confirmation_token) %>
+<%= link_to "Click here to confirm your email.", edit_confirmation_url(@confirmation_token, email: @confirmable_email) %>

app/views/user_mailer/confirmation.text.erb

@@ -1,3 +1,3 @@
 Confirmation Instructions
 
-<%= edit_confirmation_url(@confirmation_token) %>
+<%= edit_confirmation_url(@confirmation_token, email: @confirmable_email) %>

test/controllers/confirmations_controller_test.rb

@@ -11,7 +11,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
     freeze_time do
       confirmation_token = @unconfirmed_user.generate_confirmation_token
 
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: @unconfirmed_user.confirmable_email)
 
       assert @unconfirmed_user.reload.confirmed?
       assert_equal Time.now, @unconfirmed_user.confirmed_at
@@ -26,7 +26,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
     freeze_time do
       confirmation_token = @reconfirmed_user.generate_confirmation_token
 
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: unconfirmed_email)
 
       assert @reconfirmed_user.reload.confirmed?
       assert_equal Time.current, @reconfirmed_user.reload.confirmed_at
@@ -44,7 +44,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
     freeze_time do
       confirmation_token = @reconfirmed_user.generate_confirmation_token
 
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: @confirmed_user.confirmable_email)
 
       assert_equal original_email, @reconfirmed_user.reload.email
       assert_redirected_to new_confirmation_path
@@ -56,7 +56,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
     confirmation_token = @unconfirmed_user.generate_confirmation_token
 
     travel_to 601.seconds.from_now do
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: @unconfirmed_user.confirmable_email)
 
       assert_nil @unconfirmed_user.reload.confirmed_at
       assert_not @unconfirmed_user.reload.confirmed?
@@ -66,7 +66,15 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "should redirect if confirmation link is incorrect" do
-    get edit_confirmation_path("not_a_real_token")
+    get edit_confirmation_path("not_a_real_token", email: @unconfirmed_user.confirmable_email)
+    assert_redirected_to new_confirmation_path
+    assert_not_nil flash[:alert]
+  end
+
+  test "should redirect if confirmation email is incorrect" do
+    confirmation_token = @unconfirmed_user.generate_confirmation_token
+
+    get edit_confirmation_path(confirmation_token, email: "not_the_email@gmail.com")
     assert_redirected_to new_confirmation_path
     assert_not_nil flash[:alert]
   end
@@ -99,7 +107,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
 
       login @confirmed_user
 
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: @confirmed_user.confirmable_email)
 
       assert_not_equal Time.current, @confirmed_user.reload.confirmed_at
       assert_redirected_to new_confirmation_path
@@ -115,7 +123,7 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
 
       confirmation_token = @reconfirmed_user.generate_confirmation_token
 
-      get edit_confirmation_path(confirmation_token)
+      get edit_confirmation_path(confirmation_token, email: unconfirmed_email)
 
       assert_equal Time.current, @reconfirmed_user.reload.confirmed_at
       assert @reconfirmed_user.reload.confirmed?
@@ -126,6 +134,19 @@ class ConfirmationsControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test "should prevent reuse of confirmation token" do
+    login @unconfirmed_user
+
+    confirmation_token = @unconfirmed_user.generate_confirmation_token
+
+    get edit_confirmation_path(confirmation_token, email: @unconfirmed_user.confirmable_email)
+    assert @unconfirmed_user.reload.confirmed?
+    @unconfirmed_user.update(unconfirmed_email: "not_the_same@example.com")
+
+    get edit_confirmation_path(confirmation_token, email: @unconfirmed_user.unconfirmed_email)
+    assert_redirected_to new_confirmation_path
+  end
+
   test "should prevent authenticated user from submitting the confirmation form" do
     login @confirmed_user
 

test/controllers/sessions_controller_test.rb

@@ -44,6 +44,12 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
 
     assert_not_nil current_user
     assert_not_nil cookies[:remember_token]
+
+    remember_me_cookie = cookies.get_cookie("remember_token")
+
+    assert remember_me_cookie.http_only?
+    assert remember_me_cookie.secure?
+    assert_equal "Strict", remember_me_cookie.to_h["SameSite"]
   end
 
   test "should forget user when logging out" do

test/mailers/user_mailer_test.rb

@@ -7,7 +7,7 @@ class UserMailerTest < ActionMailer::TestCase
 
   test "confirmation" do
     confirmation_token = @user.generate_confirmation_token
-    mail = UserMailer.confirmation(@user, confirmation_token)
+    mail = UserMailer.confirmation(@user, confirmation_token, @user.unconfirmed_email)
     assert_equal "Confirmation Instructions", mail.subject
     assert_equal [@user.email], mail.to
     assert_equal [User::MAILER_FROM_EMAIL], mail.from