diff --git a/.changeset/olive-poets-shake.md b/.changeset/olive-poets-shake.md new file mode 100644 index 00000000000..256369417df --- /dev/null +++ b/.changeset/olive-poets-shake.md @@ -0,0 +1,5 @@ +--- +'@clerk/nextjs': patch +--- + +Fix a cross-request credential leak in `clerkMiddleware()`. When using dynamic keys (an options callback that resolves a different `secretKey` per request), a `clerkClient()` call made inside the middleware handler could be built with another concurrent request's secret key. Each request now gets its own isolated store, so the keys resolved for a request are only ever visible to that request. diff --git a/packages/nextjs/src/server/__tests__/clerkMiddlewareRequestIsolation.test.ts b/packages/nextjs/src/server/__tests__/clerkMiddlewareRequestIsolation.test.ts new file mode 100644 index 00000000000..f4643eea086 --- /dev/null +++ b/packages/nextjs/src/server/__tests__/clerkMiddlewareRequestIsolation.test.ts @@ -0,0 +1,86 @@ +import { createClerkClient } from '@clerk/backend'; +import { TokenType } from '@clerk/backend/internal'; +import type { NextFetchEvent } from 'next/server'; +import { NextRequest } from 'next/server'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { clerkClient } from '../clerkClient'; +import { clerkMiddleware } from '../clerkMiddleware'; + +const { publishableKey } = vi.hoisted(() => ({ + publishableKey: 'pk_test_Y2xlcmsuaW5jbHVkZWQua2F0eWRpZC05Mi5sY2wuZGV2JA', +})); + +let gateAuthenticateRequest: (secretKey: string) => Promise; + +vi.mock('@clerk/backend', async importOriginal => { + const actual: any = await importOriginal(); + return { + ...actual, + createClerkClient: vi.fn((options: any) => ({ + secretKey: options.secretKey, + telemetry: { record: vi.fn() }, + authenticateRequest: async () => { + await gateAuthenticateRequest(options.secretKey); + return { + toAuth: () => ({ tokenType: TokenType.SessionToken, debug: (d: any) => d }), + headers: new Headers(), + publishableKey, + }; + }, + })), + }; +}); + +vi.mock(import('../constants.js'), async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + ENCRYPTION_KEY: 'encryption-key', + PUBLISHABLE_KEY: publishableKey, + SECRET_KEY: 'sk_test_environment', + }; +}); + +// The mocked `createClerkClient` exposes the key it was constructed with +const secretKeyOf = async () => ((await clerkClient()) as unknown as { secretKey: string }).secretKey; + +const requestForTenant = (tenant: string) => + new NextRequest('https://www.clerk.com/', { headers: new Headers({ 'x-tenant': tenant }) }); + +describe('clerkMiddleware request isolation', () => { + beforeEach(() => { + vi.mocked(createClerkClient).mockClear(); + }); + + it('does not leak dynamic keys between concurrent requests', async () => { + let releaseTenantA!: () => void; + const tenantAGate = new Promise(resolve => { + releaseTenantA = resolve; + }); + gateAuthenticateRequest = secretKey => (secretKey === 'sk_test_a' ? tenantAGate : Promise.resolve()); + + const secretKeyInHandler: Record = {}; + const middleware = clerkMiddleware( + async (_auth, request) => { + const tenant = request.headers.get('x-tenant') as string; + secretKeyInHandler[tenant] = await secretKeyOf(); + }, + request => ({ publishableKey, secretKey: `sk_test_${request.headers.get('x-tenant')}` }), + ); + + // Tenant A parks inside `authenticateRequest` while tenant B runs to completion. + const tenantA = middleware(requestForTenant('a'), {} as NextFetchEvent); + await middleware(requestForTenant('b'), {} as NextFetchEvent); + releaseTenantA(); + await tenantA; + + expect(secretKeyInHandler).toEqual({ a: 'sk_test_a', b: 'sk_test_b' }); + }); + + it('falls back to the environment key outside of a middleware request', async () => { + gateAuthenticateRequest = () => Promise.resolve(); + + expect(await secretKeyOf()).toBe('sk_test_environment'); + }); +}); diff --git a/packages/nextjs/src/server/clerkMiddleware.ts b/packages/nextjs/src/server/clerkMiddleware.ts index 2da7b688428..a0e0fb9ceeb 100644 --- a/packages/nextjs/src/server/clerkMiddleware.ts +++ b/packages/nextjs/src/server/clerkMiddleware.ts @@ -37,7 +37,8 @@ import { clerkClient } from './clerkClient'; import { DOMAIN, PROXY_URL, PUBLISHABLE_KEY, SECRET_KEY, SIGN_IN_URL, SIGN_UP_URL } from './constants'; import { type ContentSecurityPolicyOptions, createContentSecurityPolicyHeaders } from './content-security-policy'; import { errorThrower } from './errorThrower'; -import { clerkMiddlewareRequestDataStorage, clerkMiddlewareRequestDataStore } from './middleware-storage'; +import type { ClerkMiddlewareRequestDataStore } from './middleware-storage'; +import { clerkMiddlewareRequestDataStorage } from './middleware-storage'; import { isNextjsNotFoundError, isNextjsRedirectError, @@ -145,63 +146,64 @@ export const clerkMiddleware = ((...args: unknown[]): NextMiddleware | NextMiddl const [request, event] = parseRequestAndEvent(args); const [handler, params] = parseHandlerAndOptions(args); - const middleware = clerkMiddlewareRequestDataStorage.run(clerkMiddlewareRequestDataStore, () => { - const baseNextMiddleware: NextMiddleware = withLogger('clerkMiddleware', logger => async (request, event) => { - // Handles the case where `options` is a callback function to dynamically access `NextRequest` - const resolvedParams = typeof params === 'function' ? await params(request) : params; + const baseNextMiddleware: NextMiddleware = withLogger('clerkMiddleware', logger => async (request, event) => { + // Handles the case where `options` is a callback function to dynamically access `NextRequest` + const resolvedParams = typeof params === 'function' ? await params(request) : params; - const publishableKey = assertKey(resolvedParams.publishableKey || PUBLISHABLE_KEY, () => - errorThrower.throwMissingPublishableKeyError(), - ); + const publishableKey = assertKey(resolvedParams.publishableKey || PUBLISHABLE_KEY, () => + errorThrower.throwMissingPublishableKeyError(), + ); - const secretKey = assertKey(resolvedParams.secretKey || SECRET_KEY, () => - errorThrower.throwMissingSecretKeyError(), - ); + const secretKey = assertKey(resolvedParams.secretKey || SECRET_KEY, () => + errorThrower.throwMissingSecretKeyError(), + ); - // Handle Frontend API proxy requests early, before authentication - const requestUrl = new URL(request.nextUrl.href); - let frontendApiProxyConfig = resolvedParams.frontendApiProxy; - - // Auto-detect when no explicit proxy or domain is configured - const hasExplicitProxyOrDomain = resolvedParams.proxyUrl || PROXY_URL || resolvedParams.domain || DOMAIN; - if ( - !frontendApiProxyConfig && - !hasExplicitProxyOrDomain && - !isAutoProxyDisabledFromEnvironment() && - isProductionFromPublishableKey(publishableKey) - ) { - if (shouldAutoProxy(requestUrl.hostname)) { - frontendApiProxyConfig = { enabled: true }; - } + // Handle Frontend API proxy requests early, before authentication + const requestUrl = new URL(request.nextUrl.href); + let frontendApiProxyConfig = resolvedParams.frontendApiProxy; + + // Auto-detect when no explicit proxy or domain is configured + const hasExplicitProxyOrDomain = resolvedParams.proxyUrl || PROXY_URL || resolvedParams.domain || DOMAIN; + if ( + !frontendApiProxyConfig && + !hasExplicitProxyOrDomain && + !isAutoProxyDisabledFromEnvironment() && + isProductionFromPublishableKey(publishableKey) + ) { + if (shouldAutoProxy(requestUrl.hostname)) { + frontendApiProxyConfig = { enabled: true }; } - if (frontendApiProxyConfig) { - const { enabled, path: proxyPath = DEFAULT_PROXY_PATH } = frontendApiProxyConfig; - - // Resolve enabled - either boolean or function - const isEnabled = typeof enabled === 'function' ? enabled(requestUrl) : enabled; - - if (isEnabled && matchProxyPath(request, { proxyPath })) { - return clerkFrontendApiProxy(request, { - proxyPath, - publishableKey, - secretKey, - }); - } + } + if (frontendApiProxyConfig) { + const { enabled, path: proxyPath = DEFAULT_PROXY_PATH } = frontendApiProxyConfig; + + // Resolve enabled - either boolean or function + const isEnabled = typeof enabled === 'function' ? enabled(requestUrl) : enabled; + + if (isEnabled && matchProxyPath(request, { proxyPath })) { + return clerkFrontendApiProxy(request, { + proxyPath, + publishableKey, + secretKey, + }); } + } + + const signInUrl = resolvedParams.signInUrl || SIGN_IN_URL; + const signUpUrl = resolvedParams.signUpUrl || SIGN_UP_URL; - const signInUrl = resolvedParams.signInUrl || SIGN_IN_URL; - const signUpUrl = resolvedParams.signUpUrl || SIGN_UP_URL; + const options = { + publishableKey, + secretKey, + signInUrl, + signUpUrl, + ...resolvedParams, + }; - const options = { - publishableKey, - secretKey, - signInUrl, - signUpUrl, - ...resolvedParams, - }; + // Propagates the request data to be accessed on the server application runtime from helpers such as `clerkClient` + const requestDataStore: ClerkMiddlewareRequestDataStore = new Map([['requestData', options]]); - // Propagates the request data to be accessed on the server application runtime from helpers such as `clerkClient` - clerkMiddlewareRequestDataStore.set('requestData', options); + return clerkMiddlewareRequestDataStorage.run(requestDataStore, async () => { const resolvedClerkClient = await clerkClient(); if (options.debug) { @@ -239,19 +241,17 @@ export const clerkMiddleware = ((...args: unknown[]): NextMiddleware | NextMiddl logger, }); }); - - // If we have a request and event, we're being called as a middleware directly - // eg, export default clerkMiddleware; - if (request && event) { - return baseNextMiddleware(request, event); - } - - // Otherwise, return a middleware that can be called with a request and event - // eg, export default clerkMiddleware(auth => { ... }); - return baseNextMiddleware; }); - return middleware; + // If we have a request and event, we're being called as a middleware directly + // eg, export default clerkMiddleware; + if (request && event) { + return baseNextMiddleware(request, event); + } + + // Otherwise, return a middleware that can be called with a request and event + // eg, export default clerkMiddleware(auth => { ... }); + return baseNextMiddleware; }) as ClerkMiddleware; const parseRequestAndEvent = (args: unknown[]) => { @@ -338,9 +338,7 @@ async function runHandlerWithRequestState({ let handlerResult: Response = NextResponse.next(); try { - const userHandlerResult = await clerkMiddlewareRequestDataStorage.run(clerkMiddlewareRequestDataStore, async () => - handler?.(authHandler, request, event), - ); + const userHandlerResult = await handler?.(authHandler, request, event); handlerResult = userHandlerResult || handlerResult; } catch (e: any) { handlerResult = handleControlFlowErrors(e, clerkRequest, request, requestState); diff --git a/packages/nextjs/src/server/middleware-storage.ts b/packages/nextjs/src/server/middleware-storage.ts index ec22bf4163f..505b74e15bb 100644 --- a/packages/nextjs/src/server/middleware-storage.ts +++ b/packages/nextjs/src/server/middleware-storage.ts @@ -2,5 +2,6 @@ import { AsyncLocalStorage } from 'node:async_hooks'; import type { AuthenticateRequestOptions } from '@clerk/backend/internal'; -export const clerkMiddlewareRequestDataStore = new Map<'requestData', AuthenticateRequestOptions>(); -export const clerkMiddlewareRequestDataStorage = new AsyncLocalStorage(); +export type ClerkMiddlewareRequestDataStore = Map<'requestData', AuthenticateRequestOptions>; + +export const clerkMiddlewareRequestDataStorage = new AsyncLocalStorage();