Skip to content

Commit 3740a83

Browse files
cn-stephennicknisi
andauthored
fix: isolate concurrent PKCE flows to prevent cookie clobbering (#403)
Co-authored-by: Nick Nisi <nick.nisi@workos.com>
1 parent dd7ef55 commit 3740a83

12 files changed

Lines changed: 276 additions & 46 deletions

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
"typecheck": "tsc --project tsconfig.app.json --noEmit && tsc --project tsconfig.test.json --noEmit"
3838
},
3939
"dependencies": {
40+
"@sindresorhus/fnv1a": "^3.1.0",
4041
"@workos-inc/node": "^8.9.0",
4142
"iron-session": "^8.0.4",
4243
"jose": "^5.10.0",

pnpm-lock.yaml

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/auth.spec.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,25 @@ describe('auth.ts', () => {
274274
expect(sessionCookie).toBeUndefined();
275275
});
276276

277+
it('should clear lingering PKCE verifier cookies (legacy and per-flow)', async () => {
278+
const nextCookies = await cookies();
279+
const nextHeaders = await headers();
280+
281+
nextHeaders.set('x-workos-middleware', 'true');
282+
nextCookies.set('wos-session', 'foo');
283+
nextCookies.set('wos-auth-verifier', 'legacy-state');
284+
nextCookies.set('wos-auth-verifier-a1b2c3d4', 'flow-a-state');
285+
nextCookies.set('wos-auth-verifier-deadbeef', 'flow-b-state');
286+
nextCookies.set('unrelated-cookie', 'keep-me');
287+
288+
await signOut();
289+
290+
expect(nextCookies.get('wos-auth-verifier')).toBeUndefined();
291+
expect(nextCookies.get('wos-auth-verifier-a1b2c3d4')).toBeUndefined();
292+
expect(nextCookies.get('wos-auth-verifier-deadbeef')).toBeUndefined();
293+
expect(nextCookies.get('unrelated-cookie')?.value).toBe('keep-me');
294+
});
295+
277296
describe('when given a `returnTo` parameter', () => {
278297
it('passes the `returnTo` through to the `getLogoutUrl` call', async () => {
279298
vi.spyOn(workos.userManagement, 'getLogoutUrl').mockReturnValue(

src/auth.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ import { revalidatePath, revalidateTag } from 'next/cache';
55
import { cookies, headers } from 'next/headers';
66
import { redirect } from 'next/navigation';
77
import { WORKOS_COOKIE_NAME } from './env-variables.js';
8-
import { getCookieOptions } from './cookie.js';
8+
import { getCookieOptions, getPKCECookieOptions } from './cookie.js';
99
import { getAuthorizationUrl } from './get-authorization-url.js';
1010
import type { AccessToken, GetAuthURLOptions, SwitchToOrganizationOptions, UserInfo } from './interfaces.js';
11-
import { setPKCECookie } from './pkce.js';
11+
import { PKCE_COOKIE_NAME, setPKCECookie } from './pkce.js';
1212
import { getSessionFromCookie, refreshSession, withAuth } from './session.js';
1313
import { getWorkOS } from './workos.js';
1414

@@ -80,6 +80,24 @@ export async function signOut({ returnTo }: { returnTo?: string } = {}) {
8080
nextCookies.delete(cookieName);
8181
}
8282

83+
// Clear any lingering PKCE verifier cookies so orphans from abandoned
84+
// flows don't accumulate toward HTTP 431 or confuse future sign-ins.
85+
const pkceOptions = getPKCECookieOptions();
86+
for (const { name } of nextCookies.getAll()) {
87+
if (!name.startsWith(PKCE_COOKIE_NAME)) continue;
88+
try {
89+
nextCookies.delete({
90+
name,
91+
domain: pkceOptions.domain,
92+
path: pkceOptions.path,
93+
sameSite: pkceOptions.sameSite,
94+
secure: pkceOptions.secure,
95+
});
96+
} catch {
97+
nextCookies.delete(name);
98+
}
99+
}
100+
83101
if (sessionId) {
84102
redirect(getWorkOS().userManagement.getLogoutUrl({ sessionId, returnTo }));
85103
} else {

src/authkit-callback-route.spec.ts

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { Mock } from 'vitest';
22
import { getWorkOS } from './workos.js';
33
import { handleAuth } from './authkit-callback-route.js';
4+
import { getPKCECookieNameForState } from './pkce.js';
45
import { getSessionFromCookie, saveSession } from './session.js';
56
import { NextRequest, NextResponse } from 'next/server';
67
import { sealData } from 'iron-session';
@@ -56,7 +57,7 @@ describe('authkit-callback-route', () => {
5657

5758
async function setAuthCookie(req: NextRequest, state: State): Promise<string> {
5859
const sealedState = await sealData(state, { password: process.env.WORKOS_COOKIE_PASSWORD! });
59-
req.cookies.set('wos-auth-verifier', sealedState);
60+
req.cookies.set(getPKCECookieNameForState(sealedState), sealedState);
6061
return sealedState;
6162
}
6263

@@ -548,10 +549,11 @@ describe('authkit-callback-route', () => {
548549
it('should return an error response when PKCE cookie is corrupted', async () => {
549550
vi.mocked(workos.userManagement.authenticateWithCode).mockResolvedValue(mockAuthResponse);
550551

551-
// Set a corrupted cookie
552-
request.cookies.set('wos-auth-verifier', 'not-a-valid-sealed-value');
552+
// Set a corrupted cookie using the flow-specific name
553+
const corruptedState = 'not-a-valid-sealed-value';
554+
request.cookies.set(getPKCECookieNameForState(corruptedState), corruptedState);
553555
request.nextUrl.searchParams.set('code', 'test-code');
554-
request.nextUrl.searchParams.set('state', 'not-a-valid-sealed-value');
556+
request.nextUrl.searchParams.set('state', corruptedState);
555557

556558
const handler = handleAuth();
557559
const response = await handler(request);
@@ -570,11 +572,12 @@ describe('authkit-callback-route', () => {
570572
const handler = handleAuth();
571573
const response = await handler(request);
572574

573-
// The response should be a redirect (success) and have a Set-Cookie header to delete the PKCE cookie
575+
// The response should be a redirect (success) and have a Set-Cookie header to delete the flow-specific PKCE cookie
574576
expect(response.status).toBe(307);
575577

578+
const flowCookieName = getPKCECookieNameForState(sealedState);
576579
const setCookieHeaders = response.headers.getSetCookie();
577-
const pkceDeletionCookie = setCookieHeaders.find((c: string) => c.startsWith('wos-auth-verifier='));
580+
const pkceDeletionCookie = setCookieHeaders.find((c: string) => c.startsWith(`${flowCookieName}=`));
578581
expect(pkceDeletionCookie).toBeDefined();
579582
expect(pkceDeletionCookie).toContain('Max-Age=0');
580583
});
@@ -590,11 +593,70 @@ describe('authkit-callback-route', () => {
590593
const response = await handler(request);
591594

592595
expect(response.status).toBe(500);
596+
const flowCookieName = getPKCECookieNameForState(sealedState);
593597
const setCookieHeaders = response.headers.getSetCookie();
594-
const pkceDeletionCookie = setCookieHeaders.find((c: string) => c.startsWith('wos-auth-verifier='));
598+
const pkceDeletionCookie = setCookieHeaders.find((c: string) => c.startsWith(`${flowCookieName}=`));
595599
expect(pkceDeletionCookie).toBeDefined();
596600
expect(pkceDeletionCookie).toContain('Max-Age=0');
597601
});
602+
603+
it('should isolate concurrent auth flows using per-flow cookie names', async () => {
604+
vi.mocked(workos.userManagement.authenticateWithCode).mockResolvedValue(mockAuthResponse);
605+
606+
// Simulate two concurrent auth flows with different sealed states
607+
const sealedStateA = await sealData(
608+
{ nonce: 'nonce-a', codeVerifier: 'verifier-a' },
609+
{ password: process.env.WORKOS_COOKIE_PASSWORD! },
610+
);
611+
const sealedStateB = await sealData(
612+
{ nonce: 'nonce-b', codeVerifier: 'verifier-b' },
613+
{ password: process.env.WORKOS_COOKIE_PASSWORD! },
614+
);
615+
616+
// Both cookies exist on the request (set by different middleware redirects)
617+
request.cookies.set(getPKCECookieNameForState(sealedStateA), sealedStateA);
618+
request.cookies.set(getPKCECookieNameForState(sealedStateB), sealedStateB);
619+
620+
// Callback for flow A — should find its own cookie
621+
request.nextUrl.searchParams.set('code', 'code-a');
622+
request.nextUrl.searchParams.set('state', sealedStateA);
623+
624+
const handler = handleAuth();
625+
const response = await handler(request);
626+
627+
expect(response.status).toBe(307);
628+
expect(workos.userManagement.authenticateWithCode).toHaveBeenCalledWith(
629+
expect.objectContaining({ codeVerifier: 'verifier-a' }),
630+
);
631+
632+
// Flow B's cookie should NOT have been deleted
633+
const setCookieHeaders = response.headers.getSetCookie();
634+
const flowBCookieName = getPKCECookieNameForState(sealedStateB);
635+
const flowBDeletion = setCookieHeaders.find((c: string) => c.startsWith(`${flowBCookieName}=`));
636+
expect(flowBDeletion).toBeUndefined();
637+
});
638+
639+
it('should fall back to the legacy shared PKCE cookie for v3.0.x in-flight flows', async () => {
640+
vi.mocked(workos.userManagement.authenticateWithCode).mockResolvedValue(mockAuthResponse);
641+
642+
const sealedState = await sealData(
643+
{ nonce: 'legacy', codeVerifier: 'legacy-verifier' },
644+
{ password: process.env.WORKOS_COOKIE_PASSWORD! },
645+
);
646+
647+
// Simulate a user mid-OAuth on v3.0.x: only the legacy cookie name exists
648+
request.cookies.set('wos-auth-verifier', sealedState);
649+
request.nextUrl.searchParams.set('code', 'test-code');
650+
request.nextUrl.searchParams.set('state', sealedState);
651+
652+
const handler = handleAuth();
653+
const response = await handler(request);
654+
655+
expect(response.status).toBe(307);
656+
expect(workos.userManagement.authenticateWithCode).toHaveBeenCalledWith(
657+
expect.objectContaining({ codeVerifier: 'legacy-verifier' }),
658+
);
659+
});
598660
});
599661
});
600662
});

src/authkit-callback-route.ts

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { NextRequest } from 'next/server';
22
import { getPKCECookieOptions } from './cookie.js';
33
import { WORKOS_CLIENT_ID } from './env-variables.js';
44
import { HandleAuthOptions } from './interfaces.js';
5-
import { PKCE_COOKIE_NAME, getStateFromPKCECookieValue } from './pkce.js';
5+
import { PKCE_COOKIE_NAME, getPKCECookieNameForState, getStateFromPKCECookieValue } from './pkce.js';
66
import { saveSession } from './session.js';
77
import { errorResponseWithFallback, redirectWithFallback, setCachePreventionHeaders } from './utils.js';
88
import { getWorkOS } from './workos.js';
@@ -25,26 +25,29 @@ export function handleAuth(options: HandleAuthOptions = {}) {
2525
}
2626

2727
return async function GET(request: NextRequest) {
28-
// Always delete the PKCE cookie after handling the callback, regardless of success or error
29-
// to avoid stale cookies affecting future auth attempts & prevent replays
30-
const deleteCookie = `${PKCE_COOKIE_NAME}=; ${getPKCECookieOptions(request.url, true, true)}`;
28+
// Fall back to standard URL parsing when nextUrl is not available (e.g., vinext)
29+
const requestUrl = request.nextUrl ?? new URL(request.url);
3130

32-
// We want to catch any & all errors and respond the same way
33-
// Firstly, by destroying the 1-use PKCE cookie to prevent replay attacks
34-
// or stale cookies affecting future auth attempts
35-
try {
36-
// Fall back to standard URL parsing when nextUrl is not available (e.g., vinext)
37-
const requestUrl = request.nextUrl ?? new URL(request.url);
38-
39-
// Gather mandatory information
40-
const code = requestUrl.searchParams.get('code');
41-
const state = requestUrl.searchParams.get('state');
42-
const pkceCookie = request.cookies.get(PKCE_COOKIE_NAME)?.value;
31+
// Gather mandatory information
32+
const code = requestUrl.searchParams.get('code');
33+
const state = requestUrl.searchParams.get('state');
4334

35+
// We want to catch any & all errors and respond the same way, always
36+
// destroying the 1-use PKCE cookie to prevent replay attacks or stale
37+
// cookies affecting future auth attempts.
38+
try {
4439
if (!code || !state) {
4540
throw new Error('Missing required auth parameter');
4641
}
4742

43+
// Derive the flow-specific cookie name from the state param so each
44+
// concurrent auth flow reads/deletes its own cookie, not a shared one.
45+
// Fall back to the legacy shared cookie name so in-flight OAuth flows
46+
// started on v3.0.x don't fail on the first callback after upgrade.
47+
// Safe to remove once v3.0.x is unsupported.
48+
const pkceCookieName = getPKCECookieNameForState(state);
49+
const pkceCookie = request.cookies.get(pkceCookieName)?.value ?? request.cookies.get(PKCE_COOKIE_NAME)?.value;
50+
4851
// CSRF verification: both channels (cookie + URL state) must be present and match
4952
if (!pkceCookie) {
5053
throw new Error(
@@ -95,7 +98,10 @@ export function handleAuth(options: HandleAuthOptions = {}) {
9598
// This is to support Next.js 13.
9699
const response = redirectWithFallback(url.toString());
97100
preventCaching(response.headers);
98-
response.headers.append('Set-Cookie', deleteCookie);
101+
102+
// Always delete the PKCE cookie after handling the callback, regardless of success or error
103+
// to avoid stale cookies affecting future auth attempts & prevent replays
104+
response.headers.append('Set-Cookie', `${pkceCookieName}=; ${getPKCECookieOptions(request.url, true, true)}`);
99105

100106
await saveSession({ accessToken, refreshToken, user, impersonator }, request);
101107

@@ -116,7 +122,16 @@ export function handleAuth(options: HandleAuthOptions = {}) {
116122
} catch (error) {
117123
console.error('[AuthKit callback error]', error);
118124
const response = await errorResponse(request, error);
119-
response.headers.append('Set-Cookie', deleteCookie);
125+
126+
// Always delete the PKCE cookie after handling the callback, regardless of success or error
127+
// to avoid stale cookies affecting future auth attempts & prevent replays
128+
if (state) {
129+
response.headers.append(
130+
'Set-Cookie',
131+
`${getPKCECookieNameForState(state)}=; ${getPKCECookieOptions(request.url, true, true)}`,
132+
);
133+
}
134+
120135
return response;
121136
}
122137
};

src/cookie.spec.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,4 +278,53 @@ describe('cookie.ts', () => {
278278
expect(ipCookie).not.toContain('Secure');
279279
});
280280
});
281+
282+
describe('getPKCECookieOptions', () => {
283+
it('should use 10-minute max-age, not the session cookie max-age', async () => {
284+
const { getPKCECookieOptions } = await import('./cookie');
285+
286+
const options = getPKCECookieOptions();
287+
288+
expect(options).toEqual(expect.objectContaining({ maxAge: 600 }));
289+
});
290+
291+
it('should use 10-minute max-age in string format', async () => {
292+
const { getPKCECookieOptions } = await import('./cookie');
293+
294+
const options = getPKCECookieOptions('http://localhost:3000', true);
295+
296+
expect(options).toContain('Max-Age=600');
297+
expect(options).not.toContain('Max-Age=34560000');
298+
});
299+
300+
it('should use max-age 0 when expired in object format', async () => {
301+
const { getPKCECookieOptions } = await import('./cookie');
302+
303+
const options = getPKCECookieOptions(undefined, false, true);
304+
305+
expect(options).toEqual(expect.objectContaining({ maxAge: 0 }));
306+
});
307+
308+
it('should use max-age 0 when expired in string format', async () => {
309+
const { getPKCECookieOptions } = await import('./cookie');
310+
311+
const options = getPKCECookieOptions('http://localhost:3000', true, true);
312+
313+
expect(options).toContain('Max-Age=0');
314+
});
315+
316+
it('should downgrade SameSite=Strict to Lax', async () => {
317+
const envVars = await import('./env-variables');
318+
Object.defineProperty(envVars, 'WORKOS_COOKIE_SAMESITE', { value: 'strict' });
319+
320+
const { getPKCECookieOptions } = await import('./cookie');
321+
322+
const objectOptions = getPKCECookieOptions();
323+
expect(objectOptions).toEqual(expect.objectContaining({ sameSite: 'lax' }));
324+
325+
const stringOptions = getPKCECookieOptions('http://localhost:3000', true);
326+
expect(stringOptions).toContain('SameSite=Lax');
327+
expect(stringOptions).not.toContain('SameSite=Strict');
328+
});
329+
});
281330
});

src/cookie.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,13 @@ export function getCookieOptions(
9292
};
9393
}
9494

95+
const PKCE_COOKIE_MAX_AGE = 600; // 10 minutes
96+
9597
/**
9698
* Cookie options for the PKCE verifier cookie.
9799
* 'strict' blocks the cookie on the cross-site redirect back from WorkOS; downgrade to 'lax'.
98100
* 'none' is more permissive and must be preserved for iframe/cross-origin embed flows.
101+
* Max-age is always capped to 10 minutes — PKCE cookies are single-use and short-lived.
99102
*/
100103
export function getPKCECookieOptions(): CookieOptions;
101104
export function getPKCECookieOptions(redirectUri: string | null | undefined, asString: true, expired?: boolean): string;
@@ -111,13 +114,16 @@ export function getPKCECookieOptions(
111114
): CookieOptions | string {
112115
if (asString) {
113116
const options = getCookieOptions(redirectUri, true, expired);
114-
return options.replace(/SameSite=Strict/i, 'SameSite=Lax');
117+
return options
118+
.replace(/SameSite=Strict/i, 'SameSite=Lax')
119+
.replace(/Max-Age=\d+/, `Max-Age=${expired ? 0 : PKCE_COOKIE_MAX_AGE}`);
115120
}
116121

117122
const options = getCookieOptions(redirectUri);
118123
return {
119124
...options,
120125
sameSite: options.sameSite.toLowerCase() === 'strict' ? 'lax' : options.sameSite,
126+
maxAge: expired ? 0 : PKCE_COOKIE_MAX_AGE,
121127
};
122128
}
123129

0 commit comments

Comments
 (0)