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.

Sign up for GitHub

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

Jump to bottom

fix(core): Hash password reset token - V08 #6573

Closed
wants to merge 2 commits into from
Closed

fix(core): Hash password reset token - V08 #6573

wants to merge 2 commits into from

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Jun 30, 2023 •
edited
Loading

@github-actions
Copy link
Contributor

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

  • If fixing bug, added test to cover scenario.
  • If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

  • Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

@RicardoE105 RicardoE105 changed the title Hash password reset token V08: Insecure password reset Jun 30, 2023
@RicardoE105 RicardoE105 changed the title V08: Insecure password reset fix(core): hash password reset token -V08 Jun 30, 2023
@RicardoE105 RicardoE105 requested a review from krynble June 30, 2023 00:04
@RicardoE105 RicardoE105 changed the title fix(core): hash password reset token -V08 fix(core): Hash password reset token -V08 Jun 30, 2023
@RicardoE105 RicardoE105 changed the title fix(core): Hash password reset token -V08 fix(core): Hash password reset token - V08 Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023 •
edited
Loading

Codecov Report

Patch coverage: 45.45% and project coverage change: -0.08 ⚠️

Comparison is base (e63b398) 28.82% compared to head (491e64d) 28.75%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6573      +/-   ##
==========================================
- Coverage   28.82%   28.75%   -0.08%     
==========================================
  Files        3042     3042              
  Lines      187564   187573       +9     
  Branches    20759    20758       -1     
==========================================
- Hits        54071    53932     -139     
- Misses     132654   132802     +148     
  Partials      839      839
Impacted Files Coverage Δ
...es/cli/src/controllers/passwordReset.controller.ts 80.21% <25.00%> (-5.33%) ⬇️
packages/cli/src/user/user.service.ts 79.16% <100.00%> (+0.90%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 30, 2023
krynble
krynble suggested changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

This means that existing password reset requests will no longer work, right? People will need to request again.

One thing we could do is create a migration that hashes existing password reset tokens so they continue to work.

Also, when reviewing the code I realised we changed the default value for the userManagement.emails.mode setting to be smtp instead of an empty string. This means that the check in the first line of @Post('/forgot-password') will never work.

packages/cli/src/controllers/passwordReset.controller.ts
@@ -199,6 +199,22 @@ export class PasswordResetController {
throw new NotFoundError('');
}

const validResetPasswordToken = await compareHash(
resetPasswordToken,
user?.resetPasswordToken as string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
user?.resetPasswordToken as string,
user.resetPasswordToken as string,

packages/cli/src/controllers/passwordReset.controller.ts
@@ -245,6 +260,21 @@ export class PasswordResetController {
throw new NotFoundError('');
}

const validResetPasswordToken = await compareHash(
resetPasswordToken,
user?.resetPasswordToken as string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
user?.resetPasswordToken as string,
user.resetPasswordToken as string,

@RicardoE105
Copy link
Contributor Author

@krynble yes but they are there only valid for 1 hours and highly unlikely to happen. I would avoid adding a migration just for that!

@RicardoE105
Copy link
Contributor Author

closing in favor of #6714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krynble krynble krynble requested changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RicardoE105 @krynble