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 <sldisek783@gmail.com>
Co-authored-by: Joe Au-Yeung <j.auyeung419@gmail.com>

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<CustomNextApiRequest, CustomNextApiResponse>({
       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

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,
           });