From c4e6e7bc119585d3699ccf0f6d076e1eca4518a0 Mon Sep 17 00:00:00 2001 From: Michael Chaney Date: Tue, 11 Jun 2024 21:04:25 -0500 Subject: [PATCH 1/3] Remove extraneous check for request.local? Closes #88. --- app/controllers/concerns/authentication.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 751f0c2..85633da 100644 --- a/app/controllers/concerns/authentication.rb +++ b/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 From 2e6e5b0757cedd4d2c530ea047f689f1fd7015cf Mon Sep 17 00:00:00 2001 From: Michael Chaney Date: Tue, 11 Jun 2024 21:11:02 -0500 Subject: [PATCH 2/3] Adds assertions for remember_me cookie. Asserts cookie is http_only, secure, and same-site is "strict". Closes #87. --- test/controllers/sessions_controller_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 928d560..09ceea5 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/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 From 9f1432a16ab959c00c37863e484430190fdc3315 Mon Sep 17 00:00:00 2001 From: Michael Chaney Date: Wed, 12 Jun 2024 10:54:07 -0500 Subject: [PATCH 3/3] 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. --- app/controllers/confirmations_controller.rb | 11 +++++- app/mailers/user_mailer.rb | 3 +- app/models/user.rb | 13 +++++-- app/views/user_mailer/confirmation.html.erb | 2 +- app/views/user_mailer/confirmation.text.erb | 2 +- .../confirmations_controller_test.rb | 35 +++++++++++++++---- test/mailers/user_mailer_test.rb | 2 +- 7 files changed, 54 insertions(+), 14 deletions(-) diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index a65fc68..5656ed0 100644 --- a/app/controllers/confirmations_controller.rb +++ b/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 diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index ae2c631..f022963 100644 --- a/app/mailers/user_mailer.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 578e289..0e0c0ac 100644 --- a/app/models/user.rb +++ b/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! diff --git a/app/views/user_mailer/confirmation.html.erb b/app/views/user_mailer/confirmation.html.erb index d064deb..b04c78a 100644 --- a/app/views/user_mailer/confirmation.html.erb +++ b/app/views/user_mailer/confirmation.html.erb @@ -1,3 +1,3 @@

Confirmation Instructions

-<%= link_to "Click here to confirm your email.", edit_confirmation_url(@confirmation_token) %> \ No newline at end of file +<%= link_to "Click here to confirm your email.", edit_confirmation_url(@confirmation_token, email: @confirmable_email) %> diff --git a/app/views/user_mailer/confirmation.text.erb b/app/views/user_mailer/confirmation.text.erb index a34b236..54bee56 100644 --- a/app/views/user_mailer/confirmation.text.erb +++ b/app/views/user_mailer/confirmation.text.erb @@ -1,3 +1,3 @@ Confirmation Instructions -<%= edit_confirmation_url(@confirmation_token) %> \ No newline at end of file +<%= edit_confirmation_url(@confirmation_token, email: @confirmable_email) %> diff --git a/test/controllers/confirmations_controller_test.rb b/test/controllers/confirmations_controller_test.rb index d196308..d5a9c81 100644 --- a/test/controllers/confirmations_controller_test.rb +++ b/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 diff --git a/test/mailers/user_mailer_test.rb b/test/mailers/user_mailer_test.rb index 9f9c9e0..0439fd8 100644 --- a/test/mailers/user_mailer_test.rb +++ b/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