From f8d542a3875d6255728ca95d802d58f16d995340 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 24 Jul 2026 14:09:18 -0400 Subject: [PATCH 1/3] =?UTF-8?q?feat(mcp):=20per-call=20approval=20card=20?= =?UTF-8?q?=E2=80=94=20S4a=20(lifts=20S3's=20allow-list-only=20wall)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 REFUSED any MCP tool the user hadn't hand-added to levelcode.ai.mcp.toolPolicy. That was the safe-but-unusable placeholder; S4 replaces it with an actual prompt. Now a tool that isn't allow-listed shows a card — server · tool · the real arguments — with Skip / Allow once / Always allow. "Always allow" writes the tool to the allow-list (Global tier, matching the setting's application scope), so future runs skip the prompt: the allow-list is still the ONE thing that grants 'allow' (G3). Autopilot does not relax any of this — an MCP tool is third-party code. Security properties, all verified end-to-end against the S2 fixture server (five paths: allow-listed runs silently; un-listed prompts→runs on Allow; un-listed prompts→NOT run on Skip; destructive prompts even when allow-listed and offers no "always"; no-webview refuses): - A destructive tool ALWAYS prompts (classifyMcpTool tightens on it) and the card hides "Always allow" — offering it would be a button that does nothing, since a destructive tool can never be allow-listed. canAllowAlways is derived from the same annotation the classifier reads, so they can't disagree. - The arguments are shown IN FULL on the card (only length-capped). That is the decision — the user owns the credentials and needs to see the repo it will touch, the row it will delete. The card is ephemeral UI, never the transcript; the debug-log redaction (G4) is unchanged. - No webview to ask through (headless / tests) → falls back to S3's refusal rather than running third-party code with no way to say no. Pieces: - mcpConfig.describeMcpCall (pure): server/tool from the route with a namespaced-name fallback, bounded+pretty args (never throws on circular input), destructive/canAllowAlways. 3 new tests, mutation-checked (making a destructive tool "always-allowable" fails). - agent.js router: refuse → prompt via ctx.approve({kind:'mcp',…}). - extension.js: mcpAllowAlways writes the allow-list (user-scoped read, Global write, rejects a non-namespaced name). - chat.html: the kind:'mcp' card. VERIFIED VISUALLY — screenshotted both the normal and destructive variants: args wrap, buttons are distinct (two sharing `.approve` was a selector bug, caught and fixed), destructive shows the amber warning and no "Always allow". Also: the MCP chips used icon:'plug', which isn't a registered codicon, so addAgentLine rendered the literal word "plug" in the rail. Swapped to 'sparkle' (the 🔌 emoji still marks it MCP). The identical latent issue on the auto-preview 'globe' chip (extension.js:701) is #35's, left for a follow-up. And explainMcpRefusal's wording ("this build has no prompt") was made accurate — it's now only the non-interactive fallback. Deferred to S4b: the G1 trust-on-first-use LAUNCH gate for workspace-file (.levelcode/mcp.json) servers — they're still read-and-listed but never started. This PR is the per-call gate; the launch gate is its own reviewable slice. Verified: 24 suites, 0 failures (mcpConfig 44 cases). Co-Authored-By: Claude Opus 4.8 --- extensions/levelcode-ai/agent.js | 37 ++++++++----- extensions/levelcode-ai/extension.js | 30 +++++++++- extensions/levelcode-ai/mcpConfig.js | 52 ++++++++++++++++-- extensions/levelcode-ai/media/chat.html | 55 +++++++++++++++++++ .../levelcode-ai/test/mcpConfig.test.js | 39 +++++++++++++ 5 files changed, 194 insertions(+), 19 deletions(-) diff --git a/extensions/levelcode-ai/agent.js b/extensions/levelcode-ai/agent.js index 29acaf1..526ea7b 100644 --- a/extensions/levelcode-ai/agent.js +++ b/extensions/levelcode-ai/agent.js @@ -17,7 +17,7 @@ const providers = require('./providers/index'); const { formatVerifyFeedback, verifyOutcome, looksUnrunnable, sniffPort, sniffPreviewUrl, looksReady } = require('./verify'); const { classifyCommand, dangerLabel } = require('./commandSafety'); const { loadProjectRules } = require('./projectRules'); -const { loadServerConfig, buildAgentTools, toolCountsByServer, classifyMcpTool, explainMcpRefusal } = require('./mcpConfig'); +const { loadServerConfig, buildAgentTools, toolCountsByServer, classifyMcpTool, explainMcpRefusal, describeMcpCall } = require('./mcpConfig'); const { connectAll, getServer } = require('./mcpClient'); const SYSTEM_BASE = [ @@ -484,19 +484,30 @@ async function runTool(tu, ctx) { const route = ctx.mcpRoutes && ctx.mcpRoutes.get(tu.name); if (route) { const verdict = classifyMcpTool(tu.name, ctx.mcp && ctx.mcp.toolPolicy, route.annotations); - if (verdict.approve !== 'allow') { - // S3 deliberately ships no approval CARD (S4 owns it), so anything the user has not - // explicitly allow-listed is REFUSED rather than run — the alternative would be silently - // executing third-party code on the user's behalf with no way to say no. The explanation - // lives in mcpConfig beside the classifier so it can't drift from it (PR #31 review): a - // destructive tool is refused for a reason the allow-list cannot fix, and must not be - // described as allow-listable. - ctx.post({ type: 'agentTool', icon: 'shield', text: '🔌 mcp · refused ' + tu.name + ' — ' + verdict.reason }); - return explainMcpRefusal(tu.name, verdict); - } const server = getServer(route.server); if (!server || !server.alive) { return 'ERROR: the MCP server "' + route.server + '" is not running.'; } - ctx.post({ type: 'agentTool', icon: 'plug', text: '🔌 ' + route.server + ' · ' + route.tool }); + // S4: a call the user hasn't allow-listed is now PROMPTED, not refused. Autopilot does not relax + // this (G3) — an MCP tool is third-party code — and a server-marked-destructive tool prompts even + // when allow-listed (classifyMcpTool tightens on it). Only 'allow' skips the card. + if (verdict.approve !== 'allow') { + if (typeof ctx.approve !== 'function') { + // No webview to ask through (headless / a test harness) — fall back to S3's safe refusal + // rather than run third-party code with no way to say no. + ctx.post({ type: 'agentTool', icon: 'shield', text: '🔌 mcp · refused ' + tu.name + ' — ' + verdict.reason }); + return explainMcpRefusal(tu.name, verdict); + } + const call = describeMcpCall(tu.name, input, route); + const approved = await ctx.approve({ + kind: 'mcp', name: tu.name, server: call.server, tool: call.tool, + args: call.argsText, destructive: call.destructive, canAllowAlways: call.canAllowAlways + }); + if (!approved) { + ctx.post({ type: 'agentTool', icon: 'shield', text: '🔌 mcp · skipped ' + tu.name }); + return 'User declined to run the MCP tool "' + tu.name + '". Do NOT retry it in this run — ' + + 'continue without it, or tell the user what you needed it for.'; + } + } + ctx.post({ type: 'agentTool', icon: 'sparkle', text: '🔌 ' + route.server + ' · ' + route.tool }); return await server.call(route.tool, input); // never throws — failures come back as `ERROR: …` } return 'ERROR: unknown tool ' + tu.name; @@ -586,7 +597,7 @@ async function setupMcp(ctx, wsFolders, dbg) { const perServer = toolCountsByServer(built.routes); const summary = handles.map((h) => h.name + ' (' + (perServer.get(h.name) || 0) + ')').join(', '); dbg('mcp.ready', { servers: handles.map((h) => h.name), tools: built.tools.length, allowed }); - ctx.post({ type: 'agentTool', icon: 'plug', text: '🔌 mcp · ' + summary + ' · ' + allowed + '/' + built.tools.length + ' allow-listed' }); + ctx.post({ type: 'agentTool', icon: 'sparkle', text: '🔌 mcp · ' + summary + ' · ' + allowed + '/' + built.tools.length + ' allow-listed' }); return built; } catch (e) { dbg('mcp.failed', { error: (e && e.message) || String(e) }); diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 2c00e98..ad1f790 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -769,6 +769,28 @@ async function restoreCheckpoint(turnId) { const pendingApprovals = new Map(); let approvalSeq = 0; +/** + * Persist an MCP tool to the allow-list (the ONLY thing that grants 'allow' — G3). Writes to the USER + * (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. + */ +async function mcpAllowAlways(name) { + if (typeof name !== 'string' || !/^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/.test(name)) { + dbg('mcp.allow.reject', { name }); return; + } + try { + const cfg = aiConfig(); + const cur = userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {}; + if (cur[name] === 'allow') { return; } + await cfg.update('mcp.toolPolicy', Object.assign({}, cur, { [name]: 'allow' }), vscode.ConfigurationTarget.Global); + post({ type: 'agentTool', icon: 'check', text: '🔌 mcp · always allow ' + name }); + dbg('mcp.allow.persisted', { name }); + } catch (e) { + dbg('mcp.allow.failed', { name, error: String((e && e.message) || e) }); + } +} + /** Ask the webview to approve an action; resolves true/false. */ function requestApproval(req) { const id = String(++approvalSeq); @@ -1358,7 +1380,13 @@ class ChatViewProvider { case 'send': await handleSend(msg.text); break; 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': resolveApproval(msg.id, msg.approved); break; + case 'approvalResponse': + // "Always allow" on an MCP card persists the tool to the allow-list BEFORE resolving, so a + // future run skips the prompt. It only ever adds an ALLOW (never a broadening default), and + // the webview offers it only for non-destructive tools — mcpAllowAlways re-checks anyway. + if (msg.approved && msg.remember && msg.mcpName) { await mcpAllowAlways(msg.mcpName); } + resolveApproval(msg.id, msg.approved); + break; case 'questionsResponse': resolveQuestions(msg.id, msg.answers, msg.notes); break; case 'accountSignIn': await accountSignIn(msg.provider, msg.create); break; case 'accountSignOut': await accountSignOut(); break; diff --git a/extensions/levelcode-ai/mcpConfig.js b/extensions/levelcode-ai/mcpConfig.js index b7dc942..2503718 100644 --- a/extensions/levelcode-ai/mcpConfig.js +++ b/extensions/levelcode-ai/mcpConfig.js @@ -400,17 +400,59 @@ function classifyMcpTool(name, policy, annotations) { * @returns {string} */ function explainMcpRefusal(name, verdict) { + // Reached only when there is NO interactive approval to fall back on (a headless run, a test harness). + // With a webview present, S4 shows the per-call card instead of this message. const head = 'ERROR: the MCP tool "' + name + '" is not approved to run (' + verdict.reason + '). '; const fix = verdict.policyCanAllow - ? 'This build has no per-call approval prompt, so the only way to permit it is for the USER to add ' + ? 'No interactive approval is available here, so the only way to permit it is for the USER to add ' + '"' + name + '": "allow" to the "levelcode.ai.mcp.toolPolicy" setting. ' - : 'Such tools always require per-call approval — which this build does not yet provide — so it ' - + 'CANNOT be enabled through the allow-list. '; + : 'Such tools always require per-call approval and CANNOT be enabled through the allow-list, so ' + + 'there is no way to run it in this non-interactive context. '; return head + fix + 'Do NOT retry it in this run — continue without it, or tell the user what you needed it for.'; } +// A tool call's arguments can be large, and the approval card must not be blown open by one. See +// describeMcpCall — the card is capped, the full args still reach the server if approved. +const MAX_ARG_CHARS = 2000; + +/** Pretty, bounded JSON for the args shown on the approval card. Never throws (circular/huge input). */ +function previewArgs(args) { + if (args == null) { return ''; } + let text; + try { text = JSON.stringify(args, null, 2); } + catch { try { text = String(args); } catch { text = '[unserializable arguments]'; } } + if (text == null) { return ''; } + return text.length > MAX_ARG_CHARS ? text.slice(0, MAX_ARG_CHARS - 1) + '…' : text; +} + +/** + * Shape an MCP call for the approval card (S4) — exactly what the user reads before deciding. + * + * Unlike the debug log (G4), the arguments are shown in FULL here, only length-capped. That is not an + * oversight: the card is ephemeral UI shown to the person who owns the credentials, and seeing the real + * arguments — the repo it will touch, the id it will delete — IS the decision. Redacting them would make + * the prompt meaningless. Nothing here is persisted; the card is not the transcript. + * + * `canAllowAlways` is false for a destructive tool: a server-marked-destructive tool can never be moved + * to the allow-list (classifyMcpTool tightens on it), so the card must not offer a button that would do + * nothing. Derived from the same annotation the classifier reads, so the two cannot disagree. + * + * @param {string} name namespaced tool name (server__tool) + * @param {*} args the arguments the model produced for this call + * @param {{server?:string, tool?:string, annotations?:object}} [route] + * @returns {{server:string, tool:string, argsText:string, destructive:boolean, canAllowAlways:boolean}} + */ +function describeMcpCall(name, args, route) { + const r = route || {}; + const fallback = String(name == null ? '' : name).split(NAME_SEPARATOR); + const server = typeof r.server === 'string' && r.server ? r.server : (fallback[0] || String(name)); + const tool = typeof r.tool === 'string' && r.tool ? r.tool : (fallback.slice(1).join(NAME_SEPARATOR) || String(name)); + const destructive = !!(r.annotations && r.annotations.destructiveHint === true); + return { server, tool, argsText: previewArgs(args), destructive, canAllowAlways: !destructive }; +} + module.exports = { loadServerConfig, userScopedSetting, namespaceToolName, assignToolNames, buildAgentTools, - toolCountsByServer, classifyMcpTool, explainMcpRefusal, - BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH + toolCountsByServer, classifyMcpTool, explainMcpRefusal, describeMcpCall, + BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_ARG_CHARS, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH }; diff --git a/extensions/levelcode-ai/media/chat.html b/extensions/levelcode-ai/media/chat.html index 46b5ac7..e47857e 100644 --- a/extensions/levelcode-ai/media/chat.html +++ b/extensions/levelcode-ai/media/chat.html @@ -380,6 +380,9 @@ .tl-ask .asksub { margin-top: 3px; font-size: 11.5px; line-height: 1.5; color: var(--muted); white-space: pre-wrap; word-break: break-word; } /* recessed well for the command; capped so a long one can't push the buttons off-screen */ .tl-ask .askcode { margin-top: 10px; border-radius: 8px; background: var(--vscode-textCodeBlock-background, rgba(127,127,127,.14)); max-height: 40vh; overflow: auto; } + /* MCP call arguments — wrap rather than force horizontal scroll, and keep them monospace + tidy. */ + .tl-ask .mcpargs { margin: 0; padding: 9px 12px; white-space: pre-wrap; word-break: break-word; font-family: var(--vscode-editor-font-family, ui-monospace, monospace); font-size: 12px; line-height: 1.5; } + .tl-ask .askbtns .mcp-always { margin-left: auto; } .tl-ask .askbtns { display: flex; justify-content: space-between; align-items: center; gap: 8px; margin-top: 11px; } .tl-ask .askbtns button { cursor: pointer; border: 1px solid transparent; border-radius: 7px; padding: 5px 14px; font-size: 12px; font-weight: 500; display: inline-flex; align-items: baseline; gap: 6px; } /* transparent border keeps both buttons the same height */ @@ -1871,7 +1874,59 @@ // I being asked", which is exactly what it is for. On decision it collapses to a one-line verdict; // the live run card follows with the output. let pendingApproval = null; // { done } while a decision is awaited — Enter approves, Esc skips + // MCP tool call (S4) — its own card: server · tool · arguments, so the user sees exactly what a + // third-party tool is about to do. Args are shown in full (capped host-side): that IS the decision. + function addMcpApproval(m){ + clearEmpty(); clearStatus(); agentBubble = null; + closeGroup(); + const card = document.createElement('div'); card.className = 'tl tl-cmd tl-ask asking'; + const dangerLine = m.destructive + ? '
' + codicon('warning') + ' The server marks this tool destructive. It will always ask, even if allow-listed.
' + : ''; + const argsWell = (m.args && String(m.args).trim()) + ? '
' + esc(m.args) + '
' + : '
No arguments.
'; + // "Always allow" only when the tool CAN be allow-listed — a destructive tool can't, so we don't + // offer a button that would silently do nothing (mirrors describeMcpCall.canAllowAlways). + // "Always allow" reuses the secondary (skip-style) look but is selected by its OWN class — two + // buttons sharing `.approve` would make querySelector('.approve') ambiguous. + const always = m.canAllowAlways + ? '' + : ''; + card.innerHTML = + '
' + codicon('shield') + '
' + + '
' + + '
Run an MCP tool?
' + + '
' + esc(m.server || '') + ' · ' + esc(m.tool || '') + ' — a third-party tool, not one of LevelCode’s own.
' + + dangerLine + + argsWell + + '
' + + '' + + always + + '' + + '
' + + '
'; + log.appendChild(card); scrollIfStuck(); + const done = (approved, remember) => { + pendingApproval = null; + vscode.postMessage({ type: 'approvalResponse', id: m.id, approved, remember: !!remember, mcpName: m.name }); + card.classList.remove('asking'); + if (!approved) { card.classList.add('skipped'); } + const verb = !approved ? 'Skipped' : (remember ? 'Always allowed' : 'Approved'); + card.querySelector('.tl-body').innerHTML = + '
' + verb + '' + + '' + esc(m.server || '') + '' + esc(m.tool || '') + '' + + '' + codicon(approved ? 'check-circle' : 'circle-slash') + '
'; + forceStick(); + }; + card.querySelector('.approve').onclick = () => done(true, false); // Allow once (primary) + card.querySelector('.skip:not(.mcp-always)').onclick = () => done(false, false); + const alt = card.querySelector('.mcp-always'); if (alt) { alt.onclick = () => done(true, true); } + pendingApproval = { done }; // Enter = Allow once, Esc = Skip + } + function addApproval(m){ + if (m.kind === 'mcp') { return addMcpApproval(m); } clearEmpty(); clearStatus(); agentBubble = null; closeGroup(); // a blocking gate never hides inside a collapsed group (D4) const card = document.createElement('div'); card.className = 'tl tl-cmd tl-ask asking'; diff --git a/extensions/levelcode-ai/test/mcpConfig.test.js b/extensions/levelcode-ai/test/mcpConfig.test.js index 55e5736..f7de219 100644 --- a/extensions/levelcode-ai/test/mcpConfig.test.js +++ b/extensions/levelcode-ai/test/mcpConfig.test.js @@ -504,4 +504,43 @@ test('SOURCE: the mcp modules contain no raw control bytes', () => { } }); +// ---- 6. describeMcpCall: the approval card's content (S4) ------------------------------------- + +test('CARD: server/tool come from the route, with a namespaced-name fallback', () => { + const d = M.describeMcpCall('github__create_issue', { title: 'x' }, { server: 'github', tool: 'create_issue' }); + assert.strictEqual(d.server, 'github'); + assert.strictEqual(d.tool, 'create_issue'); + // No route → split the namespaced name on the separator rather than showing a blank card. + const f = M.describeMcpCall('github__create_issue', {}, undefined); + assert.strictEqual(f.server, 'github'); + assert.strictEqual(f.tool, 'create_issue'); +}); + +test('CARD: a destructive tool cannot be "always allowed"', () => { + // The card must not offer a button that does nothing — a destructive tool can never be allow-listed + // (classifyMcpTool tightens on it), so canAllowAlways is derived from the SAME annotation. + const d = M.describeMcpCall('gh__nuke', {}, { server: 'gh', tool: 'nuke', annotations: { destructiveHint: true } }); + assert.strictEqual(d.destructive, true); + assert.strictEqual(d.canAllowAlways, false); + const ok = M.describeMcpCall('gh__list', {}, { server: 'gh', tool: 'list' }); + assert.strictEqual(ok.destructive, false); + assert.strictEqual(ok.canAllowAlways, true); +}); + +test('CARD: arguments are shown in full but bounded, and never throw', () => { + const d = M.describeMcpCall('s__t', { path: '/etc/passwd', n: 3 }, { server: 's', tool: 't' }); + assert.ok(d.argsText.includes('/etc/passwd') && d.argsText.includes('"n": 3'), 'the user must SEE the real args'); + + // Over the cap → truncated with an ellipsis, not dropped and not unbounded. + const big = M.describeMcpCall('s__t', { blob: 'z'.repeat(5000) }, { server: 's', tool: 't' }); + assert.ok(big.argsText.length <= M.MAX_ARG_CHARS, 'args preview must obey MAX_ARG_CHARS'); + assert.ok(big.argsText.endsWith('…')); + + // Circular / weird input must not throw inside the card renderer's data prep. + const circ = {}; circ.self = circ; + assert.doesNotThrow(() => M.describeMcpCall('s__t', circ, { server: 's', tool: 't' })); + assert.strictEqual(M.describeMcpCall('s__t', null, { server: 's', tool: 't' }).argsText, ''); + assert.strictEqual(M.describeMcpCall('s__t', undefined, { server: 's', tool: 't' }).argsText, ''); +}); + console.log('\nmcpConfig.js: ' + n + ' tests passed.'); From fab9021811105e411636a16389f18d747aae1016 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Mon, 27 Jul 2026 20:18:15 -0400 Subject: [PATCH 2/3] fix(mcp): share the tool-name rule instead of re-deriving it in extension.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #41. mcpAllowAlways hand-rolled its own regex to validate what "Always allow" writes into levelcode.ai.mcp.toolPolicy, and that second copy of the naming rule had drifted from the one mcpConfig owns. The reviewer flagged three things. One is not true, one is, and checking the first turned up a worse bug underneath it. NOT TRUE — "tool names containing extra `__` are rejected". They are not: `_` is inside the character class, so the leading group absorbs them and `github__foo__bar` matches. TRUE, and worse than reported — a name can legitimately have NO separator at all. When a server's name alone reaches the 64-char cap, namespaceToolName truncates INSIDE that first segment and appends a hash tag: namespaceToolName('s'.repeat(70), 'tool') -> 'ssss…ssss_a1b2c3' // no '__' The old regex required `__`, so it rejected a name this module itself produced and "Always allow" silently did nothing for that server. TRUE — no length bound, so a malformed message could persist an arbitrarily long settings key. And Object.assign over the existing setting hands a literal `__proto__` (which survives JSON.parse as a real own key) to the prototype setter rather than copying it. Fixed by moving the rule to where it belongs: mcpConfig now exports isNamespacedToolName (alphabet + MAX_TOOL_NAME bound) and its existing safeCopy, and extension.js uses both. One definition, beside the function whose output it describes. One trap worth recording: relaxing the separator requirement silently removed an ACCIDENTAL protection — `__proto__` failed the old regex only because it has no non-underscore character before its `__`. isNamespacedToolName therefore rejects UNSAFE_KEYS explicitly, so the guarantee no longer rides on an unrelated rule keeping a particular shape. My own new test caught that regression. 47 mcpConfig cases (up from 44); 23 suites, 0 failures. --- extensions/levelcode-ai/extension.js | 19 +++++--- extensions/levelcode-ai/mcpConfig.js | 33 +++++++++++++- .../levelcode-ai/test/mcpConfig.test.js | 45 +++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index ad1f790..128735a 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -27,7 +27,7 @@ const { loadSkills, skillsMenu, getSkillBody } = require('./skills'); const { openCustomize } = require('./customize'); const { importFromVscode } = require('./importVscode'); const { reapMcp } = require('./mcpClient'); -const { userScopedSetting } = require('./mcpConfig'); +const { userScopedSetting, isNamespacedToolName, safeCopy } = require('./mcpConfig'); const SECRET_KEY = 'levelcode.ai.anthropicKey'; // legacy Anthropic key location (kept for back-compat) const FILE_EXCLUDES = '{**/node_modules/**,**/.git/**,**/out/**,**/dist/**,**/.vscode-test/**,**/*.map}'; @@ -776,14 +776,23 @@ let approvalSeq = 0; * a tool name that is not a namespaced server__tool to avoid writing junk from a malformed message. */ async function mcpAllowAlways(name) { - if (typeof name !== 'string' || !/^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/.test(name)) { - dbg('mcp.allow.reject', { name }); return; + // 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 + // produces — a server whose name alone hits the 64-char cap truncates inside that first segment and + // comes back without one. "Always allow" then silently did nothing for that server. The shared check + // also bounds the length, so a malformed message cannot persist an arbitrarily long settings key. + if (!isNamespacedToolName(name)) { + dbg('mcp.allow.reject', { name: String(name).slice(0, 80) }); return; } try { const cfg = aiConfig(); - const cur = userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {}; + // 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'), {}) || {}); if (cur[name] === 'allow') { return; } - await cfg.update('mcp.toolPolicy', Object.assign({}, cur, { [name]: 'allow' }), vscode.ConfigurationTarget.Global); + cur[name] = 'allow'; + await cfg.update('mcp.toolPolicy', cur, vscode.ConfigurationTarget.Global); post({ type: 'agentTool', icon: 'check', text: '🔌 mcp · always allow ' + name }); dbg('mcp.allow.persisted', { name }); } catch (e) { diff --git a/extensions/levelcode-ai/mcpConfig.js b/extensions/levelcode-ai/mcpConfig.js index 2503718..a0e73e8 100644 --- a/extensions/levelcode-ai/mcpConfig.js +++ b/extensions/levelcode-ai/mcpConfig.js @@ -219,6 +219,36 @@ function namespaceToolName(server, tool) { return full.slice(0, MAX_TOOL_NAME - tag.length - 1) + '_' + tag; } +/** + * Could `name` have come out of namespaceToolName? The guard for anything that PERSISTS a tool name — + * today, "Always allow" writing a key into `levelcode.ai.mcp.toolPolicy`. + * + * 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('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. + * + * 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. + */ +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); +} + /** * Assign a final, unique, provider-legal name to every (server, tool) pair — the last line of defence * before names reach the wire. Collisions (with a built-in, or between two servers whose names @@ -452,7 +482,8 @@ function describeMcpCall(name, args, route) { } module.exports = { - loadServerConfig, userScopedSetting, namespaceToolName, assignToolNames, buildAgentTools, + loadServerConfig, userScopedSetting, namespaceToolName, isNamespacedToolName, assignToolNames, + buildAgentTools, safeCopy, toolCountsByServer, classifyMcpTool, explainMcpRefusal, describeMcpCall, BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_ARG_CHARS, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH }; diff --git a/extensions/levelcode-ai/test/mcpConfig.test.js b/extensions/levelcode-ai/test/mcpConfig.test.js index f7de219..1d999a2 100644 --- a/extensions/levelcode-ai/test/mcpConfig.test.js +++ b/extensions/levelcode-ai/test/mcpConfig.test.js @@ -543,4 +543,49 @@ test('CARD: arguments are shown in full but bounded, and never throw', () => { assert.strictEqual(M.describeMcpCall('s__t', undefined, { server: 's', tool: 't' }).argsText, ''); }); +// ---- isNamespacedToolName: the guard for anything that PERSISTS a tool name ---- +// "Always allow" writes the name into settings, so this decides what can be written. + +test('PERSIST: accepts every name namespaceToolName can produce', () => { + const produced = [ + M.namespaceToolName('github', 'list_issues'), + M.namespaceToolName('github', 'foo__bar'), // tool name already contains the separator + M.namespaceToolName('a', 'b'), + M.namespaceToolName('srv', 't'.repeat(90)), // truncated on the TOOL side + M.namespaceToolName('s'.repeat(70), 'tool'), // truncated inside the SERVER segment + M.namespaceToolName('has spaces', 'and.dots') // sanitized to the legal alphabet + ]; + for (const name of produced) { + assert.ok(M.isNamespacedToolName(name), 'must accept a name it produced: ' + name); + } + + // The regression this replaced: a server name long enough to be truncated comes back with NO `__`, + // so a validator that required the separator rejected it and "Always allow" silently did nothing. + const noSeparator = M.namespaceToolName('s'.repeat(70), 'tool'); + assert.ok(!noSeparator.includes('__'), 'this case must actually lack the separator, or the test is vacuous'); + assert.ok(M.isNamespacedToolName(noSeparator)); +}); + +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: 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"}'); + const copy = M.safeCopy(fromSettings); + + assert.strictEqual(copy.gh__list, 'allow', 'legitimate entries survive'); + assert.ok(!Object.prototype.hasOwnProperty.call(copy, '__proto__'), '__proto__ must be dropped'); + assert.ok(!Object.prototype.hasOwnProperty.call(copy, 'constructor'), 'constructor must be dropped'); + assert.strictEqual(Object.getPrototypeOf(copy), Object.prototype, 'the copy keeps a clean prototype'); +}); + console.log('\nmcpConfig.js: ' + n + ' tests passed.'); From f247f1ad8eea3ec9fe37dae954962fa4fe3d36cb Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Mon, 27 Jul 2026 20:32:56 -0400 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- extensions/levelcode-ai/extension.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 128735a..7303432 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -1389,13 +1389,14 @@ class ChatViewProvider { case 'send': await handleSend(msg.text); break; 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': - // "Always allow" on an MCP card persists the tool to the allow-list BEFORE resolving, so a - // future run skips the prompt. It only ever adds an ALLOW (never a broadening default), and - // the webview offers it only for non-destructive tools — mcpAllowAlways re-checks anyway. - if (msg.approved && msg.remember && msg.mcpName) { await mcpAllowAlways(msg.mcpName); } + case 'approvalResponse': { + // Persisting "Always allow" should not block the approved tool call. + if (msg.approved && msg.remember && msg.mcpName) { + Promise.resolve(mcpAllowAlways(msg.mcpName)).catch(() => { /* best-effort */ }); + } resolveApproval(msg.id, msg.approved); break; + } case 'questionsResponse': resolveQuestions(msg.id, msg.answers, msg.notes); break; case 'accountSignIn': await accountSignIn(msg.provider, msg.create); break; case 'accountSignOut': await accountSignOut(); break;