diff --git a/.changeset/fast-boxes-knock.md b/.changeset/fast-boxes-knock.md new file mode 100644 index 00000000000..a458eb0ef23 --- /dev/null +++ b/.changeset/fast-boxes-knock.md @@ -0,0 +1,5 @@ +--- +"@clerk/clerk-sdk-node": patch +--- + +Correctly forward errors to `next()` if the request URL is invalid diff --git a/packages/express/src/__tests__/clerkMiddleware.test.ts b/packages/express/src/__tests__/clerkMiddleware.test.ts index c3f7474ec79..bff538d4841 100644 --- a/packages/express/src/__tests__/clerkMiddleware.test.ts +++ b/packages/express/src/__tests__/clerkMiddleware.test.ts @@ -1,4 +1,4 @@ -import type { RequestHandler } from 'express'; +import type { Request, RequestHandler, Response } from 'express'; import { clerkMiddleware } from '../clerkMiddleware'; import { getAuth } from '../getAuth'; @@ -152,4 +152,20 @@ describe('clerkMiddleware', () => { expect(response.header).toHaveProperty('x-clerk-auth-status', 'handshake'); expect(response.header).toHaveProperty('location', expect.stringContaining('/v1/client/handshake?redirect_url=')); }); + + it('it calls next with an error when request URL is invalid', async () => { + const req = { + url: '//', + cookies: {}, + headers: { host: 'example.com' }, + } as Request; + const res = {} as Response; + const mockNext = jest.fn(); + + await clerkMiddleware()[0](req, res, mockNext); + + expect(mockNext.mock.calls[0][0].message).toBe('Invalid URL'); + + mockNext.mockReset(); + }); }); diff --git a/packages/express/src/__tests__/requireAuth.test.ts b/packages/express/src/__tests__/requireAuth.test.ts index afc554c5d25..07bfa07e1b7 100644 --- a/packages/express/src/__tests__/requireAuth.test.ts +++ b/packages/express/src/__tests__/requireAuth.test.ts @@ -14,8 +14,8 @@ describe('requireAuth', () => { requireAuth(mockRequestWithAuth(), response, nextFn); - expect(response.status).toBeCalledWith(401); - expect(nextFn).not.toBeCalled(); + expect(response.status).toHaveBeenCalledWith(401); + expect(nextFn).not.toHaveBeenCalled(); }); it('make application require auth - proceed with next middlewares for signed-in', async () => { @@ -25,7 +25,7 @@ describe('requireAuth', () => { requireAuth(request, response, nextFn); - expect(response.status).not.toBeCalled(); - expect(nextFn).toBeCalled(); + expect(response.status).not.toHaveBeenCalled(); + expect(nextFn).toHaveBeenCalled(); }); }); diff --git a/packages/sdk-node/src/__tests__/middleware.test.ts b/packages/sdk-node/src/__tests__/middleware.test.ts index 147d32ad2e6..ee5bd4921c9 100644 --- a/packages/sdk-node/src/__tests__/middleware.test.ts +++ b/packages/sdk-node/src/__tests__/middleware.test.ts @@ -18,6 +18,26 @@ const mockClerkClient = () => ({ }); describe('ClerkExpressWithAuth', () => { + it('should halt middleware execution by calling next with an error when request URL is invalid', async () => { + const req = { + url: '//', + cookies: {}, + headers: { host: 'example.com' }, + } as Request; + const res = {} as Response; + + const clerkClient = mockClerkClient() as any; + clerkClient.authenticateRequest.mockReturnValue({ + toAuth: () => ({ sessionId: null }), + headers: new Headers(), + } as RequestState); + + await createClerkExpressWithAuth({ clerkClient })()(req, res, mockNext as NextFunction); + expect((req as WithAuthProp).auth).toBe(undefined); + expect(mockNext.mock.calls[0][0]).toBeInstanceOf(Error); + expect(mockNext.mock.calls[0][0].message).toBe('Invalid request URL: //'); + }); + it('should decorate request with auth and move on to the next middleware when no session token exists', async () => { const req = createRequest(); const res = {} as Response; @@ -92,6 +112,26 @@ describe('ClerkExpressRequireAuth', () => { expect(mockNext.mock.calls[0][0].message).toBe('Unauthenticated'); }); + it('should halt middleware execution by calling next with an error when request URL is invalid', async () => { + const req = { + url: '//', + cookies: {}, + headers: { host: 'example.com' }, + } as Request; + const res = {} as Response; + + const clerkClient = mockClerkClient() as any; + clerkClient.authenticateRequest.mockReturnValue({ + toAuth: () => ({ sessionId: null }), + headers: new Headers(), + } as RequestState); + + await createClerkExpressRequireAuth({ clerkClient })()(req, res, mockNext as NextFunction); + expect((req as WithAuthProp).auth).toBe(undefined); + expect(mockNext.mock.calls[0][0]).toBeInstanceOf(Error); + expect(mockNext.mock.calls[0][0].message).toBe('Invalid request URL: //'); + }); + it('should decorate request with auth and move on to the next middleware when a session token does exist', async () => { const req = createRequest(); const res = {} as Response; diff --git a/packages/sdk-node/src/authenticateRequest.ts b/packages/sdk-node/src/authenticateRequest.ts index 4fcc4d7cc14..5bc1e63040a 100644 --- a/packages/sdk-node/src/authenticateRequest.ts +++ b/packages/sdk-node/src/authenticateRequest.ts @@ -50,7 +50,14 @@ const incomingMessageToRequest = (req: IncomingMessage): Request => { // req extends IncomingMessage in a useful way. No guarantee // it'll work. const protocol = req.connection?.encrypted ? 'https' : 'http'; - const dummyOriginReqUrl = new URL(req.url || '', `${protocol}://clerk-dummy`); + let dummyOriginReqUrl: URL; + + try { + dummyOriginReqUrl = new URL(req.url || '', `${protocol}://clerk-dummy`); + } catch (e) { + throw new Error(`Invalid request URL: ${req.url}`); + } + return new Request(dummyOriginReqUrl, { method: req.method, headers: new Headers(headers), diff --git a/packages/sdk-node/src/clerkExpressRequireAuth.ts b/packages/sdk-node/src/clerkExpressRequireAuth.ts index 63ae910acfa..7d03f845771 100644 --- a/packages/sdk-node/src/clerkExpressRequireAuth.ts +++ b/packages/sdk-node/src/clerkExpressRequireAuth.ts @@ -1,4 +1,5 @@ import type { createClerkClient } from '@clerk/backend'; +import type { RequestState } from '@clerk/backend/internal'; import { authenticateRequest, setResponseHeaders } from './authenticateRequest'; import type { ClerkMiddlewareOptions, MiddlewareRequireAuthProp, RequireAuthProp } from './types'; @@ -15,13 +16,20 @@ export const createClerkExpressRequireAuth = (createOpts: CreateClerkExpressMidd return (options: ClerkMiddlewareOptions = {}): MiddlewareRequireAuthProp => { return async (req, res, next) => { - const requestState = await authenticateRequest({ - clerkClient, - secretKey, - publishableKey, - req, - options, - }); + let requestState: RequestState; + + try { + requestState = await authenticateRequest({ + clerkClient, + secretKey, + publishableKey, + req, + options, + }); + } catch (e) { + next(e); + return; + } const err = setResponseHeaders(requestState, res); if (err || res.writableEnded) { diff --git a/packages/sdk-node/src/clerkExpressWithAuth.ts b/packages/sdk-node/src/clerkExpressWithAuth.ts index 32301fabc83..cc8e53ddb53 100644 --- a/packages/sdk-node/src/clerkExpressWithAuth.ts +++ b/packages/sdk-node/src/clerkExpressWithAuth.ts @@ -1,3 +1,5 @@ +import type { RequestState } from '@clerk/backend/internal'; + import { authenticateRequest, setResponseHeaders } from './authenticateRequest'; import type { CreateClerkExpressMiddlewareOptions } from './clerkExpressRequireAuth'; import type { ClerkMiddlewareOptions, MiddlewareWithAuthProp, WithAuthProp } from './types'; @@ -6,13 +8,20 @@ export const createClerkExpressWithAuth = (createOpts: CreateClerkExpressMiddlew const { clerkClient, secretKey = '', publishableKey = '' } = createOpts; return (options: ClerkMiddlewareOptions = {}): MiddlewareWithAuthProp => { return async (req, res, next) => { - const requestState = await authenticateRequest({ - clerkClient, - secretKey, - publishableKey, - req, - options, - }); + let requestState: RequestState; + + try { + requestState = await authenticateRequest({ + clerkClient, + secretKey, + publishableKey, + req, + options, + }); + } catch (e) { + next(e); + return; + } const err = setResponseHeaders(requestState, res); if (err || res.writableEnded) {