From f66c996ad472ddaeba689c0679b467e3661daaaf Mon Sep 17 00:00:00 2001 From: Leonardo Tegon Date: Mon, 12 Aug 2019 09:53:14 -0300 Subject: [PATCH 1/4] Do not confirm users when `confirmation_token` is blank As reported in https://github.com/plataformatec/devise/issues/5071, if for some reason, a user in the database had the `confirmation_token` column as a blank string, Devise would confirm that user after receiving a request with a blank `confirmation_token` parameter. After this commit, a request sending a blank `confirmation_token` parameter will receive a validation error. For applications that have users with a blank `confirmation_token` in the database, it's recommended to manually regenerate or to nullify them. --- lib/devise/models/confirmable.rb | 6 ++++++ test/integration/confirmable_test.rb | 32 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index dbf6d0ffc8..9181d5ed65 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -349,6 +349,12 @@ def send_confirmation_instructions(attributes={}) # Options must have the confirmation_token def confirm_by_token(confirmation_token) confirmable = find_first_by_auth_conditions(confirmation_token: confirmation_token) + + if confirmable && confirmation_token.blank? + confirmable.errors.add(:confirmation_token, :blank) + return confirmable + end + unless confirmable confirmation_digest = Devise.token_generator.digest(self, :confirmation_token, confirmation_token) confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_digest) diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index 73563f283f..d4dd15f07b 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -175,6 +175,38 @@ def resend_confirmation assert_current_url '/users/sign_in' end + test "should not be able to confirm an email with a blank confirmation token" do + visit_user_confirmation_with_token("") + + assert_contain "Confirmation token can't be blank" + end + + test "should not be able to confirm an email with a nil confirmation token" do + visit_user_confirmation_with_token(nil) + + assert_contain "Confirmation token can't be blank" + end + + test "should not confirm user with blank confirmation token" do + user = create_user(confirm: false) + user.update_attribute(:confirmation_token, "") + + visit_user_confirmation_with_token("") + + assert_contain "Confirmation token can't be blank" + refute user.reload.confirmed? + end + + test "should not confirm user with nil confirmation token" do + user = create_user(confirm: false) + user.update_attribute(:confirmation_token, nil) + + visit_user_confirmation_with_token(nil) + + assert_contain "Confirmation token can't be blank" + refute user.reload.confirmed? + end + test 'error message is configurable by resource name' do store_translations :en, devise: { failure: { user: { unconfirmed: "Not confirmed user" } } From 30508da4ee43d74e537d8790e66ea257172e41fb Mon Sep 17 00:00:00 2001 From: Leonardo Tegon Date: Mon, 12 Aug 2019 14:50:17 -0300 Subject: [PATCH 2/4] Add comments about the code --- lib/devise/models/confirmable.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 9181d5ed65..b82636392e 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -350,6 +350,11 @@ def send_confirmation_instructions(attributes={}) def confirm_by_token(confirmation_token) confirmable = find_first_by_auth_conditions(confirmation_token: confirmation_token) + # When the `confirmation_token` parameter is blank, if there are any users with a blank + # `confirmation_token` in the database, the first one would be confirmed here. + # The error is being manually added here to ensure no users are confirmed by mistake. + # This was done in the model for convenience, since validation errors are automatically + # displayed in the view. if confirmable && confirmation_token.blank? confirmable.errors.add(:confirmation_token, :blank) return confirmable From 624049aec1c3dcae5b354f670565c8d4d6acce09 Mon Sep 17 00:00:00 2001 From: Leonardo Tegon Date: Mon, 12 Aug 2019 14:59:46 -0300 Subject: [PATCH 3/4] Tweak the tests --- test/integration/confirmable_test.rb | 6 ++---- test/models/confirmable_test.rb | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index d4dd15f07b..5cafacb430 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -187,24 +187,22 @@ def resend_confirmation assert_contain "Confirmation token can't be blank" end - test "should not confirm user with blank confirmation token" do + test "should not be able to confirm user with blank confirmation token" do user = create_user(confirm: false) user.update_attribute(:confirmation_token, "") visit_user_confirmation_with_token("") assert_contain "Confirmation token can't be blank" - refute user.reload.confirmed? end - test "should not confirm user with nil confirmation token" do + test "should not be able to confirm user with nil confirmation token" do user = create_user(confirm: false) user.update_attribute(:confirmation_token, nil) visit_user_confirmation_with_token(nil) assert_contain "Confirmation token can't be blank" - refute user.reload.confirmed? end test 'error message is configurable by resource name' do diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb index cab1d4f387..899c9caba2 100644 --- a/test/models/confirmable_test.rb +++ b/test/models/confirmable_test.rb @@ -77,6 +77,24 @@ def setup assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join end + test 'should return a new record with errors when a blank token is given and a record exists on the database' do + user = create_user(confirmation_token: '') + + confirmed_user = User.confirm_by_token('') + + refute user.reload.confirmed? + assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join + end + + test 'should return a new record with errors when a nil token is given and a record exists on the database' do + user = create_user(confirmation_token: nil) + + confirmed_user = User.confirm_by_token(nil) + + refute user.reload.confirmed? + assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join + end + test 'should generate errors for a user email if user is already confirmed' do user = create_user user.confirmed_at = Time.now From 0ea4bd70b2ca852d3314f53b62ffd9c7583c4f0d Mon Sep 17 00:00:00 2001 From: Leonardo Tegon Date: Tue, 3 Sep 2019 18:23:30 -0300 Subject: [PATCH 4/4] Return early when `confirmation_token` is blank --- lib/devise/models/confirmable.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index b82636392e..91258f4c33 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -348,18 +348,19 @@ def send_confirmation_instructions(attributes={}) # If the user is already confirmed, create an error for the user # Options must have the confirmation_token def confirm_by_token(confirmation_token) - confirmable = find_first_by_auth_conditions(confirmation_token: confirmation_token) - # When the `confirmation_token` parameter is blank, if there are any users with a blank # `confirmation_token` in the database, the first one would be confirmed here. # The error is being manually added here to ensure no users are confirmed by mistake. # This was done in the model for convenience, since validation errors are automatically # displayed in the view. - if confirmable && confirmation_token.blank? + if confirmation_token.blank? + confirmable = new confirmable.errors.add(:confirmation_token, :blank) return confirmable end + confirmable = find_first_by_auth_conditions(confirmation_token: confirmation_token) + unless confirmable confirmation_digest = Devise.token_generator.digest(self, :confirmation_token, confirmation_token) confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_digest)