Fix rate-limit bypass

[tembo]

Summary

Fixed potential rate-limit bypass vulnerability when TRUST_PROXY=true by:

• Added IP format validation (isValidIpFormat()) to reject malformed/spoofed IPs using IPv4/IPv6 regex patterns
• Prioritized platform-specific headers (cf-connecting-ip, x-vercel-forwarded-for, x-real-ip) over user-controlled x-forwarded-for
• Applied IP validation to all proxy modes (cloudflare, vercel, true)
• Enhanced warning message for TRUST_PROXY=true mode to highlight security risks
• Made x-forwarded-for a last-resort fallback with validation

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 when TRUST_PROXY is enabled by validating client IPs and prioritizing trusted proxy headers. Adds IP-based rate limiting to /api/models and hardens OpenRouter fetch with timeouts, caching, and 502 handling.

• Bug Fixes
  • Added IPv4/IPv6 checks (isValidIpFormat) for all proxy modes.
  • Prefer cf-connecting-ip, x-vercel-forwarded-for, then x-real-ip; use x-forwarded-for only as a validated last resort.
  • Added IP rate limit (30 req/60s) and safer OpenRouter fetch (10s timeout, 502 on failure, Redis cache); improved TRUST_PROXY warnings and fail-closed when unset/invalid.

^{Written for commit 8309c31. 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]

17 issues found across 43 files

Confidence score: 2/5

• High-risk logic issues: apps/web/src/routes/api/workflow/generate-title.ts makes the OpenRouter fetch outside context.run()/call(), so retries/replays can re-execute business logic unexpectedly.
• Concurrency/data correctness risk in apps/server/convex/lib/upstashUsage.ts where the floor-to-zero correction can erase a concurrent INCRBY, leading to inaccurate usage tracking.
• Additional user-facing and validation concerns (e.g., raw error exposure in apps/web/src/hooks/use-persistent-chat.ts, unsafe userId cast in apps/web/src/routes/api/workflow/export-chat.ts, and overly permissive IPv6 regex in apps/web/src/routes/api/models.ts) add uncertainty.
• Pay close attention to apps/web/src/routes/api/workflow/generate-title.ts, apps/server/convex/lib/upstashUsage.ts, apps/web/src/hooks/use-persistent-chat.ts, apps/web/src/routes/api/workflow/export-chat.ts, apps/web/src/routes/api/models.ts - replays/races and user-facing validation/errors.

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/server/convex/users.ts">

<violation number="1" location="apps/server/convex/users.ts:870">
P2: `batchSize` is not forwarded to `deleteUserChatReadStatuses` here, unlike all other batch-delete steps in this action. The underlying mutation accepts `batchSize`, so this appears to be an accidental omission.</violation>

<violation number="2" location="apps/server/convex/users.ts:874">
P2: `batchSize` is not forwarded to `deleteUserPromptTemplates` here, unlike all other batch-delete steps in this action.</violation>
</file>

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

<violation number="1" location="apps/web/src/routes/api/workflow/generate-title.ts:246">
P1: The OpenRouter API `fetch` is not wrapped in `context.run()` or `context.call()`, so it will be re-executed on every workflow replay/retry. According to the Upstash Workflow docs, all business logic must be inside `context.run()` steps, and `context.call()` is specifically designed for HTTP requests as workflow steps. Use `context.call()` here so the request is executed once and the result is durably stored.</violation>
</file>

<file name="apps/web/src/hooks/use-persistent-chat.ts">

<violation number="1" location="apps/web/src/hooks/use-persistent-chat.ts:1126">
P2: Inconsistent error sanitization: `forkMessage` still exposes raw `parsedError.message` to the user while `editMessage` and `retryMessage` were updated to use `getUserFriendlyError()`. This could leak internal error details (API structures, DB field names, etc.).</violation>
</file>

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

<violation number="1" location="apps/server/convex/crons.ts:77">
P3: Error message is misleading: validation rejects non-integer values (e.g. `1.5`) but the message only mentions the range constraint. Include the integer requirement so callers can understand why their input was rejected.</violation>

