From db46a894b95be8f6d0e206431302179668d06170 Mon Sep 17 00:00:00 2001 From: Prudhvivuda Date: Sat, 25 Jul 2026 11:49:33 -0400 Subject: [PATCH] fix(api): prevent silent lost updates on concurrent settings writes (#7784) Add opt-in settingsRevision / If-Match optimistic concurrency so stale PATCH writers get 409 instead of silently clobbering map settings. --- .../components/ModelAliasesUnified.tsx | 29 +++- .../components/ProviderAccountRoutingCard.tsx | 7 +- src/app/api/settings/route.ts | 66 +++++++- src/lib/db/settings.ts | 44 ++++- src/lib/localDb.ts | 1 + src/shared/validation/settingsSchemas.ts | 2 + tests/unit/settings-cas-7784.test.ts | 157 ++++++++++++++++++ 7 files changed, 294 insertions(+), 12 deletions(-) create mode 100644 tests/unit/settings-cas-7784.test.ts diff --git a/src/app/(dashboard)/dashboard/settings/components/ModelAliasesUnified.tsx b/src/app/(dashboard)/dashboard/settings/components/ModelAliasesUnified.tsx index f7e7ae265c59..79697ed634b8 100644 --- a/src/app/(dashboard)/dashboard/settings/components/ModelAliasesUnified.tsx +++ b/src/app/(dashboard)/dashboard/settings/components/ModelAliasesUnified.tsx @@ -110,12 +110,24 @@ export default function ModelAliasesUnified() { if (!fromValue.trim() || !toValue.trim()) return; setSaving(true); try { - const updated = [...wildcardAliases, { pattern: fromValue.trim(), target: toValue.trim() }]; + const settingsRes = await fetch("/api/settings", { cache: "no-store" }); + if (!settingsRes.ok) throw new Error("Failed to load current settings"); + const settingsData = await settingsRes.json(); + const revision = + typeof settingsData.settingsRevision === "number" ? settingsData.settingsRevision : 0; + const currentWildcards = Array.isArray(settingsData.wildcardAliases) + ? settingsData.wildcardAliases + : wildcardAliases; + const updated = [ + ...currentWildcards, + { pattern: fromValue.trim(), target: toValue.trim() }, + ]; const res = await fetch("/api/settings", { method: "PATCH", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ wildcardAliases: updated }), + body: JSON.stringify({ wildcardAliases: updated, expectedRevision: revision }), }); + if (res.status === 409) throw new Error("Settings conflict — refresh and retry"); if (!res.ok) throw new Error("Failed to save wildcard alias"); setWildcardAliases(updated); setFromValue(""); @@ -132,12 +144,21 @@ export default function ModelAliasesUnified() { const removeWildcardAlias = async (index: number) => { setSaving(true); try { - const updated = wildcardAliases.filter((_, currentIndex) => currentIndex !== index); + const settingsRes = await fetch("/api/settings", { cache: "no-store" }); + if (!settingsRes.ok) throw new Error("Failed to load current settings"); + const settingsData = await settingsRes.json(); + const revision = + typeof settingsData.settingsRevision === "number" ? settingsData.settingsRevision : 0; + const currentWildcards = Array.isArray(settingsData.wildcardAliases) + ? settingsData.wildcardAliases + : wildcardAliases; + const updated = currentWildcards.filter((_, currentIndex) => currentIndex !== index); const res = await fetch("/api/settings", { method: "PATCH", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ wildcardAliases: updated }), + body: JSON.stringify({ wildcardAliases: updated, expectedRevision: revision }), }); + if (res.status === 409) throw new Error("Settings conflict — refresh and retry"); if (!res.ok) throw new Error("Failed to remove wildcard alias"); setWildcardAliases(updated); showStatus("success", translateOrFallback(t, "saved", "Saved")); diff --git a/src/app/(dashboard)/dashboard/settings/components/ProviderAccountRoutingCard.tsx b/src/app/(dashboard)/dashboard/settings/components/ProviderAccountRoutingCard.tsx index 40bd288f7e29..ac7c5a9f8f26 100644 --- a/src/app/(dashboard)/dashboard/settings/components/ProviderAccountRoutingCard.tsx +++ b/src/app/(dashboard)/dashboard/settings/components/ProviderAccountRoutingCard.tsx @@ -55,6 +55,7 @@ function useProviderAccountRoutingState(providerKey: string) { const res = await fetch("/api/settings", { cache: "no-store" }); if (!res.ok) throw new Error("Failed to fetch current settings"); const data = await res.json(); + const revision = typeof data.settingsRevision === "number" ? data.settingsRevision : 0; const current = (data?.providerStrategies || {}) as Record< string, ProviderStrategyOverride @@ -70,8 +71,12 @@ function useProviderAccountRoutingState(providerKey: string) { const patchRes = await fetch("/api/settings", { method: "PATCH", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ providerStrategies: updated }), + body: JSON.stringify({ providerStrategies: updated, expectedRevision: revision }), }); + if (patchRes.status === 409) { + await load(); + throw new Error("Settings conflict — refreshed from server"); + } if (!patchRes.ok) throw new Error("Failed to save provider routing settings"); } catch (e) { console.error(e); diff --git a/src/app/api/settings/route.ts b/src/app/api/settings/route.ts index 8d3a9bf6da98..29427f6943a9 100644 --- a/src/app/api/settings/route.ts +++ b/src/app/api/settings/route.ts @@ -1,5 +1,6 @@ import { NextResponse } from "next/server"; -import { getSettings, updateSettings } from "@/lib/localDb"; +import { getSettings, getSettingsRevision, updateSettings } from "@/lib/localDb"; +import { SettingsRevisionConflictError } from "@/lib/db/settings"; import { getRuntimePorts } from "@/lib/runtime/ports"; import { updateSettingsSchema } from "@/shared/validation/settingsSchemas"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; @@ -40,6 +41,31 @@ export const revalidate = 0; /** Response headers applied to every successful GET/PATCH on /api/settings. */ const SETTINGS_RESPONSE_HEADERS = { "Cache-Control": "no-store" } as const; +function settingsResponseHeaders(settingsRevision: number): Record { + return { + ...SETTINGS_RESPONSE_HEADERS, + ETag: String(settingsRevision), + }; +} + +/** Parse opt-in CAS token from If-Match (preferred) or PATCH body. */ +function parseExpectedRevision( + request: Request, + body: Record +): number | undefined { + const ifMatch = request.headers.get("If-Match"); + if (ifMatch !== null) { + const trimmed = ifMatch.replace(/^W\/"/, "").replace(/"$/, "").trim(); + const parsed = Number(trimmed); + if (Number.isInteger(parsed) && parsed >= 0) return parsed; + } + const fromBody = body.expectedRevision; + if (typeof fromBody === "number" && Number.isInteger(fromBody) && fromBody >= 0) { + return fromBody; + } + return undefined; +} + /** * Settings keys whose change broadens attack surface. Spec §Security: * password re-auth is required when any of these is present in a PATCH body. @@ -127,7 +153,11 @@ function computeSettingsDiff( function attemptedKeysOf(body: Record | null | undefined): string[] { if (!body || typeof body !== "object") return []; return Object.keys(body).filter( - (k) => k !== "currentPassword" && k !== "newPassword" && k !== "password" + (k) => + k !== "currentPassword" && + k !== "newPassword" && + k !== "password" && + k !== "expectedRevision" ); } @@ -161,6 +191,7 @@ export async function GET(request: Request) { try { const settings = await getSettings(); + const settingsRevision = await getSettingsRevision(); const { password, ...safeSettings } = settings; const runtimePorts = getRuntimePorts(); @@ -181,6 +212,7 @@ export async function GET(request: Request) { return NextResponse.json( { ...safeSettings, + settingsRevision, hasPassword: hasManagementPasswordConfigured(settings), runtimePorts, apiPort: runtimePorts.apiPort, @@ -192,7 +224,7 @@ export async function GET(request: Request) { ? { cliproxyapi_model_mapping: cliproxyapiModelMapping } : {}), }, - { headers: SETTINGS_RESPONSE_HEADERS } + { headers: settingsResponseHeaders(settingsRevision) } ); } catch (error) { console.log("Error getting settings:", error); @@ -219,6 +251,7 @@ export async function PATCH(request: Request) { ); } const attemptedKeys = attemptedKeysOf(rawBody); + const expectedRevision = parseExpectedRevision(request, rawBody); try { // Zod validation @@ -350,10 +383,29 @@ export async function PATCH(request: Request) { delete body.newPassword; } delete body.currentPassword; + delete body.expectedRevision; // Snapshot BEFORE the write so the success row can record a real diff. const beforeSnapshot = (await getSettings()) as Record; - const settings = await updateSettings(body); + let settings: Awaited>; + try { + settings = await updateSettings(body, { expectedRevision }); + } catch (error) { + if (error instanceof SettingsRevisionConflictError) { + emitSettingsFailureAudit(request, actor, "SETTINGS_REVISION_CONFLICT", attemptedKeys); + return NextResponse.json( + { + error: { + code: "SETTINGS_REVISION_CONFLICT", + message: "Settings changed since this snapshot; refresh and retry", + currentRevision: error.currentRevision, + }, + }, + { status: 409, headers: settingsResponseHeaders(error.currentRevision) } + ); + } + throw error; + } // Sync CLIProxyAPI settings to upstream_proxy_config table const cpaUrl = rawBody.cliproxyapi_url as string | undefined; @@ -437,7 +489,11 @@ export async function PATCH(request: Request) { } const { password, ...safeSettings } = settings; - return NextResponse.json(safeSettings, { headers: SETTINGS_RESPONSE_HEADERS }); + const settingsRevision = await getSettingsRevision(); + return NextResponse.json( + { ...safeSettings, settingsRevision }, + { headers: settingsResponseHeaders(settingsRevision) } + ); } catch (error) { console.log("Error updating settings:", error); return NextResponse.json({ error: "Failed to update settings" }, { status: 500 }); diff --git a/src/lib/db/settings.ts b/src/lib/db/settings.ts index 03b2740304a5..401a4f2231e8 100644 --- a/src/lib/db/settings.ts +++ b/src/lib/db/settings.ts @@ -90,6 +90,35 @@ function withFamilyDefault(value: ProxyValue): ProxyValue { // ──────────────── Settings ──────────────── +/** Internal key_value row — not exposed as a user-facing settings field. */ +export const SETTINGS_REVISION_KEY = "_settingsRevision"; + +export class SettingsRevisionConflictError extends Error { + readonly code = "SETTINGS_REVISION_CONFLICT" as const; + + constructor(public readonly currentRevision: number) { + super("Settings revision mismatch"); + this.name = "SettingsRevisionConflictError"; + } +} + +function readSettingsRevision(db: ReturnType): number { + const row = db + .prepare("SELECT value FROM key_value WHERE namespace = 'settings' AND key = ?") + .get(SETTINGS_REVISION_KEY) as { value?: string } | undefined; + if (!row?.value) return 0; + try { + const parsed = JSON.parse(row.value) as unknown; + return typeof parsed === "number" && Number.isInteger(parsed) && parsed >= 0 ? parsed : 0; + } catch { + return 0; + } +} + +export async function getSettingsRevision(): Promise { + return readSettingsRevision(getDbInstance()); +} + /** * #7274: read-fallback for the codexSessionAffinityTtlMs -> sessionAffinityTtlMs * rename. Migration 124 already backfills the new key from any pre-existing @@ -217,7 +246,7 @@ export async function getSettings() { const record = toRecord(row); const key = typeof record.key === "string" ? record.key : null; const rawValue = typeof record.value === "string" ? record.value : null; - if (!key || rawValue === null) continue; + if (!key || rawValue === null || key.startsWith("_")) continue; try { settings[key] = JSON.parse(rawValue); } catch { @@ -246,7 +275,10 @@ export async function getSettings() { return settings; } -export async function updateSettings(updates: Record) { +export async function updateSettings( + updates: Record, + options?: { expectedRevision?: number } +) { // Detect first-time setup completion before we overwrite settings. let setupJustCompleted = false; if (updates.setupComplete === true) { @@ -263,10 +295,18 @@ export async function updateSettings(updates: Record) { "INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES ('settings', ?, ?)" ); const tx = db.transaction(() => { + const currentRevision = readSettingsRevision(db); + if ( + options?.expectedRevision !== undefined && + options.expectedRevision !== currentRevision + ) { + throw new SettingsRevisionConflictError(currentRevision); + } for (const [key, value] of Object.entries(updates)) { const toStore = key === "oidcClientSecret" ? encrypt(value as string) : value; insert.run(key, JSON.stringify(toStore)); } + insert.run(SETTINGS_REVISION_KEY, JSON.stringify(currentRevision + 1)); }); tx(); backupDbFile("pre-write"); diff --git a/src/lib/localDb.ts b/src/lib/localDb.ts index 82b041f62164..d8331873a377 100755 --- a/src/lib/localDb.ts +++ b/src/lib/localDb.ts @@ -135,6 +135,7 @@ export type { export { // Settings getSettings, + getSettingsRevision, updateSettings, isCloudEnabled, diff --git a/src/shared/validation/settingsSchemas.ts b/src/shared/validation/settingsSchemas.ts index 7df36c5f0265..29b3d0756ce1 100644 --- a/src/shared/validation/settingsSchemas.ts +++ b/src/shared/validation/settingsSchemas.ts @@ -92,6 +92,8 @@ const transformObfuscateWordsSchema = z.object({ }); export const updateSettingsSchema = z.object({ + /** #7784: opt-in optimistic concurrency — must match GET settingsRevision / ETag. */ + expectedRevision: z.number().int().nonnegative().optional(), newPassword: z.string().min(1).max(200).optional(), currentPassword: z.string().max(200).optional(), credentialRedactionEnabled: z.boolean().optional(), diff --git a/tests/unit/settings-cas-7784.test.ts b/tests/unit/settings-cas-7784.test.ts new file mode 100644 index 000000000000..8d9f8df9763b --- /dev/null +++ b/tests/unit/settings-cas-7784.test.ts @@ -0,0 +1,157 @@ +/** + * #7784 — concurrent settings writers must not silently clobber map keys like + * providerStrategies. Opt-in optimistic concurrency via expectedRevision / If-Match. + */ +import { describe, test, beforeEach, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { makeManagementSessionRequest } from "../helpers/managementSession.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-settings-cas-7784-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.REQUIRE_API_KEY = "false"; +if (!process.env.API_KEY_SECRET) { + process.env.API_KEY_SECRET = "test-settings-cas-7784-" + Date.now(); +} + +const core = await import("../../src/lib/db/core.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const settingsRoute = await import("../../src/app/api/settings/route.ts"); + +beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +}); + +after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +describe("#7784 settings optimistic concurrency", () => { + test("GET /api/settings exposes settingsRevision and ETag", async () => { + const response = await settingsRoute.GET( + await makeManagementSessionRequest("http://localhost/api/settings", { method: "GET" }) + ); + const body = (await response.json()) as Record; + + assert.equal(response.status, 200); + assert.equal(typeof body.settingsRevision, "number"); + assert.equal(response.headers.get("ETag"), String(body.settingsRevision)); + }); + + test("concurrent providerStrategies writes with CAS: second writer gets 409, first change kept", async () => { + const getRes = await settingsRoute.GET( + await makeManagementSessionRequest("http://localhost/api/settings", { method: "GET" }) + ); + const snapshot = (await getRes.json()) as { settingsRevision: number }; + const baseRevision = snapshot.settingsRevision; + + const clientA = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + body: { + expectedRevision: baseRevision, + providerStrategies: { + codex: { fallbackStrategy: "round-robin", stickyRoundRobinLimit: 3 }, + }, + }, + }) + ); + assert.equal(clientA.status, 200); + + const clientB = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + body: { + expectedRevision: baseRevision, + providerStrategies: { + antigravity: { fallbackStrategy: "fill-first" }, + }, + }, + }) + ); + assert.equal(clientB.status, 409, "stale revision must reject with 409 Conflict"); + const conflictBody = (await clientB.json()) as { + error: { code: string; currentRevision?: number }; + }; + assert.equal(conflictBody.error.code, "SETTINGS_REVISION_CONFLICT"); + + const settings = await settingsDb.getSettings(); + const strategies = settings.providerStrategies as Record; + assert.ok(strategies.codex, "codex override from client A must survive"); + assert.equal(strategies.antigravity, undefined, "stale client B write must not apply"); + }); + + test("If-Match header is accepted in place of expectedRevision body field", async () => { + const getRes = await settingsRoute.GET( + await makeManagementSessionRequest("http://localhost/api/settings", { method: "GET" }) + ); + const snapshot = (await getRes.json()) as { settingsRevision: number }; + + const okRes = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + headers: { "If-Match": String(snapshot.settingsRevision) }, + body: { + providerStrategies: { + glm: { fallbackStrategy: "priority" }, + }, + }, + }) + ); + assert.equal(okRes.status, 200); + + const staleRes = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + headers: { "If-Match": String(snapshot.settingsRevision) }, + body: { + providerStrategies: { + openai: { fallbackStrategy: "random" }, + }, + }, + }) + ); + assert.equal(staleRes.status, 409); + }); + + test("PATCH without expectedRevision remains backward compatible", async () => { + await settingsDb.updateSettings({ + providerStrategies: { codex: { fallbackStrategy: "round-robin" } }, + }); + + const response = await settingsRoute.PATCH( + await makeManagementSessionRequest("http://localhost/api/settings", { + method: "PATCH", + body: { + providerStrategies: { antigravity: { fallbackStrategy: "fill-first" } }, + }, + }) + ); + + assert.equal(response.status, 200); + const settings = await settingsDb.getSettings(); + const strategies = settings.providerStrategies as Record; + assert.equal(strategies.codex, undefined, "legacy unconditional replace still overwrites"); + assert.ok(strategies.antigravity); + }); + + test("updateSettings throws SettingsRevisionConflictError on revision mismatch", async () => { + await settingsDb.updateSettings({ debugMode: false }); + const revision = await settingsDb.getSettingsRevision(); + await settingsDb.updateSettings({ debugMode: true }); + + await assert.rejects( + () => settingsDb.updateSettings({ debugMode: false }, { expectedRevision: revision }), + (err: unknown) => { + assert.ok(err instanceof settingsDb.SettingsRevisionConflictError); + assert.equal((err as settingsDb.SettingsRevisionConflictError).currentRevision, revision + 1); + return true; + } + ); + }); +});