From df7cbd5b7ba57719242d46464e60880e63440fd9 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 13 Sep 2026 15:57:46 +0900 Subject: [PATCH 1/2] fix(providers): let field-masked writes reach canonical OpenAI past stored overlays Carry #4447 from ed9655286 onto origin/dev. PATCH /api/providers, the provider editor, and reload merge onto the persisted row. Once selectedModels (or disabled, or any other operator overlay) is on disk, the exact-key canonical seed check rejected every later field-masked write with "must equal the canonical built-in provider seed". Keep that exact-key comparison for POST. Merge-based paths now require every seed-defined key to match and ignore keys the seed never defines. Fold the source review finding: overlay-tolerant comparison would otherwise let allowPrivateNetwork persist on canonical openai and short-circuit destination DNS checks (loopback, RFC1918, metadata). Canonical openai still rejects that field. CodeRabbit's proposed delete-from-candidate would have allowed persistence; this rejects it. Source PR targeted main; this carry lands on dev. The source pull request is left alone. Local product suite, build and install: NOT RUN. Hosted exact-head CI on this PR is the merge proof. Co-authored-by: Veritas-7 <234569343+Veritas-7@users.noreply.github.com> --- src/server/auth-cors.ts | 33 ++++++- src/server/management/provider-routes.ts | 13 ++- structure/gui-and-management-api.md | 2 +- .../management-provider-validation.test.ts | 98 +++++++++++++++++++ 4 files changed, 142 insertions(+), 4 deletions(-) diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index bf8ced6203..483d851b3f 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -610,6 +610,23 @@ function sameCanonicalProviderSeed(actual: Record, expected: Oc return actualKeys.every(key => JSON.stringify(actual[key]) === JSON.stringify((expected as unknown as Record)[key])); } +/** + * Operator-overlay tolerant variant of the canonical seed check: every key the registry + * seed defines must still match the submitted provider verbatim, but keys the seed never + * defines are ignored instead of failing the comparison. Field-masked writes (PATCH, + * the provider editor, reload) merge onto the persisted row, so the submitted candidate + * legitimately carries stored operator overlays like `selectedModels` or `disabled`. + * Those fields are validated by their own write boundaries and cannot widen what the + * forward proxy claims. Full-object writes (POST) keep the strict exact-key comparison + * so a forged overlay cannot ride in on a canonical transport seed. + */ +function matchesCanonicalProviderSeed(actual: Record, expected: OcxProviderConfig): boolean { + return Object.keys(expected).every( + key => Object.hasOwn(actual, key) + && JSON.stringify(actual[key]) === JSON.stringify((expected as unknown as Record)[key]), + ); +} + function positiveWindowValue(value: unknown): boolean { return typeof value === "number" && Number.isSafeInteger(value) && value > 0; } @@ -646,7 +663,11 @@ function nativeContextOverlayError(raw: Record): string | null * string, or null when the provider may be persisted. Caller-controlled names/fields are * redacted and JSON-escaped so secrets never reach the response. */ -export function providerManagementConfigError(name: unknown, provider: unknown): string | null { +export function providerManagementConfigError( + name: unknown, + provider: unknown, + options?: { allowOperatorOverlays?: boolean }, +): string | null { if (typeof name !== "string" || !provider || typeof provider !== "object" || Array.isArray(provider)) { return "provider must be a plain object"; } @@ -658,6 +679,12 @@ export function providerManagementConfigError(name: unknown, provider: unknown): if (name === "openai" && (Object.hasOwn(raw, "autoReviewModel") || Object.hasOwn(raw, "autoReviewModelOverrides"))) { return "provider openai must not include autoReviewModel or autoReviewModelOverrides"; } + // Canonical OpenAI is the ChatGPT forward seed. allowPrivateNetwork is the explicit + // opt-in that skips destination DNS classification (loopback, RFC1918, metadata). + // Overlay-tolerant comparison would otherwise treat it as an extra key and persist it. + if (name === "openai" && Object.hasOwn(raw, "allowPrivateNetwork")) { + return "provider openai must not include allowPrivateNetwork"; + } const autoReviewTargetError = autoReviewModelTargetConfigError(raw.autoReviewModel, "autoReviewModel", true); if (autoReviewTargetError) return autoReviewTargetError; const autoReviewMapError = autoReviewModelOverridesConfigError(raw.autoReviewModelOverrides, "autoReviewModelOverrides", true); @@ -700,7 +727,9 @@ export function providerManagementConfigError(name: unknown, provider: unknown): // validation and then rejected by the seed comparison, so canonical OpenAI could never // set OR clear it — the value was admitted and then refused in the same request. delete canonicalCandidate.annotateEmptyToolOutputs; - const canonical = seed && sameCanonicalProviderSeed(canonicalCandidate, seed); + const canonical = seed && (options?.allowOperatorOverlays + ? matchesCanonicalProviderSeed(canonicalCandidate, seed) + : sameCanonicalProviderSeed(canonicalCandidate, seed)); if (!canonical) { return `provider ${name} must equal the canonical built-in provider seed`; } diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 2c24ae791f..e4b0ad3242 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -259,7 +259,10 @@ function providerEditorCandidate( if (namespaceCollision) return { ok: false, status: 409, error: namespaceCollision, code: "provider_namespace_conflict" }; const merged = mergeProviderEditorRow(persisted.providers[name], baseline.providers[name], publicProvider); const transportCandidate = providerTransportValidationCandidate(merged as unknown as Record); - const providerError = providerManagementConfigError(name, transportCandidate) + // The editor merges onto the persisted row, so stored operator overlays (selectedModels, + // disabled, …) ride along in the candidate. They are owned by their own write boundaries; + // the seed check must only pin the canonical transport/auth keys. + const providerError = providerManagementConfigError(name, transportCandidate, { allowOperatorOverlays: true }) ?? providerEmptyToolOutputConfigError(name, transportCandidate) ?? providerServiceTierConfigError(name, transportCandidate); if (providerError) return { ok: false, status: 400, error: providerError, code: "invalid_provider" }; @@ -902,6 +905,9 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + // Reload validates a row straight off disk, which legitimately carries stored + // operator overlays; only the canonical transport/auth keys need to match the seed. + { allowOperatorOverlays: true }, ) ?? providerEmptyToolOutputConfigError(name, provider); if (providerError) return jsonResponse({ error: "provider reload target invalid" }, 409); @@ -1383,6 +1389,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + // PATCH merges the mask onto the persisted row, which legitimately carries + // stored operator overlays (selectedModels, disabled, …); only the canonical + // transport/auth keys need to match the seed. + { allowOperatorOverlays: true }, ) ?? providerEmptyToolOutputConfigError(name, next); if (providerError) return jsonResponse({ error: providerError }, 400); @@ -1426,6 +1436,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + { allowOperatorOverlays: true }, ) ?? providerEmptyToolOutputConfigError(name, replay.next); if (syncError) { diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index bf7e70cd8b..c445858e6b 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -597,4 +597,4 @@ The [explicit model-capability contract](config.md#explicit-per-model-capability Exact [model input declarations](config.md#explicit-per-model-capability-declarations) now feed text-only eligibility and catalog hints; existing image-description/omission handling consumes them before the main upstream send. -The raw provider editor round-trips `autoReviewModel` and `autoReviewModelOverrides` through editor-owned DTO fields. POST/PATCH/PUT share validation; PUT copies schema-normalized values into the persisted and live candidate before adoption. Canonical `openai` rejects these fields, including clear forms. Existing authentication, origin checks and stale-baseline protection still govern the writes. See [reviewer projection](catalog.md#provider-scoped-approval-reviewer). +The raw provider editor round-trips `autoReviewModel` and `autoReviewModelOverrides` through editor-owned DTO fields. POST/PATCH/PUT share validation; PUT copies schema-normalized values into the persisted and live candidate before adoption. Canonical `openai` rejects these fields, including clear forms. Field-masked writes (PATCH, editor PUT, reload) pin every registry-seed key and ignore operator overlays the seed never defines, most commonly `selectedModels`; POST keeps the exact-key comparison. Canonical `openai` still rejects `allowPrivateNetwork`, which must not short-circuit destination DNS checks on the ChatGPT forward row. Existing authentication, origin checks and stale-baseline protection still govern the writes. See [reviewer projection](catalog.md#provider-scoped-approval-reviewer). diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index d5eda1973b..824e1e505c 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1729,6 +1729,104 @@ describe("provider management validation", () => { } }); + // selectedModels is written by the dedicated /api/selected-models route, so a canonical + // provider that ever had a model chosen carries it on disk. The exact-key seed comparison + // counted that operator overlay as a transport divergence and rejected every later PATCH + // (context windows included) with "must equal the canonical built-in provider seed". + test("canonical OpenAI with selectedModels can still PATCH modelContextWindows", 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, selectedModels: ["gpt-6-astra", "gpt-5.6-luna"] }, + }, + } 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" }, + body: JSON.stringify({ modelContextWindows: { "gpt-6-astra": 872000 } }), + }); + expect(patch.status).toBe(200); + expect(loadConfig().providers.openai?.modelContextWindows).toEqual({ "gpt-6-astra": 872000 }); + expect(loadConfig().providers.openai?.selectedModels).toEqual(["gpt-6-astra", "gpt-5.6-luna"]); + } 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 }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { + openai: { ...canonicalDirect, selectedModels: ["gpt-6-astra"] }, + }, + } 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" }, + body: JSON.stringify({ baseUrl: "https://attacker.example.com/v1" }), + }); + expect(patch.status).toBe(400); + expect(loadConfig().providers.openai?.baseUrl).toBe("https://chatgpt.com/backend-api/codex"); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } + }); + + // Overlay-tolerant seed comparison ignores extra keys. allowPrivateNetwork is the + // destination-policy opt-in that skips DNS classification; it must not persist on + // the ChatGPT forward seed or the later destination probe would honor it. + test("canonical OpenAI with selectedModels still rejects allowPrivateNetwork", 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, selectedModels: ["gpt-6-astra"] }, + }, + } 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" }, + body: JSON.stringify({ allowPrivateNetwork: true }), + }); + expect(patch.status).toBe(400); + const body = await patch.json() as { error?: string }; + expect(body.error).toContain("allowPrivateNetwork"); + expect(Object.hasOwn(loadConfig().providers.openai as object, "allowPrivateNetwork")).toBe(false); + expect(loadConfig().providers.openai?.selectedModels).toEqual(["gpt-6-astra"]); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } + }); + // #1409: the add/edit form's payload type has no member for contextWindow or test("provider POST overwrite preserves an explicit annotateEmptyToolOutputs: false", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); From c39098ba3d983ac6fd3d718aadcbc76cca2334e6 Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 13 Sep 2026 17:47:13 +0900 Subject: [PATCH 2/2] fix(providers): keep operator headers off the canonical OpenAI forward row Security review of the overlay-tolerant seed comparison found one gap the allowPrivateNetwork deny does not cover. The canonical OpenAI seed defines only adapter, authMode, baseUrl and codexAccountMode, so matchesCanonicalProviderSeed ignores every other key on the merge-based write paths. headers is one of them, and it is not inert: canonical OpenAI has no registry staticHeaders, the PATCH field mask writes headers with a shallow merge, and the forward adapter applies provider.headers to the upstream request before the incoming forward headers, so a persisted value wins whenever the caller omits that header. Before this commit a dashboard-session PATCH such as {"headers":{"chatgpt-account-id":"..."}} would persist on the ChatGPT forward row and ride every subsequent request that did not carry the header itself. POST still refused it through the strict comparison; PATCH, the provider editor and reload did not. Denying headers on name === "openai" restores the pre-overlay behavior on those paths with a clearer message than a seed mismatch. The regression test was driven red before it was accepted: removing the guard fails exactly the new case and nothing else in the file. --- src/server/auth-cors.ts | 9 +++++ .../management-provider-validation.test.ts | 37 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index 483d851b3f..d72d0ff0aa 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -685,6 +685,15 @@ export function providerManagementConfigError( if (name === "openai" && Object.hasOwn(raw, "allowPrivateNetwork")) { return "provider openai must not include allowPrivateNetwork"; } + // The same reasoning applies to `headers`, and it is not hypothetical. Canonical OpenAI + // has no registry `staticHeaders`, so any header block on this row is operator-authored, + // and the forward adapter copies it onto the ChatGPT request BEFORE the incoming forward + // headers — a persisted value therefore wins whenever the caller omits that header. The + // exact-key comparison rejected it as an extra key; overlay tolerance would silently admit + // it on every merge-based write path while POST still refused it. + if (name === "openai" && Object.hasOwn(raw, "headers")) { + return "provider openai must not include headers"; + } const autoReviewTargetError = autoReviewModelTargetConfigError(raw.autoReviewModel, "autoReviewModel", true); if (autoReviewTargetError) return autoReviewTargetError; const autoReviewMapError = autoReviewModelOverridesConfigError(raw.autoReviewModelOverrides, "autoReviewModelOverrides", true); diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index 824e1e505c..e4ecb0625d 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1827,6 +1827,43 @@ describe("provider management validation", () => { } }); + // Same class of defect as allowPrivateNetwork above, and the one the overlay-tolerant + // comparison actually reaches: canonical OpenAI has no registry staticHeaders, and the + // forward adapter copies provider.headers onto the ChatGPT request before the incoming + // forward headers, so a persisted value wins whenever the caller omits that header. + test("canonical OpenAI with selectedModels still rejects a headers overlay", 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, selectedModels: ["gpt-6-astra"] }, + }, + } 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" }, + body: JSON.stringify({ headers: { "chatgpt-account-id": "spoofed-account" } }), + }); + expect(patch.status).toBe(400); + const body = await patch.json() as { error?: string }; + expect(body.error).toContain("headers"); + expect(Object.hasOwn(loadConfig().providers.openai as object, "headers")).toBe(false); + expect(loadConfig().providers.openai?.selectedModels).toEqual(["gpt-6-astra"]); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } + }); + + // #1409: the add/edit form's payload type has no member for contextWindow or test("provider POST overwrite preserves an explicit annotateEmptyToolOutputs: false", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR);