Skip to content

Commit 1d6959c

Browse files
emrysalvolnei
andauthored
chore: system wide ratelimit per path (calcom#25080)
* chore: Add system wide rate limiting * Handle and convert HttpError to 429 * Make algo 32-bit to prevent <ES2020 error + fix sms manager unit test * Change core to common to go from 10 requests per minute to 200 * Remove redundant function * Fix integration tests * Make sure we allow all legal POST routes * Allow tRPC post calls * Add matcher tests on middleware * Add matcher tests on middleware * Fix matcher to not use regex * Fix missing POST allow rule for /api/auth/callback/credentials * Missed the api/book/event endpoints * Add missing pages/api routes * Remove POST middleware for now, very risky * Remove tests for POST protection --------- Co-authored-by: Volnei Munhoz <volnei.munhoz@gmail.com>
1 parent a3fc17b commit 1d6959c

11 files changed

Lines changed: 251 additions & 160 deletions

File tree

apps/web/middleware.test.ts

Lines changed: 72 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,15 @@ import { WEBAPP_URL } from "@calcom/lib/constants";
99
import { checkPostMethod } from "./middleware";
1010
// We'll test the wrapped middleware as it would be used in production
1111
import middleware from "./middleware";
12+
import { config } from "./middleware";
1213

1314
// Mock dependencies at module level
1415
vi.mock("@vercel/edge-config", () => ({
1516
get: vi.fn(),
1617
}));
1718

1819
vi.mock("next-collect/server", () => ({
20+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1921
collectEvents: vi.fn((config: any) => config.middleware),
2022
}));
2123

@@ -29,11 +31,13 @@ vi.mock("next/server", async () => {
2931
const actual = await vi.importActual<typeof import("next/server")>("next/server");
3032

3133
// Create a NextResponse constructor that returns Response objects
34+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
3235
const NextResponse = function (body: any, init?: ResponseInit) {
3336
return new Response(body, init);
3437
};
3538

3639
// Add static methods
40+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
3741
NextResponse.json = (body: any, init?: ResponseInit) => {
3842
return new Response(JSON.stringify(body), {
3943
...init,
@@ -55,6 +59,7 @@ vi.mock("next/server", async () => {
5559
});
5660

5761
// Add cookies property
62+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
5863
(response as any).cookies = {
5964
delete: vi.fn(),
6065
set: vi.fn(),
@@ -91,6 +96,7 @@ vi.mock("next/server", async () => {
9196
});
9297

9398
// Add cookies property
99+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
94100
(response as any).cookies = {
95101
delete: vi.fn(),
96102
set: vi.fn(),
@@ -128,6 +134,7 @@ const createTestRequest = (overrides?: {
128134
return req;
129135
};
130136

137+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
131138
const createEdgeConfigMock = (config: Record<string, any>) => {
132139
return (key: string) => {
133140
if (key in config) return Promise.resolve(config[key]);
@@ -147,6 +154,7 @@ const expectStatus = (res: Response, status: number) => {
147154

148155
// Wrapper for middleware calls to handle type casting
149156
const callMiddleware = async (req: NextRequest): Promise<Response> => {
157+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
150158
return (await (middleware as any)(req)) as Response;
151159
};
152160

@@ -165,15 +173,6 @@ describe("Middleware - POST requests restriction", () => {
165173
expect(res1).toBeNull();
166174
});
167175

168-
it("should block POST requests to not-allowed app routes", async () => {
169-
const req = createRequest("/team/xyz", "POST");
170-
const res = checkPostMethod(req);
171-
expect(res).not.toBeNull();
172-
expect(res?.status).toBe(405);
173-
expect(res?.statusText).toBe("Method Not Allowed");
174-
expect(res?.headers.get("Allow")).toBe("GET");
175-
});
176-
177176
it("should allow GET requests to app routes", async () => {
178177
const req = createRequest("/team/xyz", "GET");
179178
const res = checkPostMethod(req);
@@ -213,29 +212,6 @@ describe("Middleware Integration Tests", () => {
213212
});
214213
});
215214

216-
describe("POST Method Protection", () => {
217-
it("should allow POST to /api/auth/signup", async () => {
218-
const req = createTestRequest({
219-
url: `${WEBAPP_URL}/api/auth/signup`,
220-
method: "POST",
221-
});
222-
223-
const res = await callMiddleware(req);
224-
expectStatus(res, 200);
225-
});
226-
227-
it("should block POST to regular routes", async () => {
228-
const req = createTestRequest({
229-
url: `${WEBAPP_URL}/team/test`,
230-
method: "POST",
231-
});
232-
233-
const res = await callMiddleware(req);
234-
expectStatus(res, 405);
235-
expect(getHeader(res, "Allow")).toBe("GET");
236-
});
237-
});
238-
239215
describe("Maintenance Mode", () => {
240216
it("should redirect to maintenance when enabled", async () => {
241217
(edgeConfigGet as Mock).mockImplementation(
@@ -430,22 +406,6 @@ describe("Middleware Integration Tests", () => {
430406
});
431407

432408
describe("Multiple Features", () => {
433-
it("should handle POST protection before maintenance mode", async () => {
434-
(edgeConfigGet as Mock).mockImplementation(
435-
createEdgeConfigMock({
436-
isInMaintenanceMode: true,
437-
})
438-
);
439-
440-
const req = createTestRequest({
441-
url: `${WEBAPP_URL}/team/test`,
442-
method: "POST",
443-
});
444-
445-
const res = await callMiddleware(req);
446-
// POST protection should trigger first
447-
expectStatus(res, 405);
448-
});
449409

450410
it("should handle embed route with routing forms rewrite", async () => {
451411
const req = createTestRequest({
@@ -483,3 +443,67 @@ describe("Middleware Integration Tests", () => {
483443
});
484444
});
485445
});
446+
447+
describe("Middleware Matcher - Comprehensive Coverage", () => {
448+
const matcher = config.matcher[0];
449+
const pattern = matcher.replace(/^\/|\/$/g, "");
450+
const regex = new RegExp(`^/${pattern}`);
451+
452+
const cases = [
453+
// pages & apis
454+
{ path: "/", expected: true, reason: "Root page" },
455+
{ path: "/home", expected: true, reason: "Regular page" },
456+
{ path: "/team/abc", expected: true, reason: "Nested page" },
457+
{ path: "/api/auth/login", expected: true, reason: "API route" },
458+
{ path: "/api/bookings", expected: true, reason: "Top-level API" },
459+
{ path: "/dashboard/settings", expected: true, reason: "Deep nested page" },
460+
{ path: "/user/john/profile", expected: true, reason: "Multiple nested path" },
461+
{ path: "/apps/routing_forms/form", expected: true, reason: "App page under /apps" },
462+
{ path: "/embed?ui.color-scheme=dark", expected: true, reason: "Embed query param" },
463+
464+
// should be ignored (internal / static / public)
465+
{ path: "/_next/static/chunks/app.js", expected: false, reason: "Internal static asset" },
466+
{ path: "/_next/image?url=%2Flogo.png&w=256&q=75", expected: false, reason: "Internal image handler" },
467+
{ path: "/_next/data/build-id/page.json", expected: false, reason: "Next.js data route" },
468+
{ path: "/favicon.ico", expected: false, reason: "Favicon asset" },
469+
{ path: "/robots.txt", expected: false, reason: "Robots file" },
470+
{ path: "/sitemap.xml", expected: false, reason: "Sitemap file" },
471+
{ path: "/public/images/logo.png", expected: false, reason: "Public folder asset" },
472+
{ path: "/public/fonts/inter.woff2", expected: false, reason: "Public folder font" },
473+
{ path: "/static/js/main.js", expected: false, reason: "Static folder JavaScript" },
474+
{ path: "/static/css/app.css", expected: false, reason: "Static folder stylesheet" },
475+
476+
// edge cases
477+
{ path: "/manifest.json", expected: true, reason: "Manifest is a public page, not ignored" },
478+
{ path: "/_nextsomething", expected: true, reason: "Looks like _next but not reserved" },
479+
{ path: "/nextconfig", expected: true, reason: "Normal route with 'next' in name" },
480+
{ path: "/_NEXT/image", expected: true, reason: "Case-sensitive test (should match)" },
481+
{ path: "/favicon-abc.ico", expected: true, reason: "Favicon variant should still match" },
482+
{ path: "/robots-custom.txt", expected: true, reason: "Custom robots file should match" },
483+
{ path: "/sitemap-other.xml", expected: true, reason: "Custom sitemap file should match" },
484+
{ path: "/api_", expected: true, reason: "Partial match with api underscore" },
485+
{ path: "//double-slash", expected: true, reason: "Double slash URL" },
486+
{ path: "/_next", expected: false, reason: "Bare _next path" },
487+
];
488+
489+
it("should match only the intended routes", () => {
490+
for (const { path, expected, reason } of cases) {
491+
const result = regex.test(path);
492+
expect(result, `${path}${reason}`).toBe(expected);
493+
}
494+
});
495+
496+
it("should not accidentally match internal Next.js routes", () => {
497+
const internalPaths = ["/_next/static", "/_next/image", "/_next/data"];
498+
for (const path of internalPaths) {
499+
expect(regex.test(path)).toBe(false);
500+
}
501+
});
502+
503+
it("should match all user-facing routes and APIs", () => {
504+
const publicPaths = ["/", "/api/user", "/settings", "/dashboard"];
505+
for (const path of publicPaths) {
506+
expect(regex.test(path)).toBe(true);
507+
}
508+
});
509+
});

apps/web/middleware.ts

Lines changed: 72 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,74 @@ import { collectEvents } from "next-collect/server";
33
import type { NextRequest } from "next/server";
44
import { NextResponse } from "next/server";
55

6+
import { checkRateLimitAndThrowError } from "@calcom/lib/checkRateLimitAndThrowError";
7+
import getIP from "@calcom/lib/getIP";
8+
import { HttpError } from "@calcom/lib/http-error";
9+
import { piiHasher } from "@calcom/lib/server/PiiHasher";
610
import { extendEventData, nextCollectBasicSettings } from "@calcom/lib/telemetry";
711

812
import { getCspHeader, getCspNonce } from "@lib/csp";
913

14+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1015
const safeGet = async <T = any>(key: string): Promise<T | undefined> => {
1116
try {
1217
return get<T>(key);
13-
} catch (error) {
18+
} catch {
1419
// Don't crash if EDGE_CONFIG env var is missing
1520
}
1621
};
1722

18-
export const POST_METHODS_ALLOWED_API_ROUTES = ["/api/auth/signup"];
23+
export const POST_METHODS_ALLOWED_API_ROUTES = [
24+
"/api/auth/forgot-password",
25+
"/api/auth/oauth/me",
26+
"/api/auth/oauth/refreshToken",
27+
"/api/auth/oauth/token",
28+
"/api/auth/reset-password",
29+
"/api/auth/saml/callback",
30+
"/api/auth/saml/token",
31+
"/api/auth/setup",
32+
"/api/auth/signup",
33+
"/api/auth/two-factor/totp/disable",
34+
"/api/auth/two-factor/totp/enable",
35+
"/api/auth/two-factor/totp/setup",
36+
"/api/auth/session",
37+
"/api/availability/calendar",
38+
"/api/cancel",
39+
"/api/cron/bookingReminder",
40+
"/api/cron/calendar-cache-cleanup",
41+
"/api/cron/changeTimeZone",
42+
"/api/cron/checkSmsPrices",
43+
"/api/cron/downgradeUsers",
44+
"/api/cron/monthlyDigestEmail",
45+
"/api/cron/syncAppMeta",
46+
"/api/cron/webhookTriggers",
47+
"/api/cron/workflows/scheduleEmailReminders",
48+
"/api/cron/workflows/scheduleSMSReminders",
49+
"/api/cron/workflows/scheduleWhatsappReminders",
50+
"/api/get-inbound-dynamic-variables",
51+
"/api/integrations/", // for /api/integrations/[...args] and webhooks
52+
"/api/recorded-daily-video",
53+
"/api/router",
54+
"/api/routing-forms/queued-response",
55+
"/api/scim/v2.0/", // /api/scim/v2.0/[...directory]
56+
"/api/support/conversation",
57+
"/api/sync/helpscout",
58+
"/api/twilio/webhook",
59+
"/api/username",
60+
"/api/verify-booking-token",
61+
"/api/video/guest-session",
62+
"/api/webhook/app-credential",
63+
"/api/webhooks/calendar-subscription/", // /api/webhooks/calendar-subscription/[provider]
64+
"/api/webhooks/retell-ai",
65+
"/api/workflows/sms/user-response",
66+
"/api/trpc/", // for tRPC
67+
"/api/auth/callback/", // for NextAuth
68+
"/api/book/event",
69+
"/api/book/instant-event",
70+
"/api/book/recurring-event",
71+
"/availability",
72+
];
73+
1974
export function checkPostMethod(req: NextRequest) {
2075
const pathname = req.nextUrl.pathname;
2176
if (!POST_METHODS_ALLOWED_API_ROUTES.some((route) => pathname.startsWith(route)) && req.method === "POST") {
@@ -30,14 +85,6 @@ export function checkPostMethod(req: NextRequest) {
3085
return null;
3186
}
3287

33-
export function checkStaticFiles(pathname: string) {
34-
const hasFileExtension = /\.(svg|png|jpg|jpeg|gif|webp|ico)$/.test(pathname);
35-
// Skip Next.js internal paths (_next) and static assets
36-
if (pathname.startsWith("/_next") || hasFileExtension) {
37-
return NextResponse.next();
38-
}
39-
}
40-
4188
const isPagePathRequest = (url: URL) => {
4289
const isNonPagePathPrefix = /^\/(?:_next|api)\//;
4390
const isFile = /\..*$/;
@@ -50,11 +97,21 @@ const shouldEnforceCsp = (url: URL) => {
5097
};
5198

5299
const middleware = async (req: NextRequest): Promise<NextResponse<unknown>> => {
53-
const postCheckResult = checkPostMethod(req);
54-
if (postCheckResult) return postCheckResult;
100+
const requestorIp = getIP(req);
101+
try {
102+
await checkRateLimitAndThrowError({
103+
rateLimitingType: "common",
104+
identifier: `${req.nextUrl.pathname}-${piiHasher.hash(requestorIp)}`,
105+
});
106+
} catch (error) {
107+
if (error instanceof HttpError) {
108+
return new NextResponse(error.message, { status: error.statusCode });
109+
}
110+
throw error;
111+
}
55112

56-
const isStaticFile = checkStaticFiles(req.nextUrl.pathname);
57-
if (isStaticFile) return isStaticFile;
113+
// const postCheckResult = checkPostMethod(req);
114+
// if (postCheckResult) return postCheckResult;
58115

59116
const url = req.nextUrl;
60117
const reqWithEnrichedHeaders = enrichRequestWithHeaders({ req });
@@ -168,22 +225,7 @@ function enrichRequestWithHeaders({ req }: { req: NextRequest }) {
168225
}
169226

170227
export const config = {
171-
// Next.js Doesn't support spread operator in config matcher, so, we must list all paths explicitly here.
172-
// https://github.com/vercel/next.js/discussions/42458
173-
// WARNING: DO NOT ADD AN ENDING SLASH "/" TO THE PATHS BELOW
174-
// THIS WILL MAKE THEM NOT MATCH AND HENCE NOT HIT MIDDLEWARE
175-
matcher: [
176-
// Routes to enforce CSP
177-
"/auth/login",
178-
"/login",
179-
// Routes to set cookies
180-
"/apps/installed",
181-
"/auth/logout",
182-
// Embed Routes,
183-
"/:path*/embed",
184-
// API routes
185-
"/api/auth/signup",
186-
],
228+
matcher: ["/((?!_next(?:/|$)|static(?:/|$)|public(?:/|$)|favicon\\.ico$|robots\\.txt$|sitemap\\.xml$).*)"],
187229
};
188230

189231
export default collectEvents({

packages/features/ee/workflows/lib/reminders/providers/twilioProvider.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { NextRequest } from "next/server";
22
import TwilioClient from "twilio";
33
import { v4 as uuidv4 } from "uuid";
44

5-
import { checkSMSRateLimit } from "@calcom/lib/checkRateLimitAndThrowError";
5+
import { checkSMSRateLimit } from "@calcom/lib/smsLockState";
66
import { WEBAPP_URL } from "@calcom/lib/constants";
77
import logger from "@calcom/lib/logger";
88
import { setTestSMS } from "@calcom/lib/testSMS";
@@ -85,6 +85,7 @@ export const sendSMS = async ({
8585
}
8686

8787
if (isWhatsapp) {
88+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
8889
const messageOptions: any = {
8990
contentSid: contentSid,
9091
to: getSMSNumber(phoneNumber, isWhatsapp),
@@ -172,6 +173,7 @@ export const scheduleSMS = async ({
172173
}
173174

174175
if (isWhatsapp) {
176+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
175177
const messageOptions: any = {
176178
contentSid: contentSid,
177179
to: getSMSNumber(phoneNumber, isWhatsapp),
@@ -225,7 +227,7 @@ export const verifyNumber = async (phoneNumber: string, code: string) => {
225227
.services(process.env.TWILIO_VERIFY_SID)
226228
.verificationChecks.create({ to: phoneNumber, code: code });
227229
return verification_check.status;
228-
} catch (e) {
230+
} catch {
229231
return "failed";
230232
}
231233
}
@@ -265,7 +267,7 @@ async function isLockedForSMSSending(userId?: number | null, teamId?: number | n
265267
(membership) => membership.team.smsLockState === SMSLockState.LOCKED
266268
);
267269

268-
if (!!memberOfLockedTeam) {
270+
if (memberOfLockedTeam) {
269271
return true;
270272
}
271273

packages/features/ee/workflows/lib/reminders/reminderScheduler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import * as twilio from "@calcom/features/ee/workflows/lib/reminders/providers/t
1111
import type { Workflow, WorkflowStep } from "@calcom/features/ee/workflows/lib/types";
1212
import { getSubmitterEmail } from "@calcom/features/tasker/tasks/triggerFormSubmittedNoEvent/formSubmissionValidation";
1313
import { UserRepository } from "@calcom/features/users/repositories/UserRepository";
14-
import { checkSMSRateLimit } from "@calcom/lib/checkRateLimitAndThrowError";
14+
import { checkSMSRateLimit } from "@calcom/lib/smsLockState";
1515
import { SENDER_NAME } from "@calcom/lib/constants";
1616
import { formatCalEventExtended } from "@calcom/lib/formatCalendarEvent";
1717
import { withReporting } from "@calcom/lib/sentryWrapper";

0 commit comments

Comments
 (0)