fix(mcp): bridge follow-ups from #109 review — smoke-script auth, MV3 heartbeat, pending-key collisions

HANDOFF_vault_mcp_401.md

@@ -0,0 +1,100 @@
+# HANDOFF → vault: `/mcp` cannot return its 401 (throws uncaught instead)
+
+**From:** keepkey-client MCP agent bridge work, 2026-07-15 (PR #109 review → #110).
+**For:** whoever owns `keepkey-vault`. This is a vault-side change; the client
+side needs nothing.
+
+**Repo:** `/Users/highlander/WebstormProjects/keepkey-stack/projects/keepkey-vault-v11`
+**File:** `projects/keepkey-vault/src/bun/rest-api.ts`
+(the vault repo is `github.com/keepkey/keepkey-vault`; `src/bun` lives under
+`projects/keepkey-vault/` in the v11 checkout — a known foot-gun)
+
+> A branch with this change already exists: **`fix/mcp-401-uncaught`**, pushed,
+> open as **keepkey-vault PR #361**. Merge it, cherry-pick it, or re-implement
+> from the diff below and close it — whatever suits. **Note: that branch is
+> currently checked out in the local v11 working tree**, so a rebuild from that
+> tree right now includes this fix. `git checkout develop` there to get back to
+> a clean baseline (the running vault was rebuilt at least once mid-session, so
+> its current binary already contains the fix).
+
+---
+
+## The bug
+
+`auth.requireAuth()` signals failure by **throwing** `HttpError` (`auth.ts:314-320`).
+
+The `/mcp` routing block sits at `rest-api.ts:~1267-1310` — **before** the
+`try` at `:1420` whose catch translates `HttpError` into its status
+(`:4110-4114`). `Bun.serve` has no top-level `error` handler either (verified:
+the options object carries only `port`, `maxRequestBodySize`, `fetch`, and now
+`websocket`).
+
+So a missing or invalid bearer key on `POST /mcp` escapes `fetch()` uncaught.
+The agent gets a dropped socket (`fetch failed` / `UND_ERR_SOCKET`) instead of
+the intended 401 — an inscrutable failure for the exact case that is most
+likely to happen: a misconfigured `claude mcp add`.
+
+**Scope — only `/mcp` is affected:**
+
+| Call site | Line | Status |
+|---|---|---|
+| `/mcp` POST | `:1305` | **broken** — outside the try |
+| `/bex-bridge` | `:1278` | fine — uses `auth.validate()` (returns `null`), returns its 401 cleanly |
+| everything else | `:1480`, `:1489`, … | fine — inside the try |
+
+## The fix
+
+Catch locally in the `/mcp` POST branch. Auth behavior is unchanged; only the
+failure *response* is. Deliberately does **not** route through the `json()`
+helper, to preserve `/mcp`'s no-CORS-grant posture.
+
+```ts
+if (method === 'POST') {
+  // requireAuth signals failure by THROWING HttpError, and this whole
+  // /mcp block runs before the try/catch further down that turns an
+  // HttpError into its status — Bun.serve has no top-level `error`
+  // handler either, so an uncaught throw escapes fetch() and the agent
+  // gets a dropped socket ("fetch failed") instead of 401. Catch here.
+  try {
+    auth.requireAuth(req)
+  } catch (err: any) {
+    return new Response(JSON.stringify({ error: err?.message || 'Unauthorized' }),
+      { status: typeof err?.status === 'number' ? err.status : 401,
+        headers: { 'Content-Type': 'application/json' } })
+  }
+  return handleMcpRequest(req, {})
+}
+```
+
+## Verification already done (on that branch's build)
+
+```
+POST /mcp      bad bearer  → 401 {"error":"Invalid or expired API key — re-pair via POST /auth/pair"}   (no CORS headers)
+POST /eth/sign bad bearer  → 401 same body, WITH Access-Control-Allow-Origin: *                          (the :1420 catch)
+POST /mcp      + Origin    → 403 {"error":"/mcp is not reachable from a browser"}
+GET  /docs                 → 200
+```
+
+The differing CORS headers are the discriminator proving the 401 comes from
+this fix's path rather than the general catch.
+
+`bun test src/bun/mcp.test.ts` → 16 pass. Type errors unchanged at 638 (the
+pre-existing `develop` baseline; the fix adds none).
+
+## Caveat worth knowing
+
+I never observed the **pre-fix** symptom on a healthy vault. The dropped-socket
+reading that first raised this was taken while the vault was flapping
+(the process holding `:1646` kept dying mid-request; `/docs` returned 200 once,
+then 000), so it is **not** valid evidence. The bug is established by source
+reading — throw site outside the catch, no `error` handler — not by a live
+before/after. If you want the runtime proof, test a bad bearer against a
+`develop` build with no other changes.
+
+## Gap this points at
+
+`rest-api.ts`'s routing has **no test harness** — `src/bun/mcp.test.ts` unit-tests
+`handleMcpRequest` directly, which is exactly why an auth bug in the *routing*
+slipped through. Standing one up for a single branch of a ~145-branch if/else
+chain is out of proportion, but the routing layer being untested is worth a
+ticket of its own.

chrome-extension/src/background/index.ts

@@ -1133,6 +1133,13 @@ initMcpBridge({
   getKeepKeyState: () => KEEPKEY_STATE,
   walletRequest: async (chain: string, method: string, params: any[]) => {
     if (!wallet.isInitialized()) await ensureStarted();
+    // Same post-init check WALLET_REQUEST does, so an agent calling with the
+    // vault closed gets the actionable "launch the Vault" error rather than a
+    // bare handler failure.
+    if (!wallet.isInitialized()) {
+      if (KEEPKEY_STATE === 4) throw createVaultRequiredError();
+      throw Error('Wallet not initialized');
+    }
     const requestInfo = {
       id: crypto.randomUUID(),
       method,

chrome-extension/src/background/mcpBridge.ts

@@ -21,6 +21,15 @@ const TAG = ' | mcpBridge | ';
 const BRIDGE_URL = 'ws://localhost:1646/bex-bridge';
 const RECONNECT_MAX_MS = 60_000;
 
+// MV3 evicts an idle service worker after ~30s, and an idle WebSocket does NOT
+// prevent that — only actual WS traffic resets the timer (Chrome 116+). Without
+// this ping the worker dies on a quiet machine, the socket goes with it, and
+// nothing wakes the worker back up: an agent polling bex_status would see
+// bridge "down" indefinitely. 20s keeps us inside the 30s window.
+// The vault ignores frames whose id matches no pending call, so this needs no
+// server-side handler.
+const HEARTBEAT_MS = 20_000;
+
 const KEEPKEY_STATE_NAMES: Record<number, string> = {
   0: 'unknown',
   1: 'disconnected',
@@ -55,6 +64,7 @@ let deps: McpBridgeDeps | null = null;
 let ws: WebSocket | null = null;
 let enabled = false;
 let reconnectTimer: ReturnType<typeof setTimeout> | null = null;
+let heartbeatTimer: ReturnType<typeof setInterval> | null = null;
 let reconnectDelay = 5_000;
 let connectedAt: number | null = null;
 
@@ -69,11 +79,19 @@ export function initMcpBridge(bridgeDeps: McpBridgeDeps): void {
   agentModeStorage.get().then(apply);
 }
 
+function stopHeartbeat(): void {
+  if (heartbeatTimer) {
+    clearInterval(heartbeatTimer);
+    heartbeatTimer = null;
+  }
+}
+
 function disconnect(): void {
   if (reconnectTimer) {
     clearTimeout(reconnectTimer);
     reconnectTimer = null;
   }
+  stopHeartbeat();
   if (ws) {
     try {
       ws.close();
@@ -106,6 +124,15 @@ async function connect(): Promise<void> {
     console.log(TAG, 'bridge connected');
     connectedAt = Date.now();
     reconnectDelay = 5_000;
+    stopHeartbeat();
+    heartbeatTimer = setInterval(() => {
+      if (ws !== socket || socket.readyState !== WebSocket.OPEN) return;
+      try {
+        socket.send(JSON.stringify({ ping: Date.now() }));
+      } catch {
+        /* onclose handles the reconnect */
+      }
+    }, HEARTBEAT_MS);
   };
 
   socket.onmessage = async event => {
@@ -133,6 +160,7 @@ async function connect(): Promise<void> {
     if (ws !== socket) return;
     ws = null;
     connectedAt = null;
+    stopHeartbeat();
     scheduleReconnect();
   };
 

chrome-extension/src/background/methods.ts

@@ -66,6 +66,9 @@ const requireApproval = async function (
   params?: any,
 ): Promise<any> {
   const tag = TAG + ' | requireApproval | ';
+  // Set once the bridge's approval mirror has an entry; settled on every exit
+  // path (approve/reject, timeout, throw).
+  let pendingKey: string | null = null;
   try {
     console.log(tag, 'networkId:', networkId);
 
@@ -120,7 +123,7 @@ const requireApproval = async function (
 
     // Mirror the in-memory approval queue for the MCP agent bridge
     // (bex_pending_requests). Settled again on resolve/timeout below.
-    registerPending({
+    pendingKey = registerPending({
       id: requestInfo.id,
       method: method || requestInfo.method,
       params: params ?? requestInfo.params,
@@ -140,7 +143,7 @@ const requireApproval = async function (
         chrome.runtime.onMessage.removeListener(listener);
         if (timer != null) clearTimeout(timer);
         setApprovalBadge(false);
-        settlePending(requestInfo.id);
+        settlePending(pendingKey);
       };
 
       const listener = (message: any) => {
@@ -164,7 +167,7 @@ const requireApproval = async function (
     });
   } catch (e) {
     console.error(tag, e);
-    settlePending(requestInfo?.id);
+    settlePending(pendingKey);
     return { success: false }; // Return failure in case of error
   }
 };

chrome-extension/src/background/providerLog.test.ts

@@ -47,21 +47,56 @@ describe('providerLog ring buffer', () => {
     for (let i = 0; i < 600; i++) recordProviderCall(entry());
     expect(getLogs({ limit: 10_000 }).length).toBeLessThanOrEqual(500);
   });
+
+  it('never throws on unserializable params/results', () => {
+    // Runs on the success path of every provider call — a stringify failure
+    // here must not surface as a dApp-visible error.
+    const circular: any = { self: null };
+    circular.self = circular;
+    expect(() => recordProviderCall(entry({ method: 'circular_call', params: circular }))).not.toThrow();
+    expect(() => recordProviderCall(entry({ method: 'bigint_call', result: { v: 1n } }))).not.toThrow();
+    expect(getLogs({ pattern: 'bigint_call' })[0].result).toBe('[unserializable]');
+  });
 });
 
 describe('pending request registry', () => {
-  it('registers and settles pending approvals', () => {
-    registerPending({
-      id: 'req-1',
-      method: 'personal_sign',
-      params: ['hi'],
-      origin: 'https://dapp.test',
-      chain: 'ethereum',
-      requestedAt: Date.now(),
-    });
-    expect(getPendingRequests().map(r => r.id)).toContain('req-1');
-    settlePending('req-1');
-    expect(getPendingRequests().map(r => r.id)).not.toContain('req-1');
+  const pending = (over: Partial<Omit<Parameters<typeof registerPending>[0], never>> = {}) => ({
+    id: 'req-1',
+    method: 'personal_sign',
+    params: ['hi'],
+    origin: 'https://dapp.test',
+    chain: 'ethereum',
+    requestedAt: Date.now(),
+    ...over,
+  });
+
+  it('registers and settles pending approvals by internal key', () => {
+    const key = registerPending(pending());
+    expect(getPendingRequests().map(r => r.key)).toContain(key);
+    settlePending(key);
+    expect(getPendingRequests().map(r => r.key)).not.toContain(key);
+  });
+
+  it('keeps two tabs separate when the dApps supply the SAME id', () => {
+    // dApp ids are per-page counters — both tabs say id 1.
+    const a = registerPending(pending({ id: '1', origin: 'https://a.test' }));
+    const b = registerPending(pending({ id: '1', origin: 'https://b.test' }));
+    expect(a).not.toBe(b);
+    expect(getPendingRequests().filter(r => r.id === '1')).toHaveLength(2);
+
+    // Settling one must not evict the other's prompt.
+    settlePending(a);
+    const left = getPendingRequests().filter(r => r.id === '1');
+    expect(left).toHaveLength(1);
+    expect(left[0].origin).toBe('https://b.test');
+    settlePending(b);
+  });
+
+  it('ignores a null/undefined key (register never ran)', () => {
+    const before = getPendingRequests().length;
+    settlePending(null);
+    settlePending(undefined);
+    expect(getPendingRequests()).toHaveLength(before);
   });
 });
 

chrome-extension/src/background/providerLog.ts

@@ -24,6 +24,14 @@ export interface ProviderLogEntry {
 }
 
 export interface PendingRequest {
+  /**
+   * Internally-generated, unique. `id` below is dApp-supplied (typically a
+   * per-page counter, so two tabs collide on `1`) — keying the queue by it
+   * would let one settle evict another tab's entry, and would let a Phase-2
+   * bex_approve_request(id) resolve the WRONG origin's prompt. Address the
+   * queue by `key`; `id` is for correlating with the dApp's own logs.
+   */
+  key: string;
   id: string;
   method: string;
   params: unknown;
@@ -41,8 +49,16 @@ const logBuffer: ProviderLogEntry[] = [];
 const pendingRequests = new Map<string, PendingRequest>();
 const connectedSites = new Map<string, { chains: Set<string>; firstSeen: number; lastSeen: number }>();
 
+// Never throw: this runs on the SUCCESS path of every provider call, so a
+// stringify failure (BigInt, circular ref) must not turn a good wallet response
+// into a dApp-visible error.
 const clip = (v: unknown): unknown => {
-  const s = JSON.stringify(v);
+  let s: string | undefined;
+  try {
+    s = JSON.stringify(v);
+  } catch {
+    return '[unserializable]';
+  }
   if (s && s.length > MAX_RESULT_CHARS) return `[clipped ${s.length} chars] ${s.slice(0, MAX_RESULT_CHARS)}`;
   return v;
 };
@@ -67,12 +83,16 @@ export function getLogs(filter?: { pattern?: string; since?: number; limit?: num
   return out.slice(-limit);
 }
 
-export function registerPending(req: PendingRequest): void {
-  pendingRequests.set(req.id, req);
+/** Returns the internal key to settle this entry with — never reuse the dApp id. */
+export function registerPending(req: Omit<PendingRequest, 'key'>): string {
+  const key = crypto.randomUUID();
+  pendingRequests.set(key, { ...req, key });
+  return key;
 }
 
-export function settlePending(id: string): void {
-  pendingRequests.delete(id);
+export function settlePending(key: string | null | undefined): void {
+  if (!key) return;
+  pendingRequests.delete(key);
 }
 
 export function getPendingRequests(): PendingRequest[] {

scripts/test-mcp-bridge.mjs

@@ -5,22 +5,37 @@
  * Preconditions (human, one-time): vault running on :1646, extension loaded,
  * options-page "Agent mode" toggle ON.
  *
- *   node scripts/test-mcp-bridge.mjs
+ *   KEEPKEY_API_KEY=<pairing key> node scripts/test-mcp-bridge.mjs
+ *
+ * The vault bearer-authenticates /mcp with the same pairing keys as the rest of
+ * its REST API, so the key is required. Grab the extension's own from the
+ * background console: `chrome.storage.local.get('keepkey-api-key')`.
+ *
+ * Node's fetch sends no Origin/Sec-Fetch-Site, which is what lets this pass the
+ * vault's browser-exclusion check — a browser could not run this script.
  *
  * Exits 0 when the vault serves MCP, tier-1 tools are listed, and bex_status
- * answers truthfully through the bridge (or truthfully reports bridge down).
+ * answers truthfully through the bridge; 2 when the bridge reports down.
  */
 
 const MCP_URL = 'http://localhost:1646/mcp';
 const TIER1_TOOLS = ['bex_status', 'bex_accounts', 'bex_pending_requests', 'bex_connected_sites', 'bex_logs'];
 
+const API_KEY = process.env.KEEPKEY_API_KEY;
+if (!API_KEY) {
+  console.error('KEEPKEY_API_KEY is required (the vault bearer-authenticates /mcp).');
+  console.error("Get the extension's key from the background console: chrome.storage.local.get('keepkey-api-key')");
+  process.exit(1);
+}
+
 let nextId = 1;
 async function rpc(method, params) {
   const res = await fetch(MCP_URL, {
     method: 'POST',
-    headers: { 'Content-Type': 'application/json' },
+    headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${API_KEY}` },
     body: JSON.stringify({ jsonrpc: '2.0', id: nextId++, method, ...(params ? { params } : {}) }),
   });
+  if (res.status === 401) throw new Error(`${method}: HTTP 401 — KEEPKEY_API_KEY is not a valid vault pairing key`);
   if (!res.ok) throw new Error(`${method}: HTTP ${res.status}`);
   const body = await res.json();
   if (body.error) throw new Error(`${method}: rpc error ${body.error.code} ${body.error.message}`);