Skip to content

Commit d83bdf8

Browse files
ryanwoodcoxryanwoodcox
authored
fix: improve password security check implementation (#964)
- 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
1 parent 6247ed3 commit d83bdf8

File tree

8 files changed

+131
-23
lines changed

8 files changed

+131
-23
lines changed

‎app/routes/_auth+/onboarding.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { CheckboxField, ErrorList, Field } from '#app/components/forms.tsx'
88
import { Spacer } from '#app/components/spacer.tsx'
99
import { StatusButton } from '#app/components/ui/status-button.tsx'
1010
import {
11-
checkCommonPassword,
11+
checkIsCommonPassword,
1212
requireAnonymous,
1313
sessionKey,
1414
signup,
@@ -77,7 +77,7 @@ export async function action({ request }: Route.ActionArgs) {
7777
})
7878
return
7979
}
80-
const isCommonPassword = await checkCommonPassword(data.password)
80+
const isCommonPassword = await checkIsCommonPassword(data.password)
8181
if (isCommonPassword) {
8282
ctx.addIssue({
8383
path: ['password'],

‎app/routes/_auth+/reset-password.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { GeneralErrorBoundary } from '#app/components/error-boundary.tsx'
66
import { ErrorList, Field } from '#app/components/forms.tsx'
77
import { StatusButton } from '#app/components/ui/status-button.tsx'
88
import {
9-
checkCommonPassword,
9+
checkIsCommonPassword,
1010
requireAnonymous,
1111
resetUserPassword,
1212
} from '#app/utils/auth.server.ts'
@@ -47,7 +47,7 @@ export async function action({ request }: Route.ActionArgs) {
4747
const formData = await request.formData()
4848
const submission = await parseWithZod(formData, {
4949
schema: ResetPasswordSchema.superRefine(async ({ password }, ctx) => {
50-
const isCommonPassword = await checkCommonPassword(password)
50+
const isCommonPassword = await checkIsCommonPassword(password)
5151
if (isCommonPassword) {
5252
ctx.addIssue({
5353
path: ['password'],

‎app/routes/settings+/profile.password.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { Button } from '#app/components/ui/button.tsx'
88
import { Icon } from '#app/components/ui/icon.tsx'
99
import { StatusButton } from '#app/components/ui/status-button.tsx'
1010
import {
11-
checkCommonPassword,
11+
checkIsCommonPassword,
1212
getPasswordHash,
1313
requireUserId,
1414
verifyUserPassword,
@@ -74,7 +74,7 @@ export async function action({ request }: Route.ActionArgs) {
7474
message: 'Incorrect password.',
7575
})
7676
}
77-
const isCommonPassword = await checkCommonPassword(newPassword)
77+
const isCommonPassword = await checkIsCommonPassword(newPassword)
7878
if (isCommonPassword) {
7979
ctx.addIssue({
8080
path: ['newPassword'],

‎app/routes/settings+/profile.password_.create.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { Button } from '#app/components/ui/button.tsx'
77
import { Icon } from '#app/components/ui/icon.tsx'
88
import { StatusButton } from '#app/components/ui/status-button.tsx'
99
import {
10-
checkCommonPassword,
10+
checkIsCommonPassword,
1111
getPasswordHash,
1212
requireUserId,
1313
} from '#app/utils/auth.server.ts'
@@ -47,7 +47,7 @@ export async function action({ request }: Route.ActionArgs) {
4747
const submission = await parseWithZod(formData, {
4848
async: true,
4949
schema: CreatePasswordForm.superRefine(async ({ password }, ctx) => {
50-
const isCommonPassword = await checkCommonPassword(password)
50+
const isCommonPassword = await checkIsCommonPassword(password)
5151
if (isCommonPassword) {
5252
ctx.addIssue({
5353
path: ['password'],

‎app/utils/auth.server.test.ts‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
import { http, HttpResponse } from 'msw'
2+
import { expect, test, vi } from 'vitest'
3+
import { server } from '#tests/mocks'
4+
import { checkIsCommonPassword, getPasswordHashParts } from './auth.server.ts'
5+
6+
test('checkIsCommonPassword returns true when password is found in breach database', async () => {
7+
const password = 'testpassword'
8+
const [prefix, suffix] = getPasswordHashParts(password)
9+
10+
server.use(
11+
http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => {
12+
// Include the actual suffix in the response with another realistic suffix
13+
return new HttpResponse(
14+
`1234567890123456789012345678901234A:1\n${suffix}:1234`,
15+
{ status: 200 },
16+
)
17+
}),
18+
)
19+
20+
const result = await checkIsCommonPassword(password)
21+
expect(result).toBe(true)
22+
})
23+
24+
test('checkIsCommonPassword returns false when password is not found in breach database', async () => {
25+
const password = 'sup3r-dup3r-s3cret'
26+
const [prefix] = getPasswordHashParts(password)
27+
28+
server.use(
29+
http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => {
30+
// Response with realistic suffixes that won't match
31+
return new HttpResponse(
32+
'1234567890123456789012345678901234A:1\n' +
33+
'1234567890123456789012345678901234B:2',
34+
{ status: 200 },
35+
)
36+
}),
37+
)
38+
39+
const result = await checkIsCommonPassword(password)
40+
expect(result).toBe(false)
41+
})
42+
43+
// Error cases
44+
test('checkIsCommonPassword returns false when API returns 500', async () => {
45+
const password = 'testpassword'
46+
const [prefix] = getPasswordHashParts(password)
47+
48+
server.use(
49+
http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => {
50+
return new HttpResponse(null, { status: 500 })
51+
}),
52+
)
53+
54+
const result = await checkIsCommonPassword(password)
55+
expect(result).toBe(false)
56+
})
57+
58+
test('checkIsCommonPassword times out after 1 second', async () => {
59+
const consoleWarnSpy = vi.spyOn(console, 'warn')
60+
server.use(
61+
http.get('https://api.pwnedpasswords.com/range/:prefix', async () => {
62+
const twoSecondDelay = 2000
63+
await new Promise((resolve) => setTimeout(resolve, twoSecondDelay))
64+
return new HttpResponse(
65+
'1234567890123456789012345678901234A:1\n' +
66+
'1234567890123456789012345678901234B:2',
67+
{ status: 200 },
68+
)
69+
}),
70+
)
71+
72+
const result = await checkIsCommonPassword('testpassword')
73+
expect(result).toBe(false)
74+
expect(consoleWarnSpy).toHaveBeenCalledWith('Password check timed out')
75+
consoleWarnSpy.mockRestore()
76+
})
77+
78+
test('checkIsCommonPassword returns false when response has invalid format', async () => {
79+
const consoleWarnSpy = vi.spyOn(console, 'warn')
80+
const password = 'testpassword'
81+
const [prefix] = getPasswordHashParts(password)
82+
83+
server.use(
84+
http.get(`https://api.pwnedpasswords.com/range/${prefix}`, () => {
85+
// Create a response that will cause a TypeError when text() is called
86+
const response = new Response()
87+
Object.defineProperty(response, 'text', {
88+
value: () => Promise.resolve(null),
89+
})
90+
return response
91+
}),
92+
)
93+
94+
const result = await checkIsCommonPassword(password)
95+
expect(result).toBe(false)
96+
expect(consoleWarnSpy).toHaveBeenCalledWith(
97+
'Unknown error during password check',
98+
expect.any(TypeError),
99+
)
100+
consoleWarnSpy.mockRestore()
101+
})

‎app/utils/auth.server.ts‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -257,33 +257,40 @@ export async function verifyUserPassword(
257257
return { id: userWithPassword.id }
258258
}
259259

260-
export async function checkCommonPassword(password: string) {
260+
export function getPasswordHashParts(password: string) {
261261
const hash = crypto
262262
.createHash('sha1')
263263
.update(password, 'utf8')
264264
.digest('hex')
265265
.toUpperCase()
266+
return [hash.slice(0, 5), hash.slice(5)] as const
267+
}
266268

267-
const [prefix, suffix] = [hash.slice(0, 5), hash.slice(5)]
268-
269-
const controller = new AbortController()
269+
export async function checkIsCommonPassword(password: string) {
270+
const [prefix, suffix] = getPasswordHashParts(password)
270271

271272
try {
272-
const timeoutId = setTimeout(() => controller.abort(), 1000)
273-
274-
const res = await fetch(`https://api.pwnedpasswords.com/range/${prefix}`)
275-
276-
clearTimeout(timeoutId)
273+
const response = await fetch(
274+
`https://api.pwnedpasswords.com/range/${prefix}`,
275+
{
276+
signal: AbortSignal.timeout(1000),
277+
},
278+
)
277279

278-
if (!res.ok) false
280+
if (!response.ok) return false
279281

280-
const data = await res.text()
281-
return data.split('/\r?\n/').some((line) => line.includes(suffix))
282+
const data = await response.text()
283+
return data.split(/\r?\n/).some((line) => {
284+
const [hashSuffix, ignoredPrevalenceCount] = line.split(':')
285+
return hashSuffix === suffix
286+
})
282287
} catch (error) {
283-
if (error instanceof Error && error.name === 'AbortError') {
288+
if (error instanceof DOMException && error.name === 'TimeoutError') {
284289
console.warn('Password check timed out')
290+
return false
285291
}
286-
console.warn('unknow error during password check', error)
292+
293+
console.warn('Unknown error during password check', error)
287294
return false
288295
}
289296
}

‎tests/mocks/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import closeWithGrace from 'close-with-grace'
22
import { setupServer } from 'msw/node'
33
import { handlers as githubHandlers } from './github.ts'
4-
import { handlers as pwnedPasswordApiHandlers } from './pwnedpasswords.ts'
4+
import { handlers as pwnedPasswordApiHandlers } from './pwned-passwords.ts'
55
import { handlers as resendHandlers } from './resend.ts'
66
import { handlers as tigrisHandlers } from './tigris.ts'
77

‎tests/mocks/pwnedpasswords.ts‎ renamed to ‎tests/mocks/pwned-passwords.ts‎

File renamed without changes.

0 commit comments

Comments
 (0)