FEA-1436: harden admin-key secret handling + address pre-PR review

Independent pre-PR review (two reviewers) returned SHIP WITH MINOR FIXES.
This commit resolves the actionable findings:

- admin-billing: redact key-shaped tokens (sk-…) from vendor error bodies
  before they reach a thrown error, IPC reply, or the log file. OpenAI's 401
  body echoes a copy of the key it received; the vendor response is outside our
  control, so requestAdminJson now scrubs it via redactKeyLikeTokens. Fixes the
  "key never in error message" invariant and corrects the now-accurate comments.
- claude-code-analytics-client: tokenCount rejects negative values (matches its
  non-negative contract); assertUtcDayString round-trips the parsed date so an
  impossible calendar day (e.g. 2026-02-30, which JS silently overflows) is
  rejected instead of being silently shifted.
- renderer: stop shadowing the browser `window` global in
  describeClaudeCodeResult; derive the usage-total label from the actual query
  window instead of hardcoding "7-day total".
- reconciliation-worker: document why the dashboard.db handle is intentionally
  opened read-write (a SQLITE_OPEN_READONLY connection cannot attach to the
  sidecar's live WAL database without an existing writer) — declined the
  readOnly:true suggestion to avoid an intermittent open failure.

Testing:
- pnpm typecheck: clean
- pnpm lint: clean
- pnpm test: 2035 pass / 0 fail (added redaction, negative-token, impossible-date,
  and service key-absence regression tests asserting the exact reviewed invariants)

Risks:
- redactKeyLikeTokens over-redacts by design (any sk-… run → sk-[redacted]);
  diagnostic bodies stay readable, secrets cannot leak.
- reconciliation read path unchanged (read-write open, SELECT-only) — no
  behavioral change to WAL coexistence.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 2 people

apps/desktop/package.json

@@ -1,6 +1,6 @@
 {
   "name": "desktop",
-  "version": "0.15.101",
+  "version": "0.15.102",
   "description": "ClosedLoop Desktop",
   "author": "ClosedLoop AI <support@closedloop.ai>",
   "private": true,

apps/desktop/src/main/admin-billing.ts

@@ -14,8 +14,12 @@
  *   2. Every outbound request host is checked against a fixed allowlist
  *      (api.anthropic.com / api.openai.com) via assertAllowedAdminHost, so a
  *      misconfigured base URL can never ship the Admin key to another host.
- * Errors thrown here include the HTTP status and the vendor's own error body
- * (which never contains your key) but never the key itself.
+ * Errors thrown here include the HTTP status and a truncated copy of the
+ * vendor's error body, scrubbed of any key-shaped token via redactKeyLikeTokens.
+ * We only ever put the key in request headers, but the vendor's *response* is
+ * outside our control — OpenAI's 401 body, for instance, echoes a masked copy of
+ * the key it received — so we redact before the body can reach an error message,
+ * an IPC reply, or the log file. The key itself is never placed in an error.
  */
 import {
   centsToMicroCents,
@@ -87,10 +91,29 @@ export function assertAllowedAdminHost(url: string, allowedHost: string): void {
   }
 }
 
+/**
+ * Redact anything that looks like an API key from a string before it is placed
+ * in a thrown error or a log line. We send the Admin key only in request headers,
+ * but a vendor's error *response* is outside our control and can echo a copy of
+ * the key it received — e.g. OpenAI's 401 body: "Incorrect API key provided:
+ * sk-admin-…". The token class matches the characters a real key is made of
+ * (alphanumerics, `-`, `_`) plus `*` to also catch the asterisk-masked forms
+ * vendors print; it deliberately excludes `.` so a trailing sentence period is
+ * left intact. The full (unmasked) key is the only true secret and is always a
+ * contiguous run of these characters, so it is always fully scrubbed. Over-
+ * redaction is the intended posture: a secret leak is far worse than a slightly
+ * noisier diagnostic.
+ */
+export function redactKeyLikeTokens(text: string): string {
+  return text.replace(/sk-[A-Za-z0-9*_-]{4,}/g, "sk-[redacted]");
+}
+
 /**
  * GET `url` with `headers`, returning the parsed JSON body. Throws on a non-2xx
- * with the status and a truncated copy of the vendor's error body (never the
- * Admin key, which is only ever in the request headers).
+ * with the status and a truncated, key-scrubbed copy of the vendor's error body.
+ * The Admin key is only ever in the request headers; redactKeyLikeTokens scrubs
+ * any key-shaped token the vendor may echo back, so the key never lands in the
+ * thrown error.
  */
 export async function requestAdminJson(
   url: string,
@@ -103,7 +126,9 @@ export async function requestAdminJson(
     let bodyHint = "";
     try {
       const body = await response.text();
-      bodyHint = body ? `: ${body.slice(0, 200)}` : "";
+      // Redact before truncating so a key straddling the 200-char cut can't
+      // survive in a half-scrubbed form.
+      bodyHint = body ? `: ${redactKeyLikeTokens(body).slice(0, 200)}` : "";
     } catch {
       bodyHint = "";
     }

apps/desktop/src/main/claude-code-analytics-client.ts

@@ -106,6 +106,12 @@ function assertUtcDayString(value: string, field: string): string {
   if (Number.isNaN(parsed.getTime())) {
     throw new Error(`Claude Code analytics: ${field} is not a real date`);
   }
+  // JS Date silently overflows out-of-range days (e.g. 2024-02-30 → 2024-03-01),
+  // so the NaN check above is not enough. Require the parsed date to round-trip
+  // back to the same string, which rejects impossible calendar days.
+  if (parsed.toISOString().slice(0, 10) !== value) {
+    throw new Error(`Claude Code analytics: ${field} is not a real date`);
+  }
   return value;
 }
 
@@ -243,6 +249,9 @@ function tokenCount(raw: unknown): number {
   if (typeof raw !== "number" || !Number.isFinite(raw)) {
     throw new Error("Claude Code analytics: token count must be a number");
   }
+  if (raw < 0) {
+    throw new Error("Claude Code analytics: token count must be non-negative");
+  }
   return raw;
 }
 

apps/desktop/src/main/reconciliation-worker.ts

@@ -5260,12 +5260,22 @@ <h3 class="settings-group-title">Labs</h3>
         if (count === 0) {
           return "No Claude Code usage reported for this window (requires a Team or Enterprise org).";
         }
-        const window = result.window;
-        const range = window ? ` (${window.startDay} → ${window.endDay})` : "";
+        // Local name — NOT the browser `window` global (which we never need here).
+        const queryWindow = result.window;
+        const range = queryWindow ? ` (${queryWindow.startDay} → ${queryWindow.endDay})` : "";
         return `Anthropic's estimate${range}.`;
       }
 
-      function renderClaudeCodeTotal(rows) {
+      // Inclusive day count for a {startDay, endDay} window, or null if unparseable.
+      function claudeCodeWindowDayCount(queryWindow) {
+        if (!queryWindow || !queryWindow.startDay || !queryWindow.endDay) return null;
+        const start = Date.parse(`${queryWindow.startDay}T00:00:00Z`);
+        const end = Date.parse(`${queryWindow.endDay}T00:00:00Z`);
+        if (Number.isNaN(start) || Number.isNaN(end) || end < start) return null;
+        return Math.round((end - start) / 86400000) + 1;
+      }
+
+      function renderClaudeCodeTotal(rows, queryWindow) {
         if (!claudeCodeUsageTotal) return;
         if (!Array.isArray(rows) || rows.length === 0) {
           claudeCodeUsageTotal.textContent = "";
@@ -5277,7 +5287,11 @@ <h3 class="settings-group-title">Labs</h3>
             total += row.estimatedCostMicroCents;
           }
         }
-        claudeCodeUsageTotal.textContent = `7-day total: ${formatUsdFromMicroCents(total)}`;
+        // Derive the label from the actual window so it stays correct if the
+        // window ever becomes configurable, instead of hardcoding "7-day".
+        const days = claudeCodeWindowDayCount(queryWindow);
+        const label = days ? `${days}-day total` : "Total";
+        claudeCodeUsageTotal.textContent = `${label}: ${formatUsdFromMicroCents(total)}`;
       }
 
       async function refreshClaudeCodeUsage() {
@@ -5296,7 +5310,7 @@ <h3 class="settings-group-title">Labs</h3>
         const records = result && Array.isArray(result.records) ? result.records : [];
         const rows = aggregateClaudeCodeRecords(records);
         renderClaudeCodeUsageTable(rows);
-        renderClaudeCodeTotal(rows);
+        renderClaudeCodeTotal(rows, result && result.window);
         if (claudeCodeUsageMessage) {
           claudeCodeUsageMessage.textContent = describeClaudeCodeResult(result);
         }

apps/desktop/src/renderer/index.html

@@ -15,13 +15,19 @@
  *   (4) pagination follows next_page and concatenates pages, and exceeding the
  *       page cap throws rather than returning a partial (understated) bill;
  *   (5) a non-2xx response throws with the status, and malformed money throws
- *       rather than silently dropping a charge.
+ *       rather than silently dropping a charge;
+ *   (6) the thrown non-2xx error is scrubbed of any key-shaped token the vendor
+ *       echoes back in its error body, so the Admin key never lands in an error
+ *       message (and from there an IPC reply or the log file).
  *
  * The network is never touched: a recording fake fetch returns canned bodies.
  */
 import assert from "node:assert/strict";
 import { test } from "node:test";
-import { assertAllowedAdminHost } from "../src/main/admin-billing.js";
+import {
+  assertAllowedAdminHost,
+  redactKeyLikeTokens,
+} from "../src/main/admin-billing.js";
 import { AnthropicAdminClient } from "../src/main/anthropic-admin-client.js";
 import { OpenAiAdminClient } from "../src/main/openai-admin-client.js";
 import { makeFetch } from "./helpers/admin-fetch.js";
@@ -154,15 +160,51 @@ test("Anthropic: follows next_page pagination and concatenates results", async (
   assert.ok(calls[1].url.includes("page=PAGE_2_TOKEN"));
 });
 
-test("Anthropic: a non-2xx response throws with the status", async () => {
+test("Anthropic: a non-2xx response throws with the status, key scrubbed from the body", async () => {
+  // Model a vendor 401 body that echoes the key it received (OpenAI does this;
+  // we treat any vendor body as untrusted and scrub it).
   const { fetch } = makeFetch([{ error: "nope" }], {
     status: 401,
-    bodyText: '{"error":"invalid x-api-key"}',
+    bodyText:
+      '{"error":"Incorrect API key provided: sk-ant-admin-TEST. Check your key."}',
   });
   const client = new AnthropicAdminClient({ apiKey: "sk-ant-admin-TEST", fetch });
   await assert.rejects(
     () => client.fetchCostReport({ startingAt: "2026-05-20T00:00:00Z" }),
-    /Anthropic admin API HTTP 401/,
+    (err: unknown) => {
+      const message = (err as Error).message;
+      assert.match(message, /Anthropic admin API HTTP 401/);
+      // The exact key string must NOT appear; it is redacted instead.
+      assert.ok(
+        !message.includes("sk-ant-admin-TEST"),
+        "error message must not contain the Admin key",
+      );
+      assert.match(message, /sk-\[redacted\]/);
+      return true;
+    },
+  );
+});
+
+test("redactKeyLikeTokens scrubs plain and masked keys but leaves prose intact", () => {
+  // Plain key.
+  assert.equal(
+    redactKeyLikeTokens("Incorrect API key provided: sk-ant-admin-TEST."),
+    "Incorrect API key provided: sk-[redacted].",
+  );
+  // OpenAI-style asterisk-masked key.
+  assert.equal(
+    redactKeyLikeTokens("provided: sk-admin-****************************tK8F."),
+    "provided: sk-[redacted].",
+  );
+  // Both vendor prefixes in one body.
+  assert.equal(
+    redactKeyLikeTokens("sk-admin-AAAA and sk-ant-admin-BBBB"),
+    "sk-[redacted] and sk-[redacted]",
+  );
+  // Non-key text is untouched (no false positives on ordinary words).
+  assert.equal(
+    redactKeyLikeTokens("rate limit exceeded for this organization"),
+    "rate limit exceeded for this organization",
   );
 });
 

apps/desktop/test/admin-cost-clients.test.ts

@@ -12,10 +12,10 @@
  *   (3) the client loops ONE request per UTC day across the window;
  *   (4) within a day it follows next_page and concatenates pages;
  *   (5) a non-2xx throws with the status; malformed money throws rather than
- *       silently dropping spend; exceeding the per-day page cap throws rather
- *       than returning a partial usage picture;
+ *       silently dropping spend; a negative token count throws; exceeding the
+ *       per-day page cap throws rather than returning a partial usage picture;
  *   (6) an unknown/future actor shape is surfaced (not dropped) under a stable
- *       label, and an inverted/oversized window is rejected.
+ *       label, and an inverted/oversized/impossible-date window is rejected.
  *
  * The network is never touched: the shared recording fake fetch returns canned
  * bodies (see test/helpers/admin-fetch.ts).
@@ -236,6 +236,35 @@ test("malformed estimated cost throws rather than dropping spend", async () => {
   );
 });
 
+test("a negative token count throws rather than recording a nonsensical value", async () => {
+  const { fetch } = makeFetch([
+    {
+      data: [
+        {
+          actor: { type: "user_actor", email_address: "a@example.com" },
+          model_breakdown: [
+            {
+              model: "m1",
+              estimated_cost: { amount: 100, currency: "USD" },
+              tokens: { input: -5, output: 10 },
+            },
+          ],
+        },
+      ],
+      has_more: false,
+      next_page: null,
+    },
+  ]);
+  const client = new ClaudeCodeAnalyticsClient({
+    apiKey: "sk-ant-admin-TEST",
+    fetch,
+  });
+  await assert.rejects(
+    () => client.fetchUsage({ startDay: "2026-05-20", endDay: "2026-05-20" }),
+    /token count must be non-negative/,
+  );
+});
+
 test("exceeding the per-day page cap throws instead of returning partial usage", async () => {
   const alwaysMore = {
     data: [
@@ -305,6 +334,12 @@ test("rejects an inverted window and a bad day string", async () => {
     () => client.fetchUsage({ startDay: "2026/05/20", endDay: "2026-05-20" }),
     /startDay must be YYYY-MM-DD/,
   );
+  // A well-formed but impossible calendar day (Feb 30) must be rejected — JS
+  // Date silently overflows it to March 1, so the shape check alone is not enough.
+  await assert.rejects(
+    () => client.fetchUsage({ startDay: "2026-02-30", endDay: "2026-02-30" }),
+    /startDay is not a real date/,
+  );
 });
 
 test("rejects an empty Admin key at construction", () => {

apps/desktop/test/claude-code-analytics-client.test.ts

@@ -230,6 +230,13 @@ test("one vendor failing does not abort the other (per-vendor isolation)", async
   assert.equal(summary.errors.length, 1);
   assert.equal(summary.errors[0].vendor, "anthropic");
   assert.match(summary.errors[0].message, /HTTP 401/);
+  // The Admin key the service holds must never surface in the IPC-visible
+  // summary (the real client redacts upstream in requestAdminJson; the service
+  // must not reintroduce it). Guards the renderer-visible error path.
+  assert.ok(
+    !summary.errors[0].message.includes("sk-ant-admin-bad"),
+    "summary error must not contain the Admin key",
+  );
   // The healthy vendor still produced a row.
   assert.equal(summary.rowsWritten, 1);
 });