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

[5.8] Change password min length to 8 #25957

Merged
merged 1 commit into from Oct 5, 2018
Merged

[5.8] Change password min length to 8 #25957

merged 1 commit into from Oct 5, 2018

Conversation

@jakebathman
Copy link
Contributor

@jakebathman jakebathman commented Oct 5, 2018

This PR updates the default minimum password length to be 8 (the current default is 6).

⚠️ Note: this requires a companion PR laravel/laravel#4794 in order to update the registration controller and password reset language lines.

Last year, NIST published new standards on memorized secrets (i.e. user passwords), stipulating in Section 5.1.1.1:

Memorized secrets SHALL be at least 8 characters in length if chosen by the subscriber.

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!

- 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
@taylorotwell taylorotwell merged commit a31deba into laravel:master Oct 5, 2018
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jakebathman jakebathman deleted the jakebathman:change-password-min-length-to-8 branch Oct 5, 2018
@GrahamCampbell GrahamCampbell changed the title [5.8] Change password min length to 8 [5.7] Change password min length to 8 Oct 6, 2018
@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Oct 6, 2018

You sent this 5.7 here, but framework was 5.8.

@X-Coder264
Copy link
Contributor

@X-Coder264 X-Coder264 commented Oct 6, 2018

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

@ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Oct 7, 2018

@GrahamCampbell

Can you please rename the PR title to [5.8] because it was merged to master branch.

@jakebathman jakebathman changed the title [5.7] Change password min length to 8 [5.8] Change password min length to 8 Oct 8, 2018
@jakebathman
Copy link
Contributor Author

@jakebathman jakebathman commented Oct 8, 2018

Changed back to [5.8] since this PR targeted master (which will be 5.8 when it's tagged), and does not target the current 5.7 branch.

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Oct 11, 2018

Changed back to [5.8] since this PR targeted master (which will be 5.8 when it's tagged), and does not target the current 5.7 branch.

The PR to laravel/laravel targetted it's master though, which is 5.7. Develop is 5.8.

@jakebathman
Copy link
Contributor Author

@jakebathman jakebathman commented Oct 15, 2018

The PR to laravel/laravel targeted develop:

screen shot 2018-10-15 at 10 50 33 am

That's equivalent to 5.8 in that repo. In this one (laravel/framework) master is the target for 5.8.

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Oct 15, 2018

Ah, perfect. 👍

@ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Feb 24, 2019

@driesvints
Do you think that this changes should be documented in 5.8 upgrade guide?
https://laravel.com/docs/master/upgrade

driesvints added a commit to driesvints/docs that referenced this pull request Feb 25, 2019
Introduced by laravel/framework#25957

Low impact because should only happen when choosing or changing your password.
@driesvints
Copy link
Member

@driesvints driesvints commented Feb 25, 2019

@tylernathanreed
Copy link

@tylernathanreed tylernathanreed commented Mar 1, 2019

@GrahamCampbell @jakebathman

Since this is security related, will this change be back-ported to 5.5?

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Mar 1, 2019

No, this is a not a security patch. It's a breaking change to some validation code only.

caouecs added a commit to Laravel-Lang/lang that referenced this pull request Mar 9, 2019
caouecs added a commit to Laravel-Lang/lang that referenced this pull request Mar 9, 2019
Tjoosten pushed a commit to Activisme-be/Spoon that referenced this pull request Mar 10, 2019
caouecs added a commit to Laravel-Lang/lang that referenced this pull request Mar 17, 2019
caouecs added a commit to Laravel-Lang/lang that referenced this pull request Mar 17, 2019
@chpio
Copy link

@chpio chpio commented May 7, 2019

does this mean that i can keep using password as my password?

@OzanKurt
Copy link
Contributor

@OzanKurt OzanKurt commented Jun 2, 2019

This validation should be removed if the request has validation already. If someone decides to give min:6 as rule, it is just being ignored. This doesn't make sense and can be a lot confusing for some people who don't want to check the Laravel source code.

@robclancy
Copy link
Contributor

@robclancy robclancy commented Jun 14, 2019

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 use Illuminate\Foundation\Auth\ResetsPasswords;. There shouldn't be any validation logic in the broker. And I just realized that the validation logic is in the ResetsPasswords trait. So by default validation is done twice for some reason (but in different ways, so to change it you have 2 places). Feels like it was left over in PasswordBroker when the ResetsPassword was added or something.

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 password_confirmation field is useless half the time but we literally do $credentials['password_confirmation'] = $credentials['password']; because of it before running the broker.

And again, this looks even more like an issue because Laravel has an entire validator and then hardcoded redundant validations in the broker. ResetsPasswords even puts the validation rules into a rules method so you can easily override the rules.

validateNewPassword and all method related to it should be deleted from PasswordBroker.

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.

@OzanKurt
Copy link
Contributor

@OzanKurt OzanKurt commented Jun 14, 2019

Totally agree. @robclancy

@bagus-repository
Copy link

@bagus-repository bagus-repository commented Jun 24, 2019

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

@driesvints
Copy link
Member

@driesvints driesvints commented Jun 25, 2019

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.

caouecs added a commit to Laravel-Lang/lang that referenced this pull request Aug 1, 2019
caouecs added a commit to Laravel-Lang/lang that referenced this pull request Aug 1, 2019
@driesvints
Copy link
Member

@driesvints driesvints commented Aug 8, 2019

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

#29468

@robclancy
Copy link
Contributor

@robclancy robclancy commented Aug 8, 2019

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.
https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Auth/ResetsPasswords.php#L69

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.

@driesvints
Copy link
Member

@driesvints driesvints commented Aug 8, 2019

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

taylorotwell pushed a commit to illuminate/auth that referenced this pull request Aug 9, 2019
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.
@driesvints
Copy link
Member

@driesvints driesvints commented Aug 9, 2019

@robclancy sent in a PR here: #29480

taylorotwell pushed a commit to illuminate/auth that referenced this pull request Aug 9, 2019
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)
taylorotwell pushed a commit to illuminate/contracts that referenced this pull request Aug 9, 2019
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)
@driesvints
Copy link
Member

@driesvints driesvints commented Aug 12, 2019

Password validation is now completely removed from the broker: #29480

Omranic added a commit to rinvex/cortex-auth that referenced this pull request Sep 3, 2019
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)
Omranic added a commit to rinvex/laravel-auth that referenced this pull request Sep 22, 2019
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet