fix(mcp): tighten the allow-list name check; never copy a non-object policy

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 */ });
 					}

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));
 }
 
 /**

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"}');