Always return an error when confirmation_token is blank
#5132
Conversation
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 has finished reviewing this Pull Request and has found:
But beware that this branch is 7 commits behind the You can see more details about this review at https://app.sourcelevel.io/github/plataformatec/devise/pulls/5132. |
lib/devise/models/confirmable.rb
Outdated
| # 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? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
endThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
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#digestchanges to return a digest even whenvalueis blank. This may result in a record being initialized and returned with error set to:invalidinstead of:blank.find_or_initialize_with_error_bychanges to always use the providederrorargument instead of special-casing blank errors.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
I think this no longer needs 'and a record exists on the database'
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
I think this no longer needs 'and a record exists on the database'
…#5132) As reported in heartcombo#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.
As reported in #5071, if for some reason, a user in the database had the
confirmation_tokencolumn as a blank string, Devise would confirm that user after receiving a request with a blank
confirmation_tokenparameter.After this commit, a request sending a blank
confirmation_tokenparameter will always receive a validation error, even if there are users with a blankconfirmation_tokenin the database.For applications that have users with a blank
confirmation_tokenin the database, it's recommended to manually regenerate or to nullify them.Closes #5071