fix(fastify,express): Respond 400 instead of 500 to requests that cannot be web requests (#9290)

Author: wobsoriano

.changeset/harden-request-conversion.md

@@ -0,0 +1,6 @@
+---
+'@clerk/fastify': patch
+'@clerk/express': patch
+---
+
+Respond with 400 Bad Request instead of surfacing a 500 when an incoming request cannot be represented as a fetch `Request`. Vulnerability-scanner probes such as hostless `//` request targets, targets that parse as credentialed URLs, and forbidden methods like TRACE previously threw inside the middleware and polluted error logs.

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

@@ -1,5 +1,7 @@
 import type * as ClerkBackend from '@clerk/backend';
 import type { Request, RequestHandler, Response } from 'express';
+import express from 'express';
+import supertest from 'supertest';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 const { mockClerkFrontendApiProxy } = vi.hoisted(() => ({
@@ -565,19 +567,46 @@ describe('clerkMiddleware', () => {
     });
   });
 
-  it('calls next with an error when request URL is invalid', () => {
-    const req = {
-      url: '//',
-      cookies: {},
-      headers: { host: 'example.com' },
-    } as Request;
-    const res = {} as Response;
-    const mockNext = vi.fn();
+  describe('requests that cannot be converted to a web Request', () => {
+    it('responds 400 without calling next when the request URL is invalid', async () => {
+      const req = {
+        method: 'GET',
+        url: '//',
+        cookies: {},
+        headers: { host: 'example.com' },
+      } as Request;
+      const status = vi.fn().mockReturnThis();
+      const end = vi.fn();
+      const res = { status, end } as unknown as Response;
+      const mockNext = vi.fn();
+
+      await clerkMiddleware()(req, res, mockNext);
+
+      expect(status).toHaveBeenCalledWith(400);
+      expect(end).toHaveBeenCalled();
+      expect(mockNext).not.toHaveBeenCalled();
+    });
+
+    it('responds 400 to a hostless // request target', async () => {
+      await runMiddlewareOnPath(clerkMiddleware(), '//').expect(400);
+    });
 
-    clerkMiddleware()(req, res, mockNext);
+    it('responds 400 to a request target that parses as a credentialed URL', async () => {
+      await runMiddlewareOnPath(clerkMiddleware(), '//$%7B%23context@example.com%7D.action').expect(400);
+    });
+
+    it('responds 400 to a forbidden method (TRACE)', async () => {
+      const app = express();
+      app.use(clerkMiddleware());
+      app.use((_req, res) => res.end('Hello world!'));
+
+      await supertest(app).trace('/').expect(400);
+    });
 
-    expect(mockNext.mock.calls[0][0].message).toBe('Invalid URL');
+    it('responds 400 to a hostless // request target when proxy is enabled', async () => {
+      await runMiddlewareOnPath(clerkMiddleware({ frontendApiProxy: { enabled: true } }), '//').expect(400);
 
-    mockNext.mockReset();
+      expect(mockClerkFrontendApiProxy).not.toHaveBeenCalled();
+    });
   });
 });

packages/express/src/authenticateRequest.ts

@@ -1,5 +1,5 @@
 import { createClerkClient } from '@clerk/backend';
-import type { RequestState } from '@clerk/backend/internal';
+import type { ClerkRequest, 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';
@@ -51,7 +51,7 @@ export const authenticateRequest = (opts: AuthenticateRequestParams) => {
     ...restOptions
   } = options || {};
 
-  const clerkRequest = createClerkRequest(incomingMessageToRequest(request));
+  const clerkRequest = opts.clerkRequest ?? createClerkRequest(incomingMessageToRequest(request));
   const env = { ...loadApiEnv(), ...loadClientEnv() };
 
   const secretKey = secretKeyInput || env.secretKey;
@@ -163,21 +163,42 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
       );
     }
 
+    // Node accepts request targets/methods (`//`, TRACE) the fetch spec cannot represent; reject those instead of 500ing.
+    let clerkRequest: ClerkRequest;
+    try {
+      clerkRequest = createClerkRequest(incomingMessageToRequest(request));
+    } catch {
+      response.status(400).end();
+      return;
+    }
+
     const env = { ...loadApiEnv(), ...loadClientEnv() };
     const publishableKey = options.publishableKey || env.publishableKey;
     const secretKey = options.secretKey || env.secretKey;
 
     // Handle Frontend API proxy requests early, before authentication
     if (frontendApiProxy) {
-      const requestUrl = new URL(request.originalUrl || request.url, `http://${request.headers.host}`);
+      let requestUrl: URL;
+      try {
+        requestUrl = new URL(request.originalUrl || request.url, `http://${request.headers.host}`);
+      } catch {
+        response.status(400).end();
+        return;
+      }
       const isEnabled =
         typeof frontendApiProxy.enabled === 'function'
           ? frontendApiProxy.enabled(requestUrl)
           : frontendApiProxy.enabled;
 
       if (isEnabled && (requestUrl.pathname === proxyPath || requestUrl.pathname.startsWith(proxyPath + '/'))) {
         // Convert Express request to Fetch API Request
-        const proxyRequest = requestToProxyRequest(request);
+        let proxyRequest: Request;
+        try {
+          proxyRequest = requestToProxyRequest(request);
+        } catch {
+          response.status(400).end();
+          return;
+        }
 
         // Call the core proxy function
         const proxyResponse = await clerkFrontendApiProxy(proxyRequest, {
@@ -220,7 +241,13 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
     // against the request's public origin (from x-forwarded-* headers).
     let resolvedOptions = options;
     if (frontendApiProxy && !options.proxyUrl) {
-      const requestUrl = new URL(request.originalUrl || request.url, `http://${request.headers.host}`);
+      let requestUrl: URL;
+      try {
+        requestUrl = new URL(request.originalUrl || request.url, `http://${request.headers.host}`);
+      } catch {
+        response.status(400).end();
+        return;
+      }
       const isProxyEnabled =
         typeof frontendApiProxy.enabled === 'function'
           ? frontendApiProxy.enabled(requestUrl)
@@ -235,6 +262,7 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
         clerkClient,
         request,
         options: resolvedOptions,
+        clerkRequest,
       });
 
       const err = setResponseHeaders(requestState, response);

packages/express/src/types.ts

@@ -1,5 +1,10 @@
 import type { createClerkClient } from '@clerk/backend';
-import type { AuthenticateRequestOptions, SignedInAuthObject, SignedOutAuthObject } from '@clerk/backend/internal';
+import type {
+  AuthenticateRequestOptions,
+  ClerkRequest,
+  SignedInAuthObject,
+  SignedOutAuthObject,
+} from '@clerk/backend/internal';
 import type { ShouldProxyFn } from '@clerk/shared/proxy';
 import type { PendingSessionOptions } from '@clerk/shared/types';
 import type { Request as ExpressRequest } from 'express';
@@ -59,4 +64,6 @@ export type AuthenticateRequestParams = {
   clerkClient: ClerkClient;
   request: ExpressRequest;
   options?: ClerkMiddlewareOptions;
+  /** Prebuilt ClerkRequest, so callers that already converted the request can skip re-conversion. */
+  clerkRequest?: ClerkRequest;
 };

packages/fastify/src/__tests__/frontendApiProxy.test.ts

@@ -158,6 +158,25 @@ describe('Frontend API proxy handling', () => {
     expect(mockClerkFrontendApiProxy).not.toHaveBeenCalled();
   });
 
+  it('responds 400 to a hostless // request target when proxy is enabled', async () => {
+    const response = await injectOnPath({ frontendApiProxy: { enabled: true } }, '//');
+
+    expect(response.statusCode).toEqual(400);
+    expect(mockClerkFrontendApiProxy).not.toHaveBeenCalled();
+    expect(authenticateRequestMock).not.toHaveBeenCalled();
+  });
+
+  it('responds 400 to a forbidden method (TRACE) on the proxy path', async () => {
+    const fastify = Fastify();
+    await fastify.register(clerkPlugin, { frontendApiProxy: { enabled: true } });
+
+    const response = await fastify.inject({ method: 'TRACE' as 'GET', path: '/__clerk/v1/client' });
+
+    expect(response.statusCode).toEqual(400);
+    expect(mockClerkFrontendApiProxy).not.toHaveBeenCalled();
+    expect(authenticateRequestMock).not.toHaveBeenCalled();
+  });
+
   it('auto-derives proxyUrl for authentication when proxy is enabled', async () => {
     authenticateRequestMock.mockResolvedValueOnce({
       headers: new Headers(),

packages/fastify/src/__tests__/withClerkMiddleware.test.ts

@@ -243,6 +243,47 @@ describe('withClerkMiddleware(options)', () => {
     );
   });
 
+  describe('requests that cannot be converted to a web Request', () => {
+    const setup = async () => {
+      const fastify = Fastify();
+      await fastify.register(clerkPlugin);
+      fastify.get('/', (request: FastifyRequest, reply: FastifyReply) => {
+        reply.send({ auth: getAuth(request) });
+      });
+      return fastify;
+    };
+
+    test('responds 400 to a hostless // request target instead of throwing', async () => {
+      const fastify = await setup();
+
+      const response = await fastify.inject({ method: 'GET', path: '//' });
+
+      expect(response.statusCode).toEqual(400);
+      expect(authenticateRequestMock).not.toHaveBeenCalled();
+    });
+
+    test('responds 400 to a request target that parses as a credentialed URL', async () => {
+      const fastify = await setup();
+
+      const response = await fastify.inject({
+        method: 'GET',
+        path: "//$%7B%23context['xwork.MethodAccessor.denyMethodExecution']@example.com%7D.action",
+      });
+
+      expect(response.statusCode).toEqual(400);
+      expect(authenticateRequestMock).not.toHaveBeenCalled();
+    });
+
+    test('responds 400 to a forbidden method (TRACE) instead of throwing', async () => {
+      const fastify = await setup();
+
+      const response = await fastify.inject({ method: 'TRACE' as 'GET', path: '/' });
+
+      expect(response.statusCode).toEqual(400);
+      expect(authenticateRequestMock).not.toHaveBeenCalled();
+    });
+  });
+
   test('handles signout case by populating the req.auth', async () => {
     authenticateRequestMock.mockResolvedValueOnce({
       headers: new Headers(),

packages/fastify/src/withClerkMiddleware.ts

@@ -31,18 +31,28 @@ export const withClerkMiddleware = (options: ClerkFastifyOptions) => {
     // Handle Frontend API proxy requests and auto-derive proxyUrl
     let resolvedProxyUrl = options.proxyUrl;
     if (frontendApiProxy) {
-      const requestUrl = new URL(
-        fastifyRequest.url,
-        `${fastifyRequest.protocol}://${fastifyRequest.hostname || 'localhost'}`,
-      );
+      let requestUrl: URL;
+      try {
+        requestUrl = new URL(
+          fastifyRequest.url,
+          `${fastifyRequest.protocol}://${fastifyRequest.hostname || 'localhost'}`,
+        );
+      } catch {
+        return reply.code(400).send();
+      }
       const isEnabled =
         typeof frontendApiProxy.enabled === 'function'
           ? frontendApiProxy.enabled(requestUrl)
           : frontendApiProxy.enabled;
 
       if (isEnabled) {
         if (requestUrl.pathname === proxyPath || requestUrl.pathname.startsWith(proxyPath + '/')) {
-          const proxyRequest = requestToProxyRequest(fastifyRequest);
+          let proxyRequest: Request;
+          try {
+            proxyRequest = requestToProxyRequest(fastifyRequest);
+          } catch {
+            return reply.code(400).send();
+          }
 
           const proxyResponse = await clerkFrontendApiProxy(proxyRequest, {
             proxyPath,
@@ -84,7 +94,13 @@ export const withClerkMiddleware = (options: ClerkFastifyOptions) => {
       }
     }
 
-    const req = fastifyRequestToRequest(fastifyRequest);
+    // Node accepts request targets/methods (`//`, TRACE) the fetch spec cannot represent; reject those instead of 500ing.
+    let req: Request;
+    try {
+      req = fastifyRequestToRequest(fastifyRequest);
+    } catch {
+      return reply.code(400).send();
+    }
 
     const requestState = await clerkClient.authenticateRequest(req, {
       ...options,