Prevent reuse of confirmation token.

Closes #86.

The issue is that signed tokens have a simple payload by default that
doesn't verify anything other than the record id and the token's
purpose.  This can lead to a security challenge as the token can be used
to confirm anything that has the same "purpose" for that record.

An example would be someone changing their email address to an email
address that they don't control.  Using one account, they change the
email to an email address that they control and get the confirmation
token.  They then change the email to one that they can't access and
use the token from the first request to "confirm" the second request.
The tokens can be used any number of times as long as they're used
before expiration.

With this change, the email address is included as plain text in the
request, as well as being used as part of the "purpose" in the token.
The second request fails because the plain text email address is used to
constrain the signed lookup.  If they change the plain text email
address in the link then the message will fail to be validated as the
"purpose" won't match.  Either way, the token is usable only for
confirming the original email from the token's creation.

Author: mdchaney

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/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