fix(express): prevent auth bypass from foreign req.auth values (#8804)

Co-authored-by: Robert Soriano <sorianorobertc@gmail.com>

Author: Jacek Radko

.changeset/spicy-llamas-brush.md

@@ -0,0 +1,5 @@
+---
+'@clerk/express': patch
+---
+
+Fix an authentication bypass where a `req.auth` value set by another library (such as `express-jwt`, Passport, or `express-openid-connect`) caused `clerkMiddleware()` and `requireAuth()` to silently skip Clerk authentication, and `getAuth()` to return the unverified foreign value. Clerk now brands the `req.auth` handler it installs and only trusts that handler: `clerkMiddleware()` authenticates the request and overwrites a foreign `req.auth` (logging a warning when it does), and `getAuth()` throws its "middleware required" error instead of reading foreign data.

packages/express/src/__tests__/clerkMiddleware.test.ts

@@ -25,9 +25,17 @@ vi.mock('@clerk/backend', async () => {
   };
 });
 
+const { mockWarnOnce } = vi.hoisted(() => ({
+  mockWarnOnce: vi.fn(),
+}));
+vi.mock('@clerk/shared/logger', () => ({
+  logger: { warnOnce: mockWarnOnce, logOnce: vi.fn() },
+}));
+
 import { authenticateAndDecorateRequest, authenticateRequest } from '../authenticateRequest';
 import { clerkMiddleware } from '../clerkMiddleware';
 import { getAuth } from '../getAuth';
