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.

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

Conversation

@tegon
Copy link
Member

commented Sep 2, 2019

As reported in #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 always receive a validation error, even if there are users with a blank confirmation_token in the database.

For applications that have users with a blank confirmation_token in the database, it's recommended to manually regenerate or to nullify them.

Closes #5071

tegon added 3 commits Aug 12, 2019
Do not confirm users when `confirmation_token` is blank
As reported in #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.
@sourcelevel-bot

This comment has been minimized.

Copy link

commented Sep 2, 2019

SourceLevel has finished reviewing this Pull Request and has found:

  • 3 possible new issues (including those that may have been commented here).
  • 3 fixed issues! 🎉

But beware that this branch is 7 commits behind the plataformatec:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://app.sourcelevel.io/github/plataformatec/devise/pulls/5132.

@tegon tegon self-assigned this Sep 2, 2019

@mracos
mracos approved these changes Sep 2, 2019
Copy link
Member

left a comment

:shipit:

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

This comment has been minimized.

Copy link
@amangano-privy

amangano-privy Sep 3, 2019

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?

This comment has been minimized.

Copy link
@tegon

tegon Sep 3, 2019

Author Member

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?

This comment has been minimized.

Copy link
@amangano-privy

amangano-privy Sep 3, 2019

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

This comment has been minimized.

Copy link
@tegon

tegon Sep 3, 2019

Author Member

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?

This comment has been minimized.

Copy link
@amangano-privy

amangano-privy Sep 3, 2019

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.

This comment has been minimized.

Copy link
@tegon

tegon Sep 3, 2019

Author Member

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.

This comment has been minimized.

Copy link
@tegon

tegon Sep 3, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@amangano-privy

amangano-privy Sep 4, 2019

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

This comment has been minimized.

Copy link
@tegon

tegon Sep 4, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@amangano-privy

amangano-privy Sep 4, 2019

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

@amangano-privy
Copy link

left a comment

Two minor comments about the tests, otherwise, LGTM.

@tegon tegon merged commit fee43f3 into master Sep 4, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
sourcelevel SourceLevel has found 3 possible new issues and 3 fixed issues.
Details

@tegon tegon deleted the let-blank-confirmation-token branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.