has_secure_password: fix password validation.
#55232
Conversation
| end | ||
|
|
||
| validates_confirmation_of attribute, allow_blank: true | ||
| validates_confirmation_of attribute, allow_nil: true |
There was a problem hiding this comment.
This change assumes that, given the current implementation of the writer method password=, the corresponding reader method password can only actually return nil or a string that is not empty? (whitespace-only or not). Therefore, by skipping the confirmation validation only when password is nil, we get to validate every non-empty string, which is what we want.
There was a problem hiding this comment.
Is the condition of a "blank" value here related to the fact that a <form> element submitted with an empty <input type="password"> will be transformed into an empty "" by a controller's params helper? When would an empty <input> result in a nil value?
There was a problem hiding this comment.
To my knowledge, an empty form <input> should not result in a nil value, but nonetheless, nil as a password value (e.g. assigned by other means) is explicitly supported, and has the effect of setting password_digest to nil as well, see https://github.com/rails/rails/blob/main/activemodel/lib/active_model/secure_password.rb#L187. We want to skip confirmation in that case alone.
Plus, I tried to remove the allow_ condition altogether, but that resulted in test failures.
Let me know if I answered your question!
There was a problem hiding this comment.
Makes sense to me. If there was a confirmation field in the form, you'll get "", if the field is being changed from another context, it just won't be there at all (nil).
callmesangio
force-pushed
the
fix-gh-55225
branch
from
3581bbf to
5ad4fef
Compare
| end | ||
|
|
||
| test "create a new user with validation, a spaces only password, and an incorrect password confirmation" do | ||
| @user.password = " " * 72 |
There was a problem hiding this comment.
any particular reason for 72 here?
There was a problem hiding this comment.
Isn't that BCrypt limit? https://www.ory.sh/docs/troubleshooting/bcrypt-secret-length
There was a problem hiding this comment.
@MatheusRich No technical reason, just consistency: I followed what looked like a convention applied by other test methods of the class.
But yeah, as @simi says, 72 (bytes, not characters) is the limit beyond which bcrypt implementations usually truncate the plaintext.
There was a problem hiding this comment.
Gotcha. I was just wondering if it meant something relevant for this test. I wonder if it makes sense to just use a single space, as that seems to be enough for this test.
There was a problem hiding this comment.
@MatheusRich I made the change to have a single space string, and updated the commit accordingly, see https://github.com/rails/rails/compare/8de663fde4e813aef4c7593a55c216818bd81a6c..20e28b900dad008784c36f08742b426c0e7b6ceb
callmesangio
force-pushed
the
fix-gh-55225
branch
2 times, most recently
from
8de663f to
20e28b9
Compare
|
I'm unsure if this is considered a bugfix, so IDK if we need a changelog entry for it. I'll let someone else chime in. |
Previously, the password confirmation validation added by `has_secure_password` was skipped if the password string was a sequence of whitespace characters, regardless of the string provided as confirmation, which was ignored with no error messages. This change fixes that behavior, so that the confirmation validation runs consistently, independently of the password string content. Fixes rails#55225
callmesangio
force-pushed
the
fix-gh-55225
branch
from
20e28b9 to
9fc0eff
Compare
|
Hello there 👋🏻 |
I'd say so. But it's easier to add the changelog during backport anyways. |
`has_secure_password`: fix password validation.
`has_secure_password`: fix password validation.
|
Backported to 8-0-stable and 7-2-stable. |
5 participants
Previously, the password confirmation validation added by
has_secure_passwordwas skipped if the password string was a sequence of whitespace characters, regardless of the string provided as confirmation, which was ignored with no error messages.This change fixes that behavior, so that the confirmation validation runs consistently, independently of the password string content.
Fixes #55225
Motivation / Background
This Pull Request has been created because it fixes a bug in
has_secure_passwordvalidations.Detail
This Pull Request changes how the password confirmation validation added by default by
has_secure_passwordworks.Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]