-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(security): harden deployment auth tokens #4760
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import { createHash, createHmac } from 'node:crypto' | ||
| import { createEnvMock } from '@sim/testing' | ||
| import type { NextResponse } from 'next/server' | ||
| import { describe, expect, it, vi } from 'vitest' | ||
|
|
||
| vi.mock('@/lib/core/config/env', () => | ||
| createEnvMock({ | ||
| BETTER_AUTH_SECRET: 'deployment-auth-test-secret-32-chars', | ||
| }) | ||
| ) | ||
|
|
||
| vi.mock('@/lib/core/config/feature-flags', () => ({ | ||
| isDev: true, | ||
| })) | ||
|
|
||
| import { setDeploymentAuthCookie, validateAuthToken } from './deployment' | ||
|
|
||
| const SECRET = 'deployment-auth-test-secret-32-chars' | ||
|
|
||
| function issueCookieToken(encryptedPassword?: string | null): string { | ||
| let token = '' | ||
| const response = { | ||
| cookies: { | ||
| set: vi.fn((cookie: { value: string }) => { | ||
| token = cookie.value | ||
| }), | ||
| }, | ||
| } as unknown as NextResponse | ||
|
|
||
| setDeploymentAuthCookie(response, 'chat', 'dep_test', 'password', encryptedPassword) | ||
|
|
||
| return token | ||
| } | ||
|
|
||
| function forgeUnsignedLegacyToken( | ||
| deploymentId: string, | ||
| encryptedPassword: string, | ||
| timestamp = Date.now() | ||
| ): string { | ||
| const passwordSlot = createHash('sha256').update(encryptedPassword).digest('hex').slice(0, 8) | ||
| return Buffer.from(`${deploymentId}:password:${timestamp}:${passwordSlot}`).toString('base64') | ||
| } | ||
|
|
||
| function signedLegacyToken( | ||
| deploymentId: string, | ||
| encryptedPassword: string, | ||
| timestamp = Date.now() | ||
| ): string { | ||
| const passwordSlot = createHash('sha256').update(encryptedPassword).digest('hex').slice(0, 8) | ||
| const payload = `${deploymentId}:password:${timestamp}:${passwordSlot}` | ||
| const signature = createHmac('sha256', SECRET).update(payload, 'utf8').digest('hex') | ||
|
|
||
| return Buffer.from(`${payload}:${signature}`).toString('base64') | ||
| } | ||
|
|
||
| function signedV2Token( | ||
| deploymentId: string, | ||
| encryptedPassword: string, | ||
| timestamp = Date.now() | ||
| ): string { | ||
| const payload = `v2:${deploymentId}:password:${timestamp}` | ||
| const passwordBinding = createHash('sha256').update(encryptedPassword, 'utf8').digest('hex') | ||
| const signature = createHmac('sha256', SECRET) | ||
| .update(`${payload}:${passwordBinding}`, 'utf8') | ||
| .digest('hex') | ||
|
|
||
| return Buffer.from(`${payload}:${signature}`).toString('base64') | ||
| } | ||
|
|
||
| describe('deployment auth tokens', () => { | ||
| it('validates signed server-issued tokens', () => { | ||
| const token = issueCookieToken('encrypted-password') | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'encrypted-password')).toBe(true) | ||
| expect(validateAuthToken(token, 'other-deployment', 'encrypted-password')).toBe(false) | ||
| }) | ||
|
|
||
| it('does not expose the password-derived slot in newly issued tokens', () => { | ||
| const token = issueCookieToken('encrypted-password') | ||
| const decoded = Buffer.from(token, 'base64').toString() | ||
|
|
||
| expect(decoded).toMatch(/^v2:dep_test:password:\d+:[a-f0-9]{64}$/) | ||
| expect(decoded).not.toContain( | ||
| createHash('sha256').update('encrypted-password').digest('hex').slice(0, 8) | ||
| ) | ||
| }) | ||
|
|
||
| it('rejects unsigned forged tokens using the old base64 field format', () => { | ||
| const token = forgeUnsignedLegacyToken('dep_test', 'encrypted-password') | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'encrypted-password')).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects signed tokens after the deployment password changes', () => { | ||
| const token = issueCookieToken('encrypted-password') | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'different-encrypted-password')).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects tampered signed token payloads', () => { | ||
| const token = issueCookieToken('encrypted-password') | ||
| const decoded = Buffer.from(token, 'base64').toString() | ||
| const tampered = Buffer.from(decoded.replace('dep_test', 'other-deployment')).toString('base64') | ||
|
|
||
| expect(validateAuthToken(tampered, 'other-deployment', 'encrypted-password')).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects expired signed tokens', () => { | ||
| const expiredTimestamp = Date.now() - 24 * 60 * 60 * 1000 - 1 | ||
| const token = signedV2Token('dep_test', 'encrypted-password', expiredTimestamp) | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'encrypted-password')).toBe(false) | ||
| }) | ||
|
Comment on lines
+108
to
+113
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The expiry suite tests a timestamp 1 ms past 24 h but has no counterpart asserting that a token with |
||
|
|
||
| it('accepts signed legacy tokens during the 24 hour cookie window', () => { | ||
| const token = signedLegacyToken('dep_test', 'encrypted-password') | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'encrypted-password')).toBe(true) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,19 @@ import { isDev } from '@/lib/core/config/feature-flags' | |||||||||||||||||||||||||||
| * endpoints lives in proxy.ts as the single source of truth. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function signPayload(payload: string): string { | ||||||||||||||||||||||||||||
| const AUTH_TOKEN_VERSION = 'v2' | ||||||||||||||||||||||||||||
| const AUTH_TOKEN_TTL_MS = 24 * 60 * 60 * 1000 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function passwordBinding(encryptedPassword?: string | null): string { | ||||||||||||||||||||||||||||
| if (!encryptedPassword) return '' | ||||||||||||||||||||||||||||
| return sha256Hex(encryptedPassword) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function signPayload(payload: string, encryptedPassword?: string | null): string { | ||||||||||||||||||||||||||||
| return hmacSha256Hex(`${payload}:${passwordBinding(encryptedPassword)}`, env.BETTER_AUTH_SECRET) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function signLegacyPayload(payload: string): string { | ||||||||||||||||||||||||||||
| return hmacSha256Hex(payload, env.BETTER_AUTH_SECRET) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -25,15 +37,22 @@ function generateAuthToken( | |||||||||||||||||||||||||||
| type: string, | ||||||||||||||||||||||||||||
| encryptedPassword?: string | null | ||||||||||||||||||||||||||||
| ): string { | ||||||||||||||||||||||||||||
| const payload = `${deploymentId}:${type}:${Date.now()}:${passwordSlot(encryptedPassword)}` | ||||||||||||||||||||||||||||
| const sig = signPayload(payload) | ||||||||||||||||||||||||||||
| const payload = `${AUTH_TOKEN_VERSION}:${deploymentId}:${type}:${Date.now()}` | ||||||||||||||||||||||||||||
| const sig = signPayload(payload, encryptedPassword) | ||||||||||||||||||||||||||||
| return Buffer.from(`${payload}:${sig}`).toString('base64') | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function hasValidTimestamp(timestamp: string): boolean { | ||||||||||||||||||||||||||||
| const createdAt = Number.parseInt(timestamp, 10) | ||||||||||||||||||||||||||||
| if (!Number.isFinite(createdAt)) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return Date.now() - createdAt <= AUTH_TOKEN_TTL_MS | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+45
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Validates an HMAC-signed authentication token for a deployment (chat or form). | ||||||||||||||||||||||||||||
| * Includes a password-derived slot so changing the deployment password immediately | ||||||||||||||||||||||||||||
| * invalidates existing sessions. | ||||||||||||||||||||||||||||
| * The signature is bound to the current encrypted password so changing a | ||||||||||||||||||||||||||||
| * deployment password immediately invalidates existing sessions. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function validateAuthToken( | ||||||||||||||||||||||||||||
| token: string, | ||||||||||||||||||||||||||||
|
|
@@ -48,25 +67,32 @@ export function validateAuthToken( | |||||||||||||||||||||||||||
| const payload = decoded.slice(0, lastColon) | ||||||||||||||||||||||||||||
| const sig = decoded.slice(lastColon + 1) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const expectedSig = signPayload(payload) | ||||||||||||||||||||||||||||
| if (!safeCompare(sig, expectedSig)) { | ||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||
| const parts = payload.split(':') | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (parts[0] === AUTH_TOKEN_VERSION) { | ||||||||||||||||||||||||||||
| if (parts.length !== 4) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const expectedSig = signPayload(payload, encryptedPassword) | ||||||||||||||||||||||||||||
| if (!safeCompare(sig, expectedSig)) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const [_version, storedId, _type, timestamp] = parts | ||||||||||||||||||||||||||||
| if (storedId !== deploymentId) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return hasValidTimestamp(timestamp) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const parts = payload.split(':') | ||||||||||||||||||||||||||||
| if (parts.length < 4) return false | ||||||||||||||||||||||||||||
| const [storedId, _type, timestamp, storedPwSlot] = parts | ||||||||||||||||||||||||||||
| if (parts.length !== 4) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const expectedSig = signLegacyPayload(payload) | ||||||||||||||||||||||||||||
| if (!safeCompare(sig, expectedSig)) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const [storedId, _type, timestamp, storedPwSlot] = parts | ||||||||||||||||||||||||||||
| if (storedId !== deploymentId) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const expectedPwSlot = passwordSlot(encryptedPassword) | ||||||||||||||||||||||||||||
| if (storedPwSlot !== expectedPwSlot) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const createdAt = Number.parseInt(timestamp) | ||||||||||||||||||||||||||||
| const expireTime = 24 * 60 * 60 * 1000 | ||||||||||||||||||||||||||||
| if (Date.now() - createdAt > expireTime) return false | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||
| return hasValidTimestamp(timestamp) | ||||||||||||||||||||||||||||
| } catch (_e) { | ||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR explicitly notes
npx vitest run …could not be started due to missing workspace dependencies. The test helpers (signedLegacyToken,signedV2Token) faithfully replicate the production HMAC logic (UTF-8 encoding matchessha256Hex/hmacSha256Hexin@sim/security), so the tests are structurally correct. However, for a security-hardening change the lack of a green CI run is a gap worth closing before merge, especially for the newrejects expired signed tokensandrejects tampered signed token payloadscases.