diff --git a/src/authkit-callback-route.spec.ts b/src/authkit-callback-route.spec.ts index 62e2f97..1059bb9 100644 --- a/src/authkit-callback-route.spec.ts +++ b/src/authkit-callback-route.spec.ts @@ -132,6 +132,30 @@ describe('authLoader', () => { expect(authenticateWithCode).not.toHaveBeenCalled(); }); + // Regression test for SEC-1309: an attacker who obtains a leaked callback + // URL (`?code=…&state=…`) must not be able to satisfy the double-submit + // check by replaying the URL `state` as the cookie value. The PKCE verifier + // lives only in the HttpOnly cookie, so the replayed state (which carries no + // verifier) must be rejected before any code exchange. + it('rejects a replayed URL state used as the PKCE cookie value (SEC-1309)', async () => { + const { getPKCECookieNameForState } = await import('./pkce.js'); + const attackerCookie = `${getPKCECookieNameForState(sealedState)}=${sealedState}`; + request = createRequestWithCookieAndParams(new Request('http://example.com/callback'), attackerCookie, { + code: 'test-code', + state: sealedState, + }); + + const response = (await loader({ + request, + params: {}, + context: {}, + } as LoaderFunctionArgs)) as DataWithResponseInit; + + expect(isDataWithResponseInit(response)).toBeTruthy(); + expect(response?.init?.status).toBe(500); + expect(authenticateWithCode).not.toHaveBeenCalled(); + }); + it('clears the PKCE cookie on authentication failure', async () => { authenticateWithCode.mockRejectedValue(new Error('Auth failed')); const response = (await loader({ diff --git a/src/authkit-callback-route.ts b/src/authkit-callback-route.ts index 14fc56d..2719c26 100644 --- a/src/authkit-callback-route.ts +++ b/src/authkit-callback-route.ts @@ -1,7 +1,7 @@ import { LoaderFunctionArgs, data, redirect } from 'react-router'; import { getConfig } from './config.js'; import { HandleAuthOptions } from './interfaces.js'; -import { getPKCECookieString, getStateFromPKCECookieValue, readPKCECookie } from './pkce.js'; +import { getPKCECookieString, getStateFromUrlValue, getVerifierFromPKCECookieValue, readPKCECookie } from './pkce.js'; import { sanitizeReturnPathname } from './return-pathname.js'; import { encryptSession } from './session.js'; import { configureSessionStorage } from './sessionStorage.js'; @@ -37,26 +37,31 @@ export function authLoader(options: HandleAuthOptions = {}) { const pkceCookieValue = readPKCECookie(request.headers.get('Cookie'), state); - // CSRF verification (double-submit cookie): both the cookie and the URL - // state must be present and identical. A missing cookie means either - // the browser never started this flow (forged link) or the cookie has - // been cleared (expired / tampered). + // The PKCE verifier lives only in this HttpOnly cookie, never in the URL + // state. A missing cookie means either the browser never started this + // flow (forged / leaked link) or the cookie was cleared (expired / + // tampered) — in every case we cannot recover the verifier. if (!pkceCookieValue) { throw new Error( 'Auth cookie missing — cannot verify OAuth state. Ensure Set-Cookie headers are propagated on the redirect that started this flow.', ); } - if (state !== pkceCookieValue) { + // Recover the verifier from the cookie and the flow metadata from the + // URL state. Both are sealed under the app's cookie password; replaying + // the URL `state` as the cookie fails here because it unseals to the + // state payload (no `codeVerifier`). + const { nonce: cookieNonce, codeVerifier } = await getVerifierFromPKCECookieValue(pkceCookieValue); + const { nonce: stateNonce, customState, returnPathname: returnPathnameState } = await getStateFromUrlValue(state); + + // Binding check: the secret verifier cookie must carry the same nonce as + // the URL state. Because the verifier is only ever presentable by the + // browser that initiated the flow, possession of the callback URL alone + // can never satisfy this. + if (cookieNonce !== stateNonce) { throw new Error('OAuth state mismatch'); } - const { - codeVerifier, - customState, - returnPathname: returnPathnameState, - } = await getStateFromPKCECookieValue(pkceCookieValue); - const { accessToken, refreshToken, user, impersonator, oauthTokens, authenticationMethod, organizationId } = await getWorkOS().userManagement.authenticateWithCode({ clientId: getConfig('clientId'), diff --git a/src/get-authorization-url.spec.ts b/src/get-authorization-url.spec.ts index 5b657ba..20e62df 100644 --- a/src/get-authorization-url.spec.ts +++ b/src/get-authorization-url.spec.ts @@ -2,7 +2,7 @@ import { unsealData } from 'iron-session'; import { getAuthorizationUrl } from './get-authorization-url.js'; import { getConfig } from './config.js'; import { getPKCECookieNameForState, PKCE_COOKIE_NAME } from './pkce.js'; -import type { State } from './interfaces.js'; +import type { PKCECookiePayload, State } from './interfaces.js'; describe('getAuthorizationUrl', () => { it('generates a valid WorkOS authorization URL with PKCE parameters', async () => { @@ -16,24 +16,46 @@ describe('getAuthorizationUrl', () => { expect(url).toContain('code_challenge_method=S256'); }); - it('seals return-trip state into the OAuth state parameter', async () => { + it('seals return-trip state into the OAuth state parameter without the code verifier', async () => { const { url } = await getAuthorizationUrl({ returnPathname: '/dashboard' }); const parsed = new URL(url); const state = parsed.searchParams.get('state'); expect(state).toBeTruthy(); - const unsealed = await unsealData(state!, { password: getConfig('cookiePassword') }); + const unsealed = await unsealData(state!, { + password: getConfig('cookiePassword'), + }); expect(unsealed.returnPathname).toBe('/dashboard'); - expect(unsealed.codeVerifier).toEqual(expect.any(String)); expect(unsealed.nonce).toEqual(expect.any(String)); + // The PKCE secret must never travel in the URL state. + expect(unsealed.codeVerifier).toBeUndefined(); }); - it('emits a flow-specific PKCE cookie tied to the sealed state', async () => { + it('keeps the code verifier only in the HttpOnly cookie, not the URL', async () => { const { url, headers } = await getAuthorizationUrl(); const state = new URL(url).searchParams.get('state')!; const setCookie = headers['Set-Cookie']; - expect(setCookie).toContain(`${getPKCECookieNameForState(state)}=${state}`); + const cookieName = getPKCECookieNameForState(state); + const cookieValue = setCookie.slice(`${cookieName}=`.length).split(';')[0]; + + // The cookie value is a distinct sealed blob, NOT a copy of the URL state. + expect(setCookie).toContain(`${cookieName}=`); + expect(cookieValue).not.toBe(state); + + const stateNonce = (await unsealData(state, { password: getConfig('cookiePassword') })).nonce; + const cookie = await unsealData(cookieValue, { password: getConfig('cookiePassword') }); + expect(cookie.codeVerifier).toEqual(expect.any(String)); + // The cookie is bound to the URL state via the shared nonce. + expect(cookie.nonce).toBe(stateNonce); + }); + + it('emits a flow-specific PKCE cookie with the expected attributes', async () => { + const { url, headers } = await getAuthorizationUrl(); + + const state = new URL(url).searchParams.get('state')!; + const setCookie = headers['Set-Cookie']; + expect(setCookie).toContain(`${getPKCECookieNameForState(state)}=`); expect(setCookie).toContain('Path=/'); expect(setCookie).toContain('HttpOnly'); expect(setCookie).toContain('SameSite=Lax'); diff --git a/src/get-authorization-url.ts b/src/get-authorization-url.ts index e771f4f..0afd527 100644 --- a/src/get-authorization-url.ts +++ b/src/get-authorization-url.ts @@ -1,6 +1,6 @@ import { sealData } from 'iron-session'; import { getConfig } from './config.js'; -import type { GetAuthURLOptions, GetAuthURLResult, State } from './interfaces.js'; +import type { GetAuthURLOptions, GetAuthURLResult, PKCECookiePayload, State } from './interfaces.js'; import { getPKCECookieString } from './pkce.js'; import { sanitizeReturnPathname } from './return-pathname.js'; import { getWorkOS } from './workos.js'; @@ -18,14 +18,19 @@ import { getWorkOS } from './workos.js'; * * Internally this: * 1. Generates a PKCE verifier / challenge pair (RFC 7636, S256). - * 2. Seals `{ nonce, codeVerifier, customState, returnPathname }` with - * iron-session under the configured cookie password. - * 3. Sends the sealed value as the OAuth `state` parameter. - * 4. Sets an HTTP-only, flow-specific cookie (`wos-auth-verifier-`) - * with the same sealed value so the callback can: - * - prove the response came from a flow this browser initiated - * (CSRF: `cookie === state`); and - * - recover the `codeVerifier` to complete the PKCE exchange. + * 2. Seals `{ nonce, customState, returnPathname }` (no secret) and sends it + * as the OAuth `state` parameter. + * 3. Seals `{ nonce, codeVerifier }` separately and sets it as an HTTP-only, + * flow-specific cookie (`wos-auth-verifier-`). The `codeVerifier` + * lives only in this cookie, never in the URL, so the callback can: + * - prove the response came from a flow this browser initiated by + * matching the cookie's `nonce` against the URL state's `nonce`; and + * - recover the `codeVerifier` (from the cookie) to complete the PKCE + * exchange. + * + * Because the verifier never travels in the URL, possession of a leaked + * callback URL alone cannot complete the exchange — the initiating browser's + * HttpOnly cookie is required. */ export async function getAuthorizationUrl(options: GetAuthURLOptions = {}): Promise { const { @@ -40,10 +45,10 @@ export async function getAuthorizationUrl(options: GetAuthURLOptions = {}): Prom } = options; const pkce = await getWorkOS().pkce.generate(); + const nonce = crypto.randomUUID(); const state = { - nonce: crypto.randomUUID(), - codeVerifier: pkce.codeVerifier, + nonce, customState, // Sanitize before sealing so a hostile caller can't plant a malicious // return target (absolute URL, CRLF smuggle, dot-segment traversal, etc.) @@ -58,6 +63,15 @@ export async function getAuthorizationUrl(options: GetAuthURLOptions = {}): Prom ttl: 600, }); + // The PKCE verifier is the secret that binds the authorization code to this + // browser. It lives ONLY in the HttpOnly cookie, never in the URL state, so + // a leaked callback URL can't be exchanged without the initiating browser's + // cookie. `nonce` ties it back to the URL state. + const sealedVerifier = await sealData({ nonce, codeVerifier: pkce.codeVerifier } satisfies PKCECookiePayload, { + password: getConfig('cookiePassword'), + ttl: 600, + }); + const url = getWorkOS().userManagement.getAuthorizationUrl({ provider: 'authkit', clientId: getConfig('clientId'), @@ -73,6 +87,6 @@ export async function getAuthorizationUrl(options: GetAuthURLOptions = {}): Prom return { url, - headers: { 'Set-Cookie': getPKCECookieString(sealedState, { request, redirectUri }) }, + headers: { 'Set-Cookie': getPKCECookieString(sealedState, { value: sealedVerifier, request, redirectUri }) }, }; } diff --git a/src/interfaces.ts b/src/interfaces.ts index 58a7d6a..d9211a1 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -149,19 +149,32 @@ export interface GetAuthURLResult { } /** - * Sealed state stored in the PKCE cookie and round-tripped through WorkOS as - * the OAuth `state` parameter. `codeVerifier` is the PKCE secret that binds - * the authorization code to this browser session. + * Sealed value round-tripped through WorkOS as the OAuth `state` URL + * parameter. It carries no secret: the PKCE `codeVerifier` deliberately lives + * only in the HttpOnly cookie (see `PKCECookieSchema`), never in the URL, so a + * leaked callback URL cannot complete the code exchange on its own. `nonce` + * binds the state to the verifier cookie set on the initiating browser. */ export const StateSchema = v.object({ nonce: v.string(), customState: v.optional(v.string()), returnPathname: v.optional(v.string()), - codeVerifier: v.string(), }); export type State = v.InferOutput; +/** + * Sealed payload stored only in the HttpOnly PKCE cookie. Holds the PKCE + * `codeVerifier` — the secret that binds the authorization code to the browser + * that started the flow — plus the `nonce` that ties it back to the URL state. + */ +export const PKCECookieSchema = v.object({ + nonce: v.string(), + codeVerifier: v.string(), +}); + +export type PKCECookiePayload = v.InferOutput; + export type AuthKitLoaderOptions = { ensureSignedIn?: boolean; debug?: boolean; diff --git a/src/pkce.ts b/src/pkce.ts index 741612f..d940cb2 100644 --- a/src/pkce.ts +++ b/src/pkce.ts @@ -2,7 +2,7 @@ import { createHash } from 'node:crypto'; import { unsealData } from 'iron-session'; import * as v from 'valibot'; import { getConfig } from './config.js'; -import { State, StateSchema } from './interfaces.js'; +import { PKCECookiePayload, PKCECookieSchema, State, StateSchema } from './interfaces.js'; export const PKCE_COOKIE_NAME = 'wos-auth-verifier'; @@ -96,11 +96,11 @@ function resolveSecure({ */ export function getPKCECookieString( sealedState: string, - options: { expired?: boolean; request?: Request; secure?: boolean; redirectUri?: string } = {}, + options: { value?: string; expired?: boolean; request?: Request; secure?: boolean; redirectUri?: string } = {}, ): string { - const { expired = false, request, secure, redirectUri } = options; + const { value: cookieValue = '', expired = false, request, secure, redirectUri } = options; const name = getPKCECookieNameForState(sealedState); - const value = expired ? '' : sealedState; + const value = expired ? '' : cookieValue; const parts = [ `${name}=${value}`, @@ -172,19 +172,34 @@ export function getPKCECleanupCookieStrings( } /** - * Read and unseal the PKCE cookie, returning the code verifier, nonce, and - * any caller-supplied custom state and return pathname. + * Unseal the OAuth `state` URL parameter into its (non-secret) payload: the + * flow `nonce`, plus any caller-supplied custom state and return pathname. + * + * Throws if the state was tampered with or encrypted under a different + * password. Runtime validation via valibot is an acceptable tradeoff here — + * this is not a hot path, and sealing/unsealing does not prove the unsealed + * payload has the expected shape. + */ +export async function getStateFromUrlValue(sealedState: string): Promise { + const unsealed = await unsealData(sealedState, { + password: getConfig('cookiePassword'), + }); + + return v.parse(StateSchema, unsealed); +} + +/** + * Unseal the HttpOnly PKCE cookie into the code verifier and its bound nonce. * * Throws if the cookie was tampered with, encrypted under a different - * password, or is missing required fields. Runtime validation via valibot - * is an acceptable tradeoff here — this is not a hot path, and - * sealing/unsealing does not prove the unsealed payload has the expected - * shape. + * password, or is missing required fields — which is exactly what happens when + * a caller replays a leaked `state` value as the cookie: it unseals to the + * state payload (no `codeVerifier`) and fails schema validation here. */ -export async function getStateFromPKCECookieValue(cookieValue: string): Promise { +export async function getVerifierFromPKCECookieValue(cookieValue: string): Promise { const unsealed = await unsealData(cookieValue, { password: getConfig('cookiePassword'), }); - return v.parse(StateSchema, unsealed); + return v.parse(PKCECookieSchema, unsealed); } diff --git a/src/test-utils/test-helpers.ts b/src/test-utils/test-helpers.ts index 64335a3..2253d06 100644 --- a/src/test-utils/test-helpers.ts +++ b/src/test-utils/test-helpers.ts @@ -48,17 +48,25 @@ export function createRequestWithSearchParams(request: Request, modifier: Search * `Cookie` header in the callback request. */ export async function createSealedState( - overrides: Partial = {}, + overrides: Partial = {}, ): Promise<{ sealedState: string; cookieHeader: string; codeVerifier: string }> { + const nonce = overrides.nonce ?? 'test-nonce'; + const codeVerifier = overrides.codeVerifier ?? 'test-code-verifier'; + + // The OAuth `state` URL param carries no secret — only the nonce and any + // caller flow metadata. const state: State = { - nonce: overrides.nonce ?? 'test-nonce', - codeVerifier: overrides.codeVerifier ?? 'test-code-verifier', + nonce, customState: overrides.customState, returnPathname: overrides.returnPathname, }; const sealedState = await sealData(state, { password: getConfig('cookiePassword') }); - const cookieHeader = `${getPKCECookieNameForState(sealedState)}=${sealedState}`; - return { sealedState, cookieHeader, codeVerifier: state.codeVerifier }; + + // The PKCE verifier lives only in the HttpOnly cookie, sealed separately and + // bound to the same nonce. + const sealedVerifier = await sealData({ nonce, codeVerifier }, { password: getConfig('cookiePassword') }); + const cookieHeader = `${getPKCECookieNameForState(sealedState)}=${sealedVerifier}`; + return { sealedState, cookieHeader, codeVerifier }; } /**