Fix localStorage security issue

[tembo]

Summary

Migrated sensitive chat content, drafts, and pending messages from localStorage to sessionStorage to mitigate XSS exfiltration risks and data leakage on shared devices. Added automatic cleanup of sensitive session data on sign-out.

Changes:

• Replaced localStorage with sessionStorage in app-sidebar.tsx for chat cache persistence
• Updated prompt-draft.ts store to use sessionStorage instead of localStorage
• Updated stream.ts store to use sessionStorage instead of localStorage
• Added SENSITIVE_SESSION_KEYS array in auth-client.tsx to track sensitive storage keys
• Implemented automatic cleanup of sensitive session data on sign-out to prevent data persistence on shared devices
• Updated comments and documentation to reflect sessionStorage usage

Security improvements:

• Data now scoped to browser session/tab only
• Sensitive content automatically cleared when browser session ends
• Explicit cleanup on logout prevents data leakage on shared machines
• Reduced XSS impact surface by limiting data persistence

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

---

Summary by cubic

Moves chat content, drafts, and pending messages to sessionStorage to reduce XSS risk and avoid data lingering on shared devices. Caches only minimal chat fields and clears sensitive session data on sign-out.

• Bug Fixes
  • Use sessionStorage for chat cache, drafts, and stream state; drafts auto-expire after 24h.
  • Sidebar caches only chat id, title, and updatedAt in sessionStorage.
  • Add SENSITIVE_SESSION_KEYS and clear them on sign-out; session data clears on browser close.
  • Harden /api/models with IP-based rate limiting and trusted-proxy IP extraction.

