fix(webapp): harden model-facing sanitizers, claims resolution and soft-delete idempotency (review)

Author: kathiekiwi

.changeset/chat-stream-mid-turn-reconnect.md

@@ -4,4 +4,4 @@
 "trigger.dev": patch
 ---
 
-Chat in the browser now reconnects when the connection drops mid-turn, instead of leaving the reply stuck as if it were still generating. Reports can be fetched as structured data with the `json` format, and the shortest report period is now one minute (`30m`, `1h`, `7d`). The `mint-token` command's help is clearer too: a token minted without `--cap` is read-only, and `--ttl` shows the correct maximum lifetime of 7 days.
+Chat in the browser now reconnects when the connection drops mid-turn, instead of leaving the reply stuck as if it were still generating. Reports can be fetched as structured data with the `json` format, and the shortest report period is now one minute (`1m`, `30m`, `1h`, `7d`). The `mint-token` command's help is clearer too: a token minted without `--cap` is read-only, and `--ttl` shows the correct maximum lifetime of 7 days.

apps/webapp/app/components/code/StreamdownRenderer.test.ts

@@ -20,6 +20,16 @@ describe("restrictModelUrls (image src)", () => {
     expect(restrictModelUrls("//evil.tld/pixel.gif", "src", img)).toBeUndefined();
   });
 
