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

improve customer and employee password reset token's randomness #31725

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

matthieu-rolland
Copy link
Contributor

@matthieu-rolland matthieu-rolland commented Mar 10, 2023

Questions Answers
Branch? develop
Description? Use token generation best practice instead of using mt_rand() which is not cryptographically secure
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? Indicate how to verify that this change works as expected.
Fixed ticket? none
Related PRs test that password reset still functions for customer and employee
Sponsor company @PrestaShopCorp

@matthieu-rolland matthieu-rolland requested a review from a team as a code owner March 10, 2023 14:28
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Mar 10, 2023
@matthieu-rolland matthieu-rolland changed the title improve customer and password token's randomness improve customer and employee password reset token's randomness Mar 10, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be extremely careful with this change. Customer's secure_key is validated against cart secure_key and order secure_key on many places. Make sure not to fuck up people's carts and orders after upgrading. 👍

@matthieu-rolland
Copy link
Contributor Author

matthieu-rolland commented Mar 10, 2023

Be extremely careful with this change. Customer's secure_key is validated against cart secure_key and order secure_key on many places. Make sure not to fuck up people's carts and orders after upgrading. +1

I'm launching the full automatic test suite so that such problem would be detected

https://github.com/matthieu-rolland/ga.tests.ui.pr/actions/runs/4385534307

@Hlavtox
Copy link
Contributor

Hlavtox commented Mar 10, 2023

@matthieu-rolland I am checking it right now and it should be safe. It always assigns the secure key of the customer and the customer is the first object created. And this will affect only new customers.

@matthieu-rolland
Copy link
Contributor Author

actually this simple PR breaks a lot of things :D https://github.com/matthieu-rolland/ga.tests.ui.pr/actions/runs/4385534307/jobs/7678382320

@kpodemski
Copy link
Contributor

@matthieu-rolland yep, in Customer ObjectModel there's a isMd5 for secure_key

@matthieu-rolland
Copy link
Contributor Author

matthieu-rolland commented Mar 10, 2023

@matthieu-rolland yep, in Customer ObjectModel there's a isMd5 for secure_key

yep, I'll look further into it 👍 (in a meeting right now)

Edit: So, I need to add a new validation method for these tokens, and to increase the size of the reset_password_token field. Then add unit test for the validation method. I'll do it soon.

@matthieu-rolland matthieu-rolland marked this pull request as draft March 10, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants