Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions gui/src/own-record-value.ts
Original file line number Diff line number Diff line change
@@ -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<T>(record: Record<string, T>, key: string): T | undefined {
return Object.hasOwn(record, key) ? record[key] : undefined;
}
10 changes: 5 additions & 5 deletions gui/src/pages/Models.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -816,16 +816,16 @@ export default function Models({ apiBase, restartEpoch = 0 }: { apiBase: string;
setContextError(t("models.contextInvalid"));
return;
}
const modelWindows: Record<string, number | null> = {};
const modelWindows: Record<string, number | null> = 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;
Comment thread
luvs01 marked this conversation as resolved.
}
const defaultChanged = contextDefaultTouched
Expand Down Expand Up @@ -2285,7 +2285,7 @@ export default function Models({ apiBase, restartEpoch = 0 }: { apiBase: string;
<input
className="input"
inputMode="numeric"
value={contextModelDrafts[contextModelId] ?? ""}
value={ownRecordValue(contextModelDrafts, contextModelId) ?? ""}
onChange={event => {
setContextModelDrafts(current => ({
...current,
Expand Down
50 changes: 42 additions & 8 deletions gui/tests/models-empty-provider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 }> = [];
Expand Down Expand Up @@ -216,7 +227,7 @@ test("Models page combines final visibility, atomic actions, discovery status, a

const switchFor = (id: string) => container.querySelector<HTMLButtonElement>(`button[aria-label="${provider}/${id}"]`)!;
const buttonText = (text: string) => [...container.querySelectorAll<HTMLButtonElement>("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");
Expand All @@ -225,15 +236,17 @@ test("Models page combines final visibility, atomic actions, discovery status, a
await act(async () => buttonText("Custom windows").click());
const contextDialog = container.querySelector<HTMLElement>('[role="dialog"][aria-label="Custom windows"]')!;
const contextInputs = contextDialog.querySelectorAll<HTMLInputElement>("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",
)!.set!;
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) => {
Expand All @@ -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 () => {
Expand All @@ -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<HTMLButtonElement>("button")]
.find(button => button.textContent === "Apply")!;
Expand All @@ -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();

Expand Down Expand Up @@ -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)); });
Expand All @@ -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;
Expand Down
9 changes: 8 additions & 1 deletion src/server/management/provider-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, number> = { ...(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<string, number> = 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) {
Expand Down
37 changes: 37 additions & 0 deletions tests/server/management-provider-validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1749,6 +1749,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 });
Expand Down
Loading