diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 7303432..ce1918e 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -775,6 +775,23 @@ 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 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) { + 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) { // 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 +806,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 +1413,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..6ff6aaf 100644 --- a/extensions/levelcode-ai/mcpConfig.js +++ b/extensions/levelcode-ai/mcpConfig.js @@ -226,27 +226,42 @@ 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 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) { - 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 (!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 1d999a2..d75d04a 100644 --- a/extensions/levelcode-ai/test/mcpConfig.test.js +++ b/extensions/levelcode-ai/test/mcpConfig.test.js @@ -566,17 +566,81 @@ 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; + + // 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), + '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'); + + // 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', () => { // 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"}');