^{Written for commit 252194b. 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/stores/stream.ts
• apps/web/src/lib/auth-client.tsx
• apps/web/src/stores/prompt-draft.ts
• apps/web/src/components/app-sidebar.tsx

[cubic-dev-ai]

15 issues found across 46 files

Confidence score: 3/5

• Workflow step in apps/web/src/routes/api/workflow/generate-title.ts calls OpenRouter outside context.run()/context.call(), which can break Upstash workflow execution and re-run behavior.
• Rate limiting in apps/web/src/routes/api/models.ts can be bypassed via spoofed x-forwarded-for, creating a concrete abuse risk despite otherwise normal flow.
• Several medium issues (long-running cleanup in apps/web/src/routes/api/workflow/cleanup.ts, race in apps/server/convex/lib/upstashUsage.ts) add uncertainty, so merge risk is moderate.
• Pay close attention to apps/web/src/routes/api/workflow/generate-title.ts, apps/web/src/routes/api/models.ts, apps/web/src/routes/api/workflow/cleanup.ts, apps/server/convex/lib/upstashUsage.ts - workflow correctness, rate limiting, and long-running or racy operations.

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/cleanup.ts">

<violation number="1" location="apps/web/src/routes/api/workflow/cleanup.ts:108">
P2: The `runCleanupInline` function can run for a very long time in a single HTTP request. With `MAX_CLEANUP_BATCHES = 1_000` iterations, each doing 2 network calls (15s timeout each) plus a 1s sleep, this could easily exceed request timeouts in serverless/dev environments. Consider adding a per-request time budget (e.g., 60s wall clock) and returning partial progress, or lowering `MAX_CLEANUP_BATCHES` for inline execution.</violation>

<violation number="2" location="apps/web/src/routes/api/workflow/cleanup.ts:182">
P2: Inconsistent with codebase pattern: payload parsing should be wrapped in `context.run` to match the established convention in `generate-title.ts`. This ensures the parsed result is cached by the workflow runtime and avoids unnecessary re-parsing on step replays.</violation>
</file>

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

<violation number="1" location="apps/server/convex/lib/upstashUsage.ts:145">
P2: TOCTOU race condition: the negative-total check and `SET key 0` correction is not atomic with the preceding `INCRBY`. A concurrent increment between the two pipeline calls would be silently overwritten, losing usage data. Consider using a Lua script via `EVAL` to make the decrement-and-floor-to-zero operation atomic, or accept a transiently negative counter and floor it at read time instead.</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 handling: `forkMessage` exposes raw error message to users while `editMessage` and `retryMessage` were updated to use `getUserFriendlyError()`. Apply the same sanitization here.</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`) for `retentionDays`, but the downstream mutation `cleanupSoftDeletedRecords` requires `Number.isInteger`. This means invalid values pass this check only to fail later in the mutation with a less clear context. Use `Number.isInteger` to match the mutation's validation and fail early.</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 deletion steps. The underlying mutations accept `batchSize` but it's silently dropped here.</violation>
</file>

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

<violation number="1" location="apps/web/src/lib/server-auth.ts:82">
P1: `ALLOW_AUTH_COOKIE_FALLBACK` is defined and guarded against production use, but never actually checked in the fallback code path. The cookie-based JWT fallback is always active in dev/test regardless of this env var. The gate should require both `IS_LOCAL_DEV` and `ALLOW_AUTH_COOKIE_FALLBACK` to be truthy.</violation>
</file>

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

<violation number="1" location="apps/web/src/stores/ui.ts:34">
P3: Inconsistent indentation: added lines use 9 spaces while surrounding code in the same object uses 8 spaces. This appears to be an accidental extra space introduced during editing.</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 uncollapsed consecutive spaces in the title. Move the tag strip before the space collapse, or re-collapse spaces afterward.</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:249">
P1: The `fetch` call to OpenRouter is not wrapped in `context.run()` or `context.call()`, violating the Upstash Workflow execution model. Workflow endpoints are re-invoked from the top for each step — code outside `context.run`/`context.call` runs on every replay. This means the LLM API call will be re-executed on every subsequent step (e.g., when `save-title` runs), causing duplicate API calls, wasted credits, and non-deterministic behavior.

Use `context.call()` instead, which is specifically designed for external HTTP requests in workflows — it offloads the call to Upstash infrastructure and properly integrates with the step/retry system.</violation>
</file>

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

<violation number="1" location="apps/web/src/components/app-sidebar.tsx:268">
P2: Old `localStorage` data under `CHATS_CACHE_KEY` is never cleaned up during migration. Existing users will have their sensitive chat cache persisted in `localStorage` indefinitely. Add a one-time cleanup of the old localStorage key here to complete the security migration.</violation>

<violation number="2" location="apps/web/src/components/app-sidebar.tsx:454">
P2: Matching rate-limit errors by substring (`"too many title generations"`) is fragile. If the backend message wording changes, this check silently breaks. Consider matching on a structured error property (e.g., error code or name) instead of message text.</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:120">
P2: `userId` is optional in `ExportChatPayload` but used as required in `runExportChatInline`. If the function is ever called without `userId` set, the Convex query will throw a validation error at runtime. Add a guard or make `userId` required for this function's input.</violation>
</file>

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

<violation number="1" location="apps/web/src/routes/api/models.ts:100">
P2: Rate limit bypass: the leftmost `x-forwarded-for` IP is client-controlled and trivially spoofable. An attacker can set a unique `X-Forwarded-For` header on every request to get a fresh rate-limit bucket each time. Consider using the rightmost IP (closest to your trusted proxy), or requiring the reverse proxy to overwrite (not append to) the header and documenting that requirement.</violation>
</file>

<file name="apps/web/src/lib/redis.ts">

<violation number="1" location="apps/web/src/lib/redis.ts:186">
P2: Using `Date.now()` as fallback timestamp silently masks corrupt/missing data and changes behavior from the original code (which defaulted to `0`). A current timestamp could be mistaken for a real value, whereas `0` clearly signals "unknown". Consider using `0` to preserve the original semantics.

Additionally, the IIFE-in-ternary pattern here is harder to read than a small helper function or extracting the logic beforehand.</violation>
</file>

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

[cubic-dev-ai]

P1: ALLOW_AUTH_COOKIE_FALLBACK is defined and guarded against production use, but never actually checked in the fallback code path. The cookie-based JWT fallback is always active in dev/test regardless of this env var. The gate should require both IS_LOCAL_DEV and ALLOW_AUTH_COOKIE_FALLBACK to be truthy.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/lib/server-auth.ts, line 82:

<comment>`ALLOW_AUTH_COOKIE_FALLBACK` is defined and guarded against production use, but never actually checked in the fallback code path. The cookie-based JWT fallback is always active in dev/test regardless of this env var. The gate should require both `IS_LOCAL_DEV` and `ALLOW_AUTH_COOKIE_FALLBACK` to be truthy.</comment>

<file context>
@@ -16,18 +16,81 @@ type AuthSessionResponse = {
+			if (response.status >= 400 && response.status < 500) {
+				return null;
+			}
+			if (!IS_LOCAL_DEV) return null;
+		} catch {
+			if (!IS_LOCAL_DEV) return null;
</file context>

[cubic-dev-ai]

P1: The fetch call to OpenRouter is not wrapped in context.run() or context.call(), violating the Upstash Workflow execution model. Workflow endpoints are re-invoked from the top for each step — code outside context.run/context.call runs on every replay. This means the LLM API call will be re-executed on every subsequent step (e.g., when save-title runs), causing duplicate API calls, wasted credits, and non-deterministic behavior.

Use context.call() instead, which is specifically designed for external HTTP requests in workflows — it offloads the call to Upstash infrastructure and properly integrates with the step/retry system.

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 `fetch` call to OpenRouter is not wrapped in `context.run()` or `context.call()`, violating the Upstash Workflow execution model. Workflow endpoints are re-invoked from the top for each step — code outside `context.run`/`context.call` runs on every replay. This means the LLM API call will be re-executed on every subsequent step (e.g., when `save-title` runs), causing duplicate API calls, wasted credits, and non-deterministic behavior.

Use `context.call()` instead, which is specifically designed for external HTTP requests in workflows — it offloads the call to Upstash infrastructure and properly integrates with the step/retry system.</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]

P2: Inconsistent with codebase pattern: payload parsing should be wrapped in context.run to match the established convention in generate-title.ts. This ensures the parsed result is cached by the workflow runtime and avoids unnecessary re-parsing on step replays.

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>Inconsistent with codebase pattern: payload parsing should be wrapped in `context.run` to match the established convention in `generate-title.ts`. This ensures the parsed result is cached by the workflow runtime and avoids unnecessary re-parsing on step replays.</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]

P2: The runCleanupInline function can run for a very long time in a single HTTP request. With MAX_CLEANUP_BATCHES = 1_000 iterations, each doing 2 network calls (15s timeout each) plus a 1s sleep, this could easily exceed request timeouts in serverless/dev environments. Consider adding a per-request time budget (e.g., 60s wall clock) and returning partial progress, or lowering MAX_CLEANUP_BATCHES for inline execution.

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

<comment>The `runCleanupInline` function can run for a very long time in a single HTTP request. With `MAX_CLEANUP_BATCHES = 1_000` iterations, each doing 2 network calls (15s timeout each) plus a 1s sleep, this could easily exceed request timeouts in serverless/dev environments. Consider adding a per-request time budget (e.g., 60s wall clock) and returning partial progress, or lowering `MAX_CLEANUP_BATCHES` for inline execution.</comment>

<file context>
@@ -0,0 +1,310 @@
+	let totalDeleted = 0;
+	let hitBatchLimit = false;
+
+	while (batches < MAX_CLEANUP_BATCHES) {
+		batches += 1;
+		const preview = await runCleanupBatch({
</file context>

[cubic-dev-ai]

P2: TOCTOU race condition: the negative-total check and SET key 0 correction is not atomic with the preceding INCRBY. A concurrent increment between the two pipeline calls would be silently overwritten, losing usage data. Consider using a Lua script via EVAL to make the decrement-and-floor-to-zero operation atomic, or accept a transiently negative counter and floor it at read time instead.

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>TOCTOU race condition: the negative-total check and `SET key 0` correction is not atomic with the preceding `INCRBY`. A concurrent increment between the two pipeline calls would be silently overwritten, losing usage data. Consider using a Lua script via `EVAL` to make the decrement-and-floor-to-zero operation atomic, or accept a transiently negative counter and floor it at read time instead.</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: Matching rate-limit errors by substring ("too many title generations") is fragile. If the backend message wording changes, this check silently breaks. Consider matching on a structured error property (e.g., error code or name) instead of message text.

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>Matching rate-limit errors by substring (`"too many title generations"`) is fragile. If the backend message wording changes, this check silently breaks. Consider matching on a structured error property (e.g., error code or name) instead of message text.</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 used as required in runExportChatInline. If the function is ever called without userId set, the Convex query will throw a validation error at runtime. Add a guard or make userId required for this function's input.

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

<comment>`userId` is optional in `ExportChatPayload` but used as required in `runExportChatInline`. If the function is ever called without `userId` set, the Convex query will throw a validation error at runtime. Add a guard or make `userId` required for this function's input.</comment>

<file context>
@@ -0,0 +1,226 @@
+	payload: ExportChatPayload,
+	authToken: string,
+): Promise<{ downloadUrl: string; byteLength: number; fileName: string }> {
+	const { chatId, userId, format = "markdown" } = payload;
+	const convexClient = createConvexServerClient(authToken);
+	const chatExportData = await convexClient.query(api.chats.getChatExportData, {
</file context>

[cubic-dev-ai]

P2: Using Date.now() as fallback timestamp silently masks corrupt/missing data and changes behavior from the original code (which defaulted to 0). A current timestamp could be mistaken for a real value, whereas 0 clearly signals "unknown". Consider using 0 to preserve the original semantics.

Additionally, the IIFE-in-ternary pattern here is harder to read than a small helper function or extracting the logic beforehand.

Prompt for AI agents

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

<comment>Using `Date.now()` as fallback timestamp silently masks corrupt/missing data and changes behavior from the original code (which defaulted to `0`). A current timestamp could be mistaken for a real value, whereas `0` clearly signals "unknown". Consider using `0` to preserve the original semantics.

Additionally, the IIFE-in-ternary pattern here is harder to read than a small helper function or extracting the logic beforehand.</comment>

<file context>
@@ -172,25 +163,40 @@ export async function readStream(
+				: "text",
+		timestamp:
+			typeof message.ts === "string"
+				? (() => {
+						const parsed = Number.parseInt(message.ts, 10);
+						return Number.isFinite(parsed) ? parsed : Date.now();
</file context>

Suggested change

	? (() => {
	const parsed = Number.parseInt(message.ts, 10);
	return Number.isFinite(parsed) ? parsed : Date.now();
	})()
	: typeof message.ts === "number" && Number.isFinite(message.ts)
	? message.ts
	: Date.now(),
	? (() => {
	const parsed = Number.parseInt(message.ts, 10);
	return Number.isFinite(parsed) ? parsed : 0;
	})()
	: typeof message.ts === "number" && Number.isFinite(message.ts)
	? message.ts
	: 0,

[cubic-dev-ai]

P3: Inconsistent indentation: added lines use 9 spaces while surrounding code in the same object uses 8 spaces. This appears to be an accidental extra space introduced during editing.

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: added lines use 9 spaces while surrounding code in the same object uses 8 spaces. This appears to be an accidental extra space introduced during editing.</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:56am