From 5f6a6afa953406e1874bfaf310ca0627e9509028 Mon Sep 17 00:00:00 2001 From: Abdulaziz Albahar <67667005+azooz2003-bit@users.noreply.github.com> Date: Thu, 23 Jul 2026 22:19:52 -0700 Subject: [PATCH] Make every rate-limit env var optional and fail open on deleted rules Aziz's ruling: no rate limits at all, and nothing may break when the rule ids are unset or their Vercel firewall rules are deleted. This extends the 8714/8773/8771 fail-open pattern to the remaining consumers: - env.ts: CMUX_FEEDBACK/CLIENT_CONFIG/ANALYTICS_RATE_LIMIT_ID become optional (client-config and analytics previously hard-failed production deploy env validation when unset; feedback failed every deploy). - analytics/events + client-config routes: unset id skips limiting instead of 503ing; a not-found rule warns and fails open; genuine check failures still 503. - waitlist + feedback routes: guard the limiter on the optional id and fail open on not-found instead of 503ing the endpoint. - enterprise/contact + feedback config resolvers no longer treat a missing rate-limit id as 'endpoint not configured'. - push and vault routes already guarded/failed open; unchanged. Also fixes a pre-existing red test on main (client-config-env expected CMUX_IROH_RATE_LIMIT_ID to be required, stale since 8714/8771). Co-Authored-By: Claude Fable 5 --- web/app/api/analytics/events/route.ts | 14 +++---- web/app/api/client-config/route.ts | 14 +++---- web/app/api/enterprise/contact/route.ts | 5 ++- web/app/api/feedback/route.ts | 12 +++--- web/app/api/waitlist/route.ts | 7 ++-- web/app/env.ts | 18 +++------ web/tests/client-config-env.test.ts | 30 ++++++++++----- web/tests/client-config-route.test.ts | 50 ++++++++++++++----------- web/tests/feedback-route.test.ts | 9 +++-- 9 files changed, 85 insertions(+), 74 deletions(-) diff --git a/web/app/api/analytics/events/route.ts b/web/app/api/analytics/events/route.ts index fe186567ca5a..89cfb5e81e6a 100644 --- a/web/app/api/analytics/events/route.ts +++ b/web/app/api/analytics/events/route.ts @@ -61,17 +61,17 @@ export const POST = makeAnalyticsEventsHandler(); export function makeAnalyticsEventsHandler(dependencies: AnalyticsEventsDependencies = defaultDependencies) { return async function POST(request: Request): Promise { - if (process.env.VERCEL === "1") { - const rateLimitId = process.env.CMUX_ANALYTICS_RATE_LIMIT_ID?.trim(); - if (!rateLimitId) { - console.error("analytics.events.rate_limit_not_configured"); - return jsonResponse({ error: "analytics_unavailable" }, 503); - } + // An unset rule id means no rate limiting; a deleted rule (not-found) + // fails open. Only genuine check failures reject the event. + const rateLimitId = process.env.CMUX_ANALYTICS_RATE_LIMIT_ID?.trim(); + if (process.env.VERCEL === "1" && rateLimitId) { const { error, rateLimited } = await dependencies.checkRateLimit(rateLimitId, { request }); if (rateLimited || error === "blocked") { return jsonResponse({ error: "rate_limited" }, 429); } - if (error) { + if (error === "not-found") { + console.warn("analytics.events.rate_limit_not_found; failing open", rateLimitId); + } else if (error) { console.error("analytics.events.rate_limit_error", error); return jsonResponse({ error: "analytics_unavailable" }, 503); } diff --git a/web/app/api/client-config/route.ts b/web/app/api/client-config/route.ts index 63f30ea8d4e6..7e1d7c4fbb75 100644 --- a/web/app/api/client-config/route.ts +++ b/web/app/api/client-config/route.ts @@ -18,20 +18,16 @@ export const runtime = "nodejs"; export const dynamic = "force-dynamic"; export async function POST(request: Request): Promise { - if (process.env.VERCEL === "1") { - const rateLimitId = process.env.CMUX_CLIENT_CONFIG_RATE_LIMIT_ID?.trim(); - if (!rateLimitId) { - console.error("client-config.route.rate_limit_not_configured"); - return json({ error: "client_config_unavailable" }, 503); - } - + // An unset rule id means no rate limiting; a deleted rule (not-found) fails + // open rather than making client config unavailable for every app boot. + const rateLimitId = process.env.CMUX_CLIENT_CONFIG_RATE_LIMIT_ID?.trim(); + if (process.env.VERCEL === "1" && rateLimitId) { const { error, rateLimited } = await checkRateLimit(rateLimitId, { request }); if (rateLimited || error === "blocked") { return json({ error: "rate_limited" }, 429); } if (error === "not-found") { - console.error("client-config.route.rate_limit_not_found", rateLimitId); - return json({ error: "client_config_unavailable" }, 503); + console.warn("client-config.route.rate_limit_not_found; failing open", rateLimitId); } else if (error) { console.error("client-config.route.rate_limit_error", error); return json({ error: "client_config_unavailable" }, 503); diff --git a/web/app/api/enterprise/contact/route.ts b/web/app/api/enterprise/contact/route.ts index 4f0c2fa366a7..bbd5f3eb435f 100644 --- a/web/app/api/enterprise/contact/route.ts +++ b/web/app/api/enterprise/contact/route.ts @@ -51,7 +51,7 @@ export async function POST(request: Request) { return jsonError("Enterprise contact endpoint is not configured", 503); } - if (process.env.VERCEL === "1") { + if (process.env.VERCEL === "1" && config.rateLimitId) { const { error, rateLimited } = await checkRateLimit( config.rateLimitId, { request }, @@ -148,8 +148,9 @@ export async function POST(request: Request) { function resolveEnterpriseConfig() { const resendApiKey = env.RESEND_API_KEY; const fromEmail = env.CMUX_FEEDBACK_FROM_EMAIL; + // rateLimitId is optional: unset means the route runs without rate limiting. const rateLimitId = env.CMUX_FEEDBACK_RATE_LIMIT_ID; - if (!resendApiKey || !fromEmail || !rateLimitId) return null; + if (!resendApiKey || !fromEmail) return null; return { resendApiKey, fromEmail, diff --git a/web/app/api/feedback/route.ts b/web/app/api/feedback/route.ts index efcb1c39af57..393651c49317 100644 --- a/web/app/api/feedback/route.ts +++ b/web/app/api/feedback/route.ts @@ -63,7 +63,7 @@ export async function POST(request: Request) { return jsonError("Feedback endpoint is not configured", 503); } - if (process.env.VERCEL === "1") { + if (process.env.VERCEL === "1" && feedbackConfig.rateLimitId) { const { error, rateLimited } = await checkRateLimit( feedbackConfig.rateLimitId, { request }, @@ -75,11 +75,12 @@ export async function POST(request: Request) { } if (error === "not-found") { - console.error( - "feedback.route.rate_limit_not_found", + // The rule was deleted; treat as "no limit" instead of taking the + // endpoint down. + console.warn( + "feedback.route.rate_limit_not_found; failing open", feedbackConfig.rateLimitId, ); - return jsonError("service_unavailable", 503); } else if (error) { console.error("feedback.route.rate_limit_error", error); return jsonError("service_unavailable", 503); @@ -204,9 +205,10 @@ export async function POST(request: Request) { function resolveFeedbackConfig() { const resendApiKey = env.RESEND_API_KEY; const fromEmail = env.CMUX_FEEDBACK_FROM_EMAIL; + // rateLimitId is optional: unset means the route runs without rate limiting. const rateLimitId = env.CMUX_FEEDBACK_RATE_LIMIT_ID; - if (!resendApiKey || !fromEmail || !rateLimitId) { + if (!resendApiKey || !fromEmail) { return null; } diff --git a/web/app/api/waitlist/route.ts b/web/app/api/waitlist/route.ts index 5ff204ddee2f..aeda2f238c38 100644 --- a/web/app/api/waitlist/route.ts +++ b/web/app/api/waitlist/route.ts @@ -55,7 +55,7 @@ export async function POST(request: Request) { // and unique domains miss the cache, so an unthrottled path would let a // public POST flood the resolver as well as Slack. Reuses the feedback // rule. Only active on Vercel. - if (process.env.VERCEL === "1") { + if (process.env.VERCEL === "1" && env.CMUX_FEEDBACK_RATE_LIMIT_ID) { const { error, rateLimited } = await checkRateLimit( env.CMUX_FEEDBACK_RATE_LIMIT_ID, { request }, @@ -67,8 +67,9 @@ export async function POST(request: Request) { return jsonError("Rate limit exceeded", 429); } if (error === "not-found") { - console.error("waitlist.route.rate_limit_not_found", env.CMUX_FEEDBACK_RATE_LIMIT_ID); - return jsonError("service_unavailable", 503); + // The rule was deleted; treat as "no limit" instead of taking the + // endpoint down. + console.warn("waitlist.route.rate_limit_not_found; failing open", env.CMUX_FEEDBACK_RATE_LIMIT_ID); } else if (error) { console.error("waitlist.route.rate_limit_error", error); return jsonError("service_unavailable", 503); diff --git a/web/app/env.ts b/web/app/env.ts index 8d08e1572ef2..1b9b3febc9a3 100644 --- a/web/app/env.ts +++ b/web/app/env.ts @@ -131,16 +131,6 @@ const irohBindingLimit = z.string().regex(/^[1-9][0-9]{0,3}$/).superRefine((valu }); } }); -const requireVercelProductionValue = (name: string): z.ZodType => - z.string().min(1).optional().superRefine((value, context) => { - if (isVercelProductionDeployment && !value) { - context.addIssue({ - code: z.ZodIssueCode.custom, - message: `${name} is required for Vercel production runtimes`, - }); - } - }); - const stackEnv = ( value: string | undefined, fallback: string @@ -154,9 +144,11 @@ export const env = createEnv({ server: { RESEND_API_KEY: z.string().min(1), CMUX_FEEDBACK_FROM_EMAIL: z.string().email(), - CMUX_FEEDBACK_RATE_LIMIT_ID: z.string().min(1), - CMUX_CLIENT_CONFIG_RATE_LIMIT_ID: requireVercelNonPreviewValue("CMUX_CLIENT_CONFIG_RATE_LIMIT_ID"), - CMUX_ANALYTICS_RATE_LIMIT_ID: requireVercelProductionValue("CMUX_ANALYTICS_RATE_LIMIT_ID"), + // Rate-limit rule ids are all optional: an unset id means that route runs + // without rate limiting (the operator removed the limits deliberately). + CMUX_FEEDBACK_RATE_LIMIT_ID: z.string().min(1).optional(), + CMUX_CLIENT_CONFIG_RATE_LIMIT_ID: z.string().min(1).optional(), + CMUX_ANALYTICS_RATE_LIMIT_ID: z.string().min(1).optional(), STACK_SECRET_SERVER_KEY: z.string().min(1), // APNs push (iOS notifications). Optional: the app boots without them; the // push route returns a clear "not configured" error until they are set. diff --git a/web/tests/client-config-env.test.ts b/web/tests/client-config-env.test.ts index 53479ecbc06e..b8c3014a440d 100644 --- a/web/tests/client-config-env.test.ts +++ b/web/tests/client-config-env.test.ts @@ -44,15 +44,22 @@ describe("client config env validation", () => { expect(result.stderr).not.toContain("CMUX_CLIENT_CONFIG_RATE_LIMIT_ID is required"); }); - test("requires the limiter id in explicit Vercel production deployments", () => { + test("allows explicit Vercel production deployments with all rate-limit ids unset", () => { + // Rate limiting is opt-in: production deploys must survive every + // rate-limit id being deleted from the environment. + const { CMUX_IROH_RATE_LIMIT_ID: _iroh, ...irohEnv } = requiredIrohProductionEnv; + const { CMUX_RELAY_TOKEN_RATE_LIMIT_ID: _relay, ...relayEnv } = requiredRelayProductionEnv; + const { CMUX_FEEDBACK_RATE_LIMIT_ID: _feedback, ...baseEnv } = requiredEnv; const result = importEnv({ - ...requiredEnv, + ...baseEnv, VERCEL: "1", VERCEL_ENV: "production", + ...irohEnv, + ...relayEnv, }); - expect(result.exitCode).not.toBe(0); - expect(result.stderr).toContain("CMUX_CLIENT_CONFIG_RATE_LIMIT_ID is required"); + expect(result.exitCode).toBe(0); + expect(result.stderr).not.toContain("RATE_LIMIT_ID"); }); test("accepts explicit Vercel production deployments with both limiter ids", () => { @@ -81,7 +88,7 @@ describe("client config env validation", () => { expect(result.exitCode).toBe(0); }); - test("requires the analytics limiter id in explicit Vercel production deployments", () => { + test("allows explicit Vercel production deployments without the analytics limiter id", () => { const result = importEnv({ ...requiredEnv, VERCEL: "1", @@ -91,8 +98,8 @@ describe("client config env validation", () => { ...requiredRelayProductionEnv, }); - expect(result.exitCode).not.toBe(0); - expect(result.stderr).toContain("CMUX_ANALYTICS_RATE_LIMIT_ID is required"); + expect(result.exitCode).toBe(0); + expect(result.stderr).not.toContain("CMUX_ANALYTICS_RATE_LIMIT_ID"); }); test("allows Vercel development without the analytics limiter id", () => { @@ -127,17 +134,20 @@ describe("client config env validation", () => { expect(result.exitCode).toBe(0); }); - test("requires the Iroh limiter id in explicit Vercel production deployments", () => { + test("allows explicit Vercel production deployments without the Iroh limiter id", () => { + const { CMUX_IROH_RATE_LIMIT_ID: _iroh, ...irohEnv } = requiredIrohProductionEnv; const result = importEnv({ ...requiredEnv, VERCEL: "1", VERCEL_ENV: "production", CMUX_CLIENT_CONFIG_RATE_LIMIT_ID: "client-config-rule", CMUX_ANALYTICS_RATE_LIMIT_ID: "analytics-rule", + ...irohEnv, + ...requiredRelayProductionEnv, }); - expect(result.exitCode).not.toBe(0); - expect(result.stderr).toContain("CMUX_IROH_RATE_LIMIT_ID is required"); + expect(result.exitCode).toBe(0); + expect(result.stderr).not.toContain("CMUX_IROH_RATE_LIMIT_ID"); }); test("requires the complete Iroh trust-broker configuration in production", () => { diff --git a/web/tests/client-config-route.test.ts b/web/tests/client-config-route.test.ts index 15325b0d0531..3a0edb500a40 100644 --- a/web/tests/client-config-route.test.ts +++ b/web/tests/client-config-route.test.ts @@ -274,14 +274,19 @@ describe("client config", () => { expect(fetchMock).not.toHaveBeenCalled(); }); - test("fails closed on Vercel when the client-config limiter is missing", async () => { + test("skips rate limiting on Vercel when the client-config limiter id is unset", async () => { + // An unset id means the operator wants no rate limiting; client config + // must keep serving (it gates every app boot). process.env.VERCEL = "1"; delete process.env.CMUX_CLIENT_CONFIG_RATE_LIMIT_ID; - const consoleError = mock(() => {}); - console.error = consoleError as unknown as typeof console.error; - const fetchMock = mock(async () => { - throw new Error("PostHog flags should not be reached without a rate-limit rule"); - }); + const fetchMock = mock(async () => new Response( + JSON.stringify({ + errorsWhileComputingFlags: false, + featureFlags: {}, + featureFlagPayloads: {}, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + )); globalThis.fetch = fetchMock as unknown as typeof fetch; const response = await POST(new Request("https://cmux.test/api/client-config", { @@ -290,21 +295,25 @@ describe("client config", () => { body: JSON.stringify({ distinctId: "browser-id" }), })); - expect(response.status).toBe(503); - expect(await response.json()).toEqual({ error: "client_config_unavailable" }); - expect(consoleError).toHaveBeenCalledWith("client-config.route.rate_limit_not_configured"); + expect(response.status).toBe(200); expect(checkRateLimit).not.toHaveBeenCalled(); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock).toHaveBeenCalledTimes(1); }); - test("fails closed on Vercel when the client-config limiter rule is not found", async () => { + test("fails open on Vercel when the client-config limiter rule is not found", async () => { + // A deleted rule is an operator action (no limit wanted), not an outage. process.env.VERCEL = "1"; checkRateLimit.mockResolvedValue({ rateLimited: false, error: "not-found" }); - const consoleError = mock(() => {}); - console.error = consoleError as unknown as typeof console.error; - const fetchMock = mock(async () => { - throw new Error("PostHog flags should not be reached without a valid rate-limit rule"); - }); + const consoleWarn = mock(() => {}); + console.warn = consoleWarn as unknown as typeof console.warn; + const fetchMock = mock(async () => new Response( + JSON.stringify({ + errorsWhileComputingFlags: false, + featureFlags: {}, + featureFlagPayloads: {}, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + )); globalThis.fetch = fetchMock as unknown as typeof fetch; const response = await POST(new Request("https://cmux.test/api/client-config", { @@ -313,14 +322,13 @@ describe("client config", () => { body: JSON.stringify({ distinctId: "browser-id" }), })); - expect(response.status).toBe(503); - expect(await response.json()).toEqual({ error: "client_config_unavailable" }); - expect(consoleError).toHaveBeenCalledWith( - "client-config.route.rate_limit_not_found", + expect(response.status).toBe(200); + expect(consoleWarn).toHaveBeenCalledWith( + "client-config.route.rate_limit_not_found; failing open", "cmux-client-config-test", ); expect(checkRateLimit).toHaveBeenCalledTimes(1); - expect(fetchMock).not.toHaveBeenCalled(); + expect(fetchMock).toHaveBeenCalledTimes(1); }); test("fails closed on Vercel when the client-config limiter returns an error", async () => { diff --git a/web/tests/feedback-route.test.ts b/web/tests/feedback-route.test.ts index 875f9a2dc046..b293e282c1df 100644 --- a/web/tests/feedback-route.test.ts +++ b/web/tests/feedback-route.test.ts @@ -56,15 +56,16 @@ afterAll(() => { }); describe("feedback route", () => { - test("fails closed when the Vercel firewall rule is missing", async () => { + test("fails open when the Vercel firewall rule is missing", async () => { + // A deleted rule is an operator action (no limit wanted), not an outage. process.env.VERCEL = "1"; checkRateLimit.mockResolvedValue({ rateLimited: false, error: "not-found" }); const res = await POST(feedbackRequest()); - expect(res.status).toBe(503); - expect(await res.json()).toEqual({ error: "service_unavailable" }); - expect(sendEmail).not.toHaveBeenCalled(); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ ok: true }); + expect(sendEmail).toHaveBeenCalled(); }); test("fails closed when the Vercel firewall check errors", async () => {