diff --git a/.changeset/harden-request-conversion.md b/.changeset/harden-request-conversion.md new file mode 100644 index 00000000000..68989ed3638 --- /dev/null +++ b/.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. diff --git a/packages/express/src/__tests__/clerkMiddleware.test.ts b/packages/express/src/__tests__/clerkMiddleware.test.ts index 53f6e77c240..e9fe0f48f73 100644 --- a/packages/express/src/__tests__/clerkMiddleware.test.ts +++ b/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(); + }); }); }); diff --git a/packages/express/src/authenticateRequest.ts b/packages/express/src/authenticateRequest.ts index 4e63141d88f..805a0ebd9d7 100644 --- a/packages/express/src/authenticateRequest.ts +++ b/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,13 +163,28 @@ 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) @@ -177,7 +192,13 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions = 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); diff --git a/packages/express/src/types.ts b/packages/express/src/types.ts index 4d889de3dbb..5de6e2ec3e5 100644 --- a/packages/express/src/types.ts +++ b/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; }; diff --git a/packages/fastify/src/__tests__/frontendApiProxy.test.ts b/packages/fastify/src/__tests__/frontendApiProxy.test.ts index 5b19695f0e7..09333a4704e 100644 --- a/packages/fastify/src/__tests__/frontendApiProxy.test.ts +++ b/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(), diff --git a/packages/fastify/src/__tests__/withClerkMiddleware.test.ts b/packages/fastify/src/__tests__/withClerkMiddleware.test.ts index 46a80e25d49..b9fda7b8a4e 100644 --- a/packages/fastify/src/__tests__/withClerkMiddleware.test.ts +++ b/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(), diff --git a/packages/fastify/src/withClerkMiddleware.ts b/packages/fastify/src/withClerkMiddleware.ts index 17751c0cf50..2212b23beb9 100644 --- a/packages/fastify/src/withClerkMiddleware.ts +++ b/packages/fastify/src/withClerkMiddleware.ts @@ -31,10 +31,15 @@ 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) @@ -42,7 +47,12 @@ export const withClerkMiddleware = (options: ClerkFastifyOptions) => { 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,