Skip to content

Commit af213e9

Browse files
committed
fix(react-router): re-resolve request options on a Request miss and cache the result
The Request-miss fallback read the resolved auth options from the shared context and never cached its result. Under a shared context an interleaved request could overwrite those options, so a fresh action->loader Request could authenticate with another request's domain/proxyUrl/isSatellite (or keyless options), and repeat getAuth calls re-authenticated each time. Stash only request-independent config (static options + resolved keys) and re-resolve the request-derived options from args.request on the miss, then cache the result keyed by Request.
1 parent 6a06393 commit af213e9

4 files changed

Lines changed: 106 additions & 25 deletions

File tree

packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.ts

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,9 @@ describe('clerkMiddleware + getAuth auth isolation', () => {
150150
expect(authSpy).toHaveBeenCalledTimes(1);
151151
});
152152

153-
// On a Request-instance miss (action -> loader), it re-derives exactly once.
154-
it('re-derives once on a fresh Request instance', async () => {
153+
// On a Request-instance miss (action -> loader) it re-derives once for that fresh
154+
// Request and caches it, so repeat getAuth calls on it don't re-authenticate.
155+
it('re-derives once per fresh Request and caches it', async () => {
155156
const authSpy = vi.fn((req: { url: string }) => Promise.resolve(fakeStateForRequest(req)));
156157
mockClerkClient.mockReturnValue({ authenticateRequest: authSpy } as unknown as ClerkClient);
157158

@@ -163,10 +164,70 @@ describe('clerkMiddleware + getAuth auth isolation', () => {
163164
const loaderRequest = new Request(request.url, { headers: request.headers });
164165
const loaderArgs = { request: loaderRequest, context: args.context } as unknown as LoaderFunctionArgs;
165166
expect(await readUserId(loaderArgs)).toBe('user_A');
167+
expect(await readUserId(loaderArgs)).toBe('user_A');
166168
return new Response('A');
167169
});
168170

169-
// middleware (1) + one re-derive for the fresh loader Request (1).
171+
// middleware (1) + a single re-derive for the fresh loader Request (1); the
172+
// second getAuth on that Request reused the cached result.
170173
expect(authSpy).toHaveBeenCalledTimes(2);
171174
});
175+
176+
// Request-derived options (e.g. a domain/proxyUrl/isSatellite function) must be
177+
// resolved from each request, not read off a shared context. Here domain is
178+
// derived from the URL; even when B overwrites the shared config between A's
179+
// middleware and A's fresh loader Request, A must authenticate with A's domain.
180+
it('re-resolves request-derived options per request on a miss (no options bleed)', async () => {
181+
mockLoadOptions.mockImplementation(
182+
(a: { request: Request }) =>
183+
({
184+
audience: '',
185+
authorizedParties: [],
186+
signInUrl: '',
187+
signUpUrl: '',
188+
secretKey: 'sk_live_xxx',
189+
publishableKey: 'pk_live_xxx',
190+
domain: new URL(a.request.url).searchParams.get('u'),
191+
}) as unknown as ReturnType<typeof loadOptions>,
192+
);
193+
194+
const calls: Array<{ reqUser: string | null; domain: unknown }> = [];
195+
mockClerkClient.mockReturnValue({
196+
authenticateRequest: vi.fn((req: { url: string }, opts: { domain?: unknown }) => {
197+
calls.push({ reqUser: new URL(req.url).searchParams.get('u'), domain: opts.domain });
198+
return Promise.resolve(fakeStateForRequest(req));
199+
}),
200+
} as unknown as ClerkClient);
201+
202+
const shared = new RouterContextProvider();
203+
const middleware = clerkMiddleware();
204+
205+
const reqA = new Request('http://app.test/?u=user_A', { method: 'POST' });
206+
const argsA = { request: reqA, context: shared } as unknown as LoaderFunctionArgs;
207+
const reqB = new Request('http://app.test/?u=user_B');
208+
const argsB = { request: reqB, context: shared } as unknown as LoaderFunctionArgs;
209+
210+
let releaseA!: () => void;
211+
const gateA = new Promise<void>(resolve => (releaseA = resolve));
212+
213+
const aDone = middleware(argsA, async () => {
214+
await gateA;
215+
// Fresh Request (action -> loader): misses the cache and re-derives.
216+
const loaderReqA = new Request(reqA.url, { headers: reqA.headers });
217+
const loaderArgsA = { request: loaderReqA, context: shared } as unknown as LoaderFunctionArgs;
218+
await readUserId(loaderArgsA);
219+
return new Response('A');
220+
});
221+
222+
await flushMicrotasks();
223+
await middleware(argsB, () => Promise.resolve(new Response('B')));
224+
releaseA();
225+
await aDone;
226+
227+
// Every authenticateRequest call used the domain resolved from its own request.
228+
expect(calls.length).toBeGreaterThan(0);
229+
for (const call of calls) {
230+
expect(call.domain).toBe(call.reqUser);
231+
}
232+
});
172233
});

