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 common password checking #964

Merged

Conversation

ryanwoodcox
Copy link
Contributor

@ryanwoodcox ryanwoodcox commented Mar 15, 2025

This is a follow up to #958

  • rename checkCommonPassword to checkIsCommonPassword for clarity
  • use AbortSignal.timeout instead of manual timeout handling
  • fix regex pattern for line splitting
  • properly parse HIBP API response format
  • improve error handling and messages
  • add comprehensive tests for all scenarios
  • extract getPasswordHashParts as a separate function (to be leveraged by the tests)

Test Plan

  • Run npm run test and see all tests including the 5 new tests pass.

Checklist

  • Tests updated
  • Docs updated

- rename checkCommonPassword to checkIsCommonPassword for clarity
- extract getPasswordHashParts as a separate function
- use AbortSignal.timeout instead of manual timeout handling
- fix regex pattern for line splitting
- properly parse HIBP API response format
- improve error handling and messages
- add comprehensive tests for all scenarios
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Great improvements. Thank you!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Whoops, clicked the wrong button 😅 Thanks again!

@kentcdodds kentcdodds merged commit d83bdf8 into epicweb-dev:main Mar 17, 2025
@ryanwoodcox ryanwoodcox deleted the pr/prevent-common-pwd-fixes branch March 18, 2025 01:38
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.

2 participants