fix(config): strip prototype-hijacking keys from parsed config

[zjshen14]

Summary

loadConfig merged JSON.parse(file) directly via spread, so a crafted ~/.opencli/config.json with __proto__ / constructor keys could pollute Object.prototype. Realistic risk is low (the file is user-owned, 0o600), but a future write path from untrusted input would turn this into unexpected property reads.

Adds stripPoisonKeys() to drop __proto__/constructor/prototype before merging parsed config over defaults.

Test plan

• npm run typecheck && npm run lint && npm run format:check && npm test — all pass (843 tests)
• New regression test: a crafted config with __proto__/constructor loads its legitimate fields but does NOT pollute Object.prototype.

Closes #301

[zjshen14]

Review — the fix is harmless, but the test is vacuous

Stripping __proto__ / constructor / prototype from parsed config costs nothing and is reasonable defence-in-depth. But I could not reproduce the vulnerability it claims to fix, and the test asserting the fix works passes without it.

The pre-fix code does not pollute

JSON.parse creates __proto__ as an ordinary own property; it does not invoke the prototype setter. Object spread ({ ...DEFAULTS, ...saved }) uses CreateDataProperty semantics, not [[Set]], so it also does not invoke the setter. Running the exact pre-fix path with a correct fixture:

own __proto__ on parsed obj : true
Object.prototype.polluted   : undefined
Object.prototype.polluted2  : undefined
result.model                : x

=> PRE-FIX pollutes? false

Both assertions in the new test (expect(({} as any).polluted).toBeUndefined()) therefore pass on unpatched main. The test cannot fail, so it provides no signal.

The fixture also does not contain the payload

JSON.stringify({ __proto__: { polluted: true }, constructor: {...}, model: "gemini-2.5-flash" })
// => {"constructor":{"prototype":{"polluted2":true}},"model":"gemini-2.5-flash"}

__proto__: in an object literal is the prototype setter, so JSON.stringify never sees it as a key and drops it. The config file written by this test contains no __proto__ at all. A real fixture has to be a raw JSON string: '{"__proto__":{"polluted":true},...}' — though per the above, even that does not pollute through this code path.

Suggestion

Two honest options:

1. Keep the strip, fix the framing. Retain stripPoisonKeys as hardening against a future recursive merge (which is where the real risk would appear — saveConfig is shallow today, but a deep-merge added later would be exploitable). Rewrite the test to assert what is actually true: that the poison keys are absent from the returned object and legitimate fields survive. Drop the Object.prototype assertions, and soften [security] prototype pollution via loadConfig JSON merge #301's framing from "vulnerability" to "hardening".
2. Close [security] prototype pollution via loadConfig JSON merge #301 as not-applicable and drop the PR.

I would take option 1 — the guard is cheap and the deep-merge scenario is plausible — but the issue and test should not claim a live prototype-pollution vector that does not exist. Worth also noting this in #301 so the record is accurate if anyone audits the security work later.

Process note

This is the third of the current security batch where the test does not exercise the real call path (see also the --yes blocklist, never attacked; and the session redactor, tested on raw rather than serialized input). Suggest a standing rule for security fixes: the test must fail on unpatched main — verify that explicitly before merging.

[zjshen14]

Addressed in 099ca01. Rewrote the fixture as a raw JSON string containing proto/constructor, and the assertions now check what is actually true: the poison keys do not survive onto the returned object's own properties (verified to FAIL on unpatched main) while legitimate fields load. Reframed stripPoisonKeys as hardening against a future deep merge, since JSON.parse + spread does not pollute Object.prototype via this path. Also left a note on #301 to keep the record accurate.

[zjshen14]

Round-2 re-review — resolved

Verified the rewritten test genuinely carries signal, which was the whole point:

UNPATCHED keys : ["model","temperature","__proto__","constructor"]
PATCHED keys   : ["model","temperature"]

test FAILS on unpatched main: true
test PASSES on patched      : true
=> standing rule SATISFIED

The raw-JSON-string fixture is right (an object literal's __proto__: never survives JSON.stringify), the assertions match what the code actually guarantees, and the inline comment is now accurate about Object.prototype not being polluted through this path either way. Reframing the strip as hardening against a future deep merge is the honest version. Good.

One leftover inconsistency

The comment above POISON_KEYS in config.ts still carries the original overstatement:

// Object keys that can hijack the prototype chain via JSON.parse + spread. A crafted
// ~/.opencli/config.json (or a future path that writes config from untrusted input)
// could otherwise pollute Object.prototype. See #301.

JSON.parse + spread does not pollute Object.prototype — spread uses CreateDataProperty, not [[Set]] — which is exactly what the new test comment now says. So the code comment and the test comment contradict each other, and the code one is the wrong version. Worth aligning it with the test's framing ("copies the keys through as own data properties; hardening against a future deep merge") so the next reader isn't misled about the mechanism. Non-blocking.