+  it("drops a backslash-authority image, which the browser reads as protocol-relative", () => {
+    expect(restrictModelUrls("\\\\evil.example/pixel.gif", "src", img)).toBeUndefined();
+    expect(restrictModelUrls("/\\evil.example/pixel.gif", "src", img)).toBeUndefined();
+  });
+
+  it("drops an image hidden behind a leading C0 control, which the URL parser discards", () => {
+    expect(restrictModelUrls("\u0001//evil.tld/p.gif", "src", img)).toBeUndefined();
+    expect(restrictModelUrls("\u0000https://evil.tld/p.gif", "src", img)).toBeUndefined();
+  });
+
   it("keeps inline and same-origin images", () => {
     expect(restrictModelUrls("data:image/png;base64,AAAA", "src", img)).toBe(
       "data:image/png;base64,AAAA"

apps/webapp/app/components/code/StreamdownRenderer.tsx

@@ -12,18 +12,25 @@ const SAFE_LINK_SCHEMES = new Set(["http:", "https:", "mailto:"]);
 export const restrictModelUrls: UrlTransform = (url, key, node) => {
   const value = url.trim();
   const isImage = node.tagName === "img" || key === "src" || key === "srcset";
+  // What the browser will actually resolve, which is not what `trim()` leaves: the URL parser
+  // drops C0 controls (`trim()` keeps them) and reads `\` as `/` for special schemes, so
+  // a leading-control `//evil.tld` and `\\evil.tld` both name a remote host. Classify on this; return the
+  // original `url` untouched whenever it is allowed.
+  const normalized = value
+    .replace(/[\u0000-\u001f]/g, "")
+    .replace(/^[/\\]+/, (run) => "/".repeat(run.length));
 
   if (isImage) {
     // Inline images carry their own bytes; a relative path resolves to our own origin.
-    if (/^data:/i.test(value) || /^blob:/i.test(value)) return url;
+    if (/^data:/i.test(normalized) || /^blob:/i.test(normalized)) return url;
     // Absolute or protocol-relative means a remote host — strip it so nothing is fetched.
-    if (/^[a-z][a-z0-9+.-]*:/i.test(value) || value.startsWith("//")) return undefined;
+    if (/^[a-z][a-z0-9+.-]*:/i.test(normalized) || normalized.startsWith("//")) return undefined;
     return url;
   }
 
   // Links: relative and protocol-relative are fine; otherwise require a safe scheme.
-  if (value.startsWith("//")) return url;
-  const schemeMatch = /^([a-z][a-z0-9+.-]*):/i.exec(value);
+  if (normalized.startsWith("//")) return url;
+  const schemeMatch = /^([a-z][a-z0-9+.-]*):/i.exec(normalized);
   if (!schemeMatch) return url;
   return SAFE_LINK_SCHEMES.has(`${schemeMatch[1].toLowerCase()}:`) ? url : undefined;
 };

apps/webapp/app/components/dashboard-agent/DashboardAgentMessages.render.test.ts

@@ -64,8 +64,13 @@ function markup(parts: unknown[]) {
   );
 }
 
+/** Every needle must be present: a missing one is -1, and -1 comparisons pass vacuously. */
 function order(html: string, ...needles: string[]) {
-  return needles.map((needle) => html.indexOf(needle));
+  return needles.map((needle) => {
+    const at = html.indexOf(needle);
+    expect(at, `missing "${needle}"`).toBeGreaterThan(-1);
+    return at;
+  });
 }
 
 describe("action rows render at the end of the turn", () => {

apps/webapp/app/components/dashboard-agent/model-markdown.test.ts

@@ -31,6 +31,29 @@ describe("stripModelImages", () => {
     expect(html).not.toContain("attacker.example");
   });
 
+  it("strips a nested tag, which one pass would splice back into a whole one", () => {
+    expect(stripModelImages(`<im<img src="${BEACON}">g src="${BEACON}">`)).not.toContain(
+      "attacker.example"
+    );
+    expect(stripModelImages("<scr<script>ipt>")).toBe("");
+  });
+
+  it("bails out safely on nesting too deep to unwrap, instead of looping on it", () => {
+    // Each layer needs one more pass, so this is the shape that makes the strip quadratic.
+    let nested = `<script src="${BEACON}">`;
+    for (let i = 0; i < 20_000; i++) nested = `<scr${nested}ipt>`;
+
+    const started = performance.now();
+    const stripped = stripModelImages(nested);
+    const elapsed = performance.now() - started;
+
+    // No bracket survives, so nothing can parse as an element — the URL is left as inert prose.
+    expect(stripped).not.toContain("<script");
+    expect(stripped).not.toContain("<");
+    expect(render(stripped)).not.toMatch(/<script\b/i);
+    expect(elapsed).toBeLessThan(1_000);
+  });
+
   it("strips an image hidden inside a code fence", () => {
     const stripped = stripModelImages(`\`\`\`\n![](${BEACON})\n\`\`\``);
     expect(stripped).not.toContain("attacker.example");

apps/webapp/app/components/dashboard-agent/model-markdown.ts

@@ -13,10 +13,33 @@ function plainAlt(alt: string): string {
   return alt.replace(/[![\]<>`]/g, "").trim();
 }
 
+/**
+ * One pass isn't enough: removing a match can splice the surrounding text into a fresh one
+ * (`<scr<script>ipt>`), so repeat until nothing changes. Nesting deep enough to need more than
+ * `MAX_PASSES` is adversarial, not real model output, and looping it out is quadratic on the
+ * render thread — so past the bound we stop and blunt every character the strips key on. The
+ * result is over-stripped, never half-stripped.
+ */
+const MAX_PASSES = 25;
+const STRIP_CHARS = /[![\]<>]/g;
+
+function replaceUntilStable(
+  text: string,
+  pattern: RegExp,
+  replacer: (whole: string, ...groups: string[]) => string
+): string {
+  let current = text;
+  for (let pass = 0; pass < MAX_PASSES; pass++) {
+    const next = current.replace(pattern, replacer);
+    if (next === current) return current;
+    current = next;
+  }
+  return current.replace(STRIP_CHARS, "");
+}
+
 // Strips inside code fences too: a fence-aware pass is bypassable with a half-fence.
 export function stripModelImages(text: string): string {
-  return text
-    .replace(MARKDOWN_IMAGE, (_whole, alt: string) => plainAlt(alt))
-    .replace(MARKDOWN_SHORTCUT_IMAGE, (_whole, alt: string) => plainAlt(alt))
-    .replace(FETCHING_TAG, "");
+  let out = replaceUntilStable(text, MARKDOWN_IMAGE, (_whole, alt: string) => plainAlt(alt));
+  out = replaceUntilStable(out, MARKDOWN_SHORTCUT_IMAGE, (_whole, alt: string) => plainAlt(alt));
+  return replaceUntilStable(out, FETCHING_TAG, () => "");
 }

apps/webapp/app/services/dashboardAgentBodyCap.server.ts

@@ -17,9 +17,11 @@ const INGRESS_SLACK_BYTES = 8 * 1024;
 export const DASHBOARD_AGENT_MAX_INGRESS_BYTES = MAX_MESSAGE_BODY_BYTES + INGRESS_SLACK_BYTES;
 
 // The agent's own routes only: the `/api/v1/dashboard-agent/…` endpoints and the
-// `/…/env/<env>/dashboard-agent…` chat resources. Anchored so a task named
-// `dashboard-agent` (`/api/v1/tasks/dashboard-agent/trigger`) is not capped.
-const AGENT_PATH = /^\/api\/v1\/dashboard-agent(\/|$)|\/env\/[^/]+\/dashboard-agent(\/|$)/;
+// `/resources/orgs/…/env/<env>/dashboard-agent…` chat resources. Both alternatives are
+// anchored, so neither a task named `dashboard-agent`
+// (`/api/v1/tasks/dashboard-agent/trigger`) nor a lookalike mid-path segment is capped.
+const AGENT_PATH =
+  /^(?:\/api\/v1\/dashboard-agent|\/resources\/orgs\/[^/]+\/projects\/[^/]+\/env\/[^/]+\/dashboard-agent)(\/|$)/;
 
 /** Methods that can carry one. GET and HEAD cannot, and streaming them would be wasted work. */
 const BODY_METHODS = new Set(["POST", "PUT", "PATCH", "DELETE"]);

apps/webapp/app/services/personalAccessToken.server.ts

@@ -311,18 +311,18 @@ export async function assertSourcePatActive(claims: UserActorClaims): Promise<bo
  * supplied by the RBAC plugin (the apiBuilder path). We re-verify the bearer locally so the
  * recheck reads the token's OWN `pat`, not the plugin's: a plugin image predating the `pat`
  * claim would deliver pat-less claims and silently no-op revocation here. Returns the claims
- * to act on, or undefined to deny. The direct verify sites (jwt exchange, the UAT preamble)
- * don't go through a plugin and call `assertSourcePatActive` on their own verified claims.
+ * to act on, or undefined to deny. Locally-verified claims win whenever verification succeeds
+ * — a plugin predating a claim would otherwise narrow the set the route builder scopes on —
+ * and the plugin's claims are only the fallback.
  */
 export async function resolveAndRecheckUserActorClaims(
   claims: UserActorClaims | undefined,
   bearer: string
 ): Promise<UserActorClaims | undefined> {
   const verified = await verifyUserActorToken(env.SESSION_SECRET, bearer);
-  const resolved = claims ?? verified;
+  const resolved = verified ?? claims;
   if (!resolved) return undefined;
-  // Recheck against the locally-verified claims when available, so `pat` is authoritative.
-  return (await assertSourcePatActive(verified ?? resolved)) ? resolved : undefined;
+  return (await assertSourcePatActive(resolved)) ? resolved : undefined;
 }
 
 /**

apps/webapp/test/dashboardAgentBodyCap.test.ts

@@ -72,9 +72,10 @@ describe("the dashboard agent's ingress cap", () => {
 
     // A client still uploading may see the reset rather than read the 413; either way the
     // request is over long before the body is.
-    const response = await postChunked(`${url}/env/dev/dashboard-agent/in/append`, oversized).catch(
-      () => undefined
-    );
+    const response = await postChunked(
+      `${url}/resources/orgs/acme/projects/site/env/dev/dashboard-agent/in/append`,
+      oversized
+    ).catch(() => undefined);
 
     if (response) expect(response.status).toBe(413);
     expect(buffered()).toBeLessThan(oversized / 2);
@@ -84,7 +85,7 @@ describe("the dashboard agent's ingress cap", () => {
     const { url } = await listen();
 
     const response = await postChunked(
-      `${url}/env/dev/dashboard-agent/in/append`,
+      `${url}/resources/orgs/acme/projects/site/env/dev/dashboard-agent/in/append`,
       DASHBOARD_AGENT_MAX_INGRESS_BYTES + 32 * 1024
     );
 
@@ -94,11 +95,14 @@ describe("the dashboard agent's ingress cap", () => {
 
   it("refuses a declared oversized body before reading anything", async () => {
     const { url, buffered } = await listen();
-    const response = await fetch(`${url}/env/dev/dashboard-agent`, {
-      method: "POST",
-      headers: { "content-type": "text/plain" },
-      body: "x".repeat(DASHBOARD_AGENT_MAX_INGRESS_BYTES + 1),
-    });
+    const response = await fetch(
+      `${url}/resources/orgs/acme/projects/site/env/dev/dashboard-agent`,
+      {
+        method: "POST",
+        headers: { "content-type": "text/plain" },
+        body: "x".repeat(DASHBOARD_AGENT_MAX_INGRESS_BYTES + 1),
+      }
+    );
 
     expect(response.status).toBe(413);
     expect(buffered()).toBe(0);
@@ -108,7 +112,10 @@ describe("the dashboard agent's ingress cap", () => {
     const { url } = await listen();
     const size = 32 * 1024;
 
-    const response = await postChunked(`${url}/env/dev/dashboard-agent`, size);
+    const response = await postChunked(
+      `${url}/resources/orgs/acme/projects/site/env/dev/dashboard-agent`,
+      size
+    );
 
     expect(response.status).toBe(200);
     expect(await response.json()).toEqual({ bytes: size });
@@ -149,6 +156,19 @@ describe("the dashboard agent's ingress cap", () => {
     expect(buffered()).toBe(size);
   });
 
+  it("does not cap a lookalike that only carries the chat segment mid-path", async () => {
+    const { url, buffered } = await listen();
+    const size = DASHBOARD_AGENT_MAX_INGRESS_BYTES + 1024;
+
+    const response = await fetch(`${url}/api/v1/runs/env/dev/dashboard-agent`, {
+      method: "POST",
+      body: "x".repeat(size),
+    });
+
+    expect(response.status).toBe(200);
+    expect(buffered()).toBe(size);
+  });
+
   it("does not cap a task whose id is literally dashboard-agent", async () => {
     const { url, buffered } = await listen();
     const size = DASHBOARD_AGENT_MAX_INGRESS_BYTES + 1024;

apps/webapp/test/dashboardAgentQueriesTenantIsolation.test.ts

@@ -72,4 +72,29 @@ describe("softDeleteChat tenant isolation", () => {
     },
     30_000
   );
+
+  postgresTest(
+    "deleting twice is a no-op: the retention cutoff doesn't move",
+    async ({ prisma, postgresContainer }) => {
+      const db = await boot(prisma, postgresContainer.getConnectionUri());
+      const deletedAt = async () =>
+        (
+          await prisma.$queryRawUnsafe<{ deleted_at: Date | null }[]>(
+            `select deleted_at from trigger_dashboard_agent.chats where id = 'chat_1'`
+          )
+        )[0]?.deleted_at;
+
+      await createChat(db, { id: "chat_1", organizationId: ORG, userId: USER });
+      const params = { chatId: "chat_1", userId: USER, organizationId: ORG };
+
+      expect((await softDeleteChat(db, params)).deleted).toBe(true);
+      const first = await deletedAt();
+      expect(first).toBeInstanceOf(Date);
+
+      // A retry finds nothing left to delete, so the stamp stands.
+      expect((await softDeleteChat(db, params)).deleted).toBe(false);
+      expect(await deletedAt()).toEqual(first);
+    },
+    30_000
+  );
 });