From d208dc70a4b17f217feea7d2786078d1a0be7140 Mon Sep 17 00:00:00 2001 From: "dayuan.jiang" Date: Fri, 12 Jun 2026 08:13:26 +0900 Subject: [PATCH] fix: address admin panel review findings - Security: test-model no longer resolves a stored secret when the request's baseUrl/provider differs from the stored entry, closing a path where a tampered baseUrl could exfiltrate a saved key - Save failures are now visible: the save bar shows the error in red (was masked by the persistent 'Unsaved changes' text), and per-field validation errors from the settings API are surfaced under each field - The Observability/Quota enable switch is now real: toggling off stages deletion of the group's saved values, and the toggle no longer snaps back to Enabled after saving - Env provider's default star is hidden when a panel provider is the active default (no more double star) - Clearing a credential field reverts to the stored value instead of silently deleting it; an explicit X button removes a stored secret - Form inputs are disabled during an in-flight save --- app/[lang]/admin/page.tsx | 127 ++++++++++++++++++++++++------ app/api/admin/providers/route.ts | 10 ++- app/api/admin/test-model/route.ts | 15 +++- 3 files changed, 123 insertions(+), 29 deletions(-) diff --git a/app/[lang]/admin/page.tsx b/app/[lang]/admin/page.tsx index 79c6e65..1dff9d4 100644 --- a/app/[lang]/admin/page.tsx +++ b/app/[lang]/admin/page.tsx @@ -146,23 +146,37 @@ function RestartBadge() { ) } -// Secret input: shows masked hint as placeholder, typing replaces +// Secret input: shows masked hint as placeholder, typing replaces. +// With keepOnEmpty, clearing the field reverts to the stored value +// ("keep") instead of deleting it — explicit deletion is via the X button. function SecretInput({ id, value, disabled, + keepOnEmpty, onChange, }: { id: string value: string | SecretValue | undefined disabled?: boolean - onChange: (value: string) => void + keepOnEmpty?: boolean + onChange: (value: string | SecretValue) => void }) { const [show, setShow] = useState(false) + // The stored marker as it was at mount, to revert to on empty + const [original] = useState(value) + const hadStored = isSecretValue(original) const text = typeof value === "string" ? value : "" const placeholder = isSecretValue(value) ? `Saved (${value.hint}) — type to replace` : "Not set" + const handleText = (t: string) => { + if (t === "" && keepOnEmpty && hadStored && original) { + onChange(original) + } else { + onChange(t) + } + } return (
onChange(e.target.value)} + onChange={(e) => handleText(e.target.value)} /> + {keepOnEmpty && (hadStored || text) && !disabled && ( + + )}
) } @@ -264,7 +291,9 @@ function SettingField({ : (secretState ?? currentValue) } disabled={disabled} - onChange={onChange} + onChange={(v) => + onChange(typeof v === "string" ? v : "") + } /> ) @@ -482,6 +511,7 @@ function ProviderDetail({ onUpdate({ apiKey: v })} @@ -492,6 +522,7 @@ function ProviderDetail({ onUpdate({ vertexApiKey: v })} @@ -503,6 +534,7 @@ function ProviderDetail({ @@ -513,6 +545,7 @@ function ProviderDetail({ @@ -995,14 +1028,15 @@ export default function AdminPage() { const map: SettingsMap = {} for (const s of data.settings) map[s.key] = s setSettings(map) + // Seed each toggle once from whether the group has configured + // values; don't stomp a user's explicit toggle on later saves setEnabledGroups((prev) => { const next = { ...prev } for (const group of SETTING_GROUPS) { - if (!group.toggleable) continue - const configured = SETTINGS_BY_GROUP.get(group.id)?.some( + if (!group.toggleable || group.id in next) continue + next[group.id] = !!SETTINGS_BY_GROUP.get(group.id)?.some( (d) => map[d.key]?.source !== "default", ) - if (configured) next[group.id] = true } return next }) @@ -1116,6 +1150,32 @@ export default function AdminPage() { [settings], ) + // Toggling a group off stages deletion of its saved values so the + // feature actually turns off on save; toggling on drops those deletions. + const handleGroupToggle = useCallback( + (groupId: string, enabled: boolean) => { + setSaveMessage(null) + setEnabledGroups((prev) => ({ ...prev, [groupId]: enabled })) + const keys = (SETTINGS_BY_GROUP.get(groupId) ?? []).map( + (d) => d.key, + ) + setPending((prev) => { + const next = { ...prev } + for (const key of keys) { + if (!enabled) { + // Stage deletion only for values currently set + if (settings[key]?.source !== "default") + next[key] = null + } else if (next[key] === null) { + delete next[key] + } + } + return next + }) + }, + [settings], + ) + const handleSave = useCallback(async () => { if (!authedPassword || dirtyCount === 0) return setSaving(true) @@ -1131,14 +1191,27 @@ export default function AdminPage() { applyProvidersResponse(data) } if (Object.keys(pending).length > 0) { - const data = await adminFetch( - "/api/admin/settings", - authedPassword, - { - method: "PUT", - body: JSON.stringify({ values: pending }), + const res = await fetch(getApiEndpoint("/api/admin/settings"), { + method: "PUT", + headers: { + "Content-Type": "application/json", + "x-admin-password": authedPassword, }, - ) + body: JSON.stringify({ values: pending }), + }) + const data = await res.json().catch(() => ({})) + if (!res.ok) { + // Per-field validation errors come back as {errors: {...}} + if (data.errors) { + setErrors(data.errors) + const firstKey = Object.keys(data.errors)[0] + document.getElementById(`setting-${firstKey}`)?.focus() + throw new Error("Some settings are invalid.") + } + throw new Error( + data.error || `Request failed (${res.status})`, + ) + } applySettingsResponse(data) setPending({}) } @@ -1318,7 +1391,7 @@ export default function AdminPage() { { setSaveMessage(null) @@ -1333,7 +1406,7 @@ export default function AdminPage() { const defs = SETTINGS_BY_GROUP.get(group.id) ?? [] const groupOff = group.toggleable && !enabledGroups[group.id] - const fieldsDisabled = !writable || !!groupOff + const fieldsDisabled = !writable || saving || !!groupOff return (
- setEnabledGroups( - (prev) => ({ - ...prev, - [group.id]: checked, - }), + handleGroupToggle( + group.id, + checked, ) } /> @@ -1420,7 +1491,9 @@ export default function AdminPage() { "flex min-w-0 items-center gap-1.5 truncate text-sm", saveMessage?.ok ? "text-green-600 dark:text-green-400" - : "text-muted-foreground", + : saveMessage + ? "text-destructive" + : "text-muted-foreground", )} > {saveMessage?.ok && ( @@ -1429,9 +1502,11 @@ export default function AdminPage() { aria-hidden="true" /> )} - {dirtyCount > 0 - ? "Unsaved changes" - : saveMessage?.text} + {saveMessage && !saveMessage.ok + ? saveMessage.text + : dirtyCount > 0 + ? "Unsaved changes" + : saveMessage?.text}

{dirtyCount > 0 && (
diff --git a/app/api/admin/providers/route.ts b/app/api/admin/providers/route.ts index 1db05cb..216f08c 100644 --- a/app/api/admin/providers/route.ts +++ b/app/api/admin/providers/route.ts @@ -17,15 +17,21 @@ async function payload() { // Env-based providers (AI_MODELS_CONFIG / ai-models.json) are shown // read-only in the panel; their credentials live in the environment const envConfig = await loadEnvServerModelsConfig() + const adminProviders = loadAdminProviders() + // A panel default overrides any env default (matches the merge in + // loadRawServerModelsConfig), so env stars must reflect that + const adminHasDefault = adminProviders.some( + (p) => p.isDefault && p.models.length > 0, + ) return { writable: isSettingsWritable(), - providers: maskAdminProviders(loadAdminProviders()), + providers: maskAdminProviders(adminProviders), envProviders: envConfig?.providers.map((p) => ({ name: p.name, provider: p.provider, models: p.models, - isDefault: !!p.default, + isDefault: !!p.default && !adminHasDefault, })) ?? [], } } diff --git a/app/api/admin/test-model/route.ts b/app/api/admin/test-model/route.ts index 4d6e3d3..7ab08ee 100644 --- a/app/api/admin/test-model/route.ts +++ b/app/api/admin/test-model/route.ts @@ -32,7 +32,20 @@ export async function POST(req: Request) { ) } - const [resolved] = mergeSecrets([parsed.data], loadAdminProviders()) + // SECURITY: a stored secret is only resolved from an {isSet} marker if + // the endpoint it would be sent to (provider + baseUrl) still matches + // the stored entry. Otherwise a tampered baseUrl could exfiltrate the + // stored key to an arbitrary host. Mismatches must re-supply plaintext. + const stored = loadAdminProviders().find((p) => p.id === parsed.data.id) + const sameEndpoint = + stored && + stored.provider === parsed.data.provider && + (stored.baseUrl ?? "") === (parsed.data.baseUrl ?? "") && + (stored.awsRegion ?? "") === (parsed.data.awsRegion ?? "") + const [resolved] = mergeSecrets( + [parsed.data], + sameEndpoint && stored ? [stored] : [], + ) return validateModel( new Request(new URL("/api/validate-model", req.url), {