Skip to content

Commit 8acbe42

Browse files
ndemiancclaude
andcommitted
docs(mcp): correct the safeCopy comment — unsafe keys are DROPPED, not rescued
PR #31 review (Copilot), documentation accuracy. The safeCopy JSDoc said it "stops a key legitimately named __proto__ from silently vanishing into a setter," which reads as if such a key is preserved. It is not: the loop `continue`s on __proto__/constructor/prototype, dropping them outright. The behavior is correct (dropping is the safe choice, and such a name is meaningless as an env var and unused as a top-level JSON-Schema keyword); only the comment mis-described it. Reworded to say plainly that those keys are dropped, never copied, and that this is a drop rather than a rescue — with the one-line rationale for why losing them costs nothing. Also strengthened the existing TRUST test to match the now-accurate contract: it asserts up front that the input genuinely carries own __proto__/constructor keys (so the drop path is provably exercised, not vacuous) and that they do not appear in the output. Mutation-checked: removing the `continue` (i.e. copying the keys through) fails the test. Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8ccb47d commit 8acbe42

2 files changed

Lines changed: 21 additions & 9 deletions

File tree

‎extensions/levelcode-ai/mcpConfig.js‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,19 @@ const UNSAFE_KEYS = ['__proto__', 'constructor', 'prototype'];
6868
* or a server-supplied `inputSchema`. JSON.parse creates a REAL own `__proto__` key, so a plain
6969
* Object.assign would hand it to the prototype setter instead of copying it. The string-value check in
7070
* normalizeServer already rejects the classic object-valued payload, which makes today's safety
71-
* incidental — this makes it structural, and stops a key legitimately named `__proto__` from silently
72-
* vanishing into a setter. Deliberately a normal object, not Object.create(null): later code (and
73-
* tests) may reasonably call hasOwnProperty on it.
71+
* incidental — this makes it structural.
72+
*
73+
* The unsafe keys (`__proto__`/`constructor`/`prototype`) are DROPPED outright — never copied to the
74+
* output — so none can reach the prototype setter. Note this is a drop, not a rescue: a key literally
75+
* named `__proto__` does not survive into the result. That is deliberate. Such a name is meaningless as
76+
* an env var and unused as a top-level JSON-Schema keyword, so losing it costs nothing, whereas copying
77+
* it would be exactly the pollution we are guarding against. Deliberately a normal object, not
78+
* Object.create(null): later code (and tests) may reasonably call hasOwnProperty on it.
7479
*/
7580
function safeCopy(raw) {
7681
const out = {};
7782
for (const k of Object.keys(raw || {})) {
78-
if (UNSAFE_KEYS.indexOf(k) !== -1) { continue; }
83+
if (UNSAFE_KEYS.indexOf(k) !== -1) { continue; } // dropped, not copied — see the doc above
7984
out[k] = raw[k];
8085
}
8186
return out;

‎extensions/levelcode-ai/test/mcpConfig.test.js‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,16 +105,23 @@ test('ASSIGN: two servers that sanitize to the SAME name still get distinct tool
105105
for (const t of tools) { assert.ok(LEGAL.test(t.name)); }
106106
});
107107

108-
test('TRUST: an untrusted env can never reach the prototype setter', () => {
108+
test('TRUST: an untrusted env DROPS the unsafe keys and never reaches the prototype setter', () => {
109109
// JSON.parse creates a REAL own "__proto__" key, so a plain Object.assign would hand it to the
110-
// prototype setter instead of copying it. The string-value check already rejects the object-valued
111-
// payload, but this makes the guarantee structural rather than incidental.
110+
// prototype setter instead of copying it. safeCopy makes the guarantee structural — and it does so by
111+
// DROPPING __proto__/constructor/prototype outright (not rescuing them): a key literally named
112+
// __proto__ does not survive into the result. Assert both halves so the contract the JSDoc describes
113+
// can't rot (PR #31 review flagged the doc implying preservation).
112114
const raw = JSON.parse('{"evil":{"command":"x","env":{"__proto__":"pwned","constructor":"no","SAFE":"ok"}}}');
115+
// Precondition: the input genuinely HAS these as own keys, so the drop path is actually exercised —
116+
// otherwise the assertions below would pass vacuously.
117+
assert.ok(Object.prototype.hasOwnProperty.call(raw.evil.env, '__proto__'), 'input must carry an own __proto__');
118+
assert.ok(Object.prototype.hasOwnProperty.call(raw.evil.env, 'constructor'), 'input must carry an own constructor');
119+
113120
const { servers } = M.loadServerConfig({ settings: raw });
114121
const env = servers[0].env;
115122
assert.strictEqual(env.SAFE, 'ok', 'legitimate vars must survive');
116-
assert.ok(!Object.prototype.hasOwnProperty.call(env, '__proto__'), '__proto__ must not be copied through');
117-
assert.ok(!Object.prototype.hasOwnProperty.call(env, 'constructor'), 'constructor must not be copied through');
123+
assert.ok(!Object.prototype.hasOwnProperty.call(env, '__proto__'), '__proto__ must be dropped, not copied through');
124+
assert.ok(!Object.prototype.hasOwnProperty.call(env, 'constructor'), 'constructor must be dropped, not copied through');
118125
assert.strictEqual(Object.getPrototypeOf(env), Object.prototype, 'the copy\'s prototype must not be retargeted');
119126
// @ts-expect-error — probing for global pollution
120127
assert.strictEqual({}.pwned, undefined, 'global Object.prototype must be untouched');

0 commit comments

Comments
 (0)