Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): tighten the allow-list name check; never copy a non-object policy - #56
Conversation
…policy Follow-up review on #41, after merge. Three findings, all valid. 1. isNamespacedToolName was too permissive (and its doc overclaimed). It asked "could this have come out of namespaceToolName?" but only checked the alphabet and the length, so it accepted `abc`, `read_file`, and `'x'.repeat(64)` — none of which namespacing can emit. Such a key would sit inert in mcp.toolPolicy looking like it did something. It now accepts exactly the two shapes namespaceToolName emits: 1. `server__tool` — whenever the joined name fits the cap 2. `<57 chars>_<6-char base36 hash>` — the truncated form, always exactly 64 Shape 2 is why "must contain __" is still wrong: when a server's name ALONE reaches the cap, the cut lands inside that first segment and no separator survives. That case must stay legal or "Always allow" silently does nothing for that server — the bug the previous round fixed. Because being too strict fails SILENTLY, the guard is now swept rather than sampled: 560 (server, tool) length pairs across the truncation boundary must all validate, and the test asserts it actually reached the truncated and separator-less regions so it cannot go vacuous. 2. A non-object mcp.toolPolicy would be copied key-by-key. userScopedSetting returns whatever is in settings.json, of whatever type. A policy that is accidentally a STRING would have safeCopy faithfully copy its character indices — {"0":"a","1":"l",…} — and write that back as the policy, destroying it. A malformed value is now discarded, not migrated. 3. The blocking await on mcpAllowAlways was already fixed in f247f1a. Verified it is correct; restored the security rationale that commit dropped (the button only ever adds an `allow`, is offered only for non-destructive tools, and is re-checked regardless) and recorded why it is deliberately not awaited. 49 mcpConfig cases; 23 suites, 0 failures.
There was a problem hiding this comment.
Pull request overview
This PR tightens MCP allow-list persistence by making tool-name validation match the actual output shapes of namespaceToolName, and by preventing malformed mcp.toolPolicy values from being migrated into corrupted objects when persisting “Always allow”.
Changes:
- Strengthens
isNamespacedToolNameto distinguish real namespaced/truncated MCP tool names from “safe-looking” but inert strings (with expanded adversarial + sweep coverage). - Prevents non-object
mcp.toolPolicyvalues (e.g. a string) from being copied into a bogus index-map during “Always allow”. - Restores security/behavior rationale around fire-and-forget persistence of the allow-list write.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| extensions/levelcode-ai/test/mcpConfig.test.js | Adds swept + adversarial tests to pin the intended tool-name contract and rejection cases. |
| extensions/levelcode-ai/mcpConfig.js | Tightens tool-name allow-list key validation to match the two emitted shapes. |
| extensions/levelcode-ai/extension.js | Guards allow-list persistence against malformed settings values; clarifies rationale for non-blocking persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isPlainObject(v) { | ||
| return !!v && typeof v === 'object' && !Array.isArray(v); | ||
| } |
There was a problem hiding this comment.
Correct — fixed in bb1c154. The comment claimed more than the code did, which is the same mistake this PR was already fixing elsewhere.
Verified before changing it:
new String("x") -> true <- doc said "not a boxed primitive"
new Date() -> true
new Map() -> true
The boxed string is the one that actually bites: its character indices would be copied into mcp.toolPolicy — precisely the corruption this guard was added for one round earlier.
Now prototype-checked:
new String / Date / Map / RegExp -> false
{} / Object.create(null) -> true
[] / null -> false
Object.create(null) is accepted deliberately — it is still a plain map, and some JSON handling produces one on purpose to dodge the __proto__ trap.
| if (!/^[A-Za-z0-9_-]+$/.test(name)) { return false; } | ||
| return name.indexOf(NAME_SEPARATOR) !== -1 // shape 1 | ||
| || (name.length === MAX_TOOL_NAME && HASH_TAGGED.test(name)); // shape 2 |
There was a problem hiding this comment.
Right — fixed in bb1c154. sanitizeSegment never returns empty, so a separator always has something on both sides, and abc__ / __abc are not names this module can emit.
Worth recording how the obvious fix fails, because it is the third variation of the same trap in this file. indexOf('__') > 0 looks correct and is not:
namespaceToolName("__a", "b") -> "__a__b"
A server legitimately named __a produces a name whose first separator sits at index 0, so the index test rejects it — and "Always allow" silently stops working for that server. Exactly the failure mode as the original __-required regex, just reached from the other side.
An anchored regex backtracks to the separator at index 3 and accepts it, so shape 1 is now:
/^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/Behaviour after the change:
abc__ false <- your case
__abc false <- your case
__ false
__a__b true <- leading-underscore SERVER, must stay legal
github__foo__bar true <- multiple separators
sss…sss_a1b2c3 true <- truncated, no separator at all (shape 2)
I checked the __a__b case before writing the fix rather than after, having twice now shipped a tightening that quietly rejected a legitimate name. The sweep also runs every server length over 's', '_' and '-' now, so that case is covered by construction instead of by one hand-picked example.
50 mcpConfig cases; 23 suites, 0 failures.
Both review findings on #56 were the same failure: a comment claiming more than the code did. 1. isPlainObject said "not a boxed primitive" and accepted them. `!!v && typeof v === 'object' && !Array.isArray(v)` is the reflex version, and `new String('x')`, `new Date()` and `new Map()` all pass it. The first is the one that bites: its character indices would be copied into mcp.toolPolicy — the exact corruption the guard was added to prevent, one round earlier. Now checks the prototype. A null prototype is accepted deliberately: Object.create(null) is still a plain map, and some JSON handling produces one on purpose to dodge the __proto__ trap. 2. Shape 1 accepted `abc__` and `__abc`. sanitizeSegment never returns empty, so a separator always has something on both sides; those two are not names this module can emit. The obvious fix — `indexOf('__') > 0` — is WRONG, and quietly so. A server legitimately named `__a` yields `__a__b`, whose FIRST separator is at index 0: namespaceToolName('__a', 'b') -> '__a__b' The index test rejects it and "Always allow" silently stops working for that server, which is the same class of bug as the original `__`-required regex. An anchored regex backtracks to the separator at index 3 and accepts. Verified before writing the fix, not after. The sweep now runs each server length over 's', '_' and '-' so the leading-underscore case is covered by construction rather than by one example. 50 mcpConfig cases; 23 suites, 0 failures.
| * (Global) tier, matching the application scope the setting is declared with, so a repo can never flip | ||
| * it. Reads the current value the same user-scoped way it is read at run start. Idempotent, and refuses | ||
| * a tool name that is not a namespaced server__tool to avoid writing junk from a malformed message. |
Uh oh!
There was an error while loading. Please reload this page.
Follow-up review on #41, after merge. Three findings, all valid.
1.
isNamespacedToolNamewas too permissive, and its doc overclaimedIt asked "could this have come out of
namespaceToolName?" but only checked the alphabet and the length — so it acceptedabc,read_file, and'x'.repeat(64), none of which namespacing can emit. Such a key would sit inert inmcp.toolPolicy, looking like it had done something.It now accepts exactly the two shapes
namespaceToolNameemits:server__tool— whenever the joined name fits the cap<57 chars>_<6-char base36 hash>— the truncated form, always exactly 64 charsShape 2 is why "must contain
__" is still the wrong test. When a server's name alone reaches the cap, the cut lands inside that first segment and no separator survives:That case has to stay legal or "Always allow" silently does nothing for that server — the bug the previous round fixed. So this tightens without reintroducing it.
Being too strict fails silently, so the guard is swept rather than sampled: 560 (server, tool) length pairs across the truncation boundary must all validate, and the test asserts the sweep actually reached the truncated and separator-less regions, so it cannot quietly go vacuous.
2. A non-object
mcp.toolPolicywould be copied key-by-keyuserScopedSettingreturns whatever is insettings.json, of whatever type. A policy that is accidentally a string would havesafeCopyfaithfully copy its character indices —{"0":"a","1":"l",…}— and write that back as the user's policy, destroying it. A malformed value is now discarded rather than migrated.3. The blocking
await— already fixed, verifiedf247f1alanded this before merge. I checked it rather than assuming: fire-and-forget with a.catch()is right, since the call is already approved by the click and the write only affects future runs.That commit did drop the security rationale from the comment, so I restored it — the button only ever adds an
allow, is offered only for non-destructive tools, andmcpAllowAlwaysre-checks regardless — plus a note on why it is deliberately not awaited.Verification
49 mcpConfig cases; 23 suites, 0 failures.New negative coverage is aimed squarely at the finding:
read_file,abc, at-cap-without-a-hash-tag, an upper-case hash tail, and a hash-looking tail on a short name are all rejected — while every sweptnamespaceToolNameoutput is accepted.Related
#55 (S4b, the G1 launch gate) is rebased onto develop and retargeted. The two are independent; either order works, and whichever lands second needs a trivial rebase.