<violation number="2" location="apps/server/convex/crons.ts:80">
P3: Same misleading error message: validation rejects non-integer values but the message only mentions the range. Include the integer requirement.</violation>
</file>

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

<violation number="1" location="apps/server/convex/lib/sanitize.ts:44">
P2: HTML tag stripping is ordered after space collapsing, so removing tags can leave behind uncollapsed multiple spaces (e.g., `"hello <b> world </b> test"` → `"hello  world  test"`). Move this line before the space-collapse step, or re-collapse spaces afterward.</violation>
</file>

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

<violation number="1" location="apps/server/convex/lib/upstashUsage.ts:189">
P1: Race condition: the floor-to-zero correction reads the total from one pipeline then issues `SET key 0` in a separate pipeline. A concurrent `INCRBY` between the two calls will be silently erased. Consider using a Lua script (Upstash supports `EVAL`) to atomically clamp the value, 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/web/src/lib/server-auth.ts">

<violation number="1" location="apps/web/src/lib/server-auth.ts:20">
P2: `ALLOW_AUTH_COOKIE_FALLBACK` is defined and guarded at module level but never consulted in the actual fallback path. The cookie fallback is unconditionally enabled in dev/test via `IS_LOCAL_DEV`. If this env var is meant to be an opt-in gate, it should be checked before using the fallback; otherwise, remove the dead variable and the module-level guard.</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: Unsafe `as Id<"users">` cast on a potentially `undefined` value. `ExportChatPayload.userId` is typed as `string | undefined`, so `userId` here can be `undefined`. The Convex query requires a valid `Id<"users">` and will throw a validation error at runtime if it receives `undefined`. Add an explicit check before the query call to fail with a clear error.</violation>
</file>

<file name="scripts/dev.ts">

<violation number="1" location="scripts/dev.ts:25">
P2: Local variable `process` shadows the global `process` object. Consider renaming to `subprocess`, `child`, or `proc` for clarity and to avoid accidental misuse if this function is later extended.</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 string-based error detection: matching against `message.toLowerCase().includes("too many title generations")` will silently break if the server-side error message text changes. Consider using a structured error property (e.g., `error.name`, a custom error code, or a `ConvexError` data field) instead of relying on substring matching on the human-readable message.</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 fractional values (e.g., `1.5`) that the downstream mutation rejects with `Number.isInteger()`. Use `Number.isInteger()` here to match the mutation's validation and surface a clear error message at the entry point.</violation>

<violation number="2" location="apps/server/convex/cleanupAction.ts:42">
P2: Same validation mismatch as `retentionDays`: `Number.isFinite()` should be `Number.isInteger()` to match the downstream mutation's validation.</violation>
</file>

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

<violation number="1" location="apps/web/src/routes/api/models.ts:16">
P2: IPv4 regex doesn't validate octet ranges (0–255), so values like `999.999.999.999` pass validation. Consider using Node.js `net.isIP(ip) !== 0` which validates both IPv4 and IPv6 correctly, replacing both regex patterns with a single robust check.</violation>

<violation number="2" location="apps/web/src/routes/api/models.ts:17">
P2: IPv6 regex is too permissive — it matches any hex string (e.g., `a`, `deadbeef`, `:`) as a valid IPv6 address. Since this validation exists specifically to reject spoofed/malformed values, accepting arbitrary hex strings undermines its purpose. At minimum, require the presence of a colon, or better yet use Node's built-in `net.isIP()` which handles both IPv4 and IPv6 correctly.</violation>
</file>

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

<violation number="1" location="apps/server/convex/chats.ts:571">
P3: Indentation mismatch: `body` property is indented one level less than sibling properties `method` and `headers` in the `fetch` options object. This makes the object structure visually misleading. The `body` and closing `});` should be indented to match `method` and `headers`.</violation>
</file>

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

[cubic-dev-ai]

P1: The OpenRouter API fetch is not wrapped in context.run() or context.call(), so it will be re-executed on every workflow replay/retry. According to the Upstash Workflow docs, all business logic must be inside context.run() steps, and context.call() is specifically designed for HTTP requests as workflow steps. Use context.call() here so the request is executed once and the result is durably stored.

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 246:

