From 0411d4166251dc1fe7a92343fbd35c237c1ae6b1 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 18 Sep 2026 03:47:46 +0900 Subject: [PATCH 1/2] fix(gui): keep prototype-named model context drafts safe --- gui/src/own-record-value.ts | 6 +++ gui/src/pages/Models.tsx | 10 ++--- gui/tests/models-empty-provider.test.tsx | 50 ++++++++++++++++++++---- 3 files changed, 53 insertions(+), 13 deletions(-) create mode 100644 gui/src/own-record-value.ts diff --git a/gui/src/own-record-value.ts b/gui/src/own-record-value.ts new file mode 100644 index 0000000000..71683c87a4 --- /dev/null +++ b/gui/src/own-record-value.ts @@ -0,0 +1,6 @@ +// Records keyed by server- or user-supplied IDs can be asked for `__proto__`, `constructor`, +// or `toString`. A plain `record[key]` read then returns an inherited Object.prototype member +// instead of `undefined` — a function where the caller expects a string or a number. +export function ownRecordValue(record: Record, key: string): T | undefined { + return Object.hasOwn(record, key) ? record[key] : undefined; +} diff --git a/gui/src/pages/Models.tsx b/gui/src/pages/Models.tsx index c05ab4c231..7edf6a9ee1 100644 --- a/gui/src/pages/Models.tsx +++ b/gui/src/pages/Models.tsx @@ -13,6 +13,7 @@ import type { TFn, TKey } from "../i18n/shared"; import { modelLabel } from "../model-display"; import { formatProviderDisplayName, providerDisplaySlug } from "../provider-icons"; import { readJsonIfOk, readJsonOrThrow } from "../fetch-json"; +import { ownRecordValue } from "../own-record-value"; import { describeIntegrationRefusalParts } from "./integrations/refusal-copy"; import { readSessionListCache, writeSessionListCache } from "../session-list-cache"; import { setClientResourceData } from "../client-resource"; @@ -116,7 +117,6 @@ function parseContextWindowDraft(raw: string): number | null | undefined { return Number.isSafeInteger(value) && value > 0 ? value : undefined; } - /** #2465 per-provider model-preset view, as `GET /api/model-presets` returns it. */ interface ModelPresetView { mode: "preset" | "all" | "custom"; @@ -816,16 +816,16 @@ export default function Models({ apiBase, restartEpoch = 0 }: { apiBase: string; setContextError(t("models.contextInvalid")); return; } - const modelWindows: Record = {}; + const modelWindows: Record = Object.create(null); // null prototype: a "__proto__" model ID must store an entry, not invoke the inherited setter for (const modelId of contextTouchedModels) { - const draft = contextModelDrafts[modelId] ?? ""; + const draft = ownRecordValue(contextModelDrafts, modelId) ?? ""; const parsed = parseContextWindowDraft(draft); if (parsed === undefined) { setContextError(t("models.contextInvalid")); return; } // Compare VALUES, not text. Retyping 64000 as "64,000" is not a change. - if (parsed === (contextSnapshot.modelContextWindows[modelId] ?? null)) continue; + if (parsed === (ownRecordValue(contextSnapshot.modelContextWindows, modelId) ?? null)) continue; modelWindows[modelId] = parsed; } const defaultChanged = contextDefaultTouched @@ -2285,7 +2285,7 @@ export default function Models({ apiBase, restartEpoch = 0 }: { apiBase: string; { setContextModelDrafts(current => ({ ...current, diff --git a/gui/tests/models-empty-provider.test.tsx b/gui/tests/models-empty-provider.test.tsx index 85843f64b4..55f01e0d3e 100644 --- a/gui/tests/models-empty-provider.test.tsx +++ b/gui/tests/models-empty-provider.test.tsx @@ -112,7 +112,18 @@ test("Models page combines final visibility, atomic actions, discovery status, a }); testWindow.localStorage.setItem("ocx-models-collapsed:v2", JSON.stringify([])); const provider = "fallback-provider"; - const ids = ["claude-opus", "claude-sonnet", "gemini-pro", "gemini-flash", "gpt-oss"]; + // Provider model IDs are arbitrary strings. Prototype property names must behave like normal + // IDs rather than reading inherited values from the context-window draft dictionary. + const ids = [ + "__proto__", + "constructor", + "toString", + "claude-opus", + "claude-sonnet", + "gemini-pro", + "gemini-flash", + "gpt-oss", + ]; let selected = ["gemini-pro", "gemini-flash"]; const disabled = new Set(["gpt-oss"]); const visibilityBodies: Array<{ scope: string; targets: Array<{ id: string }>; enabled: boolean }> = []; @@ -216,7 +227,7 @@ test("Models page combines final visibility, atomic actions, discovery status, a const switchFor = (id: string) => container.querySelector(`button[aria-label="${provider}/${id}"]`)!; const buttonText = (text: string) => [...container.querySelectorAll("button")].find(button => button.textContent === text)!; - expect(container.textContent).toContain("2/5 visible"); + expect(container.textContent).toContain("2/8 visible"); expect(switchFor("gemini-pro").getAttribute("aria-pressed")).toBe("true"); expect(switchFor("claude-sonnet").getAttribute("aria-pressed")).toBe("false"); expect(container.querySelector(".badge.badge-amber")?.textContent).toContain("Discovery failed"); @@ -225,7 +236,9 @@ test("Models page combines final visibility, atomic actions, discovery status, a await act(async () => buttonText("Custom windows").click()); const contextDialog = container.querySelector('[role="dialog"][aria-label="Custom windows"]')!; const contextInputs = contextDialog.querySelectorAll("input"); - expect([...contextInputs].map(input => input.value)).toEqual(["256000", "64000"]); + // The picker sorts model IDs, so "__proto__" is selected first. Its field must render an + // empty draft, not the inherited Object.prototype member a plain map read would return. + expect([...contextInputs].map(input => input.value)).toEqual(["256000", ""]); const setValue = Object.getOwnPropertyDescriptor( testWindow.HTMLInputElement.prototype, "value", @@ -233,7 +246,7 @@ test("Models page combines final visibility, atomic actions, discovery status, a await act(async () => { setValue.call(contextInputs[0]!, "350000"); contextInputs[0]!.dispatchEvent(new testWindow.Event("input", { bubbles: true })); - setValue.call(contextInputs[1]!, "100000"); + setValue.call(contextInputs[1]!, "90000"); contextInputs[1]!.dispatchEvent(new testWindow.Event("input", { bubbles: true })); }); const pickContextModel = async (modelId: string, dialog: HTMLElement = contextDialog) => { @@ -244,6 +257,17 @@ test("Models page combines final visibility, atomic actions, discovery status, a .find(candidate => candidate.textContent === modelId)!; await act(async () => option.click()); }; + await pickContextModel("claude-opus"); + expect(contextInputs[1]!.value).toBe("64000"); + await act(async () => { + setValue.call(contextInputs[1]!, "100000"); + contextInputs[1]!.dispatchEvent(new testWindow.Event("input", { bubbles: true })); + }); + // Inherited members such as Object.prototype.toString must not leak into the draft field. + await pickContextModel("constructor"); + expect(contextInputs[1]!.value).toBe(""); + await pickContextModel("toString"); + expect(contextInputs[1]!.value).toBe(""); await pickContextModel("claude-sonnet"); expect(contextInputs[1]!.value).toBe(""); await act(async () => { @@ -258,6 +282,9 @@ test("Models page combines final visibility, atomic actions, discovery status, a // or the user can neither see nor clear it. await pickContextModel("retired-model"); expect(contextInputs[1]!.value).toBe("72000"); + // The prototype-name draft is an own property, so it survives a picker round-trip. + await pickContextModel("__proto__"); + expect(contextInputs[1]!.value).toBe("90000"); await pickContextModel("claude-opus"); const applyContext = [...contextDialog.querySelectorAll("button")] .find(button => button.textContent === "Apply")!; @@ -274,7 +301,14 @@ test("Models page combines final visibility, atomic actions, discovery status, a // model mid-modal must not make Apply revert it. expect(contextBodies.at(-1)).toEqual({ contextWindow: 350_000, - modelContextWindows: { "claude-opus": 100_000, "claude-sonnet": 80_000 }, + // Object.fromEntries defines "__proto__" as a real own property; a + // `{ "__proto__": n }` literal would silently skip it, which is exactly the + // defect under test. + modelContextWindows: Object.fromEntries([ + ["__proto__", 90_000], + ["claude-opus", 100_000], + ["claude-sonnet", 80_000], + ]), }); expect(container.querySelector('[role="dialog"][aria-label="Custom windows"]')).toBeNull(); @@ -473,7 +507,7 @@ test("Models page combines final visibility, atomic actions, discovery status, a await act(async () => { switchFor("claude-sonnet").click(); await new Promise(resolve => testWindow.setTimeout(resolve, 0)); }); expect(visibilityBodies.at(-1)).toMatchObject({ scope: "models", targets: [{ id: "claude-sonnet" }], enabled: true }); - expect(container.textContent).toContain("3/5 visible"); + expect(container.textContent).toContain("3/8 visible"); failNext = true; await act(async () => { switchFor("claude-opus").click(); await new Promise(resolve => testWindow.setTimeout(resolve, 0)); }); @@ -482,10 +516,10 @@ test("Models page combines final visibility, atomic actions, discovery status, a await act(async () => { buttonText("All on").click(); await new Promise(resolve => testWindow.setTimeout(resolve, 0)); }); expect(visibilityBodies.at(-1)).toMatchObject({ scope: "provider", enabled: true }); - expect(container.textContent).toContain("5/5 visible"); + expect(container.textContent).toContain("8/8 visible"); await act(async () => { buttonText("All off").click(); await new Promise(resolve => testWindow.setTimeout(resolve, 0)); }); expect(visibilityBodies.at(-1)).toMatchObject({ scope: "provider", enabled: false }); - expect(container.textContent).toContain("0/5 visible"); + expect(container.textContent).toContain("0/8 visible"); // A failed poll must keep the catalog on screen but make the stale state visible. failCatalog = true; From d2bc19d28a1e9a37a9aaba722e69f8123dbee040 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:01:46 +0900 Subject: [PATCH 2/2] fix(server): persist __proto__-named model context overrides --- src/server/management/provider-routes.ts | 9 ++++- .../management-provider-validation.test.ts | 37 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index e4b0ad3242..14d62fac48 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -516,7 +516,14 @@ function applyProviderPatchFields( delete next.modelContextWindows; } else { if (!isPlainRecord(value)) return { error: "modelContextWindows must be a plain object or null" }; - const windows: Record = { ...(next.modelContextWindows ?? {}) }; + // A prototype-named model id must survive the merge: assigning + // "__proto__" on an ordinary object invokes the inherited setter instead + // of creating an own property, so the PATCH would report success while + // silently dropping that override. + const windows: Record = Object.assign( + Object.create(null), + next.modelContextWindows ?? {}, + ); for (const [model, window] of Object.entries(value)) { if (!model.trim()) return { error: "modelContextWindows keys must be nonblank model ids" }; if (window === null) { diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index e4ecb0625d..8cdcc24506 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1763,6 +1763,43 @@ describe("provider management validation", () => { } }); + // A "__proto__" model id is a legitimate override key once the GUI can draft it. + // The merge target must be a null-prototype map: on an ordinary object the + // assignment windows["__proto__"] = n invokes the inherited setter, so the + // PATCH would return success while silently dropping the override. + test("PATCH modelContextWindows persists a __proto__-named model override", async () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { + openai: { ...canonicalDirect }, + }, + } as OcxConfig); + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + + const server = startServer(0); + try { + const patch = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + // Written as a raw body: an object literal "__proto__" key would set the + // prototype instead of creating the own property under test. + body: '{"modelContextWindows":{"__proto__":128000}}', + }); + expect(patch.status).toBe(200); + const windows = loadConfig().providers.openai?.modelContextWindows ?? {}; + expect(Object.hasOwn(windows, "__proto__")).toBe(true); + expect(Object.getOwnPropertyDescriptor(windows, "__proto__")?.value).toBe(128000); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } + }); + test("canonical OpenAI with selectedModels still rejects transport tampering", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true });