Fix rate-limit bypass vuln

[tembo]

Summary

Fixed rate-limit bypass vulnerability when TRUST_PROXY=true without trusted proxy configuration. Added TRUSTED_PROXIES environment variable to enforce proxy allowlisting and prevent x-forwarded-for header spoofing. When TRUST_PROXY=true is set without TRUSTED_PROXIES, the system now:

• Logs a security warning about the unsafe configuration
• Returns null for client IP (failing closed) to reject requests with a 400 error
• Prevents rate-limit bypass via spoofed headers

Changes:

• Added TRUSTED_PROXIES env var parsing into TRUSTED_PROXY_IPS set
• Added validation: if TRUST_PROXY=true without TRUSTED_PROXIES, log warning and fail closed
• Modified getClientIp() to return null when TRUST_PROXY=true but no trusted proxies configured
• Added informational logging when trusted proxies are properly configured

Files modified:

• apps/web/src/routes/api/models.ts

Want tembo to make any changes? Add a review or comment with @tembo and i'll get back to work!

---

Summary by cubic

Fixes a rate-limit bypass by only trusting client IPs from known proxies or platform headers and rejecting unverifiable requests. Adds TRUSTED_PROXIES and IP-based rate limiting on /api/models.

• Bug Fixes

  • Strict IP derivation: honor x-forwarded-for only when TRUST_PROXY=true with TRUSTED_PROXIES, or in cloudflare/vercel modes via their headers; otherwise reject.
  • Fail closed on misconfig or spoofing attempts, with clear safety logs; add 30 req/60s IP rate limit and return 502 on upstream failures.

• Migration

  • Set TRUST_PROXY to cloudflare or vercel, or set TRUST_PROXY=true with TRUSTED_PROXIES (comma-separated proxy IPs).
  • If not configured, requests may be rejected when the client IP cannot be verified.

^{Written for commit b4bb6ae. Summary will update on new commits.}

[railway-app]

This PR was not deployed automatically as @tembo[bot] does not have access to the Railway project.

In order to get automatic PR deploys, please add @tembo[bot] to your workspace on Railway.

[tembo]

Requesting review from @leoisadev1 who has experience with the following files modified in this PR:

• apps/web/src/routes/api/models.ts

[cubic-dev-ai]

12 issues found across 43 files

Confidence score: 2/5

• High risk: apps/web/src/routes/api/models.ts trusts x-forwarded-for without validating TRUSTED_PROXY_IPS, which is a concrete security vulnerability.
• apps/web/src/routes/api/workflow/generate-title.ts has a non-idempotent OpenRouter fetch outside context.run(), so Upstash Workflow replays can trigger duplicate external calls.
• Several runtime correctness issues (race in apps/server/convex/lib/upstashUsage.ts, optional userId cast in apps/web/src/routes/api/workflow/export-chat.ts, and auth fallback ignoring ALLOW_AUTH_COOKIE_FALLBACK in apps/web/src/lib/server-auth.ts) add user-facing risk, which drives the lower score.
• Pay close attention to apps/web/src/routes/api/models.ts, apps/web/src/routes/api/workflow/generate-title.ts, apps/server/convex/lib/upstashUsage.ts, apps/web/src/routes/api/workflow/export-chat.ts, apps/web/src/lib/server-auth.ts - security trust checks, workflow replay safety, atomicity, null guards, and auth gating.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/web/src/routes/api/workflow/generate-title.ts">

<violation number="1" location="apps/web/src/routes/api/workflow/generate-title.ts:249">
P1: The OpenRouter API `fetch` call is not wrapped in a `context.run()` or `context.call()` step. Upstash Workflow replays the `serve` handler multiple times, skipping completed steps. Since this HTTP call isn't a step, it will be re-executed on every replay/retry — causing duplicate API calls, wasted credits, and potential inconsistent state.

Prefer `context.call()` for external HTTP requests (it offloads the call to Upstash and persists the result), or at minimum wrap the fetch in `context.run("call-llm", async () => { ... })`.</violation>
</file>

<file name="apps/web/src/components/app-sidebar.tsx">

<violation number="1" location="apps/web/src/components/app-sidebar.tsx:454">
P2: Fragile rate-limit detection: matching on error message content instead of the structured `error.name`. The server explicitly sets `error.name = "RateLimitError"` for this purpose, and line 520 in this same file still uses the `error.name` check. If the server-side action string changes from `"title generations"`, this detection silently breaks. Consider using `error.name === "RateLimitError"` for consistency and reliability.</violation>
</file>

<file name="apps/web/src/routes/api/workflow/export-chat.ts">

<violation number="1" location="apps/web/src/routes/api/workflow/export-chat.ts:124">
P2: `userId` is optional in `ExportChatPayload` but is cast to `Id<"users">` without a null check. If `userId` is `undefined`, this will cause a Convex validation error at runtime. Add a guard before the query call.</violation>
</file>

<file name="apps/web/src/routes/api/workflow/delete-account.ts">

<violation number="1" location="apps/web/src/routes/api/workflow/delete-account.ts:106">
P2: Four helper functions (`isLocalWorkflowExecutionEnabled`, `getWorkflowTriggerHeaders`, `getWorkflowCallbackUrl`, `hasWorkflowSigningKeysConfigured`) are duplicated verbatim across at least three workflow route files. Extract them into a shared utility module (e.g., `@/lib/workflow-utils.ts`) to avoid maintenance risk and inconsistency.</violation>
</file>

<file name="apps/web/src/lib/server-auth.ts">

<violation number="1" location="apps/web/src/lib/server-auth.ts:82">
P2: `ALLOW_AUTH_COOKIE_FALLBACK` is declared and validated but never actually checked in the fallback path. The cookie-based JWT fallback is unconditionally enabled when `IS_LOCAL_DEV` is true, ignoring the opt-in flag. Either gate the fallback on `ALLOW_AUTH_COOKIE_FALLBACK` or remove the dead constant.</violation>
</file>

<file name="apps/server/convex/users.ts">

<violation number="1" location="apps/server/convex/users.ts:870">
P2: `batchSize` argument is not forwarded for `delete-chat-read-statuses` and `delete-prompt-templates` steps, unlike all other steps in this action. This means callers cannot control batch size for these two steps, and they'll always use the default (100). This looks like an oversight since the underlying mutations accept `batchSize`.</violation>
</file>

<file name="apps/web/src/stores/ui.ts">

<violation number="1" location="apps/web/src/stores/ui.ts:34">
P3: Inconsistent indentation: the re-added state properties use 9-space indentation while the rest of the object (action methods) uses 8-space indentation. This appears to be an accidental extra space introduced during the removal of `jonMode`/`dynamicPrompt`.</violation>
</file>

<file name="apps/server/convex/lib/upstashUsage.ts">

<violation number="1" location="apps/server/convex/lib/upstashUsage.ts:145">
P1: Race condition: the negative-balance clamp (SET key 0) is not atomic with the preceding INCRBY. A concurrent `incrementDailyUsageInUpstash` or `reserveDailyUsageInUpstash` between the two pipeline calls will have its increment silently discarded when the SET overwrites the key. Consider using a Lua script (`EVAL`) to make the decrement-and-clamp atomic, e.g.:
```lua
local v = redis.call('INCRBY', KEYS[1], ARGV[1])
if v < 0 then redis.call('SET', KEYS[1], 0) end
redis.call('EXPIREAT', KEYS[1], ARGV[2])
return v
```</violation>
</file>

<file name="apps/server/convex/cleanupAction.ts">

<violation number="1" location="apps/server/convex/cleanupAction.ts:38">
P2: Validation mismatch: `Number.isFinite` allows non-integer values (e.g. `1.5`) that the downstream `cleanupSoftDeletedRecords` mutation rejects with `Number.isInteger`. This causes a confusing error from the wrong layer. Use `Number.isInteger` here to match the mutation's validation and fail early with a clear message.</violation>
</file>

<file name="apps/web/src/routes/api/models.ts">

<violation number="1" location="apps/web/src/routes/api/models.ts:124">
P0: **Security: `TRUSTED_PROXY_IPS` is never actually validated against the connecting IP.** The code only checks `TRUSTED_PROXY_IPS.size === 0` to decide whether to trust `x-forwarded-for`, but never verifies that the request actually originated from a listed proxy IP. Any client can bypass rate limiting by spoofing `x-forwarded-for` as long as `TRUSTED_PROXIES` is set to any non-empty value. The set must be checked against the actual connecting IP (e.g., from the underlying socket/connection) before trusting forwarded headers.</violation>
</file>

<file name="apps/server/convex/chats.test.ts">

<violation number="1" location="apps/server/convex/chats.test.ts:1287">
P2: Missing `fetch` mock in second `generateAndSetTitleInternal` test. If the action implementation calls `fetch` before checking the existing title, this test will attempt a real network request instead of using a mock, making it flaky and environment-dependent. Add a `fetch` mock (or move the shared mock into `beforeEach`) to ensure the test is fully isolated.</violation>
</file>

<file name="apps/web/src/routes/api/workflow/cleanup.ts">

<violation number="1" location="apps/web/src/routes/api/workflow/cleanup.ts:182">
P2: Payload validation throws outside a `context.run` step, which may cause unnecessary QStash retries on invalid payloads. Wrap the parsing in a `context.run` step (as done in `generate-title.ts`) so the framework can track the failure and avoid retry storms.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P0: Security: TRUSTED_PROXY_IPS is never actually validated against the connecting IP. The code only checks TRUSTED_PROXY_IPS.size === 0 to decide whether to trust x-forwarded-for, but never verifies that the request actually originated from a listed proxy IP. Any client can bypass rate limiting by spoofing x-forwarded-for as long as TRUSTED_PROXIES is set to any non-empty value. The set must be checked against the actual connecting IP (e.g., from the underlying socket/connection) before trusting forwarded headers.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/api/models.ts, line 124:

<comment>**Security: `TRUSTED_PROXY_IPS` is never actually validated against the connecting IP.** The code only checks `TRUSTED_PROXY_IPS.size === 0` to decide whether to trust `x-forwarded-for`, but never verifies that the request actually originated from a listed proxy IP. Any client can bypass rate limiting by spoofing `x-forwarded-for` as long as `TRUSTED_PROXIES` is set to any non-empty value. The set must be checked against the actual connecting IP (e.g., from the underlying socket/connection) before trusting forwarded headers.</comment>

<file context>
@@ -0,0 +1,201 @@
+		return null;
+	}
+
+	if (TRUST_PROXY_MODE === "true") {
+		// Without TRUSTED_PROXIES configured, x-forwarded-for is spoofable.
+		// Fail closed: return null so the request is rejected with a 400,
</file context>

[cubic-dev-ai]

P1: The OpenRouter API fetch call is not wrapped in a context.run() or context.call() step. Upstash Workflow replays the serve handler multiple times, skipping completed steps. Since this HTTP call isn't a step, it will be re-executed on every replay/retry — causing duplicate API calls, wasted credits, and potential inconsistent state.

Prefer context.call() for external HTTP requests (it offloads the call to Upstash and persists the result), or at minimum wrap the fetch in context.run("call-llm", async () => { ... }).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/api/workflow/generate-title.ts, line 249:

<comment>The OpenRouter API `fetch` call is not wrapped in a `context.run()` or `context.call()` step. Upstash Workflow replays the `serve` handler multiple times, skipping completed steps. Since this HTTP call isn't a step, it will be re-executed on every replay/retry — causing duplicate API calls, wasted credits, and potential inconsistent state.

Prefer `context.call()` for external HTTP requests (it offloads the call to Upstash and persists the result), or at minimum wrap the fetch in `context.run("call-llm", async () => { ... })`.</comment>

<file context>
@@ -0,0 +1,438 @@
+	let llmResponseStatus = 0;
+	let llmResponseBody: { choices?: Array<{ message?: { content?: string } }> } | null = null;
+	try {
+		const response = await fetch("https://openrouter.ai/api/v1/chat/completions", {
+			method: "POST",
+			headers: {
</file context>

[cubic-dev-ai]

P1: Race condition: the negative-balance clamp (SET key 0) is not atomic with the preceding INCRBY. A concurrent incrementDailyUsageInUpstash or reserveDailyUsageInUpstash between the two pipeline calls will have its increment silently discarded when the SET overwrites the key. Consider using a Lua script (EVAL) to make the decrement-and-clamp atomic, e.g.:

local v = redis.call('INCRBY', KEYS[1], ARGV[1])
if v < 0 then redis.call('SET', KEYS[1], 0) end
redis.call('EXPIREAT', KEYS[1], ARGV[2])
return v

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/server/convex/lib/upstashUsage.ts, line 145:

<comment>Race condition: the negative-balance clamp (SET key 0) is not atomic with the preceding INCRBY. A concurrent `incrementDailyUsageInUpstash` or `reserveDailyUsageInUpstash` between the two pipeline calls will have its increment silently discarded when the SET overwrites the key. Consider using a Lua script (`EVAL`) to make the decrement-and-clamp atomic, e.g.:
```lua
local v = redis.call('INCRBY', KEYS[1], ARGV[1])
if v < 0 then redis.call('SET', KEYS[1], 0) end
redis.call('EXPIREAT', KEYS[1], ARGV[2])
return v
```</comment>

<file context>
@@ -0,0 +1,200 @@
+
+	try {
+		const expiresAt = getMidnightUtcEpochSeconds(dateKey);
+		const result = await executePipeline([
+			["INCRBY", key, roundedCents],
+			["EXPIREAT", key, expiresAt],
</file context>

[cubic-dev-ai]

P2: Fragile rate-limit detection: matching on error message content instead of the structured error.name. The server explicitly sets error.name = "RateLimitError" for this purpose, and line 520 in this same file still uses the error.name check. If the server-side action string changes from "title generations", this detection silently breaks. Consider using error.name === "RateLimitError" for consistency and reliability.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/app-sidebar.tsx, line 454:

<comment>Fragile rate-limit detection: matching on error message content instead of the structured `error.name`. The server explicitly sets `error.name = "RateLimitError"` for this purpose, and line 520 in this same file still uses the `error.name` check. If the server-side action string changes from `"title generations"`, this detection silently breaks. Consider using `error.name === "RateLimitError"` for consistency and reliability.</comment>

<file context>
@@ -422,28 +422,37 @@ export function AppSidebar({
-      if (error instanceof Error && error.name === "RateLimitError") {
-        toast.error(error.message);
+      const message = error instanceof Error ? error.message : "";
+      if (message.toLowerCase().includes("too many title generations")) {
+        toast.error("Too many title generations. Please try again later.");
       } else {
</file context>

[cubic-dev-ai]

P2: userId is optional in ExportChatPayload but is cast to Id<"users"> without a null check. If userId is undefined, this will cause a Convex validation error at runtime. Add a guard before the query call.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/api/workflow/export-chat.ts, line 124:

<comment>`userId` is optional in `ExportChatPayload` but is cast to `Id<"users">` without a null check. If `userId` is `undefined`, this will cause a Convex validation error at runtime. Add a guard before the query call.</comment>

<file context>
@@ -0,0 +1,226 @@
+	const convexClient = createConvexServerClient(authToken);
+	const chatExportData = await convexClient.query(api.chats.getChatExportData, {
+		chatId: chatId as Id<"chats">,
+		userId: userId as Id<"users">,
+	});
+	if (!chatExportData) {
</file context>

[cubic-dev-ai]

P2: batchSize argument is not forwarded for delete-chat-read-statuses and delete-prompt-templates steps, unlike all other steps in this action. This means callers cannot control batch size for these two steps, and they'll always use the default (100). This looks like an oversight since the underlying mutations accept batchSize.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/server/convex/users.ts, line 870:

<comment>`batchSize` argument is not forwarded for `delete-chat-read-statuses` and `delete-prompt-templates` steps, unlike all other steps in this action. This means callers cannot control batch size for these two steps, and they'll always use the default (100). This looks like an oversight since the underlying mutations accept `batchSize`.</comment>

<file context>
@@ -598,18 +596,309 @@ export const updateName = mutation({
+					userId,
+					batchSize: args.batchSize,
+				});
+			case "delete-chat-read-statuses":
+				return await ctx.runMutation(internal.users.deleteUserChatReadStatuses, {
+					userId,
</file context>

Suggested change

	case "delete-chat-read-statuses":
	return await ctx.runMutation(internal.users.deleteUserChatReadStatuses, {
	userId,
	});
	case "delete-prompt-templates":
	return await ctx.runMutation(internal.users.deleteUserPromptTemplates, {
	userId,
	});
	case "delete-chat-read-statuses":
	return await ctx.runMutation(internal.users.deleteUserChatReadStatuses, {
	userId,
	batchSize: args.batchSize,
	});
	case "delete-prompt-templates":
	return await ctx.runMutation(internal.users.deleteUserPromptTemplates, {
	userId,
	batchSize: args.batchSize,
	});

[cubic-dev-ai]

P2: Validation mismatch: Number.isFinite allows non-integer values (e.g. 1.5) that the downstream cleanupSoftDeletedRecords mutation rejects with Number.isInteger. This causes a confusing error from the wrong layer. Use Number.isInteger here to match the mutation's validation and fail early with a clear message.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/server/convex/cleanupAction.ts, line 38:

<comment>Validation mismatch: `Number.isFinite` allows non-integer values (e.g. `1.5`) that the downstream `cleanupSoftDeletedRecords` mutation rejects with `Number.isInteger`. This causes a confusing error from the wrong layer. Use `Number.isInteger` here to match the mutation's validation and fail early with a clear message.</comment>

<file context>
@@ -0,0 +1,53 @@
+
+		// Bounds validation — prevent accidental full-purge or oversized batches
+		const retentionDays = args.retentionDays ?? 90;
+		if (!Number.isFinite(retentionDays) || retentionDays < 1 || retentionDays > 3650) {
+			throw new Error("retentionDays must be between 1 and 3650");
+		}
</file context>

[cubic-dev-ai]

P2: Missing fetch mock in second generateAndSetTitleInternal test. If the action implementation calls fetch before checking the existing title, this test will attempt a real network request instead of using a mock, making it flaky and environment-dependent. Add a fetch mock (or move the shared mock into beforeEach) to ensure the test is fully isolated.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/server/convex/chats.test.ts, line 1287:

<comment>Missing `fetch` mock in second `generateAndSetTitleInternal` test. If the action implementation calls `fetch` before checking the existing title, this test will attempt a real network request instead of using a mock, making it flaky and environment-dependent. Add a `fetch` mock (or move the shared mock into `beforeEach`) to ensure the test is fully isolated.</comment>

<file context>
@@ -1111,3 +1111,194 @@ describe('chats.checkExportRateLimit', () => {
+		expect(chat?.title).toBe('Helpful Testing Title');
+	});
+
+	it('does not overwrite existing custom titles unless forced', async () => {
+		await t.run(async (ctx) => {
+			await ctx.db.patch(chatId, { title: 'Custom Existing Title' });
</file context>

Suggested change

	it('does not overwrite existing custom titles unless forced', async () => {
	it('does not overwrite existing custom titles unless forced', async () => {
	vi.spyOn(globalThis, 'fetch').mockResolvedValue(
	new Response(
	JSON.stringify({
	choices: [{ message: { content: 'Should Not Be Used' } }],
	}),
	{
	status: 200,
	headers: { 'Content-Type': 'application/json' },
	},
	),
	);

[cubic-dev-ai]

P2: Payload validation throws outside a context.run step, which may cause unnecessary QStash retries on invalid payloads. Wrap the parsing in a context.run step (as done in generate-title.ts) so the framework can track the failure and avoid retry storms.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/api/workflow/cleanup.ts, line 182:

<comment>Payload validation throws outside a `context.run` step, which may cause unnecessary QStash retries on invalid payloads. Wrap the parsing in a `context.run` step (as done in `generate-title.ts`) so the framework can track the failure and avoid retry storms.</comment>

<file context>
@@ -0,0 +1,310 @@
+}
+
+const workflow = serve<CleanupPayload>(async (context) => {
+	const payload = parseCleanupPayload(context.requestPayload);
+	if (!payload) {
+		throw new Error("Invalid cleanup payload");
</file context>

[cubic-dev-ai]

P3: Inconsistent indentation: the re-added state properties use 9-space indentation while the rest of the object (action methods) uses 8-space indentation. This appears to be an accidental extra space introduced during the removal of jonMode/dynamicPrompt.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/stores/ui.ts, line 34:

<comment>Inconsistent indentation: the re-added state properties use 9-space indentation while the rest of the object (action methods) uses 8-space indentation. This appears to be an accidental extra space introduced during the removal of `jonMode`/`dynamicPrompt`.</comment>

<file context>
@@ -27,20 +25,16 @@ interface UIState {
-        filterStyle: "model" as FilterStyle,
-        jonMode: false,
-        dynamicPrompt: true,
+         sidebarOpen: true,
+         sidebarCollapsed: false,
+         commandPaletteOpen: false,
</file context>

Suggested change

	sidebarOpen: true,
	sidebarCollapsed: false,
	commandPaletteOpen: false,
	filterStyle: "model" as FilterStyle,
	sidebarOpen: true,
	sidebarCollapsed: false,
	commandPaletteOpen: false,
	filterStyle: "model" as FilterStyle,

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
osschat-web	Error		Feb 14, 2026 2:54am