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

[FIX] Hashing password-reset tokens before storing #508

Merged

Conversation

rosamarsky
Copy link
Contributor

[FIX] Fix for issue #506. Hashing password-reset tokens before storing.

@rosamarsky rosamarsky force-pushed the 506-fix-password-resets-hashing-issue branch from 214d9eb to 3e2b5b2 Compare December 13, 2021 15:26
@rosamarsky rosamarsky changed the title Fix for issue #506. Hashing password-reset tokens before storing [FIX] Hashing password-reset tokens before storing Dec 13, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #508 (af5d1ec) into 1.7 (35117d3) will decrease coverage by 0.03%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##                1.7     #508      +/-   ##
============================================
- Coverage     56.83%   56.80%   -0.04%     
- Complexity      829      830       +1     
============================================
  Files           102      102              
  Lines          2303     2308       +5     
============================================
+ Hits           1309     1311       +2     
- Misses          994      997       +3     
Impacted Files Coverage Δ
src/Auth/Passwords/PasswordBrokerManager.php 0.00% <0.00%> (ø)
src/Auth/Passwords/DoctrineTokenRepository.php 94.73% <100.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35117d3...af5d1ec. Read the comment docs.

@eigan
Copy link
Member

eigan commented Dec 14, 2021

As @dpslwk pointed out, this is invalidating all existing tokens. Though we never release a new major so cannot really see how else we can solve this...

@rosamarsky
Copy link
Contributor Author

@eigan, maybe you can create a dev branch with all backward incompatible commits and tag it with git like dev-master and merge it someday with a new major version.

BTW, I don't see a problem if this commit invalidates all tokens. You can just use the password recovery request again to get a new link with a new token.

@dpslwk
Copy link
Member

dpslwk commented Dec 14, 2021

I believe src/Auth/Passwords/PasswordBrokerManager.php also needs an update to pass the new constructor param

@dpslwk
Copy link
Member

dpslwk commented Dec 14, 2021

oh and looks like we should have also done this change
https://github.com/laravel/framework/pull/13270/files

back around v5.2.31 (2016-04-27), as the change to APP_KEY gen was introduced in v5.2.26 (2016-03-25)

@rosamarsky rosamarsky force-pushed the 506-fix-password-resets-hashing-issue branch from 3e2b5b2 to af5d1ec Compare December 14, 2021 15:09
@rosamarsky
Copy link
Contributor Author

@dpslwk sorry, I forgot to add this file to the commit

@dpslwk
Copy link
Member

dpslwk commented Dec 14, 2021

@dpslwk sorry, I forgot to add this file to the commit

no worries its why we have code review 😉

@eigan eigan merged commit b32c58b into laravel-doctrine:1.7 Dec 20, 2021
@eigan
Copy link
Member

eigan commented Dec 20, 2021

Thank you @rosamarsky and @dpslwk!

Going for a release announcement instead of bc. Sorry in advance for anyone getting angry users over this.

@dpslwk
Copy link
Member

dpslwk commented Dec 20, 2021

make some sense, I mean these a reset tokens they should only have a short lifetime anyway so effects should only be during a short transition period

DemianShtepa added a commit to DemianShtepa/orm that referenced this pull request Dec 28, 2022
1.7.13
- Hashing password-reset tokens before storing (laravel-doctrine#508 laravel-doctrine#506) @rosamarsky
- Trying to print_r circular entity dependency results in OOM in InteractsWithEntities  (laravel-doctrine#510) @k0ka

Breaking change:
- All password reset tokens will be invalidated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants