prevent use of common password using the haveibeenpwned password api #958
Conversation
There was a problem hiding this comment.
Thanks for this! I think it's a good idea. Just a couple things:
- Let's reduce the duplication by putting this logic in
auth.server.tsas a util and using that util. - Let's add a MSW mock in the
test/mocksdirectory so we don't depend on it for development and testing - Let's add a timeout to the request so if it takes more than 1 second we skip the check
- If the request fails, let's skip the check
I don't want a user to not be able to set their password just because a third party service is down or slow or unreliable. And all third party requests we make should be mocked out.
Thanks for this! I think it's great!
|
will make the necessary changes asap |
1. Reduced the core logic by extracting it in custom function in auth.server.ts 2. added timeout to the request to skip the check if request takes more than 1s 3. skips the check if request fails 4. added msw mock so that we don't depend upon it in development and testing
Co-authored-by: Kent C. Dodds <[email protected]>
| if (error instanceof Error && error.name === 'AbortError') { | ||
| console.warn('Password check timed out') | ||
| } | ||
| return false |
There was a problem hiding this comment.
It may be a good idea to add a warning if any other kind of error is thrown so we at least have some visibility into whether things are working.
There was a problem hiding this comment.
Yeah completely forgot about it
1. Added warning in case if the error is not AbortError to get more info about the error 2. updated the msw mock to follow convention of other msw mocks
…Kirad/epic-stack into prevent-common-passowords
app/utils/auth.server.ts
Outdated
| } | ||
| console.warn('unknow error during password check', error) |
There was a problem hiding this comment.
| } | |
| console.warn('unknow error during password check', error) | |
| } else { | |
| console.warn('unknow error during password check', error) | |
| } |
tests/mocks/index.ts
Outdated
| @@ -1,5 +1,6 @@ | |||
| import closeWithGrace from 'close-with-grace' | |||
| import { setupServer } from 'msw/node' | |||
| import { pwnedPasswordApiHandlers } from './common-password.ts' | |||
There was a problem hiding this comment.
Let's rename the common-password file to pwnedpasswords.ts
added checks in the schemas of onboarding and reset-password page to prevent the use of common password using the HaveIBeenPwned Password API