Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions web/app/api/analytics/events/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ export const POST = makeAnalyticsEventsHandler();

export function makeAnalyticsEventsHandler(dependencies: AnalyticsEventsDependencies = defaultDependencies) {
return async function POST(request: Request): Promise<Response> {
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);
}
Expand Down
14 changes: 5 additions & 9 deletions web/app/api/client-config/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,16 @@ export const runtime = "nodejs";
export const dynamic = "force-dynamic";

export async function POST(request: Request): Promise<Response> {
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);
Expand Down
5 changes: 3 additions & 2 deletions web/app/api/enterprise/contact/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Return a failure for non-not-found limiter errors.

Line 54 enters a branch whose current else if (error) only logs and then sends the enterprise email. This makes genuine limiter failures fail open too. Warn and continue only for not-found; return a 503 for other errors.

Proposed fix
 if (error === "not-found") {
-  console.error(
+  console.warn(
     "enterprise.contact.rate_limit_not_found",
     config.rateLimitId,
   );
 } else if (error) {
   console.error("enterprise.contact.rate_limit_error", error);
+  return jsonError("service_unavailable", 503);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (process.env.VERCEL === "1" && config.rateLimitId) {
if (error === "not-found") {
console.warn(
"enterprise.contact.rate_limit_not_found",
config.rateLimitId,
);
} else if (error) {
console.error("enterprise.contact.rate_limit_error", error);
return jsonError("service_unavailable", 503);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/app/api/enterprise/contact/route.ts` at line 54, Update the rate-limiter
error handling in the route around the VERCEL/config.rateLimitId branch so only
a not-found limiter error is warned about and allowed to continue. For any other
limiter error, return an HTTP 503 response before sending the enterprise email,
preserving the existing success and not-found paths.

const { error, rateLimited } = await checkRateLimit(
config.rateLimitId,
{ request },
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 7 additions & 5 deletions web/app/api/feedback/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down
7 changes: 4 additions & 3 deletions web/app/api/waitlist/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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);
Expand Down
18 changes: 5 additions & 13 deletions web/app/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,6 @@ const irohBindingLimit = z.string().regex(/^[1-9][0-9]{0,3}$/).superRefine((valu
});
}
});
const requireVercelProductionValue = (name: string): z.ZodType<string | undefined> =>
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
Expand All @@ -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.
Expand Down
20 changes: 13 additions & 7 deletions web/tests/client-config-env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,21 @@ 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_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",
...requiredIrohProductionEnv,
...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", () => {
Expand Down Expand Up @@ -80,7 +86,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",
Expand All @@ -90,8 +96,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", () => {
Expand Down
50 changes: 29 additions & 21 deletions web/tests/client-config-route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", {
Expand All @@ -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", {
Expand All @@ -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 () => {
Expand Down
9 changes: 5 additions & 4 deletions web/tests/feedback-route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down