packages/react-router/src/server/__tests__/getAuth.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { LoaderFunctionArgs } from 'react-router';
33
import { beforeEach, describe, expect, it, vi } from 'vitest';
44

55
import { clerkClient } from '../clerkClient';
6-
import { requestOptionsContext } from '../clerkMiddleware';
6+
import { middlewareConfigContext } from '../clerkMiddleware';
77
import { getAuth } from '../getAuth';
88

99
vi.mock('../clerkClient');
@@ -26,7 +26,7 @@ describe('getAuth', () => {
2626
// the request via authenticateRequest rather than reading a cached value.
2727
const mockContext = {
2828
get: vi.fn().mockImplementation(contextKey => {
29-
if (contextKey === requestOptionsContext) {
29+
if (contextKey === middlewareConfigContext) {
3030
return { secretKey: 'sk_test_...', publishableKey: 'pk_test_...', acceptsToken: 'any' };
3131
}
3232
return null;

packages/react-router/src/server/__tests__/rootAuthLoader.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { data, type LoaderFunctionArgs } from 'react-router';
33
import { beforeEach, describe, expect, it, vi } from 'vitest';
44

55
import { clerkClient } from '../clerkClient';
6-
import { authFnContext, requestOptionsContext, requestStateContext } from '../clerkMiddleware';
6+
import { authFnContext, middlewareConfigContext, requestStateContext } from '../clerkMiddleware';
77
import { rootAuthLoader } from '../rootAuthLoader';
88

99
vi.mock('../clerkClient');
@@ -32,7 +32,7 @@ describe('rootAuthLoader', () => {
3232
describe('with middleware context', () => {
3333
const makeContext = (additionalState: Record<string, unknown> = {}) => ({
3434
get: vi.fn().mockImplementation(contextKey => {
35-
if (contextKey === requestOptionsContext) {
35+
if (contextKey === middlewareConfigContext) {
3636
return { secretKey: 'sk_test_...', publishableKey: 'pk_test_...', acceptsToken: 'any' };
3737
}
3838
if (contextKey === requestStateContext) {

packages/react-router/src/server/clerkMiddleware.ts

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,12 @@ type RequestStateContextValue = {
2020

2121
export const authFnContext = createContext<((options?: PendingSessionOptions) => AuthObject) | null>(null);
2222
export const requestStateContext = createContext<RequestStateContextValue | null>(null);
23-
// Identity-free resolved options, reused by getAuth/rootAuthLoader to re-derive the request's user.
24-
export const requestOptionsContext = createContext<AuthenticateRequestOptions | null>(null);
23+
// Request-INDEPENDENT config (the static clerkMiddleware options plus resolved keys)
24+
// used to re-derive auth on a Request-instance miss. It deliberately does NOT hold
25+
// resolved request-derived values (domain/proxyUrl/isSatellite); those are
26+
// re-resolved per request from args.request, so a shared context can't leak one
27+
// request's resolved options to another.
28+
export const middlewareConfigContext = createContext<ClerkMiddlewareOptions | null>(null);
2529
const sharedContextProbe = createContext<Request | null>(null);
2630

2731
const sharedContextMessage =
@@ -33,31 +37,37 @@ const sharedContextMessage =
3337
const requestStateByRequest = new WeakMap<Request, RequestState<any>>();
3438

3539
/**
36-
* Auth state for this request. Reuses what clerkMiddleware already resolved
37-
* (so handshake/refresh and any machine-token verification happen once per
38-
* request), and re-authenticates only when the Request instance differs from the
39-
* one the middleware saw (e.g. React Router's action -> loader revalidation).
40+
* Auth state for this request. Reuses what clerkMiddleware already resolved (keyed
41+
* by Request, so handshake/refresh and any machine-token verification happen once
42+
* per request). On a Request-instance miss (e.g. React Router's action -> loader
43+
* revalidation), it re-authenticates from this request's own cookies and caches
44+
* the result so repeat calls on that Request reuse it too.
4045
*/
4146
export async function resolveRequestState(args: DataFunctionArgs): Promise<RequestState<any>> {
4247
const cached = requestStateByRequest.get(args.request);
4348
if (cached) {
4449
return cached;
4550
}
4651

47-
// Miss: re-derive from this request's own cookies. `options` is identity-free
48-
// config, so reading it from a possibly-shared context is safe; identity comes
49-
// from args.request, so the result is always this request's user.
50-
const options = IsOptIntoMiddleware(args.context) ? args.context.get(requestOptionsContext) : null;
51-
if (!options) {
52+
const config = IsOptIntoMiddleware(args.context) ? args.context.get(middlewareConfigContext) : null;
53+
if (!config) {
5254
throw new Error(
5355
'Clerk: clerkMiddleware() not detected. Make sure you have installed the clerkMiddleware in your root route.',
5456
);
5557
}
5658

57-
return clerkClient(args, options).authenticateRequest(createClerkRequest(patchRequest(args.request)), {
58-
...options,
59-
acceptsToken: 'any',
60-
});
59+
// Re-resolve options from THIS request: domain/proxyUrl/isSatellite are derived
60+
// from args.request, and `config` carries only request-independent values (static
61+
// options + resolved keys), so this can't pick up another request's resolved
62+
// options even when the context is shared. Identity comes from args.request.
63+
const options = loadOptions(args, config);
64+
const requestState = await clerkClient(args, config).authenticateRequest(
65+
createClerkRequest(patchRequest(args.request)),
66+
{ ...options, acceptsToken: 'any' },
67+
);
68+
69+
requestStateByRequest.set(args.request, requestState);
70+
return requestState;
6171
}
6272

6373
/**
@@ -163,11 +173,21 @@ export const clerkMiddleware = (options?: ClerkMiddlewareOptions): MiddlewareFun
163173
signUpFallbackRedirectUrl: loadedOptions.signUpFallbackRedirectUrl,
164174
};
165175

176+
// Request-independent config (static options + resolved keys) for the re-derive
177+
// fallback when the Request instance differs. Deliberately excludes resolved
178+
// domain/proxyUrl/isSatellite, which are re-resolved per request from args.request.
179+
const middlewareConfig: ClerkMiddlewareOptions = {
180+
...options,
181+
secretKey: loadedOptions.secretKey,
182+
publishableKey: loadedOptions.publishableKey,
183+
jwtKey,
184+
machineSecretKey,
185+
};
186+
166187
// Cache the resolved state keyed by this Request so getAuth/rootAuthLoader reuse
167-
// it. Stash identity-free options for the re-derive fallback when the Request
168-
// instance differs. authFnContext/requestStateContext remain for back-compat.
188+
// it. authFnContext/requestStateContext remain for back-compat.
169189
requestStateByRequest.set(args.request, requestState);
170-
args.context.set(requestOptionsContext, authenticateOptions);
190+
args.context.set(middlewareConfigContext, middlewareConfig);
171191
args.context.set(authFnContext, (opts?: PendingSessionOptions) => requestState.toAuth(opts));
172192
args.context.set(requestStateContext, { requestState, additionalState });
173193

0 commit comments

Comments
 (0)