address PR review: explicit logout-once guard, config coercion, drop dead platformUrl

mgoldsborough · mgoldsborough · commit e55ca5e594d5 · 2026-06-17T13:19:32.000-10:00
- fetch-with-refresh fires captureLogout per concurrent 401; make the single
  event explicit via a once-per-incident guard in sentry.ts (consumeLogoutOnce /
  resetLogoutGuard, reset on re-auth) instead of relying on Sentry's Dedupe.
- config.ts: Caddy now emits all values as JSON strings (incl. enabled /
  tracesSampleRate); getConfig coerces booleans/numbers so a malformed operator
  value (e.g. NB_SENTRY_ENABLED=yes) degrades that one feature to off, not the
  whole config to invalid JS.
- Drop the dead platformUrl field; rely on Sentry's default same-origin trace
  propagation. Correct the misleading "W3C traceparent" comment (Sentry
  propagates sentry-trace/baggage, not W3C).
- Tests: web/test/config.test.ts (coercion + precedence) and logout-guard cases
  in sentry-scrub.test.ts.
diff --git a/web/Caddyfile b/web/Caddyfile
@@ -28,10 +28,15 @@
 	# Helm — no writable filesystem, no entrypoint. index.html loads this before
 	# the app bundle (window.__NB_CONFIG__, read by web/src/config.ts). Public
 	# client values ONLY — never secrets. Unset env => empty field => feature off.
