From 47c2d62c1e71ebd9297e4a1778144a1cd294e330 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Mon, 27 Jul 2026 20:44:03 -0400 Subject: [PATCH 1/2] fix(mcp): tighten the allow-list name check; never copy a non-object policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- extensions/levelcode-ai/extension.js | 22 +++++++++- extensions/levelcode-ai/mcpConfig.js | 36 +++++++++------- .../levelcode-ai/test/mcpConfig.test.js | 41 ++++++++++++++++++- 3 files changed, 81 insertions(+), 18 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 7303432..494a408 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -775,6 +775,11 @@ let approvalSeq = 0; * 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. */ +/** A real map — not an array, not null, not a boxed primitive. Guards what we copy out of settings. */ +function isPlainObject(v) { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + async function mcpAllowAlways(name) { // isNamespacedToolName owns the rule (mcpConfig.js), rather than a second regex here: this used to // hand-roll one that required a `__` separator, which REJECTED names namespaceToolName legitimately @@ -789,7 +794,13 @@ async function mcpAllowAlways(name) { // safeCopy, not Object.assign: the existing value comes from the user's settings.json, where a // literal "__proto__" key survives JSON.parse as a real own property. Object.assign would hand it // to the prototype setter instead of copying it; safeCopy drops the unsafe keys outright. - const cur = safeCopy(userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {}); + // + // The plain-object check in front of it matters just as much. userScopedSetting hands back + // whatever is in settings.json, of whatever type — and a policy that is accidentally a STRING + // would have safeCopy faithfully copy its character indices ({"0":"a","1":"l",…}) and then write + // that back as the user's policy, destroying it. A malformed value is discarded, not migrated. + const stored = userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}); + const cur = safeCopy(isPlainObject(stored) ? stored : {}); if (cur[name] === 'allow') { return; } cur[name] = 'allow'; await cfg.update('mcp.toolPolicy', cur, vscode.ConfigurationTarget.Global); @@ -1390,7 +1401,14 @@ class ChatViewProvider { case 'stop': dbg('stop.clicked', { running: commandStops.size }); for (const [, stop] of commandStops) { try { stop(); } catch (e) { /* gone */ } } if (abort) { abort.abort(); } clearApprovals(); clearQuestions(); break; case 'stopCommand': { dbg('stopCommand', { id: msg.id }); const s = commandStops.get(msg.id); if (s) { try { s(); } catch (e) { /* gone */ } } break; } case 'approvalResponse': { - // Persisting "Always allow" should not block the approved tool call. + // "Always allow" persists the tool to the allow-list so a FUTURE run skips the prompt. It + // only ever adds an `allow` (never a broadening default), and the webview offers the button + // only for non-destructive tools — mcpAllowAlways re-checks regardless. + // + // Deliberately not awaited: this call is already approved by the click, and the write only + // affects later runs, so blocking the tool on a settings round-trip buys nothing. Failures + // are swallowed inside mcpAllowAlways and logged; the .catch here is belt-and-braces so a + // rejection can never surface as an unhandled one. if (msg.approved && msg.remember && msg.mcpName) { Promise.resolve(mcpAllowAlways(msg.mcpName)).catch(() => { /* best-effort */ }); } diff --git a/extensions/levelcode-ai/mcpConfig.js b/extensions/levelcode-ai/mcpConfig.js index a0e73e8..4aa24b8 100644 --- a/extensions/levelcode-ai/mcpConfig.js +++ b/extensions/levelcode-ai/mcpConfig.js @@ -226,27 +226,33 @@ function namespaceToolName(server, tool) { * It lives here, beside the function whose output it describes, because the caller was hand-rolling its * own regex, and two copies of one naming rule is how they drift apart. * - * Deliberately does NOT require the `__` separator, however much the `server__tool` shape invites it. - * When a server's name alone reaches the cap, truncation cuts INSIDE that first segment and the - * hash-tagged result carries no separator at all: + * namespaceToolName emits exactly two SHAPES, and this accepts those two and nothing else: + * + * 1. `server__tool` — the separator survives whenever the joined name fits the cap. + * 2. `<57 chars>_<6-char hash>` — the truncated form, always exactly MAX_TOOL_NAME long. + * + * Shape 2 is why a plain "must contain `__`" test is wrong: when a server's name ALONE reaches the cap, + * the cut lands inside that first segment and the result carries no separator at all — * * namespaceToolName('s'.repeat(70), 'tool') -> 'sss…sss_a1b2c3' // 64 chars, no '__' * - * Requiring it would reject a name this module itself produced, and "Always allow" would then silently - * do nothing for that server. The alphabet and the length are the real guarantee — they are what makes a - * name safe to write into settings — so those are what this checks. + * — so requiring one rejects a name this module itself produced, and "Always allow" then silently does + * nothing for that server. Checking the two shapes keeps that case legal while still refusing a bare + * `read_file` or `x`.repeat(64), which namespacing can never emit and which would only sit inert in the + * policy map. * - * UNSAFE_KEYS is then rejected EXPLICITLY. The separator requirement used to exclude `__proto__` by - * accident (it has no non-underscore character before its `__`); dropping that requirement takes the - * accident with it, since underscores are otherwise legal. Stating it outright means the protection no - * longer depends on an unrelated rule staying a certain shape. + * UNSAFE_KEYS is rejected EXPLICITLY rather than left to fall out of the shape rules, so the protection + * does not depend on an unrelated rule keeping a particular form. */ +const HASH_TAGGED = /_[0-9a-z]{6}$/; // shortHash is base36, lower-case, padded to 6 + function isNamespacedToolName(name) { - return typeof name === 'string' - && name.length > 0 - && name.length <= MAX_TOOL_NAME - && UNSAFE_KEYS.indexOf(name) === -1 - && /^[A-Za-z0-9_-]+$/.test(name); + if (typeof name !== 'string') { return false; } + if (name.length === 0 || name.length > MAX_TOOL_NAME) { return false; } + if (UNSAFE_KEYS.indexOf(name) !== -1) { return false; } + 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 } /** diff --git a/extensions/levelcode-ai/test/mcpConfig.test.js b/extensions/levelcode-ai/test/mcpConfig.test.js index 1d999a2..c92df48 100644 --- a/extensions/levelcode-ai/test/mcpConfig.test.js +++ b/extensions/levelcode-ai/test/mcpConfig.test.js @@ -566,17 +566,56 @@ test('PERSIST: accepts every name namespaceToolName can produce', () => { assert.ok(M.isNamespacedToolName(noSeparator)); }); +test('PERSIST: every name namespaceToolName can emit validates — swept, not sampled', () => { + // isNamespacedToolName is deliberately strict, and the failure mode of being too strict is SILENT: + // "Always allow" writes nothing and the user is simply asked again forever. A handful of examples + // cannot cover the boundary where truncation starts, so sweep both segment lengths across it. + let checked = 0; + let sawTruncated = 0; + let sawNoSeparator = 0; + + for (let s = 1; s <= 80; s++) { + for (const t of [1, 2, 7, 30, 63, 64, 90]) { + const name = M.namespaceToolName('s'.repeat(s), 't'.repeat(t)); + assert.ok(LEGAL.test(name), 'precondition: emitted name must be provider-legal: ' + name); + assert.ok(M.isNamespacedToolName(name), + 'rejected a name namespaceToolName produced (server=' + s + ', tool=' + t + '): ' + name); + checked++; + if (name.length === M.MAX_TOOL_NAME) { sawTruncated++; } + if (!name.includes('__')) { sawNoSeparator++; } + } + } + + // Assert the sweep actually reached the interesting regions, so it cannot quietly become vacuous. + assert.ok(checked > 500, 'swept a meaningful space'); + assert.ok(sawTruncated > 0, 'the sweep must include truncated names'); + assert.ok(sawNoSeparator > 0, 'the sweep must include the separator-less truncation case'); +}); + test('PERSIST: rejects prototype-pollution keys, junk, and unbounded names', () => { for (const bad of ['__proto__', 'constructor', 'prototype']) { assert.ok(!M.isNamespacedToolName(bad), bad + ' must never become a settings key'); } assert.ok(!M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME + 1)), 'must be bounded by MAX_TOOL_NAME'); - assert.ok(M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME)), 'the cap itself is legal'); for (const bad of ['', 'has space', 'semi;colon', 'quote"', 'slash/es', null, undefined, 42, {}, []]) { assert.ok(!M.isNamespacedToolName(bad), 'must reject ' + JSON.stringify(bad)); } }); +test('PERSIST: rejects safe-looking names that namespacing can never emit', () => { + // Length and alphabet alone are not the contract. These are all "safe" strings, but none can come + // out of namespaceToolName, so none belongs in the tool-policy map — an entry like `read_file` would + // just sit there inert, looking like it did something. + assert.ok(!M.isNamespacedToolName('read_file'), 'a built-in name is not an MCP tool name'); + assert.ok(!M.isNamespacedToolName('abc'), 'no separator, not the truncated shape'); + assert.ok(!M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME)), + 'exactly at the cap but with no hash tag — truncation always appends one'); + assert.ok(!M.isNamespacedToolName('x'.repeat(57) + '_ABCDEF'), + 'the hash tag is lower-case base36; upper-case is not a shape this module emits'); + assert.ok(!M.isNamespacedToolName('short_a1b2c3'), + 'a hash-looking tail only counts at exactly MAX_TOOL_NAME, which is the only way truncation ends'); +}); + test('PERSIST: safeCopy drops the keys that reach the prototype setter', () => { // JSON.parse creates a REAL own __proto__ key, which is how one arrives from settings.json. const fromSettings = JSON.parse('{"gh__list":"allow","__proto__":"allow","constructor":"allow"}'); From bb1c154c6c3aeb13e957407746117e75527faa90 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Mon, 27 Jul 2026 21:19:35 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(mcp):=20match=20the=20code=20to=20its?= =?UTF-8?q?=20own=20comments=20=E2=80=94=20shape=201=20and=20isPlainObject?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- extensions/levelcode-ai/extension.js | 16 ++++++++++-- extensions/levelcode-ai/mcpConfig.js | 17 ++++++++++--- .../levelcode-ai/test/mcpConfig.test.js | 25 +++++++++++++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 494a408..ce1918e 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -775,9 +775,21 @@ let approvalSeq = 0; * 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. */ -/** A real map — not an array, not null, not a boxed primitive. Guards what we copy out of settings. */ +/** + * A JSON-shaped map, and nothing else — guards what we copy out of settings. + * + * The prototype check is what makes that true. `typeof v === 'object' && !Array.isArray(v)` is the + * reflex version and it is wrong: `new String('x')`, `new Date()` and `new Map()` all pass it, and the + * first would then have its character indices copied into the policy. Settings deserialize to plain + * objects, so anything with another prototype did not come from there and is not a policy. + * + * A null prototype is accepted: `Object.create(null)` is still a plain map, and some JSON handling + * produces one deliberately to avoid the `__proto__` trap. + */ function isPlainObject(v) { - return !!v && typeof v === 'object' && !Array.isArray(v); + if (!v || typeof v !== 'object' || Array.isArray(v)) { return false; } + const proto = Object.getPrototypeOf(v); + return proto === Object.prototype || proto === null; } async function mcpAllowAlways(name) { diff --git a/extensions/levelcode-ai/mcpConfig.js b/extensions/levelcode-ai/mcpConfig.js index 4aa24b8..6ff6aaf 100644 --- a/extensions/levelcode-ai/mcpConfig.js +++ b/extensions/levelcode-ai/mcpConfig.js @@ -244,15 +244,24 @@ function namespaceToolName(server, tool) { * UNSAFE_KEYS is rejected EXPLICITLY rather than left to fall out of the shape rules, so the protection * does not depend on an unrelated rule keeping a particular form. */ -const HASH_TAGGED = /_[0-9a-z]{6}$/; // shortHash is base36, lower-case, padded to 6 +const LEGAL_NAME = /^[A-Za-z0-9_-]+$/; +// Shape 1. At least one character on EACH side of a separator, because sanitizeSegment never returns +// empty — so `abc__` and `__abc` are not names this module can emit. +// +// A regex rather than the obvious `indexOf('__') > 0` test, and that difference is not cosmetic: a +// server legitimately NAMED `__a` yields `__a__b`, whose FIRST separator sits at index 0. The +// index test rejects it; backtracking here finds the separator at index 3 and accepts. +const SHAPE_NAMESPACED = /^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/; +// Shape 2. shortHash is base36, lower-case, padded to 6, and truncation always lands exactly at the cap. +const SHAPE_TRUNCATED = /_[0-9a-z]{6}$/; function isNamespacedToolName(name) { if (typeof name !== 'string') { return false; } if (name.length === 0 || name.length > MAX_TOOL_NAME) { return false; } if (UNSAFE_KEYS.indexOf(name) !== -1) { return false; } - 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 + if (!LEGAL_NAME.test(name)) { return false; } + return SHAPE_NAMESPACED.test(name) + || (name.length === MAX_TOOL_NAME && SHAPE_TRUNCATED.test(name)); } /** diff --git a/extensions/levelcode-ai/test/mcpConfig.test.js b/extensions/levelcode-ai/test/mcpConfig.test.js index c92df48..d75d04a 100644 --- a/extensions/levelcode-ai/test/mcpConfig.test.js +++ b/extensions/levelcode-ai/test/mcpConfig.test.js @@ -574,8 +574,16 @@ test('PERSIST: every name namespaceToolName can emit validates — swept, not sa let sawTruncated = 0; let sawNoSeparator = 0; + // Segment alphabets include a leading-underscore server, which is the case a naive shape-1 check + // (`indexOf('__') > 0`) silently rejects. + const SERVER_CHARS = ['s', '_', '-']; for (let s = 1; s <= 80; s++) { for (const t of [1, 2, 7, 30, 63, 64, 90]) { + for (const ch of SERVER_CHARS) { + const nm = M.namespaceToolName(ch.repeat(s), 't'.repeat(t)); + assert.ok(M.isNamespacedToolName(nm), + 'rejected an emitted name (server=' + JSON.stringify(ch.repeat(Math.min(s, 4))) + '…×' + s + ', tool=' + t + '): ' + nm); + } const name = M.namespaceToolName('s'.repeat(s), 't'.repeat(t)); assert.ok(LEGAL.test(name), 'precondition: emitted name must be provider-legal: ' + name); assert.ok(M.isNamespacedToolName(name), @@ -614,6 +622,23 @@ test('PERSIST: rejects safe-looking names that namespacing can never emit', () = 'the hash tag is lower-case base36; upper-case is not a shape this module emits'); assert.ok(!M.isNamespacedToolName('short_a1b2c3'), 'a hash-looking tail only counts at exactly MAX_TOOL_NAME, which is the only way truncation ends'); + + // sanitizeSegment never returns empty, so a separator always has something on both sides. + assert.ok(!M.isNamespacedToolName('abc__'), 'nothing after the separator'); + assert.ok(!M.isNamespacedToolName('__abc'), 'nothing before the separator'); + assert.ok(!M.isNamespacedToolName('__'), 'separator alone'); +}); + +test('PERSIST: a server whose NAME starts with underscores still validates', () => { + // The trap in tightening shape 1: the obvious `indexOf('__') > 0` test rejects this, because the + // FIRST separator sits at index 0 — but it is a name this module really emits, and rejecting it + // would silently break "Always allow" for that server. + const emitted = M.namespaceToolName('__a', 'b'); + assert.strictEqual(emitted, '__a__b', 'precondition: this input really does produce a leading __'); + assert.ok(M.isNamespacedToolName(emitted), 'a leading-underscore server name is legitimate'); + + assert.ok(M.isNamespacedToolName(M.namespaceToolName('_', '_')), 'both segments bare underscores'); + assert.ok(M.isNamespacedToolName(M.namespaceToolName('a', '__b')), 'tool starting with the separator'); }); test('PERSIST: safeCopy drops the keys that reach the prototype setter', () => {