diff --git a/app/routes/_auth+/onboarding.tsx b/app/routes/_auth+/onboarding.tsx index f75a8e9dc..86b9e4f66 100644 --- a/app/routes/_auth+/onboarding.tsx +++ b/app/routes/_auth+/onboarding.tsx @@ -8,7 +8,7 @@ import { CheckboxField, ErrorList, Field } from '#app/components/forms.tsx' import { Spacer } from '#app/components/spacer.tsx' import { StatusButton } from '#app/components/ui/status-button.tsx' import { - checkCommonPassword, + checkIsCommonPassword, requireAnonymous, sessionKey, signup, @@ -77,7 +77,7 @@ export async function action({ request }: Route.ActionArgs) { }) return } - const isCommonPassword = await checkCommonPassword(data.password) + const isCommonPassword = await checkIsCommonPassword(data.password) if (isCommonPassword) { ctx.addIssue({ path: ['password'], diff --git a/app/routes/_auth+/reset-password.tsx b/app/routes/_auth+/reset-password.tsx index 581612313..f17ce68a0 100644 --- a/app/routes/_auth+/reset-password.tsx +++ b/app/routes/_auth+/reset-password.tsx @@ -6,7 +6,7 @@ import { GeneralErrorBoundary } from '#app/components/error-boundary.tsx' import { ErrorList, Field } from '#app/components/forms.tsx' import { StatusButton } from '#app/components/ui/status-button.tsx' import { - checkCommonPassword, + checkIsCommonPassword, requireAnonymous, resetUserPassword, } from '#app/utils/auth.server.ts' @@ -47,7 +47,7 @@ export async function action({ request }: Route.ActionArgs) { const formData = await request.formData() const submission = await parseWithZod(formData, { schema: ResetPasswordSchema.superRefine(async ({ password }, ctx) => { - const isCommonPassword = await checkCommonPassword(password) + const isCommonPassword = await checkIsCommonPassword(password) if (isCommonPassword) { ctx.addIssue({ path: ['password'], diff --git a/app/routes/settings+/profile.password.tsx b/app/routes/settings+/profile.password.tsx index f80a1da2f..750349667 100644 --- a/app/routes/settings+/profile.password.tsx +++ b/app/routes/settings+/profile.password.tsx @@ -8,7 +8,7 @@ import { Button } from '#app/components/ui/button.tsx' import { Icon } from '#app/components/ui/icon.tsx' import { StatusButton } from '#app/components/ui/status-button.tsx' import { - checkCommonPassword, + checkIsCommonPassword, getPasswordHash, requireUserId, verifyUserPassword, @@ -74,7 +74,7 @@ export async function action({ request }: Route.ActionArgs) { message: 'Incorrect password.', }) } - const isCommonPassword = await checkCommonPassword(newPassword) + const isCommonPassword = await checkIsCommonPassword(newPassword) if (isCommonPassword) { ctx.addIssue({ path: ['newPassword'], diff --git a/app/routes/settings+/profile.password_.create.tsx b/app/routes/settings+/profile.password_.create.tsx index 29e61c2ad..8fc29d78d 100644 --- a/app/routes/settings+/profile.password_.create.tsx +++ b/app/routes/settings+/profile.password_.create.tsx @@ -7,7 +7,7 @@ import { Button } from '#app/components/ui/button.tsx' import { Icon } from '#app/components/ui/icon.tsx' import { StatusButton } from '#app/components/ui/status-button.tsx' import { - checkCommonPassword, + checkIsCommonPassword, getPasswordHash, requireUserId, } from '#app/utils/auth.server.ts' @@ -47,7 +47,7 @@ export async function action({ request }: Route.ActionArgs) { const submission = await parseWithZod(formData, { async: true, schema: CreatePasswordForm.superRefine(async ({ password }, ctx) => { - const isCommonPassword = await checkCommonPassword(password) + const isCommonPassword = await checkIsCommonPassword(password) if (isCommonPassword) { ctx.addIssue({ path: ['password'], diff --git a/app/utils/auth.server.test.ts b/app/utils/auth.server.test.ts new file mode 100644 index 000000000..327b463c1 --- /dev/null +++ b/app/utils/auth.server.test.ts @@ -0,0 +1,101 @@ +import { http, HttpResponse } from 'msw' +import { expect, test, vi } from 'vitest' +import { server } from '#tests/mocks' +import { checkIsCommonPassword, getPasswordHashParts } from './auth.server.ts' + +test('checkIsCommonPassword returns true when password is found in breach database', async () => { + const password = 'testpassword' + const [prefix, suffix] = getPasswordHashParts(password) + + server.use( + http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => { + // Include the actual suffix in the response with another realistic suffix + return new HttpResponse( + `1234567890123456789012345678901234A:1\n${suffix}:1234`, + { status: 200 }, + ) + }), + ) + + const result = await checkIsCommonPassword(password) + expect(result).toBe(true) +}) + +test('checkIsCommonPassword returns false when password is not found in breach database', async () => { + const password = 'sup3r-dup3r-s3cret' + const [prefix] = getPasswordHashParts(password) + + server.use( + http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => { + // Response with realistic suffixes that won't match + return new HttpResponse( + '1234567890123456789012345678901234A:1\n' + + '1234567890123456789012345678901234B:2', + { status: 200 }, + ) + }), + ) + + const result = await checkIsCommonPassword(password) + expect(result).toBe(false) +}) + +// Error cases +test('checkIsCommonPassword returns false when API returns 500', async () => { + const password = 'testpassword' + const [prefix] = getPasswordHashParts(password) + + server.use( + http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => { + return new HttpResponse(null, { status: 500 }) + }), + ) + + const result = await checkIsCommonPassword(password) + expect(result).toBe(false) +}) + +test('checkIsCommonPassword times out after 1 second', async () => { + const consoleWarnSpy = vi.spyOn(console, 'warn') + server.use( + http.get('https://api.pwnedpasswords.com/range/:prefix', async () => { + const twoSecondDelay = 2000 + await new Promise((resolve) => setTimeout(resolve, twoSecondDelay)) + return new HttpResponse( + '1234567890123456789012345678901234A:1\n' + + '1234567890123456789012345678901234B:2', + { status: 200 }, + ) + }), + ) + + const result = await checkIsCommonPassword('testpassword') + expect(result).toBe(false) + expect(consoleWarnSpy).toHaveBeenCalledWith('Password check timed out') + consoleWarnSpy.mockRestore() +}) + +test('checkIsCommonPassword returns false when response has invalid format', async () => { + const consoleWarnSpy = vi.spyOn(console, 'warn') + const password = 'testpassword' + const [prefix] = getPasswordHashParts(password) + + server.use( + http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => { + // Create a response that will cause a TypeError when text() is called + const response = new Response() + Object.defineProperty(response, 'text', { + value: () => Promise.resolve(null), + }) + return response + }), + ) + + const result = await checkIsCommonPassword(password) + expect(result).toBe(false) + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Unknown error during password check', + expect.any(TypeError), + ) + consoleWarnSpy.mockRestore() +}) diff --git a/app/utils/auth.server.ts b/app/utils/auth.server.ts index 3b55b28ca..3d601cf66 100644 --- a/app/utils/auth.server.ts +++ b/app/utils/auth.server.ts @@ -257,33 +257,40 @@ export async function verifyUserPassword( return { id: userWithPassword.id } } -export async function checkCommonPassword(password: string) { +export function getPasswordHashParts(password: string) { const hash = crypto .createHash('sha1') .update(password, 'utf8') .digest('hex') .toUpperCase() + return [hash.slice(0, 5), hash.slice(5)] as const +} - const [prefix, suffix] = [hash.slice(0, 5), hash.slice(5)] - - const controller = new AbortController() +export async function checkIsCommonPassword(password: string) { + const [prefix, suffix] = getPasswordHashParts(password) try { - const timeoutId = setTimeout(() => controller.abort(), 1000) - - const res = await fetch(`https://api.pwnedpasswords.com/range/${prefix}`) - - clearTimeout(timeoutId) + const response = await fetch( + `https://api.pwnedpasswords.com/range/${prefix}`, + { + signal: AbortSignal.timeout(1000), + }, + ) - if (!res.ok) false + if (!response.ok) return false - const data = await res.text() - return data.split('/\r?\n/').some((line) => line.includes(suffix)) + const data = await response.text() + return data.split(/\r?\n/).some((line) => { + const [hashSuffix, ignoredPrevalenceCount] = line.split(':') + return hashSuffix === suffix + }) } catch (error) { - if (error instanceof Error && error.name === 'AbortError') { + if (error instanceof DOMException && error.name === 'TimeoutError') { console.warn('Password check timed out') + return false } - console.warn('unknow error during password check', error) + + console.warn('Unknown error during password check', error) return false } } diff --git a/tests/mocks/index.ts b/tests/mocks/index.ts index 260123fe6..5158a53c8 100644 --- a/tests/mocks/index.ts +++ b/tests/mocks/index.ts @@ -1,7 +1,7 @@ import closeWithGrace from 'close-with-grace' import { setupServer } from 'msw/node' import { handlers as githubHandlers } from './github.ts' -import { handlers as pwnedPasswordApiHandlers } from './pwnedpasswords.ts' +import { handlers as pwnedPasswordApiHandlers } from './pwned-passwords.ts' import { handlers as resendHandlers } from './resend.ts' import { handlers as tigrisHandlers } from './tigris.ts' diff --git a/tests/mocks/pwnedpasswords.ts b/tests/mocks/pwned-passwords.ts similarity index 100% rename from tests/mocks/pwnedpasswords.ts rename to tests/mocks/pwned-passwords.ts