fix(react-router): resolve getAuth per-request to prevent cross-user auth bleed#8896
Open
jacekradko wants to merge 9 commits into
Open
fix(react-router): resolve getAuth per-request to prevent cross-user auth bleed#8896jacekradko wants to merge 9 commits into
jacekradko wants to merge 9 commits into
Conversation
…auth bleed getAuth and rootAuthLoader now re-derive the request's auth from args.request instead of reading a value cached on the React Router context, so an app that shares one RouterContextProvider across requests (a custom server or getLoadContext that returns one instance) can no longer be served another user's auth under concurrency, including across the action-to-loader hop. The middleware stashes only identity-free options for the re-derivation to reuse, matching how @clerk/nextjs resolves auth, with no node:async_hooks dependency. clerkMiddleware also logs a warning once when it detects a context reused across requests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 23273d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors React Router Clerk middleware to derive auth per-request from the live request object. Introduces ChangesRequest-scoped auth isolation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-router/src/server/__tests__/getAuth.test.ts (1)
16-21: ⚡ Quick winRemove
anyfrom the mocked auth contract.Using
options?: anyandas anyweakens this test’s ability to catch type drift intoAuth/authenticateRequestwiring.Suggested typed patch
+import type { ClerkClient } from '`@clerk/backend`'; import { TokenType } from '`@clerk/backend/internal`'; import type { LoaderFunctionArgs } from 'react-router'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ mockClerkClient.mockReturnValue({ authenticateRequest: vi.fn().mockResolvedValue({ headers: new Headers(), - toAuth: (options?: any) => ({ userId: 'user_xxx', tokenType: TokenType.SessionToken, ...options }), + toAuth: (options?: Record<string, unknown>) => ({ + userId: 'user_xxx', + tokenType: TokenType.SessionToken, + ...(options ?? {}), + }), }), - } as any); + } as unknown as ClerkClient); });As per coding guidelines, “Avoid
anytype - preferunknownwhen type is uncertain” and “Noanytypes without justification in code review.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/__tests__/getAuth.test.ts` around lines 16 - 21, In the mockClerkClient setup, remove the `any` types to improve type safety. Replace the `options?: any` parameter in the toAuth function with a properly typed parameter (likely based on the actual ClerkClient type or a subset interface), and remove the `as any` type assertion by properly typing the entire mock return value. Look at the actual ClerkClient interface from the Clerk library to understand what types toAuth expects and what authenticateRequest should return, then apply those types to the mock object instead of using the loose `any` casting.Source: Coding guidelines
packages/react-router/src/server/__tests__/rootAuthLoader.test.ts (1)
27-30: ⚡ Quick winAvoid
anycasts in the new rootAuthLoader test path.These two
anycasts make it easier for auth-state shape regressions to slip through this suite.Suggested typed patch
+import type { ClerkClient } from '`@clerk/backend`'; import { TokenType } from '`@clerk/backend/internal`'; @@ mockClerkClient.mockReturnValue({ authenticateRequest: vi.fn().mockResolvedValue(mockRequestState), - } as any); + } as unknown as ClerkClient); }); @@ + type RedirectStateResult = { + clerkState: { + __internal_clerk_state: { + __signInForceRedirectUrl?: string; + __signUpForceRedirectUrl?: string; + __signInFallbackRedirectUrl?: string; + __signUpFallbackRedirectUrl?: string; + }; + }; + }; + const result = (await rootAuthLoader({ context: makeContext({ signInForceRedirectUrl: '/dashboard', @@ }), request: new Request('http://clerk.com'), - } as unknown as LoaderFunctionArgs)) as any; + } as unknown as LoaderFunctionArgs)) as RedirectStateResult;As per coding guidelines, “Avoid
anytype - preferunknownwhen type is uncertain” and “Noanytypes without justification in code review.”Also applies to: 124-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/__tests__/rootAuthLoader.test.ts` around lines 27 - 30, Replace the `as any` cast in the mockClerkClient.mockReturnValue setup with proper typing that matches the actual shape of the Clerk client object. Instead of casting to `any`, provide explicit type information for the mock object structure that includes the authenticateRequest method and any other required properties. Apply the same typing fix to the similar mock setup at the other location in the file (around line 124) to ensure both mock instances have proper type safety rather than using `any` casts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react-router/src/server/__tests__/getAuth.test.ts`:
- Around line 16-21: In the mockClerkClient setup, remove the `any` types to
improve type safety. Replace the `options?: any` parameter in the toAuth
function with a properly typed parameter (likely based on the actual ClerkClient
type or a subset interface), and remove the `as any` type assertion by properly
typing the entire mock return value. Look at the actual ClerkClient interface
from the Clerk library to understand what types toAuth expects and what
authenticateRequest should return, then apply those types to the mock object
instead of using the loose `any` casting.
In `@packages/react-router/src/server/__tests__/rootAuthLoader.test.ts`:
- Around line 27-30: Replace the `as any` cast in the
mockClerkClient.mockReturnValue setup with proper typing that matches the actual
shape of the Clerk client object. Instead of casting to `any`, provide explicit
type information for the mock object structure that includes the
authenticateRequest method and any other required properties. Apply the same
typing fix to the similar mock setup at the other location in the file (around
line 124) to ensure both mock instances have proper type safety rather than
using `any` casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eb8f63c6-035b-4c5d-8ccc-9f7bd9134392
📒 Files selected for processing (8)
.changeset/rr-request-scoped-auth.mdpackages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.tspackages/react-router/src/server/__tests__/clerkMiddleware.sharedContextWarning.test.tspackages/react-router/src/server/__tests__/getAuth.test.tspackages/react-router/src/server/__tests__/rootAuthLoader.test.tspackages/react-router/src/server/clerkMiddleware.tspackages/react-router/src/server/getAuth.tspackages/react-router/src/server/rootAuthLoader.ts
Member
Author
|
Perf note (updated for the per-request memo design) Earlier numbers here assumed Re-measured locally (real RS256 networkless, JWKS warm):
The common path is effectively free and matches the previous cached behavior (both just call |
…memo Resolve the request's auth once in clerkMiddleware and cache it keyed by the Request object (not the shared context), so getAuth/rootAuthLoader reuse it and a context reused across requests (custom server, shared getLoadContext, or a serverless adapter on a warm instance) can't return another user. Re-derive from the request's own cookies only on a Request-instance miss (action -> loader), so token verification and refresh still happen once per request.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.ts`:
- Around line 162-170: The test in the middleware callback currently only calls
readUserId once on the loaderArgs, which means it cannot verify that the
re-derived auth state is being properly cached. To make the caching behavior
observable, call readUserId on the loaderArgs twice (keeping both assertions
that the result is 'user_A'), and keep the authSpy call count expectation at 2.
This ensures that if caching is not working, the spy would be called 3 or 4
times instead of 2, making the test fail and catch the bug.
In `@packages/react-router/src/server/clerkMiddleware.ts`:
- Around line 57-60: The return statement in the clerkMiddleware function that
calls authenticateRequest() does not cache the resulting RequestState, causing
repeated calls with the same request object to re-run authentication instead of
using a cached value. Capture the result of the authenticateRequest() call,
store it in the requestStateByRequest cache using the request as the key, and
then return the cached value to ensure subsequent calls for the same request
reuse the cached state instead of re-authenticating.
- Around line 171-172: The authFnContext and requestStateContext are being set
at lines 171-172 in clerkMiddleware.ts but are not part of the public API and
should be deprecated. Add `@internal` or `@deprecated` JSDoc markers to the export
declarations of authFnContext and requestStateContext at the top of
clerkMiddleware.ts to signal they are not supported for public use. Ensure the
internal implementation (including rootAuthLoader and other server code) uses
requestStateByRequest for the request-keyed cache instead of relying on these
context objects, so the contexts can be safely removed in a future major
version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7011b6fb-e040-4ddc-843b-b988789c9c42
📒 Files selected for processing (5)
.changeset/rr-request-scoped-auth.mdpackages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.tspackages/react-router/src/server/clerkMiddleware.tspackages/react-router/src/server/getAuth.tspackages/react-router/src/server/rootAuthLoader.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/rr-request-scoped-auth.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/src/server/rootAuthLoader.ts
…ache 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router/src/server/clerkMiddleware.ts (1)
117-148:⚠️ Potential issue | 🟠 MajorAdd missing
afterSignInUrl,afterSignUpUrl, andsatelliteAutoSyncto middleware configuration.The middleware option type
ClerkMiddlewareOptionsdoes not exposeafterSignInUrl,afterSignUpUrl, orsatelliteAutoSynceven though they are supported by the backend'sAuthenticateRequestOptionsand used in authentication logic. This means configured post-sign-in/sign-up redirect URLs and satellite auto-sync settings cannot be passed through the middleware.Additionally,
getPublicEnvVariables()already reads environment variables forafterSignInUrlandafterSignUpUrl(lines 31-32 inenv.ts), but these are never integrated intoloadOptions()orClerkMiddlewareOptions. ThesatelliteAutoSyncoption is documented in the CHANGELOG as a clerkMiddleware configuration option but is currently unavailable.Add these fields to
ClerkMiddlewareOptionsintypes.ts, extract them inloadOptions(), and pass them through toauthenticateOptionsinclerkMiddleware.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/clerkMiddleware.ts` around lines 117 - 148, Add support for the missing authentication options by first ensuring `afterSignInUrl`, `afterSignUpUrl`, and `satelliteAutoSync` are included in the `ClerkMiddlewareOptions` type definition in `types.ts`, then extracting these fields in the `loadOptions()` function. In the `clerkMiddleware.ts` file, update the destructuring block that pulls properties from `loadedOptions` to include these three new fields alongside the existing ones, and then add these same three properties to the `authenticateOptions` object being constructed so they are passed through to the authentication request.
🧹 Nitpick comments (2)
packages/react-router/src/server/__tests__/getAuth.test.ts (1)
24-46: ⚡ Quick winAssert the
authenticateRequestcall path explicitly in this regression test.This test currently proves output shape, but it doesn’t lock in the critical behavior that
getAuthre-derives viaclerkClient(...).authenticateRequest(...). Please add call assertions so future refactors can’t silently bypass the request-based re-auth path.Suggested test hardening
const auth = await getAuth(args); expect(auth.userId).toBe('user_xxx'); expect(auth.tokenType).toBe('session_token'); + expect(mockClerkClient).toHaveBeenCalledWith( + args, + expect.objectContaining({ + secretKey: 'sk_test_...', + publishableKey: 'pk_test_...', + }), + ); + const returnedClient = mockClerkClient.mock.results[0]?.value as { authenticateRequest: ReturnType<typeof vi.fn> }; + expect(returnedClient.authenticateRequest).toHaveBeenCalledTimes(1);As per coding guidelines,
**/*.{test,spec}.{ts,tsx}should implement comprehensive testing, including edge-case/regression coverage for new functionality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/__tests__/getAuth.test.ts` around lines 24 - 46, The test "should re-derive auth from the request when middleware ran" currently only verifies the output shape (userId and tokenType) but doesn't assert that the critical re-derivation path via clerkClient().authenticateRequest() is actually being called. Add explicit call assertions by mocking or spying on the clerkClient and its authenticateRequest method, then verify with expect statements that authenticateRequest was invoked with the expected request object. This ensures future refactors cannot accidentally bypass the request-based re-authentication path.Source: Coding guidelines
packages/react-router/src/server/clerkMiddleware.ts (1)
23-28: ⚡ Quick winMark the new exported plumbing internal or make it public-API ready.
middlewareConfigContextandresolveRequestState()look like internal plumbing but are exported; if they can surface in generated docs or direct imports, add@internalJSDoc, or document them fully and avoid exposingRequestState<any>. As per coding guidelines, public/reference-facing APIs inpackages/**/srcrequire accurate JSDoc, and generated Clerk Docs treat such JSDoc as customer-facing documentation.📝 Internal-only option
-// Request-INDEPENDENT config (the static clerkMiddleware options plus resolved keys) -// used to re-derive auth on a Request-instance miss. It deliberately does NOT hold -// resolved request-derived values (domain/proxyUrl/isSatellite); those are -// re-resolved per request from args.request, so a shared context can't leak one -// request's resolved options to another. +/** + * `@internal` + * + * Request-independent config used to re-derive auth on a Request-instance miss. + * It deliberately does not hold resolved request-derived values. + */ export const middlewareConfigContext = createContext<ClerkMiddlewareOptions | null>(null); @@ /** + * `@internal` + * * Auth state for this request. Reuses what clerkMiddleware already resolved (keyedAlso applies to: 39-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/clerkMiddleware.ts` around lines 23 - 28, The exported symbols middlewareConfigContext and resolveRequestState() appear to be internal implementation details but lack proper JSDoc documentation. Since these symbols are exported from packages/react-router/src and could surface in generated documentation or direct imports, add `@internal` JSDoc comments above both middlewareConfigContext (the createContext call) and the resolveRequestState function definition to mark them as internal-only plumbing, preventing them from appearing in public-facing Clerk documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-router/src/server/clerkMiddleware.ts`:
- Line 37: Concurrent calls to getAuth() and rootAuthLoader() for the same
Request object are duplicating authenticateRequest() work because they don't see
the in-flight promise. Modify the requestStateByRequest WeakMap to store the
authentication promise before awaiting it, allowing concurrent calls to the same
Request to reuse the existing promise instead of creating new ones. When the
promise resolves or rejects, clear it from the map so subsequent requests will
trigger a fresh authentication. This requires storing the promise itself (not
just the final RequestState) and checking for existing in-flight promises before
calling authenticateRequest().
---
Outside diff comments:
In `@packages/react-router/src/server/clerkMiddleware.ts`:
- Around line 117-148: Add support for the missing authentication options by
first ensuring `afterSignInUrl`, `afterSignUpUrl`, and `satelliteAutoSync` are
included in the `ClerkMiddlewareOptions` type definition in `types.ts`, then
extracting these fields in the `loadOptions()` function. In the
`clerkMiddleware.ts` file, update the destructuring block that pulls properties
from `loadedOptions` to include these three new fields alongside the existing
ones, and then add these same three properties to the `authenticateOptions`
object being constructed so they are passed through to the authentication
request.
---
Nitpick comments:
In `@packages/react-router/src/server/__tests__/getAuth.test.ts`:
- Around line 24-46: The test "should re-derive auth from the request when
middleware ran" currently only verifies the output shape (userId and tokenType)
but doesn't assert that the critical re-derivation path via
clerkClient().authenticateRequest() is actually being called. Add explicit call
assertions by mocking or spying on the clerkClient and its authenticateRequest
method, then verify with expect statements that authenticateRequest was invoked
with the expected request object. This ensures future refactors cannot
accidentally bypass the request-based re-authentication path.
In `@packages/react-router/src/server/clerkMiddleware.ts`:
- Around line 23-28: The exported symbols middlewareConfigContext and
resolveRequestState() appear to be internal implementation details but lack
proper JSDoc documentation. Since these symbols are exported from
packages/react-router/src and could surface in generated documentation or direct
imports, add `@internal` JSDoc comments above both middlewareConfigContext (the
createContext call) and the resolveRequestState function definition to mark them
as internal-only plumbing, preventing them from appearing in public-facing Clerk
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4e33398a-0c9e-4da0-873f-da63f8c6eaae
📒 Files selected for processing (4)
packages/react-router/src/server/__tests__/clerkMiddleware.authIsolation.test.tspackages/react-router/src/server/__tests__/getAuth.test.tspackages/react-router/src/server/__tests__/rootAuthLoader.test.tspackages/react-router/src/server/clerkMiddleware.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-router/src/server/tests/rootAuthLoader.test.ts
- packages/react-router/src/server/tests/clerkMiddleware.authIsolation.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@clerk/react-routerderived per-request auth from the React Routercontext:clerkMiddlewarecached the resolved auth onargs.contextandgetAuth/rootAuthLoaderread it back. That is correct only while the context is unique per request. If any layer reuses the context across requests, a custom server, a sharedgetLoadContext, or a serverless adapter that reuses it on a warm instance, one request can be served another user's auth.This hardens that path. The middleware still resolves auth once, but now caches it keyed by the
Requestobject (which the runtime creates per request and an app cannot alias), andgetAuth/rootAuthLoaderread that, re-deriving from the request's own cookies only when theRequestinstance differs (React Router's action-to-loader hop). So a reused context can no longer return the wrong user, and token verification and refresh still run once per request.clerkMiddlewarealso logs a one-time warning when it detects a reused context.Worth reviewing:
resolveRequestState(theWeakMap<Request, ...>memo plus the re-derive fallback) and that the cache key is theRequest, never the context.Scope: this protects against a reused context. A layer that reuses the
Requestobject itself (stale cookies and all) would be an adapter bug this can't paper over. This came out of a cross-user bleed investigation and is defensive hardening; it is not claimed to be the root-cause fix for any specific incident. Supersedes #8890.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Versioning
@clerk/react-router.