From 1ffea493ec63127678c1ffff552c2b752de26830 Mon Sep 17 00:00:00 2001 From: Andreas Thomas Date: Thu, 23 Jan 2025 03:37:30 +0100 Subject: [PATCH] fix: use userId as ratelimit identifier (#18803) * fix: use userId as ratelimit identifier Using raw api keys as identifiers is problematic because they are logged and are accessible via Unkey's UI. My first idea was to use the api key's primary key, which would solve this just fine. But after looking at the implementation it seemed easier to use the user's id, which is what most people are interested in limiting, not the actual keys. Also the userId was already passed in the requests context, which made it easier. Sean confirmed that ratelimiting the user, not their keys made more sense, so here we are. * Update rateLimitApiKey.ts * Update rateLimitApiKey.ts * Fix tests * Fix tests --------- Co-authored-by: Benny Joo Co-authored-by: Joe Au-Yeung --- .../v1/lib/helpers/rateLimitApiKey.test.ts | 24 +++++++++++++------ apps/api/v1/lib/helpers/rateLimitApiKey.ts | 8 ++++--- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/apps/api/v1/lib/helpers/rateLimitApiKey.test.ts b/apps/api/v1/lib/helpers/rateLimitApiKey.test.ts index 9a1e9a5277..3b57cfeb4f 100644 --- a/apps/api/v1/lib/helpers/rateLimitApiKey.test.ts +++ b/apps/api/v1/lib/helpers/rateLimitApiKey.test.ts @@ -13,6 +13,8 @@ import { rateLimitApiKey } from "~/lib/helpers/rateLimitApiKey"; type CustomNextApiRequest = NextApiRequest & Request; type CustomNextApiResponse = NextApiResponse & Response; +const testUserId = 123; + vi.mock("@calcom/lib/checkRateLimitAndThrowError", () => ({ checkRateLimitAndThrowError: vi.fn(), })); @@ -26,6 +28,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: {}, + userId: testUserId, }); await rateLimitApiKey(req, res, vi.fn() as any); @@ -38,6 +41,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); (checkRateLimitAndThrowError as any).mockResolvedValueOnce({ @@ -50,7 +54,7 @@ describe("rateLimitApiKey middleware", () => { await rateLimitApiKey(req, res, vi.fn() as any); expect(checkRateLimitAndThrowError).toHaveBeenCalledWith({ - identifier: "test-key", + identifier: testUserId.toString(), rateLimitingType: "api", onRateLimiterResponse: expect.any(Function), }); @@ -60,6 +64,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); const rateLimiterResponse: RatelimitResponse = { @@ -89,6 +94,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); (checkRateLimitAndThrowError as any).mockRejectedValue(new Error("Rate limit exceeded")); @@ -104,6 +110,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); const rateLimiterResponse: RatelimitResponse = { @@ -127,8 +134,8 @@ describe("rateLimitApiKey middleware", () => { await rateLimitApiKey(req, res, vi.fn() as any); expect(handleAutoLock).toHaveBeenCalledWith({ - identifier: "test-key", - identifierType: "apiKey", + identifier: testUserId.toString(), + identifierType: "userId", rateLimitResponse: rateLimiterResponse, }); @@ -140,6 +147,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); const rateLimiterResponse: RatelimitResponse = { @@ -163,8 +171,8 @@ describe("rateLimitApiKey middleware", () => { await rateLimitApiKey(req, res, vi.fn() as any); expect(handleAutoLock).toHaveBeenCalledWith({ - identifier: "test-key", - identifierType: "apiKey", + identifier: testUserId.toString(), + identifierType: "userId", rateLimitResponse: rateLimiterResponse, }); @@ -176,6 +184,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); const rateLimiterResponse: RatelimitResponse = { @@ -200,8 +209,8 @@ describe("rateLimitApiKey middleware", () => { await rateLimitApiKey(req, res, next); expect(handleAutoLock).toHaveBeenCalledWith({ - identifier: "test-key", - identifierType: "apiKey", + identifier: testUserId.toString(), + identifierType: "userId", rateLimitResponse: rateLimiterResponse, }); @@ -214,6 +223,7 @@ describe("rateLimitApiKey middleware", () => { const { req, res } = createMocks({ method: "GET", query: { apiKey: "test-key" }, + userId: testUserId, }); // Mock checkRateLimitAndThrowError to throw HttpError diff --git a/apps/api/v1/lib/helpers/rateLimitApiKey.ts b/apps/api/v1/lib/helpers/rateLimitApiKey.ts index 734ea27c8d..b490c011bc 100644 --- a/apps/api/v1/lib/helpers/rateLimitApiKey.ts +++ b/apps/api/v1/lib/helpers/rateLimitApiKey.ts @@ -5,12 +5,14 @@ import { checkRateLimitAndThrowError } from "@calcom/lib/checkRateLimitAndThrowE import { HttpError } from "@calcom/lib/http-error"; export const rateLimitApiKey: NextMiddleware = async (req, res, next) => { + if (!req.userId) return res.status(401).json({ message: "No userId provided" }); if (!req.query.apiKey) return res.status(401).json({ message: "No apiKey provided" }); // TODO: Add a way to add trusted api keys try { + const identifier = req.userId.toString(); await checkRateLimitAndThrowError({ - identifier: req.query.apiKey as string, + identifier, rateLimitingType: "api", onRateLimiterResponse: async (response) => { res.setHeader("X-RateLimit-Limit", response.limit); @@ -19,8 +21,8 @@ export const rateLimitApiKey: NextMiddleware = async (req, res, next) => { try { const didLock = await handleAutoLock({ - identifier: req.query.apiKey as string, // Casting as this is verified in another middleware - identifierType: "apiKey", + identifier, + identifierType: "userId", rateLimitResponse: response, });