feat(protocol)!: bind the ephemeral key into the ready signature

[chrischall]

Closes the residual recorded in the private advisory GHSA-j6jv-w774-77m6, found while building the mcp-host browser bridge (chrischall/mcp-host#162) and left open by #213 because the fix breaks the wire.

⚠️ BREAKING. PROTOCOL_VERSION 2 → 3. Every package and the browser extension must be updated together; a v2 peer is refused at the hello. Reload the unpacked extension when this ships, or the bridge stops working — loudly, at the handshake, not silently.

The hole

0.4.0's mutual auth proved the extension was the one whose hello arrived. It never committed to the key the session is derived from:

sessionSig = Ed25519Sign(extEdPriv, mcpHelloNonce || extHelloNonce)   // v2
shared     = X25519(mcpIdentityPriv, extensionSessionPub)             // what we derive

So a relay could forward the genuine hellos and the genuine signature — every nonce and identity intact, the pair code unchanged — substitute an extensionSessionPub it held the private half of, and compute the same shared secret as the MCP. Nothing either side checked said otherwise. The peer path made it trivial (it verified nothing at all until #213), but the host path had the same hole from 0.4.0 onward.

The fix

sessionSig = Ed25519Sign(extEdPriv, mcpHelloNonce || extHelloNonce || extensionSessionPub)

One definition — readySignaturePayload() in @fetchproxy/protocol — used by the extension that signs it and by both server paths that verify it, so they cannot drift apart the way the comments about them already had. A relay would now have to sign its own ephemeral key with the extension's Ed25519 private key.

Why no negotiated version

A version-gated variant avoids the break, and hands the attacker the choice: a relay that can rewrite frames can rewrite the field advertising v3 support, and both ends would then agree on the weaker payload. Closing that needs the negotiation itself signed, which is the same wire change with more moving parts. The 0.4.0 precedent applies — hard break, all packages together, old version refused at the hello.

One test changed sides

KNOWN RESIDUAL: a relay can still swap the ephemeral key and share the session was added in #213 with a comment saying it should start failing the day the signature covered the ephemeral key. It did, on the first run. It now performs the same substitution and asserts the refusal, under a name that says so — the residual's own tripwire, doing its job.

Testing

1233 tests, three consecutive clean runs, tsc -b across all six workspaces, npm run build clean. The 128 tests that initially failed were fixtures hand-building v2 hellos and signatures; each now goes through readySignaturePayload, and validate.test.ts asserts v2 is refused rather than downgraded.

After merge: the advisory can be published, and mcp-host's gate 1 (chrischall/mcp-host#162) is satisfiable — its whole premise is a relay that cannot read the traffic, which until now was narrowed rather than true.

[claude]

Claude finished @chrischall's task in 4m 1s —— View job

---

Reviewing PR #222

• Read CLAUDE.md and repo conventions
• Read the PR diff
• Review protocol.ts changes (readySignaturePayload, PROTOCOL_VERSION bump)
• Review server/host.ts and server/peer.ts changes
• Review extension-core/background.ts changes
• Review test coverage changes
• Post findings

Summary

Verified the core fix end-to-end: readySignaturePayload() in packages/protocol/src/frames.ts is used consistently by the extension (both pairing and auto-trust ready-signing paths) and by both server verification paths (host.ts, peer.ts), with matching raw-byte content on both sides. PROTOCOL_VERSION 3 is enforced as a hard equality check in validate.ts, so v2 is refused with no downgrade path, matching the PR's stated rationale. The flipped "KNOWN RESIDUAL" test in extension-pin-peer.test.ts now correctly asserts refusal instead of recording the old gap, and the host.test.ts bad-ECDH fixture correctly signs over the new payload so it still exercises the intended failure path. Docs (CLAUDE.md, docs/PROTOCOL.md, docs/SECURITY.md) are updated consistently.

One 🟡 nit posted inline: six integration test files (mutual-auth-idb, graphql-query, read-cookies, all-bootstrap-verbs, two-mcps, reconnect) left concatBytes imported but unused after switching to readySignaturePayload(...). Not caught by tsc -b (tests aren't in packages/server/tsconfig.json's include, and noUnusedLocals isn't set) — cosmetic only.

Could not run npm test/tsc -b/npm run build myself in this sandbox (no node_modules, and npm install/git fetch needed approval that wasn't granted) — noted as an open item rather than a finding, relying on the PR's stated clean test/build runs.

Full review posted as a PR comment and one inline comment.

Verdict: warn

[claude]

🟡 Nit: concatBytes is now an unused import here — the sig-payload construction it used to feed was replaced by readySignaturePayload(...) in this same diff, and nothing else in the file calls it. Same leftover in the other five integration tests touched by this PR: graphql-query.test.ts, read-cookies.test.ts, all-bootstrap-verbs.test.ts, two-mcps.test.ts, and reconnect.test.ts (all import concatBytes but no longer call it). Not caught by tsc -b since packages/server/tsconfig.json only includes src/**/*, and noUnusedLocals isn't set in tsconfig.base.json, but worth a cleanup pass.

Fix this →

[claude]

Review: bind the ephemeral key into the ready signature

Read CLAUDE.md, walked the full diff (git diff origin/main...HEAD), and traced the fix end-to-end.

Core change checks out. readySignaturePayload(mcpHelloNonce, extHelloNonce, extensionSessionPub) in packages/protocol/src/frames.ts is the single definition used by all three sites that need to agree on it:

• packages/extension-core/src/background.ts (both the pairing-flow onServerHello path and the auto-trust onApproval path) — signs with the raw ephemeral pub (result.extensionSessionPub / ephemeral.publicKey), matching what's later base64-encoded and sent as ready.extensionSessionPub.
• packages/server/src/host.ts — verifies with fromB64(frame.extensionSessionPub).
• packages/server/src/peer.ts — verifies with fromB64(extensionSessionPub) (now threaded through authenticateExtension's new second parameter).

Byte content matches on both ends in every call site I checked, so the signature does what the PR claims: it commits to the key the ECDH actually uses.

PROTOCOL_VERSION bump 2 → 3 is a hard equality check in packages/protocol/src/validate.ts:508 (raw.protocolVersion !== PROTOCOL_VERSION), so a v2 hello is refused rather than negotiated down — matches the "no downgrade path" rationale in the PR body and docs/SECURITY.md.

Test coverage is exactly what a change like this needs. extension-pin-peer.test.ts's KNOWN RESIDUAL test now asserts the refusal it used to record as a gap (rejects.toThrow(/identity refused/), trust.writes empty) — the PR description's claim that "one test changed sides" checks out. host.test.ts's bad-ECDH-payload test correctly signs over the short (8-byte) pub so the signature check passes before the ECDH throw is reached, which is the right way to keep that test exercising the intended failure path under the new payload. All protocolVersion: 2 fixtures across the touched test files were bumped to 3, with the one exception being the intentional negative case in validate.test.ts (asserting v2 is now rejected).

Docs (CLAUDE.md, docs/PROTOCOL.md, docs/SECURITY.md §T-host-MITM) are updated consistently with the code and with each other, and correctly stop describing the residual as open.

One nit (inline on mutual-auth-idb.test.ts:12): six integration test files (mutual-auth-idb, graphql-query, read-cookies, all-bootstrap-verbs, two-mcps, reconnect) still import concatBytes after switching their sig-payload construction to readySignaturePayload(...); grepping each file turns up no remaining call site. Not caught by tsc -b (server's tsconfig.json only includes src/**/*, and noUnusedLocals isn't set), so it's cosmetic leftover from the refactor rather than a build break.

Not independently verified: I don't have node_modules installed in this sandbox and network-touching commands (npm install, git fetch) require approval I wasn't granted, so I could not re-run npm test / tsc -b / npm run build myself. Static review of the diff (including the full server/extension/protocol/test set) found the payload construction and verification consistent everywhere it's used; I'm relying on the PR's stated "1233 tests, three consecutive clean runs" for the dynamic check.

No security or correctness issues found in the actual fix — it closes the gap described (GHSA-j6jv-w774-77m6) the way the PR body describes, and the hard-break handling matches the repo's stated 0.4.0 precedent.

Verdict: warn

[github-actions]

🟡 Auto-review verdict: warn — The ready-signature fix (binding extensionSessionPub into the signed payload via readySignaturePayload) is correctly implemented and consistently used across the extension and both server verification paths, with matching test and doc updates; one minor unused-import nit found, no important findings.
📋 Tracking follow-ups: #223