Guard every URL decode behind decodedOrNull (#174)

* Add tests for malformed URL escapes

CL-6434: sweeps apps/web and packages/ for every unguarded
decodeURIComponent call reachable from a URL and adds the red half of
each fix first:

- @corbits/url-path's decodedOrNull (the shared helper every guarded
  call site below routes through).
- workbench-path.ts's id/section/entity parsers, including an
  unrecognized (never mind malformed) section id.
- insights-path.ts's route parser: a malformed workbench/run id now
  reads as the landing default, not a detail mode with no entity —
  and a render test proves InsightsPage actually renders that landing
  dashboard for both malformed paths, not just that the parse result
  looks right.
- kind-filter.ts's Files deep-link id parser.
- chat/routes.ts's participant-removal route: a malformed address
  400s instead of 500ing.
- apps/hub's createStaticHandler: the SPA fallback sees every
  unauthenticated, attacker-controlled request path ahead of the
  platform's own routes, and decodeURIComponent's throw there was a
  500 on any non-/api request with a malformed escape (e.g. `GET
  /%zz`) — the highest-value call site in this sweep.
- isWorkbenchSettingsSectionId, validating a decoded section id
  against WorkbenchSettingsSectionId's own values instead of the
  unchecked `as` cast that was there before.

Claude-Session: https://claude.ai/code/session_01Shhie5zM8L54bLHq5gFQti

* Guard every URL decode behind decodedOrNull

Routes every URL-reachable decodeURIComponent call in apps/web and
packages/ through one shared, guarded helper instead of leaving each
call site to throw on a malformed percent-escape:

- New leaf package @corbits/url-path (no deps, safe in the browser
  bundle and hub code alike) holding decodedOrNull —
  packages/artifact-ui, packages/chat, and apps/hub can't depend on
  apps/web, so the helper can't stay local to path-ids.ts.
- apps/web/src/path-ids.ts imports it instead of keeping its own copy.
- apps/web/src/workbench-path.ts: workbenchIdFromPath,
  workbenchSettingsSectionFromPath (now also validated against
  WorkbenchSettingsSectionId via the new isWorkbenchSettingsSectionId,
  replacing the unchecked `as` cast — packages/chat-ui exports both
  the guard and its backing WORKBENCH_SETTINGS_SECTION_IDS list),
  workbenchSettingsEntityIdFromPath.
- apps/web/src/insights-path.ts (moved out of the 1700-line
  insights-page.tsx, which now just imports it): parseInsightsPath's
  workbenchId/runId. A malformed escape now falls all the way back to
  {mode: "landing"} — returning {mode: "workbench", workbenchId:
  null} would render a bench-scoped dashboard mislabeled "All
  workbenches" with permanently-empty panels instead of the plain
  landing view every other unresolvable path already gets.
- packages/artifact-ui/src/kind-filter.ts: libraryArtifactIdFromPath.
- packages/chat/src/routes.ts: DELETE .../participants/:address now
  400s on a malformed address instead of throwing mid-request.
- apps/hub/src/index.ts's createStaticHandler (exported for the new
  test): the SPA static-file fallback ahead of every non-/api route,
  reachable unauthenticated — GET /%zz threw a 500 before this;
  now falls through to the platform's own 404 like any other
  unresolvable path.

packages/connections/src/oauth-routes.ts's decode was already
try/caught and is left as-is. Not in scope: apps/hub's
dbConfigFromUrl decodes a database URL's own user/password at boot —
operator-supplied config, never a client-reachable URL, so it's
noted here rather than guarded (its shape is mirrored by six test
doubles across apps/hub, packages/insights, packages/inbox, and
packages/approvals, none of them production code either).

Claude-Session: https://claude.ai/code/session_01Shhie5zM8L54bLHq5gFQti

Author: TheGreatAxios

apps/hub/package.json

@@ -44,6 +44,7 @@
     "@corbits/tasks": "workspace:*",
     "@corbits/tool-registry-publish": "workspace:*",
     "@corbits/turn-artifacts": "workspace:*",
