Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always return an error when confirmation_token is blank #5132

Merged
merged 4 commits into from
Sep 4, 2019
+60 −0
Merged

Always return an error when confirmation_token is blank #5132

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f66c996
Do not confirm users when `confirmation_token` is blank
tegon Aug 12, 2019
30508da
Add comments about the code
tegon Aug 12, 2019
624049a
Tweak the tests
tegon Aug 12, 2019
0ea4bd7
Return early when `confirmation_token` is blank
tegon Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/devise/models/confirmable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,17 @@ 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)

# 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?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, this code may leak information about the internal state of the database (that some records have blank tokens). Could we check for presence of confirmation_token before the call to find_first_by_auth_conditions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code may leak information about the internal state of the database (that some records have blank tokens)

I'm not sure about that. It's the same response as when there are no records with blank tokens. Do you think this is still leaking information?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case then I guess it's probably ok for now, but if the find_or_initialize_with_error_by or token_generator methods change, it could unintentionally create an information leakage issue here. I think it'd be safer to just return early (without making any queries) if we detect that the token is blank.

if confirmation_token.blank?
  confirmable = new
  confirmable.errors.add(:confirmation_token, :blank)
  return confirmable
end

Copy link
Member Author

@tegon tegon Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if the find_or_initialize_with_error_by or token_generator methods change, it could unintentionally create an information leakage issue here

Do you have examples of which kind of changes could cause leaks?

Copy link

@amangano-privy amangano-privy Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if the find_or_initialize_with_error_by or token_generator methods change, it could unintentionally create an information leakage issue here

Do you have examples of which kind of changes could cause leaks?

Any change that would result in a different response being returned when the token is blank and there is no record in the database.

Examples:

  • TokenGenerator#digest changes to return a digest even when value is blank. This may result in a record being initialized and returned with error set to :invalid instead of :blank.
  • find_or_initialize_with_error_by changes to always use the provided error argument instead of special-casing blank errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, get it. Both of these cases would be caught by our integration tests, so I guess this implementation is fine.
But since we already know there's no need to perform the query I guess it's better to return early as you mentioned. It also shows better the intention behind the code, which is to validate whether the parameter is present or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the changes, thanks for the review!

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)
Expand Down
30 changes: 30 additions & 0 deletions test/integration/confirmable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,36 @@ 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 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"
end

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"
end

test 'error message is configurable by resource name' do
store_translations :en, devise: {
failure: { user: { unconfirmed: "Not confirmed user" } }
Expand Down
18 changes: 18 additions & 0 deletions test/models/confirmable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this no longer needs 'and a record exists on the database'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to keep since that's the behavior we're trying to ensure it doesn't happen anymore.
I know that since the current code validates the parameter before doing anything else, it's unlikely for this to happen, but this test ensures we don't add regressions in the future.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this no longer needs 'and a record exists on the database'

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
Expand Down