diff --git a/src/lib/browserBinding.ts b/src/lib/browserBinding.ts new file mode 100644 index 0000000..2ee6f78 --- /dev/null +++ b/src/lib/browserBinding.ts @@ -0,0 +1,68 @@ +/** + * Browser binding via stable per-browser `__Host-oauth_browser` cookie. + * + * Stores a SHA-256 hash of the cookie in the CSRF state; verified constant-time + * at callback before any code exchange. Only the hash leaves the server. + * `__Host-` requires `Secure` + `Path=/` + no `Domain` (HTTPS only). + */ + +import { createHash, randomBytes, timingSafeEqual } from 'node:crypto'; +import type { Request } from '../types.ts'; + +export const BROWSER_SECRET_COOKIE_NAME = '__Host-oauth_browser'; + +/** Rolling 7-day lifetime; refreshed on every flow initiation. */ +const BROWSER_SECRET_MAX_AGE_S = 7 * 24 * 60 * 60; + +export function generateBrowserSecret(): string { + return randomBytes(32).toString('base64url'); +} + +/** SHA-256 (base64url) of the browser secret. Only the hash is stored server-side. */ +export function hashBrowserSecret(secret: string): string { + return createHash('sha256').update(secret).digest('base64url'); +} + +/** Build Set-Cookie value; `__Host-` requires `Secure` + `Path=/` + no `Domain`. Refreshes Max-Age. */ +export function buildBrowserSecretCookie(secret: string): string { + return `${BROWSER_SECRET_COOKIE_NAME}=${secret}; Max-Age=${BROWSER_SECRET_MAX_AGE_S}; Path=/; Secure; HttpOnly; SameSite=Lax`; +} + +/** Extract Cookie header, joining Harper 4 Header array crumbs; falls back to plain-object for test doubles. */ +function readCookieHeader(request: Request | undefined): string | undefined { + const headers = request?.headers as any; + if (!headers) return undefined; + // Harper 4 runtime: Headers object with .get() + if (typeof headers.get === 'function') { + const raw = headers.get('cookie'); + if (raw == null) return undefined; + return Array.isArray(raw) ? raw.join('; ') : String(raw); + } + // Test doubles: plain object + const raw = headers.cookie; + if (raw == null) return undefined; + return Array.isArray(raw) ? raw.join('; ') : String(raw); +} + +/** Parse the `__Host-oauth_browser` cookie value from the request. */ +export function readBrowserSecret(request: Request | undefined): string | undefined { + const header = readCookieHeader(request); + if (typeof header !== 'string' || !header) return undefined; + for (const part of header.split(';')) { + const eq = part.indexOf('='); + if (eq === -1) continue; + if (part.slice(0, eq).trim() === BROWSER_SECRET_COOKIE_NAME) { + const value = part.slice(eq + 1).trim(); + return /^[A-Za-z0-9_-]{1,64}$/.test(value) ? value : undefined; + } + } + return undefined; +} + +/** Constant-time check that `secret` hashes to `expectedHash`. */ +export function browserSecretMatches(secret: string | undefined, expectedHash: string | undefined): boolean { + if (typeof secret !== 'string' || typeof expectedHash !== 'string' || !secret || !expectedHash) return false; + const actual = Buffer.from(hashBrowserSecret(secret)); + const expected = Buffer.from(expectedHash); + return actual.length === expected.length && timingSafeEqual(actual, expected); +} diff --git a/src/lib/handlers.ts b/src/lib/handlers.ts index bb8784c..87bff04 100644 --- a/src/lib/handlers.ts +++ b/src/lib/handlers.ts @@ -18,6 +18,13 @@ import type { OnLoginResultNeedsConfirmation, } from '../types.ts'; import type { HookManager } from './hookManager.ts'; +import { + browserSecretMatches, + buildBrowserSecretCookie, + generateBrowserSecret, + hashBrowserSecret, + readBrowserSecret, +} from './browserBinding.ts'; /** * Sanitize a redirect parameter to prevent open redirect attacks @@ -128,12 +135,17 @@ export async function handleLogin( const referer = request.headers?.referer ? sanitizeRedirect(request.headers.referer) : undefined; const originalUrl = redirectParam || referer || config.postLoginRedirect || '/'; + // Browser binding: read or mint the stable __Host- cookie; store its hash in the CSRF state. + const existingSecret = readBrowserSecret(request); + const browserSecret = existingSecret ?? generateBrowserSecret(); + // Generate CSRF token with metadata // Bind token to provider to prevent cross-provider CSRF attacks const csrfToken = await provider.generateCSRFToken({ originalUrl, sessionId: request.session?.id, providerName, // Bind state token to this provider + browserNonceHash: hashBrowserSecret(browserSecret), }); // Build authorization URL with CSRF token as state parameter @@ -144,7 +156,8 @@ export async function handleLogin( return { status: 302, headers: { - Location: authUrl, + 'Location': authUrl, + 'Set-Cookie': buildBrowserSecretCookie(browserSecret), }, }; } @@ -167,20 +180,19 @@ export async function handleCallback( const error = target.get?.('error'); const errorDescription = target.get?.('error_description'); - // Handle OAuth errors from provider - if (error) { - logger?.error?.(`OAuth error: ${error} - ${errorDescription}`); - const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'oauth_failed', reason: error }); - return { - status: 302, - headers: { - Location: errorUrl, - }, - }; - } - - // Validate parameters - if (!code || !state) { + // No state token: handle stateless errors and missing params without token verification. + if (!state) { + if (error) { + // JSON.stringify: CRLF-safe logging of browser-controlled params (CWE-117). + logger?.error?.(`OAuth error: ${JSON.stringify(error)} - ${JSON.stringify(errorDescription)}`); + const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'oauth_failed', reason: error }); + return { + status: 302, + headers: { + Location: errorUrl, + }, + }; + } logger?.warn?.('Missing required OAuth callback parameters'); const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'invalid_request' }); return { @@ -191,7 +203,7 @@ export async function handleCallback( }; } - // Verify CSRF token + // Consume the single-use state before handling any upstream error or binding check. const tokenData = await provider.verifyCSRFToken(state); if (!tokenData) { logger?.warn?.('Invalid or expired CSRF token'); @@ -247,6 +259,52 @@ export async function handleCallback( }; } + // Browser binding: verify the __Host- cookie hash before the code exchange; absent hash passes (pre-upgrade tokens). + if (tokenData.browserNonceHash) { + if (!browserSecretMatches(readBrowserSecret(request), tokenData.browserNonceHash)) { + logger?.warn?.(`OAuth callback: login browser binding mismatch (provider '${providerName}')`); + const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { + error: 'auth_failed', + reason: 'csrf', + }); + return { + status: 302, + headers: { + Location: errorUrl, + }, + }; + } + } + + // Handle provider errors after state consumption and binding verification. + if (error) { + // JSON.stringify: CRLF-safe logging of browser-controlled params (CWE-117). + logger?.error?.(`OAuth error: ${JSON.stringify(error)} - ${JSON.stringify(errorDescription)}`); + const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { + error: 'oauth_failed', + reason: error, + }); + return { + status: 302, + headers: { + Location: errorUrl, + }, + }; + } + + if (!code) { + logger?.warn?.('Missing required OAuth callback parameters'); + const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', { + error: 'invalid_request', + }); + return { + status: 302, + headers: { + Location: errorUrl, + }, + }; + } + try { // Exchange code for tokens const tokenResponse = await provider.exchangeCodeForToken(code, config.redirectUri || ''); @@ -281,7 +339,9 @@ export async function handleCallback( if (isGatedLoginOutcome(hookData)) { const denied = hookData.status === 'denied'; const reason = denied ? hookData.error : undefined; - logger?.info?.(`OAuth login ${denied ? 'denied' : 'deferred'} by onLogin hook for user: ${user.username}`); + logger?.info?.( + `OAuth login ${denied ? 'denied' : 'deferred'} by onLogin hook for user: ${JSON.stringify(user.username)}` + ); if (hookData.redirect) { return { status: 302, headers: { Location: resolveHookRedirect(hookData.redirect) } }; } @@ -354,7 +414,7 @@ export async function handleCallback( } logger?.info?.( - `OAuth login successful for user: ${user.username}${tokenResponse.expires_in ? `, token expires in ${tokenResponse.expires_in}s` : ', token does not expire'}` + `OAuth login successful for user: ${JSON.stringify(user.username)}${tokenResponse.expires_in ? `, token expires in ${tokenResponse.expires_in}s` : ', token does not expire'}` ); } else { logger?.warn?.('No session available for OAuth user'); diff --git a/test/lib/browserBinding.test.js b/test/lib/browserBinding.test.js new file mode 100644 index 0000000..b1214d1 --- /dev/null +++ b/test/lib/browserBinding.test.js @@ -0,0 +1,135 @@ +// Tests for the browser-binding cookie helpers. +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { + BROWSER_SECRET_COOKIE_NAME, + browserSecretMatches, + buildBrowserSecretCookie, + generateBrowserSecret, + hashBrowserSecret, + readBrowserSecret, +} from '../../dist/lib/browserBinding.js'; + +describe('browserBinding', () => { + it('generates unique url-safe browser secrets', () => { + assert.notEqual(generateBrowserSecret(), generateBrowserSecret()); + // base64url — no +, /, or = padding + assert.match(generateBrowserSecret(), /^[A-Za-z0-9_-]+$/); + }); + + it('hashBrowserSecret produces consistent SHA-256 base64url output', () => { + const secret = 'test-secret'; + assert.equal(hashBrowserSecret(secret), hashBrowserSecret(secret)); + assert.notEqual(hashBrowserSecret(secret), hashBrowserSecret('other')); + assert.match(hashBrowserSecret(secret), /^[A-Za-z0-9_-]+$/); + }); + + it('buildBrowserSecretCookie includes all required cookie attributes', () => { + const cookie = buildBrowserSecretCookie('mysecret'); + assert.ok(cookie.startsWith(`${BROWSER_SECRET_COOKIE_NAME}=mysecret`)); + assert.ok(cookie.includes('Path=/')); + assert.ok(cookie.includes('Secure')); + assert.ok(cookie.includes('HttpOnly')); + assert.ok(cookie.includes('SameSite=Lax')); + assert.ok(cookie.includes('Max-Age=')); + }); + + describe('readBrowserSecret', () => { + it('reads the secret from a plain-object headers cookie', () => { + const secret = generateBrowserSecret(); + const request = { headers: { cookie: `${BROWSER_SECRET_COOKIE_NAME}=${secret}` } }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('reads the secret when other cookies precede it', () => { + const secret = 'abc123'; + const request = { + headers: { cookie: `other=value; ${BROWSER_SECRET_COOKIE_NAME}=${secret}; trailing=x` }, + }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('reads the secret via .get() (Harper 4 runtime Headers shape)', () => { + const secret = 'runtime-secret'; + const request = { + headers: { + get: (name) => (name === 'cookie' ? `${BROWSER_SECRET_COOKIE_NAME}=${secret}` : null), + }, + }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('reads the secret when .get() returns an array of cookie crumbs (HTTP/2 crumbling)', () => { + // Harper 4 Headers can split Cookie across array crumbs — binding cookie may be in any. + const secret = 'crumbled-secret'; + const request = { + headers: { + get: (name) => + name === 'cookie' ? ['session=abc', `${BROWSER_SECRET_COOKIE_NAME}=${secret}`, 'other=1'] : null, + }, + }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('reads the secret from a plain-object headers.cookie array', () => { + const secret = 'plain-array-secret'; + const request = { + headers: { cookie: [`other=x`, `${BROWSER_SECRET_COOKIE_NAME}=${secret}`] }, + }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('returns undefined when the cookie is absent', () => { + assert.equal(readBrowserSecret({ headers: { cookie: 'other=value' } }), undefined); + assert.equal(readBrowserSecret({ headers: {} }), undefined); + assert.equal(readBrowserSecret(undefined), undefined); + }); + + it('returns the secret for a valid 43-char base64url value', () => { + const secret = generateBrowserSecret(); + assert.equal(secret.length, 43); + assert.match(secret, /^[A-Za-z0-9_-]+$/); + const request = { headers: { cookie: `${BROWSER_SECRET_COOKIE_NAME}=${secret}` } }; + assert.equal(readBrowserSecret(request), secret); + }); + + it('returns undefined for a malformed cookie value (contains disallowed chars)', () => { + const malformed = 'abc!@#$%^&*()malformed value with spaces'; + const request = { headers: { cookie: `${BROWSER_SECRET_COOKIE_NAME}=${malformed}` } }; + assert.equal(readBrowserSecret(request), undefined); + }); + + it('returns undefined for an over-length cookie value (65+ chars)', () => { + const overLength = 'a'.repeat(65); + const request = { headers: { cookie: `${BROWSER_SECRET_COOKIE_NAME}=${overLength}` } }; + assert.equal(readBrowserSecret(request), undefined); + }); + }); + + describe('browserSecretMatches', () => { + it('returns true for a matching secret and hash', () => { + const secret = generateBrowserSecret(); + assert.ok(browserSecretMatches(secret, hashBrowserSecret(secret))); + }); + + it('returns false for a mismatched secret', () => { + const secret = generateBrowserSecret(); + assert.ok(!browserSecretMatches('wrong-secret', hashBrowserSecret(secret))); + }); + + it('returns false when secret or hash is undefined/empty', () => { + assert.ok(!browserSecretMatches(undefined, hashBrowserSecret('x'))); + assert.ok(!browserSecretMatches('x', undefined)); + assert.ok(!browserSecretMatches('', 'anyhash')); + }); + + it('returns false without throwing when either argument is a non-string (number, object)', () => { + assert.doesNotThrow(() => { + assert.ok(!browserSecretMatches(/** @type {any} */ (42), hashBrowserSecret('x'))); + assert.ok(!browserSecretMatches('x', /** @type {any} */ (42))); + assert.ok(!browserSecretMatches(/** @type {any} */ ({ valueOf: () => 'x' }), hashBrowserSecret('x'))); + assert.ok(!browserSecretMatches(/** @type {any} */ (null), hashBrowserSecret('x'))); + }); + }); + }); +}); diff --git a/test/lib/handlers.test.js b/test/lib/handlers.test.js index 36dc2a0..d963c4c 100644 --- a/test/lib/handlers.test.js +++ b/test/lib/handlers.test.js @@ -5,6 +5,11 @@ import { describe, it, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { handleLogin, handleCallback, handleLogout, handleUserInfo, handleTestPage } from '../../dist/lib/handlers.js'; +import { + BROWSER_SECRET_COOKIE_NAME, + buildBrowserSecretCookie, + hashBrowserSecret, +} from '../../dist/lib/browserBinding.js'; import { createMockFn, createMockLogger } from '../helpers/mockFn.js'; describe('OAuth Handlers', () => { @@ -151,6 +156,41 @@ describe('OAuth Handlers', () => { const csrfCall = mockProvider.generateCSRFToken.mock.calls[0]; assert.equal(csrfCall.arguments[0].sessionId, 'session-123'); }); + + it('mints a browser-binding secret: hash in the state token, stable secret in a __Host- cookie', async () => { + const result = await handleLogin(mockRequest, mockTarget, mockProvider, mockConfig, 'test-provider', mockLogger); + + const meta = mockProvider.generateCSRFToken.mock.calls[0].arguments[0]; + assert.ok(meta.browserNonceHash, 'secret hash stored in the state token'); + + const setCookie = result.headers['Set-Cookie']; + const [pair, ...attrs] = setCookie.split('; '); + const eq = pair.indexOf('='); + assert.equal(pair.slice(0, eq), BROWSER_SECRET_COOKIE_NAME, 'stable cookie name'); + assert.equal( + hashBrowserSecret(pair.slice(eq + 1)), + meta.browserNonceHash, + 'cookie value hashes to the bound hash' + ); + for (const attr of ['Path=/', 'Secure', 'HttpOnly', 'SameSite=Lax']) { + assert.ok(attrs.includes(attr), `cookie carries ${attr}`); + } + }); + + it('reuses an existing browser secret cookie rather than generating a new one', async () => { + const existingSecret = 'existing-browser-secret-abc'; + mockRequest.headers.cookie = buildBrowserSecretCookie(existingSecret).split(';')[0]; + + const result = await handleLogin(mockRequest, mockTarget, mockProvider, mockConfig, 'test-provider', mockLogger); + + const meta = mockProvider.generateCSRFToken.mock.calls[0].arguments[0]; + assert.equal(meta.browserNonceHash, hashBrowserSecret(existingSecret), 'reuses existing secret hash'); + + // The Set-Cookie refreshes the Max-Age on the same secret + const setCookie = result.headers['Set-Cookie']; + const [pair] = setCookie.split('; '); + assert.equal(pair.slice(pair.indexOf('=') + 1), existingSecret, 'cookie value unchanged'); + }); }); describe('handleCallback', () => { @@ -215,6 +255,53 @@ describe('OAuth Handlers', () => { assert.equal(result.headers.Location, '/dashboard?error=oauth_failed&reason=access_denied'); }); + it('error= callback with a present state token consumes the token before returning (Fix 2)', async () => { + // state present: token must be consumed (single-use) even on error. + mockTarget.get = createMockFn((key) => { + if (key === 'error') return 'access_denied'; + if (key === 'state') return 'csrf-token-123'; + return null; + }); + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + // verifyCSRFToken must have been called (state consumed / single-use enforced) + assert.equal(mockProvider.verifyCSRFToken.mock.calls.length, 1, 'state token must be consumed on error path'); + assert.equal(result.status, 302); + assert.ok(result.headers.Location.includes('error=oauth_failed'), 'error code surfaced'); + assert.ok(result.headers.Location.includes('reason=access_denied'), 'reason surfaced'); + }); + + it('error= callback without state does not attempt token verification', async () => { + // No state: no token to consume; must still redirect with the error reason. + mockTarget.get = createMockFn((key) => { + if (key === 'error') return 'server_error'; + return null; + }); + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + assert.equal(mockProvider.verifyCSRFToken.mock.calls.length, 0, 'no token to consume when state absent'); + assert.equal(result.status, 302); + assert.ok(result.headers.Location.includes('error=oauth_failed')); + }); + it('should handle missing code parameter', async () => { mockTarget.get = createMockFn(() => null); @@ -806,6 +893,50 @@ describe('OAuth Handlers', () => { }); }); + describe('handleCallback — CRLF-safe username logging (Fix 3)', () => { + const callbackWith = (request, target) => + handleCallback(request, target, mockProvider, mockConfig, mockHookManager, 'test-provider', mockLogger); + + it('CRLF in username is JSON-encoded on the denied-login log line', async () => { + const maliciousUsername = 'admin\r\nX-Injected: evil'; + mockProvider.mapUserToHarper = createMockFn(() => ({ + username: maliciousUsername, + role: 'user', + email: 'x@x.com', + name: 'X', + provider: 'test', + })); + mockHookManager.callOnLogin = createMockFn(async () => ({ status: 'denied' })); + + await callbackWith(mockRequest, mockTarget); + + for (const call of mockLogger.info.mock.calls) { + const msg = String(call.arguments[0]); + assert.ok(!msg.includes('\r'), 'CR must not appear raw in log output'); + assert.ok(!msg.includes('\n'), 'LF must not appear raw in log output'); + } + }); + + it('CRLF in username is JSON-encoded on the successful-login log line', async () => { + const maliciousUsername = 'user\r\nX-Injected: evil'; + mockProvider.mapUserToHarper = createMockFn(() => ({ + username: maliciousUsername, + role: 'user', + email: 'u@u.com', + name: 'U', + provider: 'test', + })); + + await callbackWith(mockRequest, mockTarget); + + for (const call of mockLogger.info.mock.calls) { + const msg = String(call.arguments[0]); + assert.ok(!msg.includes('\r'), 'CR must not appear raw in log output'); + assert.ok(!msg.includes('\n'), 'LF must not appear raw in log output'); + } + }); + }); + describe('handleLogout', () => { it('should clear session data', async () => { // Add delete method mock to session @@ -996,4 +1127,95 @@ describe('OAuth Handlers', () => { assert.equal(result.headers.Location, '/dashboard'); }); }); + + describe('handleCallback — login browser binding (GHSA-xf67)', () => { + const SECRET = 'login-binding-secret'; + + beforeEach(() => { + // Logged-out flow: no sessionId in the token, so only browser binding applies. + mockProvider.verifyCSRFToken = createMockFn(async () => ({ + originalUrl: '/dashboard', + timestamp: Date.now(), + providerName: 'test-provider', + browserNonceHash: hashBrowserSecret(SECRET), + })); + }); + + it('completes when the callback arrives in the browser that initiated the login', async () => { + mockRequest.headers.cookie = buildBrowserSecretCookie(SECRET).split(';')[0]; + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + assert.equal(result.status, 302); + assert.equal(result.headers.Location, '/dashboard'); + assert.equal(mockProvider.exchangeCodeForToken.mock.calls.length, 1); + }); + + it('rejects when the binding cookie is missing — attacker-minted state in a victim browser', async () => { + delete mockRequest.headers.cookie; + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + assert.equal(result.status, 302); + assert.equal(result.headers.Location, '/dashboard?error=auth_failed&reason=csrf'); + // Rejected before any upstream call or session write. + assert.equal(mockProvider.exchangeCodeForToken.mock.calls.length, 0); + assert.equal(mockRequest.session.update.mock.calls.length, 0); + }); + + it('rejects when the browser-secret cookie does not hash-match', async () => { + mockRequest.headers.cookie = buildBrowserSecretCookie('some-other-browser-secret').split(';')[0]; + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + assert.equal(result.headers.Location, '/dashboard?error=auth_failed&reason=csrf'); + assert.equal(mockProvider.exchangeCodeForToken.mock.calls.length, 0); + }); + + it('tolerates state tokens without a browserNonceHash (pre-upgrade in-flight logins)', async () => { + mockProvider.verifyCSRFToken = createMockFn(async () => ({ + originalUrl: '/dashboard', + timestamp: Date.now(), + providerName: 'test-provider', + })); + delete mockRequest.headers.cookie; + + const result = await handleCallback( + mockRequest, + mockTarget, + mockProvider, + mockConfig, + mockHookManager, + 'test-provider', + mockLogger + ); + + assert.equal(result.status, 302); + assert.equal(result.headers.Location, '/dashboard'); + }); + }); });