From 4751fb961d6a374cdf7d6227b2fe4bf73f882dac Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 14:11:29 +0200 Subject: [PATCH 1/6] fix lock file --- package-lock.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 96ab16c36f1..90026686d7c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -35790,9 +35790,9 @@ } }, "node_modules/pkg-types": { - "version": "1.1.3", - "resolved": "https://registry.npmjs.org/pkg-types/-/pkg-types-1.1.3.tgz", - "integrity": "sha512-+JrgthZG6m3ckicaOB74TwQ+tBWsFl3qVQg7mN8ulwSOElJ7gBhKzj2VkCPnZ4NlF6kEquYU+RIYNVAvzd54UA==", + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/pkg-types/-/pkg-types-1.2.0.tgz", + "integrity": "sha512-+ifYuSSqOQ8CqP4MbZA5hDpb97n3E8SVWdJe+Wms9kj745lmd3b7EZJiqvmLwAlmRfjrI7Hi5z3kdBJ93lFNPA==", "dev": true, "dependencies": { "confbox": "^0.1.7", @@ -46576,13 +46576,13 @@ "@next/eslint-plugin-next": "^12.3.0", "@vercel/style-guide": "^5.0.1", "any-eslint-parser": "^1.0.1", - "eslint-config-prettier": "^9.0.0", + "eslint-config-prettier": "^9.1.0", "eslint-config-turbo": "^2.0.3", "eslint-plugin-playwright": "^0.22.0", - "eslint-plugin-qunit": "^8.0.1", + "eslint-plugin-qunit": "^8.1.2", "eslint-plugin-regex": "^1.10.0", "eslint-plugin-simple-import-sort": "^10.0.0", - "eslint-plugin-unused-imports": "^3.0.0" + "eslint-plugin-unused-imports": "^3.2.0" }, "peerDependencies": { "typescript": "*" From 203e672300093ef9b2a7a8e5e806e475b909e45e Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 14:11:37 +0200 Subject: [PATCH 2/6] add tests --- .../sdk-node/src/__tests__/middleware.test.ts | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) 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; From 090b3f721776db8d641564aeff647b55db1cca73 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 14:13:06 +0200 Subject: [PATCH 3/6] implementation --- packages/sdk-node/src/authenticateRequest.ts | 12 ++++++++-- .../sdk-node/src/clerkExpressRequireAuth.ts | 22 ++++++++++++------ packages/sdk-node/src/clerkExpressWithAuth.ts | 23 +++++++++++++------ 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/packages/sdk-node/src/authenticateRequest.ts b/packages/sdk-node/src/authenticateRequest.ts index 4fcc4d7cc14..6a1f6c23eb6 100644 --- a/packages/sdk-node/src/authenticateRequest.ts +++ b/packages/sdk-node/src/authenticateRequest.ts @@ -13,7 +13,8 @@ export const authenticateRequest = (opts: AuthenticateRequestParams) => { const { clerkClient, secretKey, publishableKey, req: incomingMessage, options } = opts; const { jwtKey, authorizedParties, audience } = options || {}; - const clerkRequest = createClerkRequest(incomingMessageToRequest(incomingMessage)); + const req = incomingMessageToRequest(incomingMessage); + const clerkRequest = createClerkRequest(req); const env = { ...loadApiEnv(), ...loadClientEnv() }; const isSatellite = handleValueOrFn(options?.isSatellite, clerkRequest.clerkUrl, env.isSatellite); const domain = handleValueOrFn(options?.domain, clerkRequest.clerkUrl) || env.domain; @@ -50,7 +51,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) { From 5fa971a1b1ab9fe1762f11341f9f666906773967 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 15:12:00 +0200 Subject: [PATCH 4/6] add express test coverage --- .../src/__tests__/clerkMiddleware.test.ts | 18 +++++++++++++++++- .../express/src/__tests__/requireAuth.test.ts | 8 ++++---- 2 files changed, 21 insertions(+), 5 deletions(-) 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(); }); }); From 43f1548e6675e129b09b5de2a0424d100ab399f5 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 15:13:28 +0200 Subject: [PATCH 5/6] add changeset --- .changeset/fast-boxes-knock.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fast-boxes-knock.md diff --git a/.changeset/fast-boxes-knock.md b/.changeset/fast-boxes-knock.md new file mode 100644 index 00000000000..c6938e09ee5 --- /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 From f3c3152b354f611b9aabaae332667821fbadbbd9 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 3 Sep 2024 15:18:38 +0200 Subject: [PATCH 6/6] small update --- .changeset/fast-boxes-knock.md | 2 +- packages/sdk-node/src/authenticateRequest.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.changeset/fast-boxes-knock.md b/.changeset/fast-boxes-knock.md index c6938e09ee5..a458eb0ef23 100644 --- a/.changeset/fast-boxes-knock.md +++ b/.changeset/fast-boxes-knock.md @@ -2,4 +2,4 @@ "@clerk/clerk-sdk-node": patch --- -Correctly forward errors to next() if the request URL is invalid +Correctly forward errors to `next()` if the request URL is invalid diff --git a/packages/sdk-node/src/authenticateRequest.ts b/packages/sdk-node/src/authenticateRequest.ts index 6a1f6c23eb6..5bc1e63040a 100644 --- a/packages/sdk-node/src/authenticateRequest.ts +++ b/packages/sdk-node/src/authenticateRequest.ts @@ -13,8 +13,7 @@ export const authenticateRequest = (opts: AuthenticateRequestParams) => { const { clerkClient, secretKey, publishableKey, req: incomingMessage, options } = opts; const { jwtKey, authorizedParties, audience } = options || {}; - const req = incomingMessageToRequest(incomingMessage); - const clerkRequest = createClerkRequest(req); + const clerkRequest = createClerkRequest(incomingMessageToRequest(incomingMessage)); const env = { ...loadApiEnv(), ...loadClientEnv() }; const isSatellite = handleValueOrFn(options?.isSatellite, clerkRequest.clerkUrl, env.isSatellite); const domain = handleValueOrFn(options?.domain, clerkRequest.clerkUrl) || env.domain;