+import { requireAuth } from '../requireAuth';
 import { assertNoDebugHeaders, assertSignedOutDebugHeaders, runMiddleware, runMiddlewareOnPath } from './helpers';
 
 describe('clerkMiddleware', () => {
@@ -331,6 +339,87 @@ describe('clerkMiddleware', () => {
     expect(response.header).toHaveProperty('location', expect.stringContaining('/v1/client/handshake?redirect_url='));
   });
 
+  describe('foreign req.auth values (authentication bypass hardening)', () => {
+    beforeEach(() => {
+      mockWarnOnce.mockClear();
+    });
+
+    it('authenticates the request even when a previous middleware set req.auth to a plain object (express-jwt style)', async () => {
+      const foreignAuth: RequestHandler = (req, _res, next) => {
+        Object.assign(req, { auth: { sub: 'attacker' } });
+        next();
+      };
+
+      const response = await runMiddleware([foreignAuth, clerkMiddleware()], {
+        Cookie: '__clerk_db_jwt=deadbeef;',
+      }).expect(200, 'Hello world!');
+
+      assertSignedOutDebugHeaders(response);
+      expect(mockWarnOnce).toHaveBeenCalledTimes(1);
+    });
+
+    it('overwrites a foreign req.auth function so getAuth returns Clerk state', async () => {
+      const foreignAuth: RequestHandler = (req, _res, next) => {
+        Object.assign(req, { auth: () => ({ userId: 'attacker' }) });
+        next();
+      };
+
+      let capturedUserId: string | null | undefined;
+      const capture: RequestHandler = (req, _res, next) => {
+        capturedUserId = getAuth(req).userId;
+        next();
+      };
+
+      const response = await runMiddleware([foreignAuth, clerkMiddleware(), capture], {
+        Cookie: '__clerk_db_jwt=deadbeef;',
+      }).expect(200, 'Hello world!');
+
+      assertSignedOutDebugHeaders(response);
+      expect(capturedUserId).toBeNull();
+    });
+
+    it('does not re-authenticate when clerkMiddleware runs twice', async () => {
+      const authenticateRequestSpy = vi.fn().mockResolvedValue({
+        headers: new Headers(),
+        status: 'signed-out',
+        toAuth: () => ({ userId: null }),
+      });
+      const clerkClient = { authenticateRequest: authenticateRequestSpy } as any;
+
+      await runMiddleware([clerkMiddleware({ clerkClient }), clerkMiddleware({ clerkClient })]).expect(
+        200,
+        'Hello world!',
+      );
+
+      expect(authenticateRequestSpy).toHaveBeenCalledTimes(1);
+      expect(mockWarnOnce).not.toHaveBeenCalled();
+    });
+
+    it('requireAuth is not bypassed by a foreign req.auth that reports a userId', async () => {
+      const foreignAuth: RequestHandler = (req, _res, next) => {
+        Object.assign(req, { auth: () => ({ userId: 'attacker' }) });
+        next();
+      };
+
+      const response = await runMiddleware([foreignAuth, requireAuth({ signInUrl: '/sign-in' })]);
+
+      expect(response.status).toBe(302);
+      expect(response.headers.location).toBe('/sign-in');
+    });
+
+    it('requireAuth redirects (does not 500) when a foreign req.auth is a plain object', async () => {
+      const foreignAuth: RequestHandler = (req, _res, next) => {
+        Object.assign(req, { auth: { sub: 'attacker' } });
+        next();
+      };
+
+      const response = await runMiddleware([foreignAuth, requireAuth({ signInUrl: '/sign-in' })]);
+
+      expect(response.status).toBe(302);
+      expect(response.headers.location).toBe('/sign-in');
+    });
+  });
+
   describe('Frontend API proxy handling', () => {
     beforeEach(() => {
       mockClerkFrontendApiProxy.mockReset();

packages/express/src/__tests__/getAuth.test.ts

@@ -1,4 +1,5 @@
 import type { AuthenticatedMachineObject } from '@clerk/backend/internal';
+import type { Request as ExpressRequest } from 'express';
 import { describe, expect, it } from 'vitest';
 
 import { getAuth } from '../getAuth';
@@ -9,6 +10,16 @@ describe('getAuth', () => {
     expect(() => getAuth(mockRequest())).toThrow(/The "clerkMiddleware" should be registered before using "getAuth"/);
   });
 
+  it('throws error if req.auth is a plain object set by another library (express-jwt style)', () => {
+    const req = { auth: { sub: 'attacker' } } as unknown as ExpressRequest;
+    expect(() => getAuth(req)).toThrow(/The "clerkMiddleware" should be registered before using "getAuth"/);
+  });
+
+  it('throws error if req.auth is a function set by another library', () => {
+    const req = { auth: () => ({ userId: 'attacker' }) } as unknown as ExpressRequest;
+    expect(() => getAuth(req)).toThrow(/The "clerkMiddleware" should be registered before using "getAuth"/);
+  });
+
   it('returns auth from request for signed-out request', () => {
     const req = mockRequestWithAuth({ userId: null, tokenType: 'session_token' });
     const auth = getAuth(req);

packages/express/src/__tests__/helpers.ts

@@ -5,6 +5,7 @@ import supertest from 'supertest';
 import { expect, vi } from 'vitest';
 
 import type { ExpressRequestWithAuth } from '../types';
+import { brandRequestAuth } from '../utils';
 
 // Inspired by https://github.com/helmetjs/helmet/blob/main/test/helpers.ts
 export function runMiddleware(middleware: RequestHandler | RequestHandler[], headers: Record<string, string> = {}) {
@@ -41,13 +42,13 @@ export function mockRequest(): ExpressRequest {
 
 export function mockRequestWithAuth(auth: Partial<AuthObject> = {}): ExpressRequestWithAuth {
   return {
-    auth: () => ({
+    auth: brandRequestAuth(() => ({
       getToken: () => Promise.resolve(''),
       has: () => false,
       debug: () => ({}),
       tokenType: 'session_token',
       ...auth,
-    }),
+    })),
   } as unknown as ExpressRequestWithAuth;
 }
 

packages/express/src/__tests__/requireAuth.test.ts

@@ -4,7 +4,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest';
 
 import { clerkMiddleware } from '../clerkMiddleware';
 import { requireAuth } from '../requireAuth';
-import type { ExpressRequestWithAuth } from '../types';
+import { brandRequestAuth, requestHasAuthObject } from '../utils';
 import { mockRequestWithAuth, runMiddleware } from './helpers';
 
 let mockAuthenticateAndDecorateRequest: Mock;
@@ -87,11 +87,11 @@ describe('requireAuth', () => {
 
     mockAuthenticateAndDecorateRequest.mockImplementation((): RequestHandler => {
       return (req, _res, next) => {
-        if ((req as ExpressRequestWithAuth).auth) {
+        if (requestHasAuthObject(req)) {
           return next();
         }
         const requestState = mockAuthenticateRequest({ request: req });
-        Object.assign(req, { auth: () => requestState.toAuth() });
+        Object.assign(req, { auth: brandRequestAuth(() => requestState.toAuth()) });
         next();
       };
     });

packages/express/src/authenticateRequest.ts

@@ -3,15 +3,23 @@ import type { RequestState } from '@clerk/backend/internal';
 import { AuthStatus, createClerkRequest } from '@clerk/backend/internal';
 import { clerkFrontendApiProxy, DEFAULT_PROXY_PATH, stripTrailingSlashes } from '@clerk/backend/proxy';
 import { isDevelopmentFromSecretKey } from '@clerk/shared/keys';
+import { logger } from '@clerk/shared/logger';
 import { isHttpOrHttps, isProxyUrlRelative, isValidProxyUrl } from '@clerk/shared/proxy';
 import { handleValueOrFn } from '@clerk/shared/utils';
 import type { RequestHandler, Response } from 'express';
 import { Readable } from 'stream';
 
 import { clerkClient as defaultClerkClient } from './clerkClient';
 import { satelliteAndMissingProxyUrlAndDomain, satelliteAndMissingSignInUrl } from './errors';
-import type { AuthenticateRequestParams, ClerkMiddlewareOptions, ExpressRequestWithAuth } from './types';
-import { incomingMessageToRequest, loadApiEnv, loadClientEnv, requestToProxyRequest } from './utils';
+import type { AuthenticateRequestParams, ClerkMiddlewareOptions } from './types';
+import {
+  brandRequestAuth,
+  incomingMessageToRequest,
+  loadApiEnv,
+  loadClientEnv,
+  requestHasAuthObject,
+  requestToProxyRequest,
+} from './utils';
 
 /**
  * @internal
@@ -140,10 +148,21 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
 
   // eslint-disable-next-line @typescript-eslint/no-misused-promises
   const middleware: RequestHandler = async (request, response, next) => {
-    if ((request as ExpressRequestWithAuth).auth) {
+    // Skip authentication only when a Clerk middleware has already processed
+    // this request. The brand check matters: a truthy `req.auth` is not proof
+    // of that, since express-jwt, passport and other libraries set the same
+    // property, and treating their value as ours would silently disable Clerk
+    // authentication.
+    if (requestHasAuthObject(request)) {
       return next();
     }
 
+    if ('auth' in request) {
+      logger.warnOnce(
+        'Clerk: another middleware has already set `req.auth` on this request. Clerk authentication will run anyway and overwrite it. To use another auth library alongside Clerk, configure it to store its state on a different request property.',
+      );
+    }
+
     const env = { ...loadApiEnv(), ...loadClientEnv() };
     const publishableKey = options.publishableKey || env.publishableKey;
     const secretKey = options.secretKey || env.secretKey;
@@ -226,7 +245,7 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
         return;
       }
 
-      const auth = (opts: Parameters<typeof requestState.toAuth>[0]) => requestState.toAuth(opts);
+      const auth = brandRequestAuth((opts: Parameters<typeof requestState.toAuth>[0]) => requestState.toAuth(opts));
 
       Object.assign(request, { auth });
 

packages/express/src/utils.ts

@@ -4,8 +4,22 @@ import { Readable } from 'stream';
 
 import type { ExpressRequestWithAuth } from './types';
 
+/**
+ * Brand attached to the `req.auth` handler installed by Clerk middleware.
+ * `req.auth` is a property name shared with other auth libraries (express-jwt,
+ * passport, express-openid-connect), so its mere presence proves nothing about
+ * whether Clerk authenticated the request. `Symbol.for` registers the brand
+ * globally so it stays stable across multiple copies of this package loaded in
+ * the same process.
+ */
+const clerkAuthBrand = Symbol.for('@clerk/express.auth');
+
+export const brandRequestAuth = <T extends (...args: never[]) => unknown>(authHandler: T): T =>
+  Object.assign(authHandler, { [clerkAuthBrand]: true });
+
 export const requestHasAuthObject = (req: ExpressRequest): req is ExpressRequestWithAuth => {
-  return 'auth' in req;
+  const auth = (req as Partial<ExpressRequestWithAuth>).auth;
+  return typeof auth === 'function' && (auth as unknown as Record<symbol, unknown>)[clerkAuthBrand] === true;
 };
 
 export const loadClientEnv = () => {