diff --git a/.changeset/rr-request-scoped-auth.md b/.changeset/rr-request-scoped-auth.md new file mode 100644 index 00000000000..77818337a86 --- /dev/null +++ b/.changeset/rr-request-scoped-auth.md @@ -0,0 +1,5 @@ +--- +'@clerk/react-router': patch +--- + +Fixed a cross-user authentication issue for apps that share a single React Router `context` across requests (for example a custom server or `getLoadContext` that returns one `RouterContextProvider` instance). `getAuth()` and `rootAuthLoader()` now resolve the current request's auth from that request rather than from a value cached on the context, so a shared context can no longer cause one request to be served another user's auth under concurrency, including across React Router's action-to-loader revalidation. `clerkMiddleware()` also logs a warning once when it detects a context reused across requests, since a shared context can still leak an application's own per-request data. diff --git a/packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.ts b/packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.ts new file mode 100644 index 00000000000..508ec2e797c --- /dev/null +++ b/packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.ts @@ -0,0 +1,131 @@ +// getAuth re-derives auth from `args.request`, so it returns the right user even +// when an app shares one RouterContextProvider across requests. authenticateRequest +// is mocked to resolve each request to the user encoded in its URL (?u=...). +import type { ClerkClient } from '@clerk/backend'; +import { AuthStatus, TokenType } from '@clerk/backend/internal'; +import type { LoaderFunctionArgs } from 'react-router'; +import { RouterContextProvider } from 'react-router'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { clerkClient } from '../clerkClient'; +import { clerkMiddleware } from '../clerkMiddleware'; +import { getAuth } from '../getAuth'; +import { loadOptions } from '../loadOptions'; + +vi.mock('../clerkClient'); +vi.mock('../loadOptions'); + +const mockClerkClient = vi.mocked(clerkClient); +const mockLoadOptions = vi.mocked(loadOptions); + +function fakeStateForRequest(req: { url: string }) { + const userId = new URL(req.url).searchParams.get('u'); + return { + status: AuthStatus.SignedIn, + headers: new Headers(), + publishableKey: 'pk_live_xxx', + toAuth: () => ({ userId, tokenType: TokenType.SessionToken }), + }; +} + +const flushMicrotasks = () => new Promise(resolve => setTimeout(resolve, 0)); + +async function readUserId(args: LoaderFunctionArgs): Promise { + const auth = (await getAuth(args, { acceptsToken: 'any' })) as { userId?: string | null }; + return auth.userId; +} + +describe('clerkMiddleware + getAuth auth isolation', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockLoadOptions.mockReturnValue({ + audience: '', + authorizedParties: [], + signInUrl: '', + signUpUrl: '', + secretKey: 'sk_live_xxx', + // pk_live -> production instance -> shared-context probe warns (does not throw). + publishableKey: 'pk_live_xxx', + } as unknown as ReturnType); + mockClerkClient.mockReturnValue({ + authenticateRequest: vi.fn((req: { url: string }) => Promise.resolve(fakeStateForRequest(req))), + } as unknown as ClerkClient); + }); + + // Interleave two concurrent requests, each using `contextFor(request)`: + // 1. A's middleware runs, then parks inside next(). + // 2. B's middleware runs, B's loader reads its own auth in next(). + // 3. A unparks and reads its auth. + async function runInterleaved(contextFor: (req: Request) => RouterContextProvider) { + const middleware = clerkMiddleware(); + const results: { A?: string | null; B?: string | null } = {}; + + let releaseA!: () => void; + const gateA = new Promise(resolve => (releaseA = resolve)); + + const reqA = new Request('http://app.test/?u=user_A'); + const reqB = new Request('http://app.test/?u=user_B'); + const argsA = { request: reqA, context: contextFor(reqA) } as unknown as LoaderFunctionArgs; + const argsB = { request: reqB, context: contextFor(reqB) } as unknown as LoaderFunctionArgs; + + const aDone = middleware(argsA, async () => { + await gateA; + results.A = await readUserId(argsA); + return new Response('A'); + }); + + await flushMicrotasks(); + + await middleware(argsB, async () => { + results.B = await readUserId(argsB); + return new Response('B'); + }); + + releaseA(); + await aDone; + + return results; + } + + it('keeps auth per-request with a shared RouterContextProvider', async () => { + const shared = new RouterContextProvider(); + const results = await runInterleaved(() => shared); + + expect(results.A).toBe('user_A'); + expect(results.B).toBe('user_B'); + }); + + it('keeps auth per-request with a fresh RouterContextProvider per request', async () => { + const perRequest = new Map(); + const results = await runInterleaved(req => { + if (!perRequest.has(req)) { + perRequest.set(req, new RouterContextProvider()); + } + return perRequest.get(req)!; + }); + + expect(results.A).toBe('user_A'); + expect(results.B).toBe('user_B'); + }); + + // React Router mints a NEW Request for post-action loader revalidation. getAuth + // re-derives from whatever request the loader was invoked with, so it resolves + // the right user even reading via the fresh Request on a shared context. + it('resolves the right user when the loader reads via a fresh Request (action -> loader)', async () => { + const shared = new RouterContextProvider(); + const middleware = clerkMiddleware(); + + const reqA = new Request('http://app.test/?u=user_A', { method: 'POST' }); + const argsA = { request: reqA, context: shared } as unknown as LoaderFunctionArgs; + + let seen: string | null | undefined; + await middleware(argsA, async () => { + const loaderRequest = new Request(reqA.url, { headers: reqA.headers }); + const loaderArgs = { request: loaderRequest, context: shared } as unknown as LoaderFunctionArgs; + seen = await readUserId(loaderArgs); + return new Response('A'); + }); + + expect(seen).toBe('user_A'); + }); +}); diff --git a/packages/react-router/src/server/__tests__/clerkMiddleware.sharedContextWarning.test.ts b/packages/react-router/src/server/__tests__/clerkMiddleware.sharedContextWarning.test.ts new file mode 100644 index 00000000000..ef11a5ea320 --- /dev/null +++ b/packages/react-router/src/server/__tests__/clerkMiddleware.sharedContextWarning.test.ts @@ -0,0 +1,89 @@ +// clerkMiddleware warns once when it detects a React Router context reused across +// requests (the shared-RouterContextProvider footgun). We spy on logger.warnOnce +// so assertions don't depend on its per-process dedup. +import type { ClerkClient } from '@clerk/backend'; +import { AuthStatus, TokenType } from '@clerk/backend/internal'; +import { logger } from '@clerk/shared/logger'; +import type { LoaderFunctionArgs } from 'react-router'; +import { RouterContextProvider } from 'react-router'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { clerkClient } from '../clerkClient'; +import { clerkMiddleware } from '../clerkMiddleware'; +import { loadOptions } from '../loadOptions'; + +vi.mock('../clerkClient'); +vi.mock('../loadOptions'); + +const mockClerkClient = vi.mocked(clerkClient); +const mockLoadOptions = vi.mocked(loadOptions); + +function fakeStateForRequest(req: { url: string }) { + const userId = new URL(req.url).searchParams.get('u'); + return { + status: AuthStatus.SignedIn, + headers: new Headers(), + publishableKey: 'pk', + toAuth: () => ({ userId, tokenType: TokenType.SessionToken }), + }; +} + +const noop = () => Promise.resolve(new Response('ok')); + +describe('clerkMiddleware shared-context detection', () => { + let warnOnceSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + warnOnceSpy = vi.spyOn(logger, 'warnOnce').mockImplementation(() => {}); + mockLoadOptions.mockReturnValue({ + audience: '', + authorizedParties: [], + signInUrl: '', + signUpUrl: '', + secretKey: 'sk_live_xxx', + publishableKey: 'pk_live_xxx', + } as unknown as ReturnType); + mockClerkClient.mockReturnValue({ + authenticateRequest: vi.fn((req: { url: string }) => Promise.resolve(fakeStateForRequest(req))), + } as unknown as ClerkClient); + }); + + it('warns once when two requests share one RouterContextProvider', async () => { + const middleware = clerkMiddleware(); + const shared = new RouterContextProvider(); + + await middleware( + { request: new Request('http://app.test/?u=user_A'), context: shared } as unknown as LoaderFunctionArgs, + noop, + ); + await middleware( + { request: new Request('http://app.test/?u=user_B'), context: shared } as unknown as LoaderFunctionArgs, + noop, + ); + + expect(warnOnceSpy).toHaveBeenCalledTimes(1); + expect(warnOnceSpy).toHaveBeenCalledWith(expect.stringContaining('reused across requests')); + }); + + it('does not warn when each request gets its own RouterContextProvider', async () => { + const middleware = clerkMiddleware(); + + await middleware( + { + request: new Request('http://app.test/?u=user_A'), + context: new RouterContextProvider(), + } as unknown as LoaderFunctionArgs, + noop, + ); + await middleware( + { + request: new Request('http://app.test/?u=user_B'), + context: new RouterContextProvider(), + } as unknown as LoaderFunctionArgs, + noop, + ); + + expect(warnOnceSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/react-router/src/server/__tests__/getAuth.test.ts b/packages/react-router/src/server/__tests__/getAuth.test.ts index 31173c8c7d6..4958a398024 100644 --- a/packages/react-router/src/server/__tests__/getAuth.test.ts +++ b/packages/react-router/src/server/__tests__/getAuth.test.ts @@ -2,24 +2,32 @@ import { TokenType } from '@clerk/backend/internal'; import type { LoaderFunctionArgs } from 'react-router'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { authFnContext } from '../clerkMiddleware'; +import { clerkClient } from '../clerkClient'; +import { requestOptionsContext } from '../clerkMiddleware'; import { getAuth } from '../getAuth'; +vi.mock('../clerkClient'); +const mockClerkClient = vi.mocked(clerkClient); + describe('getAuth', () => { beforeEach(() => { vi.clearAllMocks(); process.env.CLERK_SECRET_KEY = 'sk_test_...'; + mockClerkClient.mockReturnValue({ + authenticateRequest: vi.fn().mockResolvedValue({ + headers: new Headers(), + toAuth: (options?: any) => ({ userId: 'user_xxx', tokenType: TokenType.SessionToken, ...options }), + }), + } as any); }); - it('should work when middleware context exists', async () => { + it('should re-derive auth from the request when middleware ran', async () => { + // Middleware stashes identity-free options; getAuth re-derives the user from + // the request via authenticateRequest rather than reading a cached value. const mockContext = { get: vi.fn().mockImplementation(contextKey => { - if (contextKey === authFnContext) { - return vi.fn().mockImplementation((options?: any) => ({ - userId: 'user_xxx', - tokenType: TokenType.SessionToken, - ...options, - })); + if (contextKey === requestOptionsContext) { + return { secretKey: 'sk_test_...', publishableKey: 'pk_test_...', acceptsToken: 'any' }; } return null; }), diff --git a/packages/react-router/src/server/__tests__/rootAuthLoader.test.ts b/packages/react-router/src/server/__tests__/rootAuthLoader.test.ts index a1da41dae47..2e3cb845d29 100644 --- a/packages/react-router/src/server/__tests__/rootAuthLoader.test.ts +++ b/packages/react-router/src/server/__tests__/rootAuthLoader.test.ts @@ -2,49 +2,54 @@ import { TokenType } from '@clerk/backend/internal'; import { data, type LoaderFunctionArgs } from 'react-router'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { authFnContext, requestStateContext } from '../clerkMiddleware'; +import { clerkClient } from '../clerkClient'; +import { authFnContext, requestOptionsContext, requestStateContext } from '../clerkMiddleware'; import { rootAuthLoader } from '../rootAuthLoader'; +vi.mock('../clerkClient'); +const mockClerkClient = vi.mocked(clerkClient); + describe('rootAuthLoader', () => { + const mockRequestState = { + toAuth: vi.fn().mockImplementation(() => ({ + userId: 'user_xxx', + tokenType: TokenType.SessionToken, + })), + headers: new Headers(), + status: 'signed-in', + }; + beforeEach(() => { vi.clearAllMocks(); process.env.CLERK_SECRET_KEY = 'sk_test_...'; + // rootAuthLoader re-derives the request state from the request rather than + // reading the value cached on the context. + mockClerkClient.mockReturnValue({ + authenticateRequest: vi.fn().mockResolvedValue(mockRequestState), + } as any); }); describe('with middleware context', () => { - const mockRequestState = { - toAuth: vi.fn().mockImplementation(() => ({ - userId: 'user_xxx', - tokenType: TokenType.SessionToken, - })), - headers: new Headers(), - status: 'signed-in', - }; - - const mockContext = { + const makeContext = (additionalState: Record = {}) => ({ get: vi.fn().mockImplementation(contextKey => { + if (contextKey === requestOptionsContext) { + return { secretKey: 'sk_test_...', publishableKey: 'pk_test_...', acceptsToken: 'any' }; + } if (contextKey === requestStateContext) { - return { - requestState: mockRequestState, - additionalState: {}, - }; + return { requestState: mockRequestState, additionalState }; } if (contextKey === authFnContext) { - return vi.fn().mockImplementation((options?: any) => ({ - userId: 'user_xxx', - tokenType: TokenType.SessionToken, - ...options, - })); + return vi.fn().mockReturnValue({ userId: 'user_xxx', tokenType: TokenType.SessionToken }); } return null; }), set: vi.fn(), - }; + }); const args = { - context: mockContext, + context: makeContext(), request: new Request('http://clerk.com'), - } as LoaderFunctionArgs; + } as unknown as LoaderFunctionArgs; it('should work with a callback', async () => { await rootAuthLoader(args, () => ({ data: 'test' })); @@ -108,31 +113,15 @@ describe('rootAuthLoader', () => { }); it('should forward redirect URL options from additionalState into clerkState', async () => { - const mockContext2 = { - get: vi.fn().mockImplementation(contextKey => { - if (contextKey === requestStateContext) { - return { - requestState: mockRequestState, - additionalState: { - signInForceRedirectUrl: '/dashboard', - signUpForceRedirectUrl: '/welcome', - signInFallbackRedirectUrl: '/home', - signUpFallbackRedirectUrl: '/home', - }, - }; - } - if (contextKey === authFnContext) { - return vi.fn().mockReturnValue({ userId: 'user_xxx', tokenType: TokenType.SessionToken }); - } - return null; - }), - set: vi.fn(), - }; - const result = (await rootAuthLoader({ - context: mockContext2, + context: makeContext({ + signInForceRedirectUrl: '/dashboard', + signUpForceRedirectUrl: '/welcome', + signInFallbackRedirectUrl: '/home', + signUpFallbackRedirectUrl: '/home', + }), request: new Request('http://clerk.com'), - } as LoaderFunctionArgs)) as any; + } as unknown as LoaderFunctionArgs)) as any; const internalState = result.clerkState.__internal_clerk_state; expect(internalState.__signInForceRedirectUrl).toBe('/dashboard'); diff --git a/packages/react-router/src/server/clerkMiddleware.ts b/packages/react-router/src/server/clerkMiddleware.ts index 1f49251408a..3271e797a41 100644 --- a/packages/react-router/src/server/clerkMiddleware.ts +++ b/packages/react-router/src/server/clerkMiddleware.ts @@ -1,6 +1,7 @@ import type { AuthObject } from '@clerk/backend'; -import type { RequestState } from '@clerk/backend/internal'; +import type { AuthenticateRequestOptions, RequestState } from '@clerk/backend/internal'; import { AuthStatus, constants, createClerkRequest } from '@clerk/backend/internal'; +import { logger } from '@clerk/shared/logger'; import { handleNetlifyCacheInDevInstance } from '@clerk/shared/netlifyCacheHandler'; import type { PendingSessionOptions } from '@clerk/shared/types'; import type { MiddlewareFunction } from 'react-router'; @@ -8,9 +9,9 @@ import { createContext } from 'react-router'; import { clerkClient } from './clerkClient'; import { resolveKeysWithKeylessFallback } from './keyless/utils'; -import { loadOptions } from './loadOptions'; +import { type DataFunctionArgs, loadOptions } from './loadOptions'; import type { AdditionalStateOptions, ClerkMiddlewareOptions } from './types'; -import { patchRequest } from './utils'; +import { IsOptIntoMiddleware, patchRequest } from './utils'; type RequestStateContextValue = { requestState: RequestState; @@ -19,6 +20,33 @@ type RequestStateContextValue = { export const authFnContext = createContext<((options?: PendingSessionOptions) => AuthObject) | null>(null); export const requestStateContext = createContext(null); +// Identity-free resolved options, reused by getAuth/rootAuthLoader to re-derive the request's user. +export const requestOptionsContext = createContext(null); +const sharedContextProbe = createContext(null); + +const sharedContextMessage = + "Clerk: The React Router `context` is being reused across requests. clerkMiddleware() resolves each request's auth from that request, so sign-in state stays correct, but sharing one context across requests is unsupported and can leak your application's own per-request data. This usually comes from a custom server or `getLoadContext()` that returns a single RouterContextProvider; return a new RouterContextProvider() for each request instead."; + +/** + * Re-derives the request's auth from `args.request` using the identity-free options + * stashed by clerkMiddleware, so a shared context can never return another user. + */ +export async function authenticateFromRequest( + args: DataFunctionArgs, + acceptsToken: AuthenticateRequestOptions['acceptsToken'] = 'any', +): Promise> { + const options = IsOptIntoMiddleware(args.context) ? args.context.get(requestOptionsContext) : null; + if (!options) { + throw new Error( + 'Clerk: clerkMiddleware() not detected. Make sure you have installed the clerkMiddleware in your root route.', + ); + } + + return clerkClient(args, options).authenticateRequest(createClerkRequest(patchRequest(args.request)), { + ...options, + acceptsToken, + }); +} /** * Middleware that integrates Clerk authentication into your React Router application. @@ -38,6 +66,15 @@ export const requestStateContext = createContext => { return async (args, next) => { + // A context reused across requests means the app is sharing one + // RouterContextProvider. Auth stays correct (it is re-derived per request), + // but a shared context can leak the app's own per-request data, so warn once. + const probedRequest = args.context.get(sharedContextProbe); + if (probedRequest && probedRequest !== args.request) { + logger.warnOnce(sharedContextMessage); + } + args.context.set(sharedContextProbe, args.request); + const clerkRequest = createClerkRequest(patchRequest(args.request)); const loadedOptions = loadOptions(args, options); @@ -71,7 +108,7 @@ export const clerkMiddleware = (options?: ClerkMiddlewareOptions): MiddlewareFun organizationSyncOptions, } = loadedOptions; - const requestState = await clerkClient(args, options).authenticateRequest(clerkRequest, { + const authenticateOptions: AuthenticateRequestOptions = { apiUrl, secretKey: loadedOptions.secretKey, jwtKey, @@ -86,7 +123,9 @@ export const clerkMiddleware = (options?: ClerkMiddlewareOptions): MiddlewareFun signInUrl, signUpUrl, acceptsToken: 'any', - }); + }; + + const requestState = await clerkClient(args, options).authenticateRequest(clerkRequest, authenticateOptions); const locationHeader = requestState.headers.get(constants.Headers.Location); if (locationHeader) { @@ -103,18 +142,20 @@ export const clerkMiddleware = (options?: ClerkMiddlewareOptions): MiddlewareFun throw new Error('Clerk: handshake status without redirect'); } + const additionalState: AdditionalStateOptions = { + __keylessClaimUrl, + __keylessApiKeysUrl, + signInForceRedirectUrl: loadedOptions.signInForceRedirectUrl, + signUpForceRedirectUrl: loadedOptions.signUpForceRedirectUrl, + signInFallbackRedirectUrl: loadedOptions.signInFallbackRedirectUrl, + signUpFallbackRedirectUrl: loadedOptions.signUpFallbackRedirectUrl, + }; + + // Stash identity-free options for re-derivation; authFnContext/requestStateContext + // remain for back-compat but identity is no longer read from them. + args.context.set(requestOptionsContext, authenticateOptions); args.context.set(authFnContext, (opts?: PendingSessionOptions) => requestState.toAuth(opts)); - args.context.set(requestStateContext, { - requestState, - additionalState: { - __keylessClaimUrl, - __keylessApiKeysUrl, - signInForceRedirectUrl: loadedOptions.signInForceRedirectUrl, - signUpForceRedirectUrl: loadedOptions.signUpForceRedirectUrl, - signInFallbackRedirectUrl: loadedOptions.signInFallbackRedirectUrl, - signUpFallbackRedirectUrl: loadedOptions.signUpFallbackRedirectUrl, - }, - }); + args.context.set(requestStateContext, { requestState, additionalState }); const response = await next(); diff --git a/packages/react-router/src/server/getAuth.ts b/packages/react-router/src/server/getAuth.ts index 3455666a15e..a3d21c242a5 100644 --- a/packages/react-router/src/server/getAuth.ts +++ b/packages/react-router/src/server/getAuth.ts @@ -6,9 +6,8 @@ import { import type { PendingSessionOptions } from '@clerk/shared/types'; import type { LoaderFunctionArgs } from 'react-router'; -import { IsOptIntoMiddleware } from '../server/utils'; import { noLoaderArgsPassedInGetAuth } from '../utils/errors'; -import { authFnContext } from './clerkMiddleware'; +import { authenticateFromRequest } from './clerkMiddleware'; type GetAuthOptions = PendingSessionOptions & { acceptsToken?: AuthenticateRequestOptions['acceptsToken'] }; @@ -22,15 +21,12 @@ export const getAuth: GetAuthFn = (async ( const { acceptsToken, treatPendingAsSignedOut } = opts || {}; - const authObjectFn = IsOptIntoMiddleware(args.context) && args.context.get(authFnContext); - if (!authObjectFn) { - throw new Error( - 'Clerk: clerkMiddleware() not detected. Make sure you have installed the clerkMiddleware in your root route.', - ); - } + // Re-derive auth from this request rather than reading a cached value, so a + // shared context can never return another user. + const requestState = await authenticateFromRequest(args, 'any'); return getAuthObjectForAcceptedToken({ - authObject: authObjectFn({ treatPendingAsSignedOut }), + authObject: requestState.toAuth({ treatPendingAsSignedOut }), acceptsToken, }); }) as GetAuthFn; diff --git a/packages/react-router/src/server/rootAuthLoader.ts b/packages/react-router/src/server/rootAuthLoader.ts index 5a1d625e604..f5d23440eb2 100644 --- a/packages/react-router/src/server/rootAuthLoader.ts +++ b/packages/react-router/src/server/rootAuthLoader.ts @@ -2,7 +2,7 @@ import type { RequestState } from '@clerk/backend/internal'; import type { LoaderFunctionArgs } from 'react-router'; import { invalidRootLoaderCallbackReturn } from '../utils/errors'; -import { authFnContext, requestStateContext } from './clerkMiddleware'; +import { authenticateFromRequest, authFnContext, requestStateContext } from './clerkMiddleware'; import type { AdditionalStateOptions, LoaderFunctionArgsWithAuth, @@ -128,6 +128,8 @@ export const rootAuthLoader: RootAuthLoader = async ( ); } - const { requestState, additionalState } = contextValue; - return processRootAuthLoader(args, requestState, additionalState, handler); + // Re-derive from this request rather than the cached context value, so identity + // can never bleed across concurrent requests. + const requestState = await authenticateFromRequest(args, 'any'); + return processRootAuthLoader(args, requestState, contextValue.additionalState, handler); };