[5.8] Change password min length to 8 #25957
Conversation
- Update the validation method in `PasswordBroker` to check against length of 8 - Update `ResetsPasswords` `rules()` method to use validation rule `min:8` - Change test method name and example password in `AuthPasswordBrokerTest` to reflect new minimum length
GrahamCampbell
changed the title
|
You sent this 5.7 here, but framework was 5.8. |
|
@GrahamCampbell What do you mean? This was merged into the framework master branch only (which is 5.8) so I don't understand why did you rename the title from 5.8 to 5.7 as this change is not on the 5.7 branch. |
|
Can you please rename the PR title to |
jakebathman
changed the title
|
Changed back to [5.8] since this PR targeted |
The PR to laravel/laravel targetted it's master though, which is 5.7. Develop is 5.8. |
|
The PR to laravel/laravel targeted
That's equivalent to |
|
Ah, perfect. 👍 |
|
@driesvints |
|
@ankurk91 done: laravel/docs#5009 |
|
Since this is security related, will this change be back-ported to 5.5? |
|
No, this is a not a security patch. It's a breaking change to some validation code only. |
(bkz: laravel/framework#25957) closes #32
|
does this mean that i can keep using password as my password? |
|
This validation should be removed if the request has validation already. If someone decides to give |
|
Why is this even hardcoded? We had issues because our client side no longer matched our serverside because of this. I seem to have missed it in the upgrade docs but still... why is this hardcoded? What if I wanted 10? Hell I don't even understand why it is doing it at all, validation logic should be in Another issue is we basically have to hack a confirmation in because we confirm the password client side (like everyone should) instead of serverside. Sending a And again, this looks even more like an issue because Laravel has an entire validator and then hardcoded redundant validations in the broker.
We have our own custom controller for resetting passwords but still use the broker, which I imagine is common practice. You don't expect an exception because of password length and you shouldn't be forced to send the confirmation field when you handle all that yourself. The more I look into this the more it looks wrong. Because if you hit the password length problem you can't get an error about that, you have to catch an exception and return the proper error for that case. But because it doesn't use the laravel validator you don't even know if the issue was the confirmation field not matching or the length. |
|
Totally agree. @robclancy |
|
Agree @robclancy , why i can not override simple rule for authentication things like in another basic request validation, this is to much harder for me as a new user in this framework, thanks |
|
FYI: I have this on my list to review for the next release. Appreciating PR's that would help with this. Please target the master branch. |
|
@robclancy I've sent in a PR for this which removes the hardcoded length from the broker. All validation for the length can now be done in user land. I didn't go as far as to remove all validation because the basics of matching passwords and a minimum length of 1 still applies imo. Have a look and feel free to reply on the PR with your thoughts. |
|
Why would the broker require a confirmation? This is such a weird specific thing to do. It's not the job for it to validate the password. And anywhere that uses the broker already has the confirmation validation like you normally do with the validator that comes with Laravel. So if I make a register form with a confirmation I need to make a hack. Laravel has such a tight relationship with vue that this makes even less sense because you would do this client side before sending to the server because it is purely a UX issue. It will never make sense to me to force something like this in a serverside library/api. |
|
@robclancy I see. I'll give this some more thought soon and maybe send in a follow up pr. But for now the hardcoded length will be gone at least. |
This removes duplicate (and hardcoded) length check from the PasswordBroker. At the moment this is duplicated in the ResetsPasswords trait where it can be customized by overwriting the rules method and the PasswordBroker where it cannot be adjusted. A length of 8 is a sensible default to have (see laravel/framework#25957) but allowing people to overwrite when needed allows people to modify as they see fit. I've left the rules in user land (laravel/laravel repo & ResetsPassword trait) as is because it still makes sense to ship with a default length in user land.
|
@robclancy sent in a PR here: #29480 |
These changes remove all hardcoded valdation from the PasswordBroker. The reason for this is because this is a hardcoded constraint in validation and thus limits people from building password reset forms with their specific flow. The validation is already done within the ResetsPassword trait and is only duplicated in the Broker. The Broker also seems like the wrong place to do this as it only facilitates the retrieval of the user and the token validation (which is still the correct place for these two). This also allows us to remove the https://github.com/laravel/laravel/blob/develop/resources/lang/en/passwords.php#L16 language line. See laravel/framework#25957 (comment)
These changes remove all hardcoded valdation from the PasswordBroker. The reason for this is because this is a hardcoded constraint in validation and thus limits people from building password reset forms with their specific flow. The validation is already done within the ResetsPassword trait and is only duplicated in the Broker. The Broker also seems like the wrong place to do this as it only facilitates the retrieval of the user and the token validation (which is still the correct place for these two). This also allows us to remove the https://github.com/laravel/laravel/blob/develop/resources/lang/en/passwords.php#L16 language line. See laravel/framework#25957 (comment)
|
Password validation is now completely removed from the broker: #29480 |
These changes remove all hardcoded valdation from the PasswordBroker. The reason for this is because this is a hardcoded constraint in validation and thus limits people from building password reset forms with their specific flow. The validation is already done within the ResetsPassword trait and is only duplicated in the Broker. The Broker also seems like the wrong place to do this as it only facilitates the retrieval of the user and the token validation (which is still the correct place for these two). This also allows us to remove the https://github.com/laravel/laravel/blob/develop/resources/lang/en/passwords.php#L16 language line. See laravel/framework#25957 (comment) (comment)
These changes remove all hardcoded valdation from the PasswordBroker. The reason for this is because this is a hardcoded constraint in validation and thus limits people from building password reset forms with their specific flow. The validation is already done within the ResetsPassword trait and is only duplicated in the Broker. The Broker also seems like the wrong place to do this as it only facilitates the retrieval of the user and the token validation (which is still the correct place for these two). This also allows us to remove the https://github.com/laravel/laravel/blob/develop/resources/lang/en/passwords.php#L16 language line. See laravel/framework#25957 (comment) (comment)
This PR updates the default minimum password length to be 8 (the current default is 6).
Last year, NIST published new standards on memorized secrets (i.e. user passwords), stipulating in Section 5.1.1.1:
Password length is only a part of a security strategy employed by the framework, but the move to an 8-character minimum is long overdue.
Please let me know if there are any questions or comments. Thanks!