feat(server): pin the extension's identity, and verify it on the peer path - #213
Conversation
github-actions
Bot
added
the
auto-review
|
Claude finished @chrischall's task in 6m 21s —— View job Review — PR #213: pin the extension's identity, verify it on the peer path
The shape of this is right and the writing is unusually honest about what was broken. Two things I'd want addressed before this merges. 🔴 Important1. The peer's signature check doesn't bind The extension signs The change is still a real improvement — wholesale identity impersonation by the concentrator is now refused — but 2.
🟡 Nits
Open question (not a finding)I could not run the suite — this checkout has no Verdict: fail |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return true; | ||
| } | ||
|
|
||
| const payload = concatBytes(sessionNonce, fromB64(extensionHello.sessionNonce)); |
There was a problem hiding this comment.
🔴 The signature does not cover extensionSessionPub, so the exact MITM this PR describes still works.
The payload signed by the extension is nonces only — packages/extension-core/src/background.ts:1094:
const sessionSig = await ed25519Sign(
extIdentity.ed25519Priv,
concatBytes(result.mcpSessionNonce, currentExtSessionNonce),
);(and identically at background.ts:2674). ReadyFrame.extensionSessionPub is not in it.
Both nonces are values a malicious concentrator forwards verbatim: it relays the peer's hello unchanged (so the extension signs over the peer's real nonce), and relays the extension's hello unchanged (so the peer verifies against the real nonce). It then forwards the genuine sessionSig and substitutes only extensionSessionPub. This check passes, and peer.ts:294 derives
shared = X25519(peerIdentityPriv, hostEphemeralPub)
which the host computes as X25519(hostEphemeralPriv, peerIdentityX25519Pub) — the peer's long-term X25519 pub is public material the host already holds (host.ts stores peers.set(frame.mcpId, { ws, helloFrame: frame }), and HelloFrameFromServer.identityX25519Pub is in it). The HKDF salt is the peer's own sessionNonce, also public. Same key on both sides.
The suite's own passing test demonstrates this: extension-pin-peer.test.ts:122 mints const eph = await generateX25519() — an ephemeral pair with no cryptographic relationship to the extension identity — and the peer accepts the session. Only the forge: true case (a garbage 64-byte signature) is refused, which is not what a relaying concentrator would send.
What lands as a result is still a real improvement: wholesale identity impersonation by the concentrator is now refused. But the specific claim is not delivered, and it's asserted in three places that should not overstate:
docs/PROTOCOL.md: "the signature is what makes that fail, because it cannot be produced without the extension's Ed25519 private key" — it doesn't have to be produced, only relayed.docs/SECURITY.md: "a peer verifiesEd25519Sign(extPriv, ownHelloNonce || extHelloNonce)against it before deriving anything… A concentrator cannot produce that signature without the extension's private key."- the comment at
peer.ts:287and the test comment atextension-pin-peer.test.ts:178-179.
Two ways forward, either acceptable:
- Bind the key: have the extension sign
(mcpNonce || extNonce || extensionSessionPub)— additively, e.g. a secondsessionSigV2field so old peers/hosts keep working — and verify that on both the host and peer paths. This actually closesT-host-MITM. - Ship as-is and correct the claims: say the peer now authenticates which extension is behind the concentrator, and record under Residual risk that the ephemeral key is still unbound, so a concentrator can substitute it and read/answer peer traffic until the signature covers it.
Note the host path has the same gap (a relay in front of the host↔extension socket can do the same substitution) — pre-existing since 0.4.0, but relevant since SECURITY.md is being edited here anyway.
| for (const pin of pins) await clearExtensionPin(pin.serverName, identityDir); | ||
| io.err( | ||
| `cleared ${pins.length} extension pin(s): ${pins.map((p) => p.serverName).join(', ')} — ` + | ||
| `each re-pins on the next browser to complete a handshake with it`, | ||
| ); | ||
| return EXIT.OK; | ||
| } | ||
|
|
||
| const had = await clearExtensionPin(cmd.serverName!, identityDir); |
There was a problem hiding this comment.
🔴 trust clear can't clear a pin belonging to a scoped server name — including via --all.
listPins derives serverName from the filename (name.slice(0, -SUFFIX.length)), but the filename is the sanitised base: safeIdentityFileBase maps / → _, so @fetchproxy/example-mcp is stored as @fetchproxy_example-mcp.extension-trust.json. Feeding that back into clearExtensionPin re-validates it against packages/server/src/identity.ts:27-28:
const SAFE_PLAIN = /^[A-Za-z0-9._-]+$/;
const SAFE_SCOPED = /^@[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/;@fetchproxy_example-mcp matches neither (@ isn't in SAFE_PLAIN; there's no / for SAFE_SCOPED), so safeIdentityFileBase throws unsafe serverName for identity file: …. Two consequences:
fpx trust clear --all— documented in the PR as "what a re-install actually needs" — throws on the first scoped pin in the loop at line 89.runClicatches it asfpx: unexpected error: …(main.ts:135-138), so the fleet is left partially cleared with no indication of which pins survived.fpx trust clear @fetchproxy_example-mcp— copied verbatim from whattrust listprints — fails the same way. Only guessing the original@fetchproxy/example-mcpworks.
Scoped names are explicitly supported by the identity layer (SAFE_SCOPED, and the docblock says "scoped packages like @fetchproxy/example-mcp are OK"), so this isn't hypothetical. Since this verb is the escape hatch from a lockout, it should be the most robust part of the change.
Suggested shape: have listPins reverse the mapping for @-prefixed names (@scope_name → @scope/name) and record it as the serverName, and/or wrap the --all loop in a per-pin try/catch that falls back to unlinking pin.file directly and reports failures rather than aborting.
| ws.close(1008, 'extension already connected'); | ||
| return; | ||
| } | ||
| // #208: is this the extension we paired with? Checked HERE, before | ||
| // the connection becomes the extension slot, so a stranger never | ||
| // reaches the session machinery at all. The pin is only WRITTEN | ||
| // later, once the ready signature has proved the key — claiming an | ||
| // identity must not be enough to become the pinned one. | ||
| let pin: Awaited<ReturnType<ExtensionTrustPort['read']>>; | ||
| try { | ||
| pin = await opts.extensionTrust.read(); | ||
| } catch (e) { | ||
| // An unreadable pin is the one state where carrying on would | ||
| // quietly mean "trust anybody". | ||
| console.error(`[fetchproxy] ${String(e)}`); | ||
| ws.close(1008, 'extension pin unreadable'); | ||
| return; | ||
| } | ||
| const outcome = decideExtensionTrust({ | ||
| pin, | ||
| hello: frame, | ||
| allowNew: opts.extensionTrust.allowNew, | ||
| serverName: opts.ownServerName, | ||
| location: opts.extensionTrust.location, | ||
| }); | ||
| if (outcome.decision === 'refused') { | ||
| console.warn(outcome.message); | ||
| ws.close(1008, 'extension identity is not the pinned one'); | ||
| return; | ||
| } | ||
| if (outcome.decision === 'replace') console.warn(outcome.message); | ||
| // Pin on first use, or replace a pin the operator chose to drop — | ||
| // but only after the ready frame proves the key (see below). | ||
| pinOnReady = outcome.decision !== 'pinned'; | ||
| identified = 'extension'; |
There was a problem hiding this comment.
🟡 The await opts.extensionTrust.read() at line 202 now sits between the if (extensionWs) … 'extension already connected' guard (line 190-193) and the extensionWs = ws assignment (line 227). Check-and-set used to be synchronous within one message handler; it isn't any more, and each connection's ws.on('message', …) handler runs independently, so two extension hellos arriving in the same tick can both pass the guard and the second silently replaces the first in the slot (the first's identified === 'extension' stays true but extensionWs === ws is now false, so its close handler no longer clears state).
Related: pinOnReady is startHost-scoped rather than per-connection, so in that same interleaving the second hello's decision overwrites the first's.
Neither is reachable except from a local process, and the ready-signature check still gates the session — so this is a tidiness issue, not an exploit. Capturing the slot synchronously (set extensionWs = ws before the await and roll it back on refusal), or keying pinOnReady off the connection, would keep the invariant the 1008 was written to enforce.
| serverName: string, | ||
| dir: string = defaultIdentityDir(), | ||
| ): Promise<Identity> { | ||
| export function safeIdentityFileBase(serverName: string): string { |
There was a problem hiding this comment.
🟡 The extraction left loadOrCreateIdentity's docblock attached to safeIdentityFileBase. "Read the identity for serverName from dir, generating + persisting a fresh X25519/Ed25519 keypair if no file exists. The file is written with mode 0o600…" describes the function two definitions below; safeIdentityFileBase neither reads nor writes anything. Worth moving it back down onto loadOrCreateIdentity and leaving the "reject unsafe names, translate / to _" half here.
| // fleet re-pairs on the next connection), so it takes saying `--all` | ||
| // rather than happening by omitting an argument. | ||
| if (values.all) return { kind: 'trust', action: 'clear', all: true }; | ||
| const serverName = rest[1]; |
There was a problem hiding this comment.
🟡 values.all is tested before rest[1], so fpx trust clear opentable-mcp --all silently ignores the named server and clears the whole fleet. Given the comment right above says clearing everything "takes saying --all rather than happening by omitting an argument", the inverse — a named server being swallowed by a stray --all — is worth a UsageError rather than a silent widening.
Minor and separate: runTrust's list branch always printJsons, ignoring the --json flag that parseCliArgs registers, unlike the other verbs.
|
🟡 Auto-review verdict: warn — The previous round's 🔴 (extension socket closing during the awaited pin read wedging the host) is fixed at the cause with a |
… scoped pins Auto-review on #213. **The important one: the signature does not bind `extensionSessionPub`.** `sessionSig` covers `(mcpNonce || extNonce)` only, so a relay that forwards the genuine hellos and the genuine signature can still substitute its own ephemeral public key in the `ready` frame, derive the same shared secret, and read and rewrite the session. Everything the receiver checks still passes, because none of it commits to the key the ECDH actually uses. That is true of the host path too, and has been since 0.4.0 — `host.ts`'s own comment anticipated the wrong half of it ("the signature won't verify because the MCP nonce differs" holds only for a MITM that terminates the MCP connection with a hello of its own; a relay that forwards the MCP's hello verbatim gets a signature over exactly the nonce the MCP expects). So the docs no longer claim this PR closes T-host-MITM. What it does is narrow it: an attacker must be positioned to relay a live session and can no longer simply BE the extension, which is what an unauthenticated `ready` allowed. Closing the rest means signing the ephemeral key, which is a wire change on both sides; filed privately per docs/SECURITY.md rather than described in a public issue. **`fpx trust clear` was broken for scoped MCPs.** `listPins` returned the pin FILE's stem as if it were a server name, and `@fetchproxy/example-mcp` is stored as `@fetchproxy_example-mcp` — which is refused as unsafe when fed back in. So clearing a scoped MCP threw, and `--all` aborted on the first one, leaving the fleet half-cleared while reporting nothing. Deletion now works from the path, `--all` continues past a failure and names what it could not clear, and the field is called `pinFile` so the next reader doesn't repeat the mistake. Also: claim the extension slot synchronously before the awaited pin read, so the check-and-set cannot interleave with a second hello; move the identity docblock back onto the function that reads identities; refuse `trust clear <name> --all` instead of silently doing the broader thing; and honour `--json` on `trust list` (defaulting to a readable line per pin, since someone running it is usually staring at a refusal). One flake fixed at the cause rather than the symptom: reading the pin on every `ready` put file I/O on the renegotiation path, widening the window in which a call is sealed with the stale session key and then rejected. The peer reads it once and updates the cache when it writes. Closes #214 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… path Closes #208. The extension has pinned the MCP since 0.2.0 — `trustedMcps`, keyed by the SHA-256 of its X25519 pub, re-pair prompt on change. Nothing pinned in the other direction. `host.ts` verified `ready.sessionSig` against the identity presented in the SAME connection, which proves the connecting party holds the key it just showed us and says nothing about whether it is the party we paired with, and `peer.ts` verified nothing at all. On loopback that asymmetry is the local trust boundary doing its job. It stops being harmless the moment the far end of the socket can be something other than a process on this machine: "whichever extension said hello" means a stolen relay credential buys a working session with every bridged MCP — every request they make, and answers of the attacker's choosing — with no prompt anywhere, because the MCP has nothing to compare against and the user's browser is not involved. So the MCP pins the extension too, in the same TOFU shape the extension uses: first contact is recorded at `<identityDir>/<server-name>.extension-trust.json` (0600), a different identity afterwards is refused with 1008 before any session exists, and the pin is written only AFTER the ready signature proves the key — claiming an identity must never be enough to become the pinned one. WHAT THE PEER PATH TURNED OUT TO BE. Not just a missing pin. A peer derived its session key from `ready.extensionSessionPub` while verifying nothing, so a concentrator could put its own ephemeral pub in that frame, derive the same shared secret with the peer, and read and rewrite everything the peer believed was end-to-end encrypted — the exact attack `T-host-MITM` says is closed. It was closed for the host's own session in 0.4.0 and never for peers. The host now relays the extension's hello to peers (additive; old peers ignore it) and peers verify the same signature before deriving. docs/SECURITY.md says so plainly rather than quietly fixing it. One compatibility seam, deliberate: hosts before this relay no hello, so a new peer behind an old host has nothing to verify against. It warns loudly and proceeds, because the port election picks the concentrator arbitrarily and refusing would break a mixed-version fleet at random. `requireExtensionIdentity` turns that into a refusal for deployments where the concentrator is not another MCP on the same laptop. The peer guarantee is only in force once every MCP on the machine has this. GETTING OUT OF IT. A re-installed extension mints a new identity and would otherwise lock every MCP out at once, so: the refusal names the exact file, `fpx trust list` shows what is pinned, `fpx trust clear <server>` drops one and `--all` drops the fleet, and `FETCHPROXY_TRUST_NEW_EXTENSION=1` re-pairs an MCP whose source you do not own — the one lever that reaches an unmodified consumer. No consumer MCP needs to change: `startHost`/`startPeer` gained required options, but those are internal, and `FetchproxyServerOpts` gained only optional ones. The reconnect tests did need changing, and the change is the point — they minted a fresh extension identity per connection, which is a browser nobody has; a real one persists in `chrome.storage.local`, and is now refused if it doesn't. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… scoped pins Auto-review on #213. **The important one: the signature does not bind `extensionSessionPub`.** `sessionSig` covers `(mcpNonce || extNonce)` only, so a relay that forwards the genuine hellos and the genuine signature can still substitute its own ephemeral public key in the `ready` frame, derive the same shared secret, and read and rewrite the session. Everything the receiver checks still passes, because none of it commits to the key the ECDH actually uses. That is true of the host path too, and has been since 0.4.0 — `host.ts`'s own comment anticipated the wrong half of it ("the signature won't verify because the MCP nonce differs" holds only for a MITM that terminates the MCP connection with a hello of its own; a relay that forwards the MCP's hello verbatim gets a signature over exactly the nonce the MCP expects). So the docs no longer claim this PR closes T-host-MITM. What it does is narrow it: an attacker must be positioned to relay a live session and can no longer simply BE the extension, which is what an unauthenticated `ready` allowed. Closing the rest means signing the ephemeral key, which is a wire change on both sides; filed privately per docs/SECURITY.md rather than described in a public issue. **`fpx trust clear` was broken for scoped MCPs.** `listPins` returned the pin FILE's stem as if it were a server name, and `@fetchproxy/example-mcp` is stored as `@fetchproxy_example-mcp` — which is refused as unsafe when fed back in. So clearing a scoped MCP threw, and `--all` aborted on the first one, leaving the fleet half-cleared while reporting nothing. Deletion now works from the path, `--all` continues past a failure and names what it could not clear, and the field is called `pinFile` so the next reader doesn't repeat the mistake. Also: claim the extension slot synchronously before the awaited pin read, so the check-and-set cannot interleave with a second hello; move the identity docblock back onto the function that reads identities; refuse `trust clear <name> --all` instead of silently doing the broader thing; and honour `--json` on `trust list` (defaulting to a readable line per pin, since someone running it is usually staring at a refusal). One flake fixed at the cause rather than the symptom: reading the pin on every `ready` put file I/O on the renegotiation path, widening the window in which a call is sealed with the stale session key and then rejected. The peer reads it once and updates the cache when it writes. Closes #214 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c6208ff to
70bd76f
Compare
|
Claude finished @chrischall's task in 4m 54s —— View job Review — PR #213: pin the extension's identity, verify it on the peer path
Previous roundBoth 🔴 findings are addressed, and the three 🟡 nits with them.
The 🔴 Important (new)Closing the extension socket during the awaited pin read wedges the host permanently — inline
🟡 Nits
Open question (not a finding)Still no Verdict: fail |
The #214 item was two things and I had fixed one: `extensionClaim` closed the check-and-set race, but the flag itself was still host-wide, kept correct only by the claim and a reset on close. Scope is a better argument than discipline — it now lives in the connection closure, so the ready handler cannot consult a decision made for a socket that has since gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| if (outcome.decision === 'replace') console.warn(outcome.message); | ||
| // Pin on first use, or replace a pin the operator chose to drop — | ||
| // but only after the ready frame proves the key (see below). | ||
| pinOnReady = outcome.decision !== 'pinned'; |
There was a problem hiding this comment.
🔴 An extension WS that closes during the awaited pin read leaves extensionWs pointing at a dead socket, and the host never accepts another extension.
extensionClaim fixed the two-hellos interleaving, but the other thing the new await at line 215 admits is a close in the same window. The close handler (line 443) can only clean up a connection that has already reached line 241-242:
ws.on('close', () => {
if (extensionClaim === ws) extensionClaim = null;
if (identified === 'extension' && extensionWs === ws) { … }Sequence:
- hello arrives →
extensionClaim = ws(207) → suspend atawait opts.extensionTrust.read()(215); - the socket drops (MV3 SW eviction right after connect,
chrome://extensionsreload, a FIN arriving behind the hello).wsemits'close'while the handler is suspended —identifiedis stillnullandextensionWsis stillnull, so the branch at 445 does nothing. Only the claim is cleared; - the read resolves, the handler resumes and runs
identified = 'extension'; extensionWs = ws;(241-242) on a socket that is already CLOSED.
ws.send() on a closed socket with no callback is swallowed, so nothing throws — the bridge is just silently dead:
- every later extension connection hits
if (extensionWs || extensionClaim)(197) and is closed with1008 'extension already connected', and nothing ever clearsextensionWsagain (grep -n "extensionWs" packages/server/src/host.ts→ assigned only at 242, nulled only at 446 behind that guard). Recovery requires restarting the MCP; onExtensionDisconnectnever fires, sorejectAllPending/resetSessionPromisenever run andsendOwnInner(499) sees a truthyextensionWsand writes into the dead socket — callers hang to their own timeout.
Pre-PR this was structurally impossible: the guard and the assignment were adjacent with no await between them (git diff origin/main...HEAD -- packages/server/src/host.ts shows the read inserted between if (extensionWs) and identified = 'extension').
The codebase already has the guard for exactly this class of gap, twenty lines below at 387 — if (extensionWs !== ws) return; after the ECDH/HKDF awaits, with a comment saying "the extension WS may close during derivation". The same check belongs after the pin read:
// The pin read yielded; the socket may be gone.
if (extensionClaim !== ws || ws.readyState !== WebSocket.OPEN) return;placed before identified = 'extension' (and mirrored on the two refusal paths, which currently ws.close() a socket that may already be closed — harmless, but the early return makes it moot).
| }); | ||
|
|
||
| it('refuses a ready whose signature does not verify — the MITM case', async () => { | ||
| // A concentrator substituting its own ephemeral pub cannot produce this |
There was a problem hiding this comment.
🟡 This comment is the claim the rest of the PR just walked back. readyFor(..., { forge: true }) sends new Uint8Array(64).fill(3) — a garbage signature, which is the concentrator that invents an extension, not the one that substitutes an ephemeral pub. The substituting concentrator relays the genuine signature and is accepted; the passing test above (derives a session when the forwarded hello signs the ready) already mints an unrelated generateX25519() ephemeral and gets a session.
Worth saying what the case actually is — "a concentrator that cannot sign as the extension is refused; one that relays a genuine signature can still swap extensionSessionPub, per docs/SECURITY.md §T-host-MITM" — so the next reader doesn't take the test as proof of the stronger property. Same for the file's opening docblock, whose closing line ("The fix is the same material in both places…") reads as though the substitution is now blocked.
|
Claude finished @chrischall's task in 3m 55s —— View job Re-review — PR #213: pin the extension's identity, verify it on the peer path
Both 🔴 findings from the last pass are properly resolved, and resolved the honest way. The overstated
The three nits also landed: One new issue, introduced by the fix to the third nit. 🔴 Important1. An extension socket that closes during the awaited pin read wedges the host permanently — inline
The window is one file read, so this is unlikely rather than routine. It's flagged because the state is unrecoverable and the symptom points away from the cause, and because a 🟡 Nits
Open question (not a finding)Still unverified on my side: this checkout has no Verdict: fail |
| // but only after the ready frame proves the key (see below). | ||
| pinOnReady = outcome.decision !== 'pinned'; | ||
| identified = 'extension'; | ||
| extensionWs = ws; |
There was a problem hiding this comment.
🔴 An extension socket that closes during the awaited pin read leaves extensionWs pointing at a dead socket forever.
extensionClaim closes the interleaving race, but it introduced a second one. On origin/main the extension branch was fully synchronous up to the assignment:
git show origin/main:packages/server/src/host.ts | sed -n '174,181p'
if (frame.type === 'hello' && frame.role === 'extension') {
if (extensionWs) {
ws.close(1008, 'extension already connected');
return;
}
identified = 'extension';
extensionWs = ws;
Now await opts.extensionTrust.read() (line 217) sits between the guard and both identified = 'extension' (243) and extensionWs = ws (244). If the socket closes while that read is in flight — MV3 eviction right after the hello, a chrome://extensions reload, the user toggling the extension — the close handler runs first, and its guard is
if (extensionClaim === ws) extensionClaim = null;
if (identified === 'extension' && extensionWs === ws) { … } // identified is still null hereso it clears the claim and nothing else. The message handler then resumes and sets identified = 'extension'; extensionWs = ws on an already-closed socket. ws.send() in the ws library no-ops on a non-OPEN socket rather than throwing (sendAfterClose only does buffered-byte accounting when no callback is passed), so the branch completes normally.
extensionWs = null has exactly one assignment site:
$ grep -n "extensionWs = null" packages/server/src/host.ts
448: extensionWs = null;
— inside the close handler that has already fired for this socket. From that point the host refuses every subsequent extension connection with 1008 'extension already connected', never fires onExtensionDisconnect, and never recovers short of restarting the MCP. Because the host holds the port, every peer on the machine is down with it, and the diagnostic ("extension already connected" while no extension is connected) points away from the cause.
The window is one file read, so this is unlikely rather than impossible — but it's a new, unrecoverable state on the path this PR added, and it's cheap to close. Either set identified = 'extension' synchronously alongside extensionClaim = ws so the existing close guard matches, or re-check liveness after the await:
if (ws.readyState !== WebSocket.OPEN) { extensionClaim = null; return; }
| // #208: authenticate the extension BEFORE deriving anything from a | ||
| // key it supplied. Without this a concentrator can put its own | ||
| // ephemeral pub here, derive the same shared secret with us, and read | ||
| // the traffic `T-host-MITM` says it cannot. |
There was a problem hiding this comment.
🟡 docs/SECURITY.md and docs/PROTOCOL.md were corrected in this PR to say the signature does not bind extensionSessionPub — but the three code comments that assert the opposite were left as-is, so the source now contradicts the threat model it points at:
- here (
peer.ts:301-303): "Without this a concentrator can put its own ephemeral pub here, derive the same shared secret with us, and read the trafficT-host-MITMsays it cannot" — it still can, with this check in place. packages/server/tests/extension-pin-peer.test.ts:178-179: "A concentrator substituting its own ephemeral pub cannot produce this signature" — it doesn't have to produce one; relaying the genuine one works. And the file's docblock (:22-28) still frames the fix as closing "the exact attackT-host-MITM… says is closed".packages/server/src/host.ts:332-335(unchanged by this PR, but the commit message for 70bd76f calls it out explicitly): "relay the real extension's bytes (signature won't verify because the MCP nonce differs)" — true only for a MITM that terminates the MCP connection with a hello of its own.
The prose fix landed; the comments are what the next reader will actually reach for.
| // at a refusal and wants to know which browser is pinned and since when, | ||
| // not to parse 32 bytes of base64. | ||
| for (const pin of pins) { | ||
| io.out(`${pin.pinFile} pinned ${pin.pinnedAt} x25519=${pin.identityX25519Pub}`); |
There was a problem hiding this comment.
🟡 --all and clear <real-server-name> both work now, and the pinFile rename makes the mismatch legible in the source. What's left is the operator-facing half: trust list prints the stem, and for a scoped MCP the stem is the one string trust clear will reject.
$ grep -n "SAFE_PLAIN\|SAFE_SCOPED" packages/server/src/identity.ts
27:const SAFE_PLAIN = /^[A-Za-z0-9._-]+$/;
28:const SAFE_SCOPED = /^@[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/;
So fpx trust list prints @fetchproxy_example-mcp, and pasting that into fpx trust clear throws unsafe serverName for identity file: …, surfacing as fpx: unexpected error (main.ts:135-138). The user has to know to guess back to @fetchproxy/example-mcp. Since this verb exists precisely for someone already staring at a refusal, printing a copy-pasteable name (reverse the @scope_name → @scope/name mapping, unambiguous for @-prefixed stems) would finish the job the rename started.
…ready gone Auto-review round three on #213. The claim I added to close a check-and-set race opened a worse hole, and the reviewer is right that it is worse: the pin read is awaited, so the socket can close inside it, and the resuming handler then set `extensionWs` to a dead connection. Its close event had already fired, so nothing would ever clear it — every later extension refused with 1008 "already connected", for the life of the process. A wedged bridge, from the guard meant to protect it. The handler now checks liveness, not just identity, after the await: a per-connection `closed` flag set the instant the socket goes, plus a `readyState` check. The test drives the exact ordering — gate the pin read, close the socket, let the server PROCESS the close, then release — and it fails without the guard. Three nits with it: - The peer tests claimed a concentrator "cannot produce this signature" when substituting its own ephemeral pub. It cannot forge one, which is what that test shows; it does not need to, because it can relay a genuine one. Wording corrected, and the residual is now pinned by a test that derives the relay's key and asserts it equals the peer's session key. It records today's behaviour and should fail the day the signature covers the ephemeral key. - `host.ts` still carried the claim three docs had already corrected. - `fpx profile remove` deleted the identity and left the pin, so a profile re-created under the same name inherited a browser identity it never met. Closes #214 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @chrischall's task in 4m 34s —— View job Review — PR #213: pin the extension's identity, verify it on the peer path
Previous roundThe 🔴 is fixed, and fixed at the level it should be. The three nits landed too: No 🔴 findings this round. 🟡 Nits
Notes (not findings)
Open questionStill no Verdict: warn |
…ng peer.ts Two items from the round-three review comment that its follow-up checklist did not carry over. `fpx trust list` printed the pin FILE's stem, so a scoped MCP listed as `@fetchproxy_example-mcp` — the exact string `fpx trust clear` refuses as an unsafe server name. The listing is what an operator copies, and they copy it at the moment they are already locked out, so it now prints the recovered server name (`@scope/name`) and a test round-trips list → clear. `--all` covered the lockout case; this closes the single-server path. And the last of the three code comments that still asserted what the docs had been corrected about: `peer.ts` claimed the check stops a concentrator substituting its own ephemeral pub. It does not — that is the residual the advisory tracks and `extension-pin-peer.test.ts` now pins in place. The comment says so, and points at both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| if (closed || ws.readyState !== WebSocket.OPEN) { | ||
| extensionClaim = null; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 The liveness guard is right, but this releases the claim unconditionally — and by the time it runs, the claim may belong to somebody else.
The close handler uses the guarded form for exactly this reason (host.ts:470):
if (extensionClaim === ws) extensionClaim = null;The unguarded form here (and on the two refusal paths at :226 and :239) can reopen the window the claim was added to close:
- ext1 hello →
extensionClaim = ws1→ suspends in the pin read; - ws1 closes → close handler clears the claim (
extensionClaim === ws1); - ext2 connects → guard at
:203passes (both slots null) →extensionClaim = ws2→ suspends in its pin read; - ws1's read resolves →
closedis true →extensionClaim = null, clobbering ws2's claim; - ext3 connects → guard passes → claims → both ws2 and ws3 resume live and each run
identified = 'extension'; extensionWs = ws.
Bounded (last writer wins the slot, no permanent wedge) and only reachable from a local process racing three connections through one file read — so a nit, not a repeat of the round-3 bug. But it is one word:
| if (closed || ws.readyState !== WebSocket.OPEN) { | |
| extensionClaim = null; | |
| return; | |
| } | |
| if (closed || ws.readyState !== WebSocket.OPEN) { | |
| if (extensionClaim === ws) extensionClaim = null; | |
| return; | |
| } |
Same for :226 and :239.
| // #208: authenticate the extension BEFORE deriving anything from a | ||
| // key it supplied. Without this, anything that could reach us could BE | ||
| // the extension with no key material at all. | ||
| // |
There was a problem hiding this comment.
🟡 The last place still carrying the claim the rest of the PR walked back.
54f5754 corrected host.ts:343-358 ("It does NOT stop a relay that forwards the real hellos and the real signature… nothing here commits to extensionSessionPub"), and the test file's docblock and case name with it. This comment sits directly above the authenticateExtension call it describes and still reads as though the check is what stops ephemeral-pub substitution:
Without this a concentrator can put its own ephemeral pub here, derive the same shared secret with us, and read the traffic
T-host-MITMsays it cannot.
With this, a concentrator relaying a genuine hello and a genuine sessionSig can still put its own ephemeral pub here — that is what extension-pin-peer.test.ts's KNOWN RESIDUAL case now asserts, and what docs/SECURITY.md §T-host-MITM now says. What the check actually buys is that the concentrator can no longer be an extension of its own invention.
chrischall
added
the
ready-to-merge
|
Claude finished @chrischall's task in 4m 57s —— View job Re-review — PR #213: pin the extension's identity, verify it on the peer path
Previous roundThe 🔴 is fixed at the cause, and fixed with a regression test that would actually have caught it.
The three 🟡s landed too:
Nothing new at 🔴 level. Three minor things, none blocking. 🟡 Nits
Open question (not a finding)Unchanged from the last two rounds: Worth noting one thing I checked but could not execute: the peer path now verifies Verdict: warn |
| // extension would be refused "already connected" until the process | ||
| // restarts. Check liveness, not just identity. | ||
| if (closed || ws.readyState !== WebSocket.OPEN) { | ||
| extensionClaim = null; |
There was a problem hiding this comment.
🟡 The liveness guard is right, but this clear is unconditional — it drops whatever claim is current, which by this point may belong to a different socket.
The close handler already released this socket's claim (if (extensionClaim === ws) extensionClaim = null;, line 470), so by the time the read resolves extensionClaim can legitimately be a newer connection:
- A's hello →
extensionClaim = A→ suspends inread(); - A closes → close handler sets
extensionClaim = null; - B's hello → guard passes (
extensionWsandextensionClaimboth null) →extensionClaim = B→ suspends in its ownread(); - A resumes, sees
closed, and runsextensionClaim = null— clearing B's claim; - C's hello now passes the guard too (
extensionWsstill null, claim cleared), so B and C both go on to runidentified = 'extension'; extensionWs = ws;— the exact interleavingextensionClaimwas introduced to prevent.
It self-heals (whichever socket ends up in extensionWs clears it on close), and it needs a close inside a file read plus two reconnects, so this is narrow rather than dangerous. But the fix is the same shape as the guard twenty lines below:
| extensionClaim = null; | |
| if (extensionClaim === ws) extensionClaim = null; |
Worth applying to the two refusal paths above (lines 226, 239) as well, for the same reason.
| */ | ||
| function serverNameFromPinFile(stem: string): string { | ||
| if (!stem.startsWith('@')) return stem; | ||
| const at = stem.indexOf('_'); |
There was a problem hiding this comment.
🟡 indexOf('_') splits on the first underscore, and the docblock's justification ("a scoped one has exactly one separator") isn't quite true — SAFE_SCOPED in identity.ts:28 admits _ on both sides of the /:
$ grep -n "SAFE_SCOPED" packages/server/src/identity.ts
28:const SAFE_SCOPED = /^@[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/;
So @my_org/tool-mcp is a legal server name, stores as @my_org_tool-mcp, and lists back as @my/org_tool-mcp. The listed string is wrong but harmlessly so — safeIdentityFileBase maps it to the same file, so clear still deletes the right pin. Purely a display defect, and the mapping is genuinely lossy in that case; the cheap version is to translate only when the stem has exactly one _, and otherwise print the stem with a note.
| if (seen) return seen; | ||
| await new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error(`no server hello for ${mcpId}`)), 5_000); | ||
| helloWaiters.push(() => { |
There was a problem hiding this comment.
🟡 The waiter is dropped, not re-queued, when a server hello for a different mcpId arrives first.
ws.on('message', …) does helloWaiters.splice(0) — draining every waiter — and each callback returns early if its own mcpId isn't in serverHellos yet. So a waiter for B that is woken by A's hello is discarded, and B's later hello finds an empty waiter list: the promise only settles when the 5 s timeout rejects with no server hello for B.
Every current caller waits on a single mcpId, so nothing is failing today. But this helper is the obvious thing to reach for the first time a pinning test involves a host plus a peer, and the failure mode is a 5-second timeout that reads like a protocol bug. Re-push the waiter when it doesn't match (or key the waiter list by mcpId).
…scoped names (#219) The three nits from #218 — the post-merge review of #213 — on a fresh branch off the squashed main. ## The claim cleanup could drop someone else's claim `extensionClaim` exists so a second extension hello cannot interleave with the awaited pin read. Its own cleanup could defeat it: the abandon and refusal paths cleared the claim unconditionally, but the close handler may have released this socket's claim already and a newer connection may hold it by now. ``` A claims → A closes (handler clears A's claim) → B claims → A's handler resumes, clears B's claim → C passes the guard → B and C both reach `extensionWs = ws` ``` Self-healing and improbable — it needs a close inside a file read plus two reconnects — but it is the exact interleaving the claim was added to prevent, so all four sites now check `=== ws` before releasing. ## The test helper's waiters were keyed by nothing `waitForServerHello` drained *every* waiter on each hello and dropped the ones whose `mcpId` didn't match, so a caller waiting on two MCPs could only settle by timing out. No current caller waits on two ids; the next person to write one would have paid for it with a 5-second mystery. Keyed by `mcpId` now. ## `serverNameFromPinFile` guessed where it cannot know `_` is legal on both sides of a scope's `/` (`SAFE_SCOPED`), so `@my_org/tool-mcp` and `@my/org_tool-mcp` share the stem `@my_org_tool-mcp` and neither is recoverable from it. My docblock claimed the mapping was unambiguous; it isn't, and printing a guess names a package that may not exist. It now translates only the unambiguous case (leading `@`, exactly one `_`) and shows the stem otherwise — and because a bare stem is the one string `clear` used to reject, `clear` now accepts a stem as well as a server name. Everything `list` prints stays usable, rather than being legible and rejected at the same time. ## Testing 1232 tests, three consecutive clean runs, `tsc -b` across all six workspaces. New test covers the ambiguous-stem path end to end: list shows the stem, does *not* show a fabricated name, and `clear` takes the stem. Closes #218 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v1.11.0...v2.0.0) (2026-08-06) ### ⚠ BREAKING CHANGES * **protocol:** bind the ephemeral key into the ready signature ([#222](#222)) ### Features * **protocol:** add write_cookies, the one verb that can repair a rotated session ([#211](#211)) ([b2557c2](b2557c2)) * **protocol:** bind the ephemeral key into the ready signature ([#222](#222)) ([c13aeed](c13aeed)) * **server:** let a request name the tab that relays it ([#207](#207)) ([c5d3f4d](c5d3f4d)) * **server:** pin the extension's identity, and verify it on the peer path ([#213](#213)) ([0eeced7](0eeced7)) ### Bug Fixes * **cli:** let a real filesystem error be itself, not "no extension pin" ([#221](#221)) ([c87a864](c87a864)), closes [#220](#220) * **cli:** validate --via-tab before connecting, like the request URL ([#210](#210)) ([959fcc5](959fcc5)) * **extension:** reattach the write_cookies doc block, and name the writable cookies as writable ([#215](#215)) ([2730c4a](2730c4a)) * **extension:** use the guarded caps local for the cookie heading ([#217](#217)) ([f95c832](f95c832)) * **server:** release only our own extension claim, and stop guessing scoped names ([#219](#219)) ([3d90a64](3d90a64)), closes [#218](#218) * **server:** type no-tab rejections so they stop reading as version mismatches ([#205](#205)) ([dc30bd9](dc30bd9)) ### Refactor * **server:** drop the concatBytes imports the signature change orphaned ([#224](#224)) ([4985ba7](4985ba7)), closes [#223](#223) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Closes #208.
What was missing
The extension has pinned the MCP since 0.2.0 —
trustedMcps, keyed by the SHA-256 of its X25519 pub, re-pair prompt on change. Nothing pinned in the other direction:host.tsverifiedready.sessionSigagainst the identity presented in the same connection — proof that the connecting party holds the key it just showed us, and no evidence at all that it is the party we paired with;peer.tsverified nothing.On loopback that is the local trust boundary doing its job. It stops being harmless the moment the far end of that socket can be something other than a process on this machine: "whichever extension said hello" means a stolen relay credential buys a working session with every bridged MCP — seeing every request they make (URLs, headers, bodies) and answering with content of its choosing — and there is no prompt anywhere, because the MCP has nothing to compare against and the user's browser is never involved.
What this does
The mirror of
trustedMcps, in the same TOFU shape:<identityDir>/<server-name>.extension-trust.json, mode 0600, written only after the ready signature proves the key — claiming an identity must never be enough to become the pinned one;1008before any session exists;The peer path was worse than the issue said
Not just a missing pin. A peer derived its session key from
ready.extensionSessionPubwhile verifying nothing — so a concentrator could put its own ephemeral public key in that frame, derive the same shared secret with the peer, and read and rewrite everything the peer believed was end-to-end encrypted, forwarding to the real extension to keep the illusion. That is precisely the attackT-host-MITMclaims is closed. It was closed for the host's own session in 0.4.0 and never for peers.Fixed with material 0.4.0 already defined: the host relays the extension's hello to peers (additive — old peers ignore the frame), and peers verify
Ed25519Sign(extPriv, ownHelloNonce || extHelloNonce)before deriving.This narrows T-host-MITM; it does not close it, and the docs now say so. Auto-review caught that
sessionSigcovers the two nonces and notextensionSessionPub— so a relay forwarding genuine hellos and a genuine signature can still substitute its own ephemeral key and derive the same session key. That is true of the host path too, and has been since 0.4.0. What this PR buys is that an attacker must be positioned to relay a live session and can no longer simply be the extension, which is what an unauthenticatedreadyallowed. Signing the ephemeral key is a wire change on both sides and is filed as a private security advisory perdocs/SECURITY.md's reporting policy.One deliberate compatibility seam: hosts before this relay no hello, so a new peer behind an old host has nothing to verify against. It warns loudly and proceeds, because the port election picks the concentrator arbitrarily and a strict default would break a mixed-version fleet at random.
requireExtensionIdentity: trueturns the warning into a refusal for deployments where the concentrator is not simply another MCP on the same laptop. The peer guarantee is only fully in force once every MCP on the machine has this version.Getting out of a pin
A re-installed extension mints a new identity and would otherwise lock every MCP out at once:
fpx trust listfpx trust clear <server-name>fpx trust clear --allFETCHPROXY_TRUST_NEW_EXTENSION=1The refusal message names the exact file, resolved from the MCP's real
identityDirrather than the default (an early version pointed at~/.fetchproxy/...while the MCP was using a different directory — caught by a test's own error output).Compatibility
No consumer MCP needs to change.
startHost/startPeergained required options, but neither is exported;FetchproxyServerOptsgained only optional ones (allowNewExtensionIdentity,requireExtensionIdentity).The reconnect integration tests did need changing, and that change is the point: they minted a fresh extension identity per connection, which is a browser nobody has — a real one persists in
chrome.storage.local. They now reuse one identity across a reconnect, and are refused if they don't.Testing
86 files / 1188 tests green on this branch, typecheck and build clean. New coverage: the trust store (TOFU, half-match refusal, 0600, corrupt-file refusal, atomic replace), the host path (pins after proof, does NOT pin a forged signature, recognises a returning browser, refuses a stranger, honours the override, refuses an unreadable pin), the peer path (verifies, refuses a MITM'd ready, refuses an unpinned identity, warns behind an old host, refuses when required), and the CLI.
Follows the mcp-host browser-bridge experiment (chrischall/mcp-host#166), which is what surfaced this; that work gates itself on this shipping and being consumed.
Closes #214 — five auto-review findings addressed, including a scoped-MCP bug in
fpx trust clear(--allaborted partway, leaving a fleet half-cleared) and a 1-in-5 flake I introduced by putting file I/O on the renegotiation path.