diff --git a/HANDOFF_vault_mcp_401.md b/HANDOFF_vault_mcp_401.md new file mode 100644 index 0000000..b59fbf5 --- /dev/null +++ b/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. diff --git a/chrome-extension/src/background/index.ts b/chrome-extension/src/background/index.ts index e1f440e..758cb11 100644 --- a/chrome-extension/src/background/index.ts +++ b/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, diff --git a/chrome-extension/src/background/mcpBridge.ts b/chrome-extension/src/background/mcpBridge.ts index 669e8b2..fc47e40 100644 --- a/chrome-extension/src/background/mcpBridge.ts +++ b/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 = { 0: 'unknown', 1: 'disconnected', @@ -55,6 +64,7 @@ let deps: McpBridgeDeps | null = null; let ws: WebSocket | null = null; let enabled = false; let reconnectTimer: ReturnType | null = null; +let heartbeatTimer: ReturnType | 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 { 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 { if (ws !== socket) return; ws = null; connectedAt = null; + stopHeartbeat(); scheduleReconnect(); }; diff --git a/chrome-extension/src/background/methods.ts b/chrome-extension/src/background/methods.ts index a6ba861..3b28adf 100644 --- a/chrome-extension/src/background/methods.ts +++ b/chrome-extension/src/background/methods.ts @@ -66,6 +66,9 @@ const requireApproval = async function ( params?: any, ): Promise { 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 } }; diff --git a/chrome-extension/src/background/providerLog.test.ts b/chrome-extension/src/background/providerLog.test.ts index 7b72d87..763c039 100644 --- a/chrome-extension/src/background/providerLog.test.ts +++ b/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[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); }); }); diff --git a/chrome-extension/src/background/providerLog.ts b/chrome-extension/src/background/providerLog.ts index 8c413b9..6fa4d6b 100644 --- a/chrome-extension/src/background/providerLog.ts +++ b/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(); const connectedSites = new Map; 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): 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[] { diff --git a/scripts/test-mcp-bridge.mjs b/scripts/test-mcp-bridge.mjs index 82ae96e..047f717 100644 --- a/scripts/test-mcp-bridge.mjs +++ b/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= 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}`);