+	#
+	# Every value is emitted as a JSON *string* (incl. enabled/tracesSampleRate),
+	# and config.ts coerces booleans/numbers. This way a malformed operator value
+	# (e.g. NB_SENTRY_ENABLED=yes) degrades that one feature to "off" instead of
+	# producing invalid JS that would disable every feature at once.
 	handle /config.js {
 		header Content-Type "application/javascript; charset=utf-8"
 		header Cache-Control "no-store"
-		respond `window.__NB_CONFIG__ = {"tenantId":"{$NB_TENANT_ID:}","environment":"{$NB_SENTRY_ENV:}","release":"{$NB_RELEASE:}","sentry":{"dsn":"{$NB_SENTRY_DSN:}","enabled":{$NB_SENTRY_ENABLED:false},"tracesSampleRate":{$NB_SENTRY_TRACES_SAMPLE_RATE:0}},"turnstile":{"siteKey":"{$NB_TURNSTILE_SITE_KEY:}","enabled":{$NB_TURNSTILE_ENABLED:false}},"posthog":{"key":"{$NB_POSTHOG_KEY:}","enabled":{$NB_POSTHOG_ENABLED:false}}};` 200
+		respond `window.__NB_CONFIG__ = {"tenantId":"{$NB_TENANT_ID:}","environment":"{$NB_SENTRY_ENV:}","release":"{$NB_RELEASE:}","sentry":{"dsn":"{$NB_SENTRY_DSN:}","enabled":"{$NB_SENTRY_ENABLED:false}","tracesSampleRate":"{$NB_SENTRY_TRACES_SAMPLE_RATE:0}"},"turnstile":{"siteKey":"{$NB_TURNSTILE_SITE_KEY:}","enabled":"{$NB_TURNSTILE_ENABLED:false}"},"posthog":{"key":"{$NB_POSTHOG_KEY:}","enabled":"{$NB_POSTHOG_ENABLED:false}"}};` 200
 	}
 	handle {
 		root * /srv
diff --git a/web/src/config.ts b/web/src/config.ts
@@ -7,24 +7,28 @@
  * configured per tenant via Helm env, with no per-tenant builds and no writable
  * filesystem.
  *
+ * Caddy renders every value as a JSON *string* (so a malformed env value can't
+ * produce a syntax error that breaks the whole file); `getConfig()` coerces
+ * booleans/numbers, so a bad value degrades that one feature to "off" rather
+ * than silently disabling everything.
+ *
  * The committed placeholder `public/config.js` (empty object) is what `bun run
- * dev` serves locally; in a container Caddy's handler supersedes it.
- * For local-dev convenience `getConfig()` falls back to Vite env when a field is
- * absent — in a built image `window.__NB_CONFIG__` always wins because CI builds
- * with no `VITE_*` values.
+ * dev` serves locally; in a container Caddy's handler supersedes it. For local
+ * dev `getConfig()` falls back to Vite env when a field is absent — in a built
+ * image `window.__NB_CONFIG__` always wins because CI builds with no `VITE_*`.
  *
  * This is the single source the web client reads for Sentry, Turnstile, and
  * PostHog config. All values here are public client keys — never secrets.
  */
+
+/** Resolved config the app reads, with booleans/numbers coerced. */
 export interface NbRuntimeConfig {
   /** Deployment tenant id (from NB_TENANT_ID); the Sentry `tenant_id` tag. */
   tenantId?: string;
   /** Deployment environment label (from NB_SENTRY_ENV), e.g. "production". */
   environment?: string;
   /** Build SHA, optional. */
   release?: string;
-  /** Platform API base URL (for distributed-trace propagation). */
-  platformUrl?: string;
   sentry?: {
     dsn?: string;
     enabled?: boolean;
@@ -34,33 +38,60 @@ export interface NbRuntimeConfig {
   posthog?: { key?: string; enabled?: boolean };
 }
 
+/**
+ * Raw injected shape: Caddy renders every value as a JSON string, and the dev
+ * placeholder may omit fields. Booleans/numbers arrive as strings and are
+ * coerced by `getConfig()`.
+ */
+interface RawConfig {
+  tenantId?: string;
+  environment?: string;
+  release?: string;
+  sentry?: { dsn?: string; enabled?: string | boolean; tracesSampleRate?: string | number };
+  turnstile?: { siteKey?: string; enabled?: string | boolean };
+  posthog?: { key?: string; enabled?: string | boolean };
+}
+
 declare global {
   interface Window {
-    __NB_CONFIG__?: NbRuntimeConfig;
+    __NB_CONFIG__?: RawConfig;
   }
 }
 
+/** Coerce a string/boolean flag; `undefined` stays undefined (dev default). */
+function asBool(v: string | boolean | undefined): boolean | undefined {
+  if (v === undefined) return undefined;
+  return v === true || v === "true";
+}
+
+/** Coerce a string/number to a finite number, falling back on garbage/empty. */
+function asNum(v: string | number | undefined, fallback: number): number {
+  const n = typeof v === "number" ? v : Number(v);
+  return Number.isFinite(n) ? n : fallback;
+}
+
 /** Resolved runtime config: `window.__NB_CONFIG__` over Vite-env dev fallbacks. */
 export function getConfig(): NbRuntimeConfig {
-  const w = (typeof window !== "undefined" && window.__NB_CONFIG__) || {};
+  const w: RawConfig = (typeof window !== "undefined" && window.__NB_CONFIG__) || {};
   return {
-    tenantId: w.tenantId,
-    environment: w.environment ?? import.meta.env.MODE,
-    release: w.release,
-    platformUrl: w.platformUrl ?? import.meta.env.VITE_API_BASE,
+    tenantId: w.tenantId || undefined,
+    environment: w.environment || import.meta.env.MODE,
+    release: w.release || undefined,
     sentry: {
-      dsn: w.sentry?.dsn ?? import.meta.env.VITE_SENTRY_DSN,
-      enabled: w.sentry?.enabled,
-      tracesSampleRate:
-        w.sentry?.tracesSampleRate ?? Number(import.meta.env.VITE_SENTRY_TRACES_SAMPLE_RATE ?? 0),
+      dsn: w.sentry?.dsn || import.meta.env.VITE_SENTRY_DSN,
+      enabled: asBool(w.sentry?.enabled),
+      tracesSampleRate: asNum(
+        w.sentry?.tracesSampleRate ?? import.meta.env.VITE_SENTRY_TRACES_SAMPLE_RATE,
+        0,
+      ),
     },
     turnstile: {
-      siteKey: w.turnstile?.siteKey ?? import.meta.env.VITE_TURNSTILE_SITE_KEY,
-      enabled: w.turnstile?.enabled,
+      siteKey: w.turnstile?.siteKey || import.meta.env.VITE_TURNSTILE_SITE_KEY,
+      enabled: asBool(w.turnstile?.enabled),
     },
     posthog: {
-      key: w.posthog?.key ?? import.meta.env.VITE_POSTHOG_KEY,
-      enabled: w.posthog?.enabled,
+      key: w.posthog?.key || import.meta.env.VITE_POSTHOG_KEY,
+      enabled: asBool(w.posthog?.enabled),
     },
   };
 }
diff --git a/web/src/sentry.ts b/web/src/sentry.ts
@@ -16,6 +16,28 @@ import { getConfig } from "./config";
  */
 
 let initialized = false;
+// One Sentry event per involuntary-logout *incident*, not per concurrent 401.
+// The app fires parallel /v1 requests; a real logout resolves the single-flighted
+// refresh as rejected for all of them, so each awaiting caller would otherwise
+// call captureLogout. This guard makes singularity explicit rather than relying
+// on Sentry's Dedupe integration. Reset on re-auth (setSentryUser).
+let loggedOutCaptured = false;
+
+/**
+ * Test seam for the once-per-incident logout guard: returns `true` the first
+ * time and `false` until {@link resetLogoutGuard} re-arms it. Exported so the
+ * singularity can be unit-tested without initializing Sentry.
+ */
+export function consumeLogoutOnce(): boolean {
+  if (loggedOutCaptured) return false;
+  loggedOutCaptured = true;
+  return true;
+}
+
+/** Re-arm the logout guard (called on re-auth). Exported for tests. */
+export function resetLogoutGuard(): void {
+  loggedOutCaptured = false;
+}
 
 export function initSentry(): void {
   if (initialized) return;
@@ -35,8 +57,11 @@ export function initSentry(): void {
     // results, file contents) — the exact content the trust rule forbids. Same
     // intent as telemetry.ts disabling PostHog session recording.
     tracesSampleRate: s.tracesSampleRate ?? 0,
-    // Extend the trace into the platform's OTel layer over W3C traceparent.
-    tracePropagationTargets: [cfg.platformUrl || /^\//],
+    // tracePropagationTargets is left at the SDK default (same-origin), which is
+    // exactly our case — all API calls are same-origin /v1/* proxied to the
+    // platform. Note browserTracingIntegration propagates Sentry's own
+    // sentry-trace + baggage headers (not W3C traceparent), so this does not
+    // auto-link into the kernel's OTLP traces; the platform ignores the headers.
     beforeSend,
     beforeBreadcrumb,
     // Strip query strings from transaction names so workspace/conversation ids
@@ -84,6 +109,8 @@ export function beforeBreadcrumb(crumb: Sentry.Breadcrumb): Sentry.Breadcrumb |
 /** Set the per-session opaque user id (never email/displayName). */
 export function setSentryUser(userId: string | null | undefined): void {
   if (!initialized) return;
+  // A new authenticated session starts a fresh logout incident.
+  if (userId) resetLogoutGuard();
   Sentry.setUser(userId ? { id: userId } : null);
 }
 
@@ -107,11 +134,12 @@ export function addAuthBreadcrumb(message: string): void {
 }
 
 /**
- * Emit one event at the terminal involuntary logout, carrying the preceding
- * auth breadcrumb trail — turns a "random logout" into a reconstructable
- * sequence. Not called on manual user logout.
+ * Emit ONE event per involuntary-logout incident, carrying the preceding auth
+ * breadcrumb trail — turns a "random logout" into a reconstructable sequence.
+ * Idempotent across the concurrent 401s that resolve one rejected refresh; reset
+ * by setSentryUser on re-auth. Not called on manual user logout.
  */
 export function captureLogout(reason: string): void {
-  if (!initialized) return;
+  if (!initialized || !consumeLogoutOnce()) return;
   Sentry.captureMessage(`involuntary logout: ${reason}`, "warning");
 }
diff --git a/web/test/config.test.ts b/web/test/config.test.ts
@@ -0,0 +1,75 @@
+// ---------------------------------------------------------------------------
+// getConfig — the runtime config seam every client feature reads through.
+//
+// Caddy injects window.__NB_CONFIG__ with all values as JSON strings; getConfig
+// coerces booleans/numbers so a malformed operator value degrades that one
+// feature to "off" instead of breaking the whole config. These tests pin the
+// coercion and precedence so the seam can't silently regress.
+// ---------------------------------------------------------------------------
+
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import { getConfig } from "../src/config";
+
+// Mutate only window.__NB_CONFIG__ — never the shared (happy-dom) window object,
+// or we'd tear down the DOM for every test that runs after this file.
+type Win = { __NB_CONFIG__?: unknown } | undefined;
+const getWin = (): Win => (globalThis as unknown as { window?: Win }).window as Win;
+const ensureWin = (): { __NB_CONFIG__?: unknown } => {
+  const g = globalThis as unknown as { window?: { __NB_CONFIG__?: unknown } };
+  g.window ??= {};
+  return g.window;
+};
+
+function setConfig(c: unknown): void {
+  ensureWin().__NB_CONFIG__ = c;
+}
+
+beforeEach(() => {
+  const w = getWin();
+  if (w) w.__NB_CONFIG__ = undefined;
+});
+
+afterEach(() => {
+  const w = getWin();
+  if (w) w.__NB_CONFIG__ = undefined;
+});
+
+describe("getConfig", () => {
+  test("uses injected values and coerces string flags/numbers", () => {
+    setConfig({
+      tenantId: "tenant-a",
+      sentry: { dsn: "https://k@o1.ingest.sentry.io/1", enabled: "true", tracesSampleRate: "0.25" },
+      turnstile: { siteKey: "0xAAA", enabled: "false" },
+      posthog: { key: "phc_x", enabled: "true" },
+    });
+    const c = getConfig();
+    expect(c.tenantId).toBe("tenant-a");
+    expect(c.sentry?.dsn).toBe("https://k@o1.ingest.sentry.io/1");
+    expect(c.sentry?.enabled).toBe(true);
+    expect(c.sentry?.tracesSampleRate).toBe(0.25);
+    expect(c.turnstile?.enabled).toBe(false);
+    expect(c.posthog?.enabled).toBe(true);
+  });
+
+  test("a malformed flag/number degrades to off/0, not a crash", () => {
+    setConfig({ sentry: { dsn: "d", enabled: "yes", tracesSampleRate: "" } });
+    const c = getConfig();
+    expect(c.sentry?.enabled).toBe(false); // anything but "true" => off
+    expect(c.sentry?.tracesSampleRate).toBe(0); // empty/garbage => 0
+  });
+
+  test("absent enable flag stays undefined (dev: on iff key present)", () => {
+    setConfig({ sentry: { dsn: "d" } });
+    expect(getConfig().sentry?.enabled).toBeUndefined();
+  });
+
+  test("empty config leaves every feature unconfigured", () => {
+    setConfig({});
+    const c = getConfig();
+    expect(c.sentry?.dsn).toBeUndefined();
+    expect(c.sentry?.enabled).toBeUndefined();
+    expect(c.turnstile?.siteKey).toBeUndefined();
+    expect(c.posthog?.key).toBeUndefined();
+    expect(c.sentry?.tracesSampleRate).toBe(0);
+  });
+});
diff --git a/web/test/sentry-scrub.test.ts b/web/test/sentry-scrub.test.ts
@@ -10,8 +10,13 @@
 // ---------------------------------------------------------------------------
 
 import type { Breadcrumb, ErrorEvent } from "@sentry/react";
-import { describe, expect, test } from "bun:test";
-import { beforeBreadcrumb, beforeSend } from "../src/sentry";
+import { beforeEach, describe, expect, test } from "bun:test";
+import {
+  beforeBreadcrumb,
+  beforeSend,
+  consumeLogoutOnce,
+  resetLogoutGuard,
+} from "../src/sentry";
 
 describe("beforeSend", () => {
   test("strips cookies and headers from the request", () => {
@@ -64,3 +69,21 @@ describe("beforeBreadcrumb", () => {
     expect(out?.data?.url).toBe("https://app.example/v1/tools");
   });
 });
+
+describe("logout guard (one event per incident)", () => {
+  beforeEach(() => resetLogoutGuard());
+
+  test("emits once across the concurrent 401s of a single logout", () => {
+    // N concurrent 401s resolve one rejected refresh; captureLogout runs per
+    // caller but must collapse to a single emit.
+    expect(consumeLogoutOnce()).toBe(true);
+    expect(consumeLogoutOnce()).toBe(false);
+    expect(consumeLogoutOnce()).toBe(false);
+  });
+
+  test("re-auth re-arms the guard for a later logout", () => {
+    expect(consumeLogoutOnce()).toBe(true);
+    resetLogoutGuard(); // setSentryUser does this on a fresh session
+    expect(consumeLogoutOnce()).toBe(true);
+  });
+});
