Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/harden-request-conversion.md
Original file line number Diff line number Diff line change
@@ -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.
51 changes: 40 additions & 11 deletions packages/express/src/__tests__/clerkMiddleware.test.ts
Original file line number Diff line number Diff line change
@@ -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(() => ({
Expand Down Expand Up @@ -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());
Comment thread
wobsoriano marked this conversation as resolved.
Dismissed
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();
});
});
});
38 changes: 33 additions & 5 deletions packages/express/src/authenticateRequest.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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)
Expand All @@ -235,6 +262,7 @@ export const authenticateAndDecorateRequest = (options: ClerkMiddlewareOptions =
clerkClient,
request,
options: resolvedOptions,
clerkRequest,
});

const err = setResponseHeaders(requestState, response);
Expand Down
9 changes: 8 additions & 1 deletion packages/express/src/types.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;
};
19 changes: 19 additions & 0 deletions packages/fastify/src/__tests__/frontendApiProxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
41 changes: 41 additions & 0 deletions packages/fastify/src/__tests__/withClerkMiddleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
28 changes: 22 additions & 6 deletions packages/fastify/src/withClerkMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Loading