+    "@corbits/url-path": "workspace:*",
     "@corbits/webhook-triggers": "workspace:*",
     "@corbits/workflow-catalog": "workspace:*",
     "@corbits/workflow-source": "workspace:*",

apps/hub/src/index.ts

@@ -95,6 +95,7 @@ import {
 } from "@corbits/chat";
 import type { RelaunchNoticePort } from "@corbits/chat";
 import type { FinalizedTurnToolCall } from "@corbits/turn-artifacts";
+import { decodedOrNull } from "@corbits/url-path";
 import {
   createCryptoProviderCache,
   createTopLevelRunRoutes,
@@ -413,12 +414,12 @@ function dbConfigFromUrl(databaseUrl: string) {
 // Serves the single-page application from the hub origin: a real file
 // when one exists, index.html otherwise so client-side routes deep-link,
 // and never anything under /api, which stays with the platform routes.
-function createStaticHandler(staticDir: string) {
+export function createStaticHandler(staticDir: string) {
   return async (c: Context<AppEnv>, next: Next) => {
     if (c.req.path === "/api" || c.req.path.startsWith("/api/")) return next();
-    const rel = path
-      .normalize(decodeURIComponent(c.req.path))
-      .replace(/^[/\\]+/, "");
+    const decodedPath = decodedOrNull(c.req.path);
+    if (decodedPath === null) return next();
+    const rel = path.normalize(decodedPath).replace(/^[/\\]+/, "");
     if (rel === ".." || rel.startsWith(`..${path.sep}`)) return next();
     const asset = Bun.file(path.join(staticDir, rel));
     if (await asset.exists()) return new Response(asset);

apps/hub/test/static-handler.test.ts

@@ -0,0 +1,55 @@
+// `createStaticHandler` serves the SPA from the hub origin and sits ahead
+// of every non-/api route, so it sees any attacker-controlled path a
+// client cares to send — including one with a malformed percent-escape
+// (`GET /%zz`). `decodeURIComponent` throws on that, and until this was
+// guarded the throw escaped the handler entirely: a 500 on any
+// non-/api request, not the 404/index.html fallback every other
+// unresolvable path gets.
+
+import { afterAll, beforeAll, describe, expect, test } from "bun:test";
+import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import path from "node:path";
+import { Hono } from "hono";
+import type { Context } from "hono";
+
+import { createStaticHandler } from "../src/index.ts";
+
+let staticDir: string;
+
+beforeAll(() => {
+  staticDir = mkdtempSync(path.join(tmpdir(), "hub-static-"));
+  writeFileSync(path.join(staticDir, "index.html"), "<html>spa</html>");
+});
+
+afterAll(() => {
+  rmSync(staticDir, { recursive: true, force: true });
+});
+
+function buildApp() {
+  const app = new Hono();
+  app.get("/*", createStaticHandler(staticDir) as never);
+  app.notFound((c: Context) => c.text("not found", 404));
+  return app;
+}
+
+describe("createStaticHandler", () => {
+  test("serves index.html for a normal SPA route", async () => {
+    const app = buildApp();
+    const response = await app.request("/workbenches/ch_1");
+    expect(response.status).toBe(200);
+    expect(await response.text()).toBe("<html>spa</html>");
+  });
+
+  test("a malformed percent-escape 404s instead of throwing", async () => {
+    const app = buildApp();
+    const response = await app.request("/%zz");
+    expect(response.status).toBe(404);
+  });
+
+  test("/api paths are left for the platform routes regardless", async () => {
+    const app = buildApp();
+    const response = await app.request("/api/%zz");
+    expect(response.status).toBe(404);
+  });
+});

apps/web/package.json

@@ -37,6 +37,7 @@
     "@corbits/shell-layout": "workspace:*",
     "@corbits/slug": "workspace:*",
     "@corbits/tasks-ui": "workspace:*",
+    "@corbits/url-path": "workspace:*",
     "@corbits/workflow-catalog": "workspace:*",
     "@corbits/icons": "workspace:*",
     "@intx/types": "0.3.0",

apps/web/src/bench-context.tsx

@@ -45,7 +45,10 @@ export type BenchState = {
   readonly onBenchCreated: (tenantId: string) => void;
 };
 
-const BenchContext = createContext<BenchState | null>(null);
+/** Exported only so a render test can inject a fixed `BenchState` without
+ * standing up `BenchProvider`'s own `/api/me/principals` fetch — every
+ * real caller still goes through `useBench`/`BenchProvider`. */
+export const BenchContext = createContext<BenchState | null>(null);
 
 /** The membership this context currently treats as selected: the stored
  * choice if it still names a bench the account belongs to *and* still

apps/web/src/insights-path.test.ts

@@ -0,0 +1,54 @@
+import { describe, expect, test } from "bun:test";
+
+import { parseInsightsPath } from "./insights-path";
+
+describe("parseInsightsPath", () => {
+  test("resolves a workbench deep link", () => {
+    expect(parseInsightsPath("/insights/workbench/tnt_1")).toEqual({
+      mode: "workbench",
+      runId: null,
+      workbenchId: "tnt_1",
+    });
+  });
+
+  test("resolves a run deep link", () => {
+    expect(parseInsightsPath("/insights/runs/run_1")).toEqual({
+      mode: "run",
+      runId: "run_1",
+      workbenchId: null,
+    });
+  });
+
+  test("resolves the landing and runs-history paths", () => {
+    expect(parseInsightsPath("/insights")).toEqual({
+      mode: "landing",
+      runId: null,
+      workbenchId: null,
+    });
+    expect(parseInsightsPath("/insights/runs")).toEqual({
+      mode: "runs",
+      runId: null,
+      workbenchId: null,
+    });
+  });
+
+  test("a malformed escape on a workbench deep link falls back to landing, not a throw", () => {
+    expect(() =>
+      parseInsightsPath("/insights/workbench/%E0%A4%A"),
+    ).not.toThrow();
+    expect(parseInsightsPath("/insights/workbench/%E0%A4%A")).toEqual({
+      mode: "landing",
+      runId: null,
+      workbenchId: null,
+    });
+  });
+
+  test("a malformed escape on a run deep link falls back to landing, not a throw", () => {
+    expect(() => parseInsightsPath("/insights/runs/%")).not.toThrow();
+    expect(parseInsightsPath("/insights/runs/%")).toEqual({
+      mode: "landing",
+      runId: null,
+      workbenchId: null,
+    });
+  });
+});

apps/web/src/insights-path.ts

@@ -0,0 +1,51 @@
+// Insights' own route parser — its own module (not inlined in
+// insights-page.tsx) so it can be exercised directly without dragging in
+// that page's bench/query-client wiring.
+
+import { decodedOrNull } from "@corbits/url-path";
+
+import { INSIGHTS_PATH_PREFIX, INSIGHTS_RUNS_PATH } from "./path-ids";
+
+/**
+ * `/insights/workbench/:workbenchId` (CL-5879) is its own dedicated route — a
+ * conversation's own scoped view, resolved by `InsightsWorkbenchPage`, never
+ * a sub-mode of the landing. Every other path stays the cross-workbench
+ * default landing: no per-mode branch needed there, since scoping happens
+ * in InsightsRoute (which tenantId every query below targets), not here.
+ * A stale `/insights/workbench/:tenantId` link (that route is retired,
+ * hard cut) falls through to the plain landing default below rather than
+ * matching anything.
+ *
+ * A malformed percent-escape in the id segment reads as the plain landing
+ * default too, never as a detail mode with no entity to show — `mode:
+ * "workbench"` (or `"run"`) with a `null` id would otherwise render that
+ * mode's own scoped, permanently-empty dashboard instead of falling back
+ * to the landing view any other unresolvable path already gets.
+ */
+export function parseInsightsPath(path: string): {
+  mode: "landing" | "runs" | "run" | "workbench";
+  runId: string | null;
+  workbenchId: string | null;
+} {
+  const workbenchMatch = /^\/insights\/workbench\/([^/]+)\/?$/.exec(path);
+  if (workbenchMatch !== null && workbenchMatch[1] !== undefined) {
+    const workbenchId = decodedOrNull(workbenchMatch[1]);
+    if (workbenchId !== null) {
+      return { mode: "workbench", runId: null, workbenchId };
+    }
+  }
+  if (path === INSIGHTS_PATH_PREFIX || path === `${INSIGHTS_PATH_PREFIX}/`) {
+    return { mode: "landing", runId: null, workbenchId: null };
+  }
+  if (path === INSIGHTS_RUNS_PATH || path === `${INSIGHTS_RUNS_PATH}/`) {
+    return { mode: "runs", runId: null, workbenchId: null };
+  }
+  const match = /^\/insights\/runs\/([^/]+)\/?$/.exec(path);
+  if (match !== null && match[1] !== undefined) {
+    const runId = decodedOrNull(match[1]);
+    if (runId !== null) {
+      return { mode: "run", runId, workbenchId: null };
+    }
+  }
+  return { mode: "landing", runId: null, workbenchId: null };
+}

apps/web/src/pages/insights-page-render.test.tsx

@@ -0,0 +1,93 @@
+// A malformed percent-escape on an Insights deep link
+// (`/insights/workbench/%E0%A4%A`, `/insights/runs/%`) must render the
+// same landing dashboard any other unrecognized Insights path gets — never
+// a blank page, and never `InsightsWorkbenchPage`/the run-detail route with
+// no entity to show (see `insights-path.ts`'s `parseInsightsPath`, which
+// InsightsPage calls with the exact same `path` prop this test passes).
+
+import { afterEach, describe, expect, test } from "bun:test";
+import { act } from "react";
+import { createRoot } from "react-dom/client";
+import type { Root } from "react-dom/client";
+
+import type { APIQuery } from "@corbits/api-query";
+import { EMPTY_OVERALL_USAGE } from "@corbits/insights/client";
+
+import { InsightsPage, useInsightsWindow } from "./insights-page";
+import { BenchContext } from "../bench-context";
+import type { BenchState } from "../bench-context";
+import { NavigationProvider } from "../navigation";
+
+let container: HTMLDivElement | null = null;
+let root: Root | null = null;
+
+afterEach(() => {
+  if (root !== null) {
+    act(() => root?.unmount());
+    root = null;
+  }
+  if (container !== null) {
+    container.remove();
+    container = null;
+  }
+});
+
+const readyEmpty = <T,>(data: T): APIQuery<T> => ({ kind: "ready", data });
+
+const benchState: BenchState = {
+  memberships: { kind: "ready", data: { data: [], nextCursor: null } },
+  selectedTenantId: "tnt_bench_a",
+  selectedPrincipalId: "prn_bench_a",
+  selectTenant: () => {},
+  onBenchCreated: () => {},
+};
+
+function InsightsPageAtPath({ path }: { readonly path: string }) {
+  const range = useInsightsWindow();
+  return (
+    <NavigationProvider navigate={() => {}}>
+      <BenchContext.Provider value={benchState}>
+        <InsightsPage
+          path={path}
+          summary={readyEmpty(EMPTY_OVERALL_USAGE)}
+          activity={readyEmpty([])}
+          byTool={readyEmpty([])}
+          runs={readyEmpty({ data: [], nextCursor: null })}
+          routines={readyEmpty([])}
+          workbenches={readyEmpty({ items: [] })}
+          latency={{ kind: "loading" }}
+          range={range}
+          scope={null}
+          resolveWorkbenchIdForTenant={() => null}
+          scopeLabel="All workbenches"
+        />
+      </BenchContext.Provider>
+    </NavigationProvider>
+  );
+}
+
+function render(path: string) {
+  container = document.createElement("div");
+  document.body.appendChild(container);
+  root = createRoot(container);
+  act(() => {
+    root?.render(<InsightsPageAtPath path={path} />);
+  });
+  return container;
+}
+
+describe("InsightsPage with a malformed URL escape", () => {
+  test("a malformed workbench deep link still renders the landing dashboard", () => {
+    const el = render("/insights/workbench/%E0%A4%A");
+    expect(el.textContent).not.toBe("");
+    expect(el.textContent).toContain("Insights");
+    expect(el.textContent).toContain("All workbenches");
+  });
+
+  test("a malformed run deep link still renders the landing dashboard, not run detail", () => {
+    const el = render("/insights/runs/%");
+    expect(el.textContent).not.toBe("");
+    expect(el.textContent).toContain("Insights");
+    expect(el.textContent).toContain("All workbenches");
+  });
+});

apps/web/src/pages/insights-page.tsx

@@ -61,6 +61,7 @@ import {
   type WorkbenchInsightsResolution,
 } from "../insights-workbench-scope";
 import { workbenchInsightsPath } from "../insights-deeplinks";
+import { parseInsightsPath } from "../insights-path";
 import {
   ActivityResponseSchema,
   InsightsScopeSchema,
@@ -1077,46 +1078,6 @@ export function InsightsRunDetail({
   );
 }
 
-/**
- * `/insights/workbench/:workbenchId` (CL-5879) is its own dedicated route — a
- * conversation's own scoped view, resolved by `InsightsWorkbenchPage`, never
- * a sub-mode of the landing. Every other path stays the cross-workbench
- * default landing: no per-mode branch needed there, since scoping happens
- * in InsightsRoute (which tenantId every query below targets), not here.
- * A stale `/insights/workbench/:tenantId` link (that route is retired,
- * hard cut) falls through to the plain landing default below rather than
- * matching anything.
- */
-function parseInsightsPath(path: string): {
-  mode: "landing" | "runs" | "run" | "workbench";
-  runId: string | null;
-  workbenchId: string | null;
-} {
-  const workbenchMatch = /^\/insights\/workbench\/([^/]+)\/?$/.exec(path);
-  if (workbenchMatch !== null && workbenchMatch[1] !== undefined) {
-    return {
-      mode: "workbench",
-      runId: null,
-      workbenchId: decodeURIComponent(workbenchMatch[1]),
-    };
-  }
-  if (path === INSIGHTS_PATH_PREFIX || path === `${INSIGHTS_PATH_PREFIX}/`) {
-    return { mode: "landing", runId: null, workbenchId: null };
-  }
-  if (path === INSIGHTS_RUNS_PATH || path === `${INSIGHTS_RUNS_PATH}/`) {
-    return { mode: "runs", runId: null, workbenchId: null };
-  }
-  const match = /^\/insights\/runs\/([^/]+)\/?$/.exec(path);
-  if (match !== null && match[1] !== undefined) {
-    return {
-      mode: "run",
-      runId: decodeURIComponent(match[1]),
-      workbenchId: null,
-    };
-  }
-  return { mode: "landing", runId: null, workbenchId: null };
-}
-
 /**
  * The landing view's default scope, and every non-landing mode's scope,
  * as one pure decision so it can be unit-tested without mounting the

apps/web/src/path-ids.ts

@@ -8,6 +8,7 @@
 // below.
 
 import { isValidSlug, type Slug } from "@corbits/slug";
+import { decodedOrNull } from "@corbits/url-path";
 
 export const SETTINGS_PATH_PREFIX = "/settings";
 export const AGENTS_PATH_PREFIX = "/agents";
@@ -43,19 +44,6 @@ function rawSegmentFromTopLevelPath(
   return rest === "" ? null : rest;
 }
 
-/** A URL segment carries percent-escapes an id needs decoded, and a
- * hand-typed or truncated URL can carry a malformed one — which
- * `decodeURIComponent` answers with a throw. A path that cannot be decoded
- * names no entity, so it reads as no selection at all rather than taking
- * the render down with it. */
-function decodedOrNull(segment: string): string | null {
-  try {
-    return decodeURIComponent(segment);
-  } catch {
-    return null;
-  }
-}
-
 /** Extract a sub-selection from a flat top-level route (`/agents/:id`,
  * `/skills/:id`) — `null` for the bare prefix or a path outside it. */
 function entityIdFromTopLevelPath(path: string, prefix: string): string | null {