From a8ed63378a70b0fc8b2249acfa7cf894c5f3421e Mon Sep 17 00:00:00 2001 From: Flowershangfromthebranches <152056395+Flowershangfromthebranches@users.noreply.github.com> Date: Tue, 1 Sep 2026 01:54:43 +0800 Subject: [PATCH 1/3] fix(openai): exclude user-owned alias overlays from canonical seed validation --- src/server/auth-cors.ts | 11 +++ tests/management-provider-validation.test.ts | 87 ++++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index 0174794c6a..c1277b982b 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -587,6 +587,17 @@ 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; + // Alias fields are user-owned display/routing overlays written by the alias management + // API (PUT /api/providers/{name}/alias, /model-aliases, /api/default-aliases) and read at + // runtime (provider.defaultAliases ?? config.defaultModelAliases; provider.alias in + // router.ts; modelAliases via effectiveModelAliases). None of them alter the canonical + // transport identity (adapter/baseUrl/authMode/apiKeyTransport), and without these + // exclusions the same admitted-then-refused failure hit any canonical OpenAI provider + // that had ever set an alias: the seed comparison rejected the very next full-object + // write with "must equal the canonical built-in provider seed". + delete canonicalCandidate.alias; + delete canonicalCandidate.modelAliases; + delete canonicalCandidate.defaultAliases; const canonical = seed && sameCanonicalProviderSeed(canonicalCandidate, seed); if (!canonical) { return `provider ${name} must equal the canonical built-in provider seed`; diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index ac976fc785..168bdd6c0d 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -914,6 +914,93 @@ describe("provider management validation", () => { } }); + // Regression: the alias overlays (defaultAliases, alias, modelAliases) are user-owned + // display/routing preferences written by the alias management API (PUT /api/default-aliases, + // /api/providers/{name}/alias, /api/providers/{name}/model-aliases) and read at runtime + // (defaultAliasesEnabled, router.ts alias lookup, effectiveModelAliases). They never alter + // the canonical transport identity, so the seed comparison must ignore them exactly like + // requestPacing and modelCosts already do. Before the fix, a canonical OpenAI provider that + // had ever set any alias was permanently locked out of full-object writes (POST/PATCH/PUT) + // with "must equal the canonical built-in provider seed". + test("canonical seed comparison ignores user-owned alias overlays", () => { + expect(providerManagementConfigError("openai", { ...canonicalDirect, defaultAliases: true })).toBeNull(); + expect(providerManagementConfigError("openai", { ...canonicalDirect, defaultAliases: false })).toBeNull(); + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + alias: "my-openai", + modelAliases: { "gpt-5.6-luna": "luna" }, + })).toBeNull(); + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + defaultAliases: true, + alias: "my-openai", + modelAliases: { "gpt-5.6-luna": "luna" }, + })).toBeNull(); + }); + + test("canonical seed guard still rejects transport tampering alongside alias overlays", () => { + // The alias exclusions must not weaken the canonical guard: mutating the transport + // identity (baseUrl / adapter / authMode) is still rejected even when alias overlays + // are present on the same payload. + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + defaultAliases: true, + baseUrl: "https://attacker.example/backend-api/codex", + })).toContain("canonical built-in provider seed"); + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + defaultAliases: true, + adapter: "openai-chat", + })).toContain("canonical built-in provider seed"); + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + defaultAliases: true, + authMode: "key", + })).toContain("canonical built-in provider seed"); + // Non-openai providers keep their existing behavior: no canonical seed comparison. + expect(providerManagementConfigError("deepseek", { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com/v1", + apiKey: "sk-test", + defaultAliases: true, + })).toBeNull(); + }); + + test("canonical OpenAI with defaultAliases can still PATCH modelContextWindows", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + // Match a post-migration config (openaiProviderTierVersion set) so the startup + // openai tier migration does not rewrite the row: this test targets the seed + // comparison, not the one-time legacy migration. + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { + openai: { ...canonicalDirect, defaultAliases: true }, + }, + } as OcxConfig); + // This test targets the seed comparison, not the DNS policy; stub the destination + // probe so the assertion stays independent of how chatgpt.com resolves locally. + 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-5.6-luna": 900000 } }), + }); + expect(patch.status).toBe(200); + expect(loadConfig().providers.openai?.modelContextWindows).toEqual({ "gpt-5.6-luna": 900000 }); + // The alias overlay itself must survive the patch untouched. + expect(loadConfig().providers.openai?.defaultAliases).toBe(true); + } finally { + 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)) rmSync(TEST_DIR, { recursive: true }); From 39b465b05a7ce1472ca8a25e63905132443bad77 Mon Sep 17 00:00:00 2001 From: Flowershangfromthebranches <152056395+Flowershangfromthebranches@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:38:52 +0800 Subject: [PATCH 2/3] fix(openai): preserve alias API ownership on provider writes --- src/server/auth-cors.ts | 11 - src/server/management/provider-routes.ts | 92 ++++++- tests/management-provider-validation.test.ts | 269 +++++++++++++++---- 3 files changed, 307 insertions(+), 65 deletions(-) diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index c1277b982b..0174794c6a 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -587,17 +587,6 @@ 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; - // Alias fields are user-owned display/routing overlays written by the alias management - // API (PUT /api/providers/{name}/alias, /model-aliases, /api/default-aliases) and read at - // runtime (provider.defaultAliases ?? config.defaultModelAliases; provider.alias in - // router.ts; modelAliases via effectiveModelAliases). None of them alter the canonical - // transport identity (adapter/baseUrl/authMode/apiKeyTransport), and without these - // exclusions the same admitted-then-refused failure hit any canonical OpenAI provider - // that had ever set an alias: the seed comparison rejected the very next full-object - // write with "must equal the canonical built-in provider seed". - delete canonicalCandidate.alias; - delete canonicalCandidate.modelAliases; - delete canonicalCandidate.defaultAliases; const canonical = 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 626148fd0c..c70cf2edcc 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -105,6 +105,63 @@ type ProviderPatchApplication = headersTouched: boolean; }; +const PROVIDER_ALIAS_OVERLAY_FIELDS = ["alias", "modelAliases", "defaultAliases"] as const; +type ProviderAliasOverlayField = typeof PROVIDER_ALIAS_OVERLAY_FIELDS[number]; + +/** + * Alias overlays are owned by the dedicated alias management routes. A full provider POST + * may round-trip an already-persisted value, but it must not create, clear, or change one. + * PATCH is field-masked and rejects these keys outright below. + */ +function providerAliasOverlayOwnershipError( + submitted: Record, + existing: OcxProviderConfig | undefined, +): string | null { + for (const field of PROVIDER_ALIAS_OVERLAY_FIELDS) { + if (!Object.hasOwn(submitted, field)) continue; + if (!existing || !Object.hasOwn(existing, field)) { + return `${field} is managed by the dedicated alias API`; + } + const incoming = submitted[field]; + const persisted = existing[field]; + if (field === "modelAliases") { + if (!isPlainRecord(incoming) || !isPlainRecord(persisted)) { + return "modelAliases is managed by the dedicated alias API"; + } + const incomingEntries = Object.entries(incoming); + const persistedEntries = Object.entries(persisted); + if ( + incomingEntries.length !== persistedEntries.length + || incomingEntries.some(([model, alias]) => typeof alias !== "string" || persisted[model] !== alias) + ) { + return "modelAliases is managed by the dedicated alias API"; + } + continue; + } + if (incoming !== persisted) return `${field} is managed by the dedicated alias API`; + } + return null; +} + +/** Remove only alias overlays whose ownership has already been established by the caller. */ +function providerTransportValidationCandidate(provider: Record): Record { + const candidate = { ...provider }; + for (const field of PROVIDER_ALIAS_OVERLAY_FIELDS) delete candidate[field]; + return candidate; +} + +/** Preserve the authoritative alias values from the stored provider during a full edit. */ +function restorePersistedAliasOverlays(target: OcxProviderConfig, existing: OcxProviderConfig | undefined): void { + for (const field of PROVIDER_ALIAS_OVERLAY_FIELDS) { + delete (target as Record)[field]; + if (!existing || !Object.hasOwn(existing, field)) continue; + const value = existing[field]; + (target as Record)[field] = field === "modelAliases" + ? structuredClone(value) + : value; + } +} + /** * Apply the recognized PATCH field mask onto a provider copy. The caller runs this once * for validation and again inside the config mutation lock against the newest provider, @@ -501,7 +558,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + ) ?? providerEmptyToolOutputConfigError(name, provider); if (providerError) return jsonResponse({ error: "provider reload target invalid" }, 409); const namespaceCollision = codexAccountNamespaceProviderCollisionError( @@ -561,12 +621,17 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise Object.hasOwn(rawBody, field)); + if (aliasField) return jsonResponse({ error: `${aliasField} is managed by the dedicated alias API` }, 400); const hasMode = Object.hasOwn(rawBody, "codexAccountMode"); const hasSetDefault = Object.hasOwn(rawBody, "setDefault"); const canonicalBudgetOnly = name === "openai" @@ -746,7 +816,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + ) ?? providerEmptyToolOutputConfigError(name, next); if (providerError) return jsonResponse({ error: providerError }, 400); if (!canonicalBudgetOnly) { @@ -786,7 +859,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise), + ) ?? providerEmptyToolOutputConfigError(name, replay.next); if (syncError) { replayError = syncError; diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 168bdd6c0d..6ce8b7e9a3 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -914,56 +914,233 @@ describe("provider management validation", () => { } }); - // Regression: the alias overlays (defaultAliases, alias, modelAliases) are user-owned - // display/routing preferences written by the alias management API (PUT /api/default-aliases, - // /api/providers/{name}/alias, /api/providers/{name}/model-aliases) and read at runtime - // (defaultAliasesEnabled, router.ts alias lookup, effectiveModelAliases). They never alter - // the canonical transport identity, so the seed comparison must ignore them exactly like - // requestPacing and modelCosts already do. Before the fix, a canonical OpenAI provider that - // had ever set any alias was permanently locked out of full-object writes (POST/PATCH/PUT) - // with "must equal the canonical built-in provider seed". - test("canonical seed comparison ignores user-owned alias overlays", () => { - expect(providerManagementConfigError("openai", { ...canonicalDirect, defaultAliases: true })).toBeNull(); - expect(providerManagementConfigError("openai", { ...canonicalDirect, defaultAliases: false })).toBeNull(); - expect(providerManagementConfigError("openai", { - ...canonicalDirect, - alias: "my-openai", - modelAliases: { "gpt-5.6-luna": "luna" }, - })).toBeNull(); - expect(providerManagementConfigError("openai", { - ...canonicalDirect, - defaultAliases: true, - alias: "my-openai", + test("full provider edit preserves aliases owned by the dedicated APIs", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const overlays = { + alias: "codex-native", modelAliases: { "gpt-5.6-luna": "luna" }, - })).toBeNull(); + defaultAliases: false, + } as const; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { openai: { ...canonicalDirect, ...overlays } }, + } as OcxConfig); + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + const server = startServer(0); + try { + // The dashboard's full editor omits alias-owned fields. The stored values must survive. + const omitted = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: canonicalDirect }), + }); + expect(omitted.status).toBe(200); + expect(loadConfig().providers.openai).toMatchObject(overlays); + + // A full-object client may round-trip the exact stored values, but still does not own them. + const roundTrip = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: { ...canonicalDirect, ...overlays } }), + }); + expect(roundTrip.status).toBe(200); + expect(loadConfig().providers.openai).toMatchObject(overlays); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } }); - test("canonical seed guard still rejects transport tampering alongside alias overlays", () => { - // The alias exclusions must not weaken the canonical guard: mutating the transport - // identity (baseUrl / adapter / authMode) is still rejected even when alias overlays - // are present on the same payload. - expect(providerManagementConfigError("openai", { - ...canonicalDirect, - defaultAliases: true, - baseUrl: "https://attacker.example/backend-api/codex", - })).toContain("canonical built-in provider seed"); - expect(providerManagementConfigError("openai", { - ...canonicalDirect, - defaultAliases: true, - adapter: "openai-chat", - })).toContain("canonical built-in provider seed"); - expect(providerManagementConfigError("openai", { - ...canonicalDirect, - defaultAliases: true, - authMode: "key", - })).toContain("canonical built-in provider seed"); - // Non-openai providers keep their existing behavior: no canonical seed comparison. - expect(providerManagementConfigError("deepseek", { - adapter: "openai-chat", - baseUrl: "https://api.deepseek.com/v1", - apiKey: "sk-test", + test("general provider writes cannot introduce a provider alias collision", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { + openai: { ...canonicalDirect }, + deepseek: { adapter: "openai-chat", baseUrl: "https://api.deepseek.com/v1" }, + }, + } as OcxConfig); + const server = startServer(0); + try { + const before = readFileSync(join(TEST_DIR, "config.json")); + const post = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: { ...canonicalDirect, alias: "deepseek" } }), + }); + expect(post.status).toBe(400); + + const patch = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ alias: "deepseek" }), + }); + expect(patch.status).toBe(400); + expect(readFileSync(join(TEST_DIR, "config.json"))).toEqual(before); + expect(loadConfig().providers.openai?.alias).toBeUndefined(); + } finally { + await server.stop(true); + } + }); + + test("general provider writes reject reserved duplicate and invalid model aliases", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { openai: { ...canonicalDirect } }, + } as OcxConfig); + const server = startServer(0); + try { + const before = readFileSync(join(TEST_DIR, "config.json")); + for (const modelAliases of [ + { "gpt-5.6-luna": "gpt-5.6-sol" }, + { "gpt-5.6-sol": "same", "gpt-5.6-luna": "SAME" }, + { "gpt-5.6-luna": "not an alias" }, + ]) { + const response = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: { ...canonicalDirect, modelAliases } }), + }); + expect(response.status).toBe(400); + } + const patch = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelAliases: { "gpt-5.6-luna": "luna" } }), + }); + expect(patch.status).toBe(400); + expect(readFileSync(join(TEST_DIR, "config.json"))).toEqual(before); + } finally { + await server.stop(true); + } + }); + + test("malformed alias overlays return bounded 4xx without config persistence", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { openai: { ...canonicalDirect } }, + } as OcxConfig); + const server = startServer(0); + try { + const before = readFileSync(join(TEST_DIR, "config.json")); + for (const overlay of [ + { defaultAliases: "yes" }, + { modelAliases: null }, + { modelAliases: [] }, + { modelAliases: { "gpt-5.6-luna": 42 } }, + ]) { + const post = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: { ...canonicalDirect, ...overlay } }), + }); + expect(post.status).toBeGreaterThanOrEqual(400); + expect(post.status).toBeLessThan(500); + + const patch = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(overlay), + }); + expect(patch.status).toBeGreaterThanOrEqual(400); + expect(patch.status).toBeLessThan(500); + } + expect(readFileSync(join(TEST_DIR, "config.json"))).toEqual(before); + } finally { + await server.stop(true); + } + }); + + test("canonical transport tampering stays rejected with persisted alias overlays", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const overlays = { + alias: "codex-native", + modelAliases: { "gpt-5.6-luna": "luna" }, defaultAliases: true, - })).toBeNull(); + } as const; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { openai: { ...canonicalDirect, ...overlays } }, + } as OcxConfig); + const server = startServer(0); + try { + for (const tampering of [ + { baseUrl: "https://attacker.example/backend-api/codex" }, + { adapter: "openai-chat" }, + { authMode: "key" }, + ]) { + const response = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(tampering), + }); + expect(response.status).toBe(400); + } + expect(loadConfig().providers.openai).toMatchObject({ ...canonicalDirect, ...overlays }); + } finally { + await server.stop(true); + } + }); + + test("unrelated non-openai provider edits preserve persisted alias overlays", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "deepseek", + providers: { + deepseek: { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com/v1", + alias: "ds", + modelAliases: { "deepseek-v4": "ds4-custom" }, + defaultAliases: false, + }, + }, + } as OcxConfig); + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + const server = startServer(0); + try { + const response = await fetch(new URL("/api/providers?name=deepseek", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ contextWindow: 128000 }), + }); + expect(response.status).toBe(200); + expect(loadConfig().providers.deepseek).toMatchObject({ + alias: "ds", + modelAliases: { "deepseek-v4": "ds4-custom" }, + defaultAliases: false, + contextWindow: 128000, + }); + } finally { + resolvedError.mockRestore(); + await server.stop(true); + } }); test("canonical OpenAI with defaultAliases can still PATCH modelContextWindows", async () => { From dd60f05d73599991b12e11298f741d34ed84ccbf Mon Sep 17 00:00:00 2001 From: Flowershangfromthebranches <152056395+Flowershangfromthebranches@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:19:57 +0800 Subject: [PATCH 3/3] test(openai): restore alias regression destination spy --- tests/management-provider-validation.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 6ce8b7e9a3..6149eeefb6 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -1160,7 +1160,7 @@ describe("provider management validation", () => { } as OcxConfig); // This test targets the seed comparison, not the DNS policy; stub the destination // probe so the assertion stays independent of how chatgpt.com resolves locally. - spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); const server = startServer(0); try { @@ -1174,6 +1174,7 @@ describe("provider management validation", () => { // The alias overlay itself must survive the patch untouched. expect(loadConfig().providers.openai?.defaultAliases).toBe(true); } finally { + resolvedError.mockRestore(); await server.stop(true); } });