From bccdc76c6efb1d60619bfdea082a9d1447dce40c Mon Sep 17 00:00:00 2001 From: Zhijie Shen Date: Tue, 4 Aug 2026 07:48:36 +0800 Subject: [PATCH 1/3] fix(config): strip prototype-hijacking keys from parsed config (#301) loadConfig merged JSON.parse(output) 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), but a future write path from untrusted input would turn this into unexpected property reads. Adds stripPoisonKeys() to drop __proto__/constructor/prototype before merge. Closes #301 --- src/state/config.test.ts | 27 +++++++++++++++++++++++++++ src/state/config.ts | 17 ++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/state/config.test.ts b/src/state/config.test.ts index de331ad..ebd0412 100644 --- a/src/state/config.test.ts +++ b/src/state/config.test.ts @@ -34,6 +34,33 @@ describe("loadConfig", () => { expect(config.historySize).toBe(100); expect(config.temperature).toBe(0.7); // default preserved }); + + it("strips prototype-hijacking keys from a crafted config file (#301)", async () => { + // Write a config that attempts to pollute Object.prototype via __proto__. + const { writeFile, mkdir } = await import("node:fs/promises"); + const cfgDir = join(tmpHome, ".opencli"); + await mkdir(cfgDir, { recursive: true }); + await writeFile( + join(cfgDir, "config.json"), + JSON.stringify({ + __proto__: { polluted: true }, + constructor: { prototype: { polluted2: true } }, + model: "gemini-2.5-flash", + }), + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const before: any = {}; + expect(before.polluted).toBeUndefined(); // sanity: not yet polluted + const config = await loadConfig(); + // Legitimate field still loaded + expect(config.model).toBe("gemini-2.5-flash"); + // Prototype was NOT polluted + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(({} as any).polluted).toBeUndefined(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect(({} as any).polluted2).toBeUndefined(); + }); }); describe("saveConfig", () => { diff --git a/src/state/config.ts b/src/state/config.ts index cb47d53..dc2a902 100644 --- a/src/state/config.ts +++ b/src/state/config.ts @@ -47,10 +47,25 @@ const DEFAULTS: Config = { historySize: 50, }; +// 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. +const POISON_KEYS = new Set(["__proto__", "constructor", "prototype"]); + +/** Return a copy of `obj` with prototype-hijacking keys removed. */ +function stripPoisonKeys(obj: Record): Record { + const clean: Record = {}; + for (const [k, v] of Object.entries(obj)) { + if (POISON_KEYS.has(k)) continue; + clean[k] = v; + } + return clean; +} + export async function loadConfig(): Promise { try { const raw = await readFile(CONFIG_FILE, "utf8"); - const saved = JSON.parse(raw) as Record; + const saved = stripPoisonKeys(JSON.parse(raw) as Record); // Migrate legacy apiKey → geminiApiKey if (typeof saved["apiKey"] === "string" && !saved["geminiApiKey"]) { saved["geminiApiKey"] = saved["apiKey"]; From 099ca017c7fd807f83ef7558d6f6ff9557f57073 Mon Sep 17 00:00:00 2001 From: Zhijie Shen Date: Sat, 8 Aug 2026 00:19:53 +0800 Subject: [PATCH 2/3] test(config): make the prototype-hijack test non-vacuous and reframe as hardening (#301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review: the test wrote JSON.stringify({__proto__: ...}), but __proto__: in an object literal is the prototype setter and never serialises — so the fixture contained no __proto__ at all. Separately, JSON.parse + object spread does not pollute Object.prototype via this path, so the prototype assertions passed vacuously (on unpatched main too). Rewrite the fixture as a RAW JSON string containing __proto__/constructor, and assert what is actually true: the poison keys do not survive onto the returned object's own properties (fails on unpatched main — verified) while legitimate fields load. stripPoisonKeys is now positioned as hardening against a future deep merge, which is where a live vector would appear. References #301 --- src/state/config.test.ts | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/state/config.test.ts b/src/state/config.test.ts index ebd0412..d78355c 100644 --- a/src/state/config.test.ts +++ b/src/state/config.test.ts @@ -36,30 +36,29 @@ describe("loadConfig", () => { }); it("strips prototype-hijacking keys from a crafted config file (#301)", async () => { - // Write a config that attempts to pollute Object.prototype via __proto__. + // Write a RAW JSON string (not JSON.stringify of an object literal — `__proto__:` + // in a literal is the prototype setter and never serialises) so the file genuinely + // contains __proto__/constructor as JSON keys. const { writeFile, mkdir } = await import("node:fs/promises"); const cfgDir = join(tmpHome, ".opencli"); await mkdir(cfgDir, { recursive: true }); await writeFile( join(cfgDir, "config.json"), - JSON.stringify({ - __proto__: { polluted: true }, - constructor: { prototype: { polluted2: true } }, - model: "gemini-2.5-flash", - }), + '{"__proto__":{"polluted":true},"constructor":{"prototype":{"polluted2":true}},"model":"x"}', ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const before: any = {}; - expect(before.polluted).toBeUndefined(); // sanity: not yet polluted const config = await loadConfig(); - // Legitimate field still loaded - expect(config.model).toBe("gemini-2.5-flash"); - // Prototype was NOT polluted + // Legitimate field survives. + expect(config.model).toBe("x"); + // The poison keys are NOT carried onto the returned object as own properties. + // (Without stripPoisonKeys, `{ ...saved }` copies them through as data properties — + // this assertion fails on unpatched main. Object.prototype itself is not polluted + // either way via this code path; the strip is hardening against a future deep merge.) + const keys = Object.keys(config); + expect(keys).not.toContain("__proto__"); + expect(keys).not.toContain("constructor"); // eslint-disable-next-line @typescript-eslint/no-explicit-any expect(({} as any).polluted).toBeUndefined(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - expect(({} as any).polluted2).toBeUndefined(); }); }); From f4b7955c0a6dab07c561489a505021b2ff7e2f91 Mon Sep 17 00:00:00 2001 From: Zhijie Shen Date: Sun, 9 Aug 2026 17:03:31 +0800 Subject: [PATCH 3/3] docs(config): align POISON_KEYS comment with the test (hardening, not live vuln) (#301) Review round 2: the code comment claimed the pre-fix path 'could otherwise pollute Object.prototype', contradicting the test comment (and the round-2 verification), which shows JSON.parse + spread does not pollute via this path. Correct the code comment to match: this is hardening against a future deep merge, not a fix for a current prototype-pollution vector. References #301 --- src/state/config.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/state/config.ts b/src/state/config.ts index dc2a902..22fd500 100644 --- a/src/state/config.ts +++ b/src/state/config.ts @@ -47,9 +47,11 @@ const DEFAULTS: Config = { historySize: 50, }; -// 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. +// Object keys that can hijack the prototype chain if the merge ever becomes deep. +// Today JSON.parse + object spread does NOT pollute Object.prototype via this path +// (spread uses [[DefineOwnProperty]], not [[Set]]), so this is hardening against a +// future deep/recursive merge — where a live prototype-pollution vector would appear +// — not a fix for a current one. See #301. const POISON_KEYS = new Set(["__proto__", "constructor", "prototype"]); /** Return a copy of `obj` with prototype-hijacking keys removed. */