<comment>The OpenRouter API `fetch` is not wrapped in `context.run()` or `context.call()`, so it will be re-executed on every workflow replay/retry. According to the Upstash Workflow docs, all business logic must be inside `context.run()` steps, and `context.call()` is specifically designed for HTTP requests as workflow steps. Use `context.call()` here so the request is executed once and the result is durably stored.</comment>

<file context>
@@ -0,0 +1,438 @@
+		TITLE_STYLE_PROMPTS[length],
+	].join(" ");
+
+	let llmResponseStatus = 0;
+	let llmResponseBody: { choices?: Array<{ message?: { content?: string } }> } | null = null;
+	try {
</file context>

[cubic-dev-ai]

P1: Race condition: the floor-to-zero correction reads the total from one pipeline then issues SET key 0 in a separate pipeline. A concurrent INCRBY between the two calls will be silently erased. Consider using a Lua script (Upstash supports EVAL) to atomically clamp the value, 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 189:

<comment>Race condition: the floor-to-zero correction reads the total from one pipeline then issues `SET key 0` in a separate pipeline. A concurrent `INCRBY` between the two calls will be silently erased. Consider using a Lua script (Upstash supports `EVAL`) to atomically clamp the value, 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 @@
+				: typeof total === "string"
+					? Number.parseInt(total, 10)
+					: null;
+		if (typeof parsedTotal === "number" && Number.isFinite(parsedTotal) && parsedTotal < 0) {
+			await executePipeline([
+				["SET", key, 0],
</file context>

[cubic-dev-ai]

P2: batchSize is not forwarded to deleteUserPromptTemplates here, unlike all other batch-delete steps in this action.

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 874:

<comment>`batchSize` is not forwarded to `deleteUserPromptTemplates` here, unlike all other batch-delete steps in this action.</comment>

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

Suggested change

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

[cubic-dev-ai]

P2: batchSize is not forwarded to deleteUserChatReadStatuses here, unlike all other batch-delete steps in this action. The underlying mutation accepts batchSize, so this appears to be an accidental omission.

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` is not forwarded to `deleteUserChatReadStatuses` here, unlike all other batch-delete steps in this action. The underlying mutation accepts `batchSize`, so this appears to be an accidental omission.</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-chat-read-statuses":
	return await ctx.runMutation(internal.users.deleteUserChatReadStatuses, {
	userId,
	batchSize: args.batchSize,
	});

[cubic-dev-ai]

P2: Inconsistent error sanitization: forkMessage still exposes raw parsedError.message to the user while editMessage and retryMessage were updated to use getUserFriendlyError(). This could leak internal error details (API structures, DB field names, etc.).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/hooks/use-persistent-chat.ts, line 1126:

<comment>Inconsistent error sanitization: `forkMessage` still exposes raw `parsedError.message` to the user while `editMessage` and `retryMessage` were updated to use `getUserFriendlyError()`. This could leak internal error details (API structures, DB field names, etc.).</comment>

<file context>
@@ -1146,47 +1103,43 @@ export function usePersistentChat({
+		} catch (err) {
+			const parsedError = err instanceof Error ? err : new Error("Unknown error");
+			toast.error("Failed to branch off", {
+				description: parsedError.message,
+			});
+			return undefined;
</file context>

[cubic-dev-ai]

P2: IPv4 regex doesn't validate octet ranges (0–255), so values like 999.999.999.999 pass validation. Consider using Node.js net.isIP(ip) !== 0 which validates both IPv4 and IPv6 correctly, replacing both regex patterns with a single robust check.

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 16:

<comment>IPv4 regex doesn't validate octet ranges (0–255), so values like `999.999.999.999` pass validation. Consider using Node.js `net.isIP(ip) !== 0` which validates both IPv4 and IPv6 correctly, replacing both regex patterns with a single robust check.</comment>

<file context>
@@ -0,0 +1,199 @@
+ * Basic IPv4/IPv6 format validation.
+ * Rejects obviously spoofed or malformed values used in x-forwarded-for.
+ */
+const IPV4_REGEX = /^(\d{1,3}\.){3}\d{1,3}$/;
+const IPV6_REGEX = /^[0-9a-fA-F:]+$/;
+
</file context>

[cubic-dev-ai]

P2: IPv6 regex is too permissive — it matches any hex string (e.g., a, deadbeef, :) as a valid IPv6 address. Since this validation exists specifically to reject spoofed/malformed values, accepting arbitrary hex strings undermines its purpose. At minimum, require the presence of a colon, or better yet use Node's built-in net.isIP() which handles both IPv4 and IPv6 correctly.

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 17:

<comment>IPv6 regex is too permissive — it matches any hex string (e.g., `a`, `deadbeef`, `:`) as a valid IPv6 address. Since this validation exists specifically to reject spoofed/malformed values, accepting arbitrary hex strings undermines its purpose. At minimum, require the presence of a colon, or better yet use Node's built-in `net.isIP()` which handles both IPv4 and IPv6 correctly.</comment>

<file context>
@@ -0,0 +1,199 @@
+ * Rejects obviously spoofed or malformed values used in x-forwarded-for.
+ */
+const IPV4_REGEX = /^(\d{1,3}\.){3}\d{1,3}$/;
+const IPV6_REGEX = /^[0-9a-fA-F:]+$/;
+
+function isValidIpFormat(ip: string): boolean {
</file context>

[cubic-dev-ai]

P3: Same misleading error message: validation rejects non-integer values but the message only mentions the range. Include the integer requirement.

Prompt for AI agents

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

<comment>Same misleading error message: validation rejects non-integer values but the message only mentions the range. Include the integer requirement.</comment>

<file context>
@@ -73,6 +73,12 @@ export const cleanupSoftDeletedRecords = internalMutation({
+			throw new Error("retentionDays must be between 1 and 3650");
+		}
+		if (!Number.isInteger(batchSize) || batchSize < 1 || batchSize > 1000) {
+			throw new Error("batchSize must be between 1 and 1000");
+		}
 
</file context>

Suggested change

	throw new Error("batchSize must be between 1 and 1000");
	throw new Error("batchSize must be an integer between 1 and 1000");

[cubic-dev-ai]

P3: Error message is misleading: validation rejects non-integer values (e.g. 1.5) but the message only mentions the range constraint. Include the integer requirement so callers can understand why their input was rejected.

Prompt for AI agents

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

<comment>Error message is misleading: validation rejects non-integer values (e.g. `1.5`) but the message only mentions the range constraint. Include the integer requirement so callers can understand why their input was rejected.</comment>

<file context>
@@ -73,6 +73,12 @@ export const cleanupSoftDeletedRecords = internalMutation({
 		const batchSize = args.batchSize ?? 100;
 		const dryRun = args.dryRun ?? false;
+		if (!Number.isInteger(retentionDays) || retentionDays < 1 || retentionDays > 3650) {
+			throw new Error("retentionDays must be between 1 and 3650");
+		}
+		if (!Number.isInteger(batchSize) || batchSize < 1 || batchSize > 1000) {
</file context>

Suggested change

	throw new Error("retentionDays must be between 1 and 3650");
	throw new Error("retentionDays must be an integer between 1 and 3650");

[cubic-dev-ai]

P3: Indentation mismatch: body property is indented one level less than sibling properties method and headers in the fetch options object. This makes the object structure visually misleading. The body and closing }); should be indented to match method and headers.

Prompt for AI agents

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

<comment>Indentation mismatch: `body` property is indented one level less than sibling properties `method` and `headers` in the `fetch` options object. This makes the object structure visually misleading. The `body` and closing `});` should be indented to match `method` and `headers`.</comment>

<file context>
@@ -526,6 +526,87 @@ const TITLE_STYLE_PROMPTS: Record<"short" | "standard" | "long", string> = {
+				"HTTP-Referer": process.env.CONVEX_SITE_URL || "https://osschat.io",
+				"X-Title": "OSSChat",
+			},
+		body: JSON.stringify({
+			model: TITLE_MODEL_ID,
+			messages: [
</file context>

Suggested change

	body: JSON.stringify({
	body: JSON.stringify({

[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:57am