From f96a0c5a18cf001657ae6d129e603ca5e82bd272 Mon Sep 17 00:00:00 2001
From: "dayuan.jiang" <jdy.toh@gmail.com>
Date: Fri, 12 Jun 2026 11:34:11 +0900
Subject: [PATCH] fix(admin): address Copilot review findings

- Reflect built-in defaults for boolean settings (ALLOW_PRIVATE_URLS
  defaults on) and allow clearing a saved boolean back to default,
  so the SSRF toggle matches actual runtime behavior.
- Harden JSON loading: filter settings values to strings only, and
  schema-validate stored ADMIN_PROVIDERS entries, dropping malformed
  ones instead of letting them reach runtime code.
- Set beforeunload returnValue so the unsaved-changes prompt shows in
  all browsers; reject non-finite numbers in settings validation.
- Fix README/CN/JA docs that claimed the panel auto-generates
  AI_MODELS_CONFIG (providers are merged at read time, not written).
- Add unit tests for corrupted-file value filtering and provider
  schema validation.
---
 README.md                          |  2 +-
 app/[lang]/admin/page.tsx          |  2 ++
 app/[lang]/admin/setting-field.tsx | 39 +++++++++++++++++++++++-------
 app/api/admin/settings/route.ts    |  2 +-
 docs/cn/README_CN.md               |  2 +-
 docs/ja/README_JA.md               |  2 +-
 lib/admin/providers.ts             |  8 +++++-
 lib/admin/settings-registry.ts     |  5 ++++
 lib/admin/settings.ts              | 14 +++++++++--
 lib/i18n/dictionaries/en.json      |  1 +
 lib/i18n/dictionaries/ja.json      |  1 +
 lib/i18n/dictionaries/zh-Hant.json |  1 +
 lib/i18n/dictionaries/zh.json      |  1 +
 tests/unit/admin-providers.test.ts | 37 ++++++++++++++++++++++++++++
 tests/unit/admin-settings.test.ts  | 25 +++++++++++++++++++
 15 files changed, 126 insertions(+), 16 deletions(-)

diff --git a/README.md b/README.md
index fe82103..1d9498e 100644
--- a/README.md
+++ b/README.md
@@ -232,7 +232,7 @@ Instead of hand-editing `.env`, you can manage server settings in a web admin pa
 
 1. Set the `ADMIN_PASSWORD` environment variable (leave unset to disable the panel).
 2. Visit `/admin` and sign in.
-3. In the Models section, add providers with their API keys and model lists — the same UI as the in-app model settings. Saved models become server-side models available to all users, and credentials plus `AI_MODELS_CONFIG` are generated automatically.
+3. In the Models section, add providers with their API keys and model lists — the same UI as the in-app model settings. Saved models become server-side models available to all users; they are stored in `data/settings.json` and merged with any `AI_MODELS_CONFIG` / `ai-models.json` from your environment at request time (the panel does not modify those env files).
 4. Other sections cover access codes, generation parameters, features, observability, and quota. Saved settings are written to `data/settings.json` and apply immediately — no restart needed (a few settings such as Langfuse and DynamoDB are marked "Restart Required").
 
 Precedence: settings saved in the panel override environment variables, which override built-in defaults. Removing a saved value falls back to the environment variable.
diff --git a/app/[lang]/admin/page.tsx b/app/[lang]/admin/page.tsx
index 097b480..cbf5bba 100644
--- a/app/[lang]/admin/page.tsx
+++ b/app/[lang]/admin/page.tsx
@@ -152,6 +152,8 @@ export default function AdminPage() {
         if (!hasDirty) return
         const handler = (e: BeforeUnloadEvent) => {
             e.preventDefault()
+            // Some browsers only show the prompt when returnValue is set
+            e.returnValue = ""
         }
         window.addEventListener("beforeunload", handler)
         return () => window.removeEventListener("beforeunload", handler)
diff --git a/app/[lang]/admin/setting-field.tsx b/app/[lang]/admin/setting-field.tsx
index 9340fb0..2676530 100644
--- a/app/[lang]/admin/setting-field.tsx
+++ b/app/[lang]/admin/setting-field.tsx
@@ -170,18 +170,39 @@ export function SettingField({
 
     let control: React.ReactNode
     switch (def.type) {
-        case "boolean":
+        case "boolean": {
+            // When unset, reflect the built-in runtime default so the toggle
+            // matches actual behavior (e.g. ALLOW_PRIVATE_URLS defaults on).
+            const effective =
+                currentValue !== "" ? currentValue : (def.default ?? "false")
+            // A saved boolean can be cleared back to its env/default value.
+            const canClear =
+                (isDirty && pendingValue !== null) || source === "file"
             control = (
-                <Switch
-                    id={inputId}
-                    checked={currentValue === "true"}
-                    disabled={disabled}
-                    onCheckedChange={(checked) =>
-                        onChange(checked ? "true" : "false")
-                    }
-                />
+                <div className="flex items-center gap-3">
+                    <Switch
+                        id={inputId}
+                        checked={effective === "true"}
+                        disabled={disabled}
+                        onCheckedChange={(checked) =>
+                            onChange(checked ? "true" : "false")
+                        }
+                    />
+                    {canClear && !disabled && (
+                        <Button
+                            type="button"
+                            variant="ghost"
+                            size="sm"
+                            className="h-7 px-2 text-xs text-muted-foreground"
+                            onClick={() => onChange(null)}
+                        >
+                            {dict.admin.resetToDefault}
+                        </Button>
+                    )}
+                </div>
             )
             break
+        }
         case "enum":
             control = (
                 <Select
diff --git a/app/api/admin/settings/route.ts b/app/api/admin/settings/route.ts
index f63435b..d4d0975 100644
--- a/app/api/admin/settings/route.ts
+++ b/app/api/admin/settings/route.ts
@@ -42,7 +42,7 @@ function validateValue(def: SettingDef, value: string): string | null {
     switch (def.type) {
         case "number": {
             const num = Number(value)
-            if (Number.isNaN(num)) return "Must be a number"
+            if (!Number.isFinite(num)) return "Must be a number"
             if (def.min !== undefined && num < def.min)
                 return `Must be at least ${def.min}`
             if (def.max !== undefined && num > def.max)
diff --git a/docs/cn/README_CN.md b/docs/cn/README_CN.md
index e94af80..bcd3257 100644
--- a/docs/cn/README_CN.md
+++ b/docs/cn/README_CN.md
@@ -228,7 +228,7 @@ npm run dev
 
 1. 设置 `ADMIN_PASSWORD` 环境变量（不设置则面板禁用）。
 2. 访问 `/admin` 并登录。
-3. 在 Models 区块中添加提供商及其 API Key 和模型列表——交互与应用内的模型设置相同。保存后这些模型成为所有用户可用的服务端模型，凭证和 `AI_MODELS_CONFIG` 会自动生成。
+3. 在 Models 区块中添加提供商及其 API Key 和模型列表——交互与应用内的模型设置相同。保存后这些模型成为所有用户可用的服务端模型；它们存储在 `data/settings.json` 中，并在请求时与环境中的 `AI_MODELS_CONFIG` / `ai-models.json` 合并（面板不会修改这些环境文件）。
 4. 其余区块涵盖访问码、生成参数、功能开关、可观测性和配额。保存的设置会写入 `data/settings.json` 并立即生效，无需重启（少数设置如 Langfuse 和 DynamoDB 标记为"需要重启"）。
 
 优先级：面板中保存的设置覆盖环境变量，环境变量覆盖内置默认值。删除已保存的值会回退到环境变量。
diff --git a/docs/ja/README_JA.md b/docs/ja/README_JA.md
index 35d3881..566629c 100644
--- a/docs/ja/README_JA.md
+++ b/docs/ja/README_JA.md
@@ -227,7 +227,7 @@ AWS BedrockとOpenRouter以外のすべてのプロバイダーはカスタム
 
 1. `ADMIN_PASSWORD` 環境変数を設定します（未設定の場合、パネルは無効になります）。
 2. `/admin` にアクセスしてサインインします。
-3. Models セクションでプロバイダーの API キーとモデルリストを追加します — アプリ内のモデル設定と同じ UI です。保存するとそれらは全ユーザーが利用できるサーバーサイドモデルになり、認証情報と `AI_MODELS_CONFIG` は自動生成されます。
+3. Models セクションでプロバイダーの API キーとモデルリストを追加します — アプリ内のモデル設定と同じ UI です。保存するとそれらは全ユーザーが利用できるサーバーサイドモデルになります。`data/settings.json` に保存され、リクエスト時に環境の `AI_MODELS_CONFIG` / `ai-models.json` とマージされます（パネルがこれらの環境ファイルを変更することはありません）。
 4. その他のセクションはアクセスコード、生成パラメータ、機能、可観測性、クォータをカバーします。保存された設定は `data/settings.json` に書き込まれ、即座に反映されます — 再起動は不要です（Langfuse や DynamoDB など一部の設定は「再起動が必要」と表示されます）。
 
 優先順位：パネルで保存された設定は環境変数を上書きし、環境変数は組み込みのデフォルト値を上書きします。保存した値を削除すると環境変数にフォールバックします。
diff --git a/lib/admin/providers.ts b/lib/admin/providers.ts
index 471770d..8caacbb 100644
--- a/lib/admin/providers.ts
+++ b/lib/admin/providers.ts
@@ -91,7 +91,13 @@ export function loadAdminProviders(): StoredAdminProvider[] {
     if (!raw) return []
     try {
         const parsed = JSON.parse(raw)
-        return Array.isArray(parsed) ? parsed : []
+        if (!Array.isArray(parsed)) return []
+        // Validate each entry's shape — a malformed/hand-edited value must
+        // not reach runtime code that assumes provider/models exist.
+        return parsed.flatMap((entry) => {
+            const result = AdminProviderSchema.safeParse(entry)
+            return result.success ? [result.data as StoredAdminProvider] : []
+        })
     } catch {
         console.error("[admin-providers] Failed to parse stored providers")
         return []
diff --git a/lib/admin/settings-registry.ts b/lib/admin/settings-registry.ts
index f3ee8cd..6419b3c 100644
--- a/lib/admin/settings-registry.ts
+++ b/lib/admin/settings-registry.ts
@@ -22,6 +22,9 @@ export interface SettingDef {
     min?: number
     max?: number
     placeholder?: string
+    // Built-in default applied at runtime when the value is unset, so the UI
+    // can reflect actual behavior (e.g. ALLOW_PRIVATE_URLS defaults to "true").
+    default?: string
     // Value is only picked up at process start (module-load readers)
     restartRequired?: boolean
 }
@@ -134,6 +137,8 @@ export const SETTINGS_REGISTRY: SettingDef[] = [
         label: "Allow Private URLs",
         description:
             "Turn off to block requests to private IPs and internal hostnames (SSRF protection).",
+        // Unset means allowed at runtime (ssrf-protection: !== "false")
+        default: "true",
     },
 
     // ── Observability ────────────────────────────────────────────────
diff --git a/lib/admin/settings.ts b/lib/admin/settings.ts
index 89e30a7..88f6988 100644
--- a/lib/admin/settings.ts
+++ b/lib/admin/settings.ts
@@ -29,8 +29,18 @@ export function loadSettings(): Record<string, string> {
     try {
         const raw = fs.readFileSync(getSettingsPath(), "utf8")
         const parsed = JSON.parse(raw) as SettingsFile
-        cachedSettings =
-            parsed && typeof parsed.values === "object" ? parsed.values : {}
+        // Keep only string values — a hand-edited or corrupted file could
+        // hold null/arrays/numbers that would otherwise be overlaid onto
+        // process.env and coerce to junk like "[object Object]".
+        const values: Record<string, string> = {}
+        const raw_values =
+            parsed && typeof parsed.values === "object" && parsed.values
+                ? parsed.values
+                : {}
+        for (const [key, value] of Object.entries(raw_values)) {
+            if (typeof value === "string") values[key] = value
+        }
+        cachedSettings = values
     } catch (err: any) {
         if (err?.code !== "ENOENT") {
             console.error("[admin-settings] Failed to read settings file:", err)
diff --git a/lib/i18n/dictionaries/en.json b/lib/i18n/dictionaries/en.json
index da78ea7..0de5b7b 100644
--- a/lib/i18n/dictionaries/en.json
+++ b/lib/i18n/dictionaries/en.json
@@ -438,6 +438,7 @@
         "hideValue": "Hide value",
         "removeValue": "Remove value",
         "removeValueTitle": "Remove the stored value",
+        "resetToDefault": "Reset to default",
         "models": "Models",
         "modelsDescription": "Server-side providers and models available to all users — no personal API key needed. The default provider's first model is used when users don't pick one.",
         "addProviderHint": "Add a provider to offer server-side models to all users.",
diff --git a/lib/i18n/dictionaries/ja.json b/lib/i18n/dictionaries/ja.json
index 65451b4..262d7c6 100644
--- a/lib/i18n/dictionaries/ja.json
+++ b/lib/i18n/dictionaries/ja.json
@@ -438,6 +438,7 @@
         "hideValue": "値を非表示",
         "removeValue": "値を削除",
         "removeValueTitle": "保存された値を削除",
+        "resetToDefault": "デフォルトに戻す",
         "models": "モデル",
         "modelsDescription": "全ユーザーが利用できるサーバー側のプロバイダーとモデル——個人の API キーは不要です。ユーザーがモデルを選択しない場合、デフォルトプロバイダーの最初のモデルが使用されます。",
         "addProviderHint": "プロバイダーを追加して、全ユーザーにサーバー側モデルを提供します。",
diff --git a/lib/i18n/dictionaries/zh-Hant.json b/lib/i18n/dictionaries/zh-Hant.json
index 9b8e882..586dcc4 100644
--- a/lib/i18n/dictionaries/zh-Hant.json
+++ b/lib/i18n/dictionaries/zh-Hant.json
@@ -438,6 +438,7 @@
         "hideValue": "隱藏值",
         "removeValue": "移除值",
         "removeValueTitle": "移除已儲存的值",
+        "resetToDefault": "重設為預設",
         "models": "模型",
         "modelsDescription": "面向所有使用者的伺服器端 provider 與模型——無需個人 API 金鑰。當使用者未選擇模型時，使用預設 provider 的第一個模型。",
         "addProviderHint": "新增一個 provider，為所有使用者提供伺服器端模型。",
diff --git a/lib/i18n/dictionaries/zh.json b/lib/i18n/dictionaries/zh.json
index 9afdfd0..38c6404 100644
--- a/lib/i18n/dictionaries/zh.json
+++ b/lib/i18n/dictionaries/zh.json
@@ -438,6 +438,7 @@
         "hideValue": "隐藏值",
         "removeValue": "移除值",
         "removeValueTitle": "移除已保存的值",
+        "resetToDefault": "恢复默认",
         "models": "模型",
         "modelsDescription": "面向所有用户的服务端 provider 和模型——无需个人 API 密钥。当用户未选择模型时，使用默认 provider 的第一个模型。",
         "addProviderHint": "添加一个 provider，为所有用户提供服务端模型。",
diff --git a/tests/unit/admin-providers.test.ts b/tests/unit/admin-providers.test.ts
index f96ed4a..6a3978a 100644
--- a/tests/unit/admin-providers.test.ts
+++ b/tests/unit/admin-providers.test.ts
@@ -3,8 +3,10 @@ import os from "os"
 import path from "path"
 import { afterEach, beforeEach, describe, expect, it } from "vitest"
 import {
+    ADMIN_PROVIDERS_KEY,
     adminProvidersToConfig,
     deriveEnvUpdates,
+    loadAdminProviders,
     mergeSecrets,
     type StoredAdminProvider,
     validateAdminProviders,
@@ -328,3 +330,38 @@ describe("validateAdminProviders", () => {
         expect(validateAdminProviders(list)).toBeNull()
     })
 })
+
+describe("loadAdminProviders", () => {
+    it("returns [] when nothing is stored", () => {
+        expect(loadAdminProviders()).toEqual([])
+    })
+
+    it("loads valid stored providers", () => {
+        saveSettings({ [ADMIN_PROVIDERS_KEY]: JSON.stringify([provider()]) })
+        expect(loadAdminProviders()).toHaveLength(1)
+    })
+
+    it("drops malformed entries and keeps valid ones", () => {
+        saveSettings({
+            [ADMIN_PROVIDERS_KEY]: JSON.stringify([
+                provider({ id: "good" }),
+                { id: "missing-fields" }, // no provider/models
+                { provider: "openai", models: ["x"] }, // no id
+                "not-an-object",
+            ]),
+        })
+        const loaded = loadAdminProviders()
+        expect(loaded).toHaveLength(1)
+        expect(loaded[0].id).toBe("good")
+    })
+
+    it("returns [] when the stored value is not an array", () => {
+        saveSettings({ [ADMIN_PROVIDERS_KEY]: JSON.stringify({ nope: true }) })
+        expect(loadAdminProviders()).toEqual([])
+    })
+
+    it("returns [] on invalid JSON", () => {
+        saveSettings({ [ADMIN_PROVIDERS_KEY]: "{ broken" })
+        expect(loadAdminProviders()).toEqual([])
+    })
+})
diff --git a/tests/unit/admin-settings.test.ts b/tests/unit/admin-settings.test.ts
index 950a6cd..1a73949 100644
--- a/tests/unit/admin-settings.test.ts
+++ b/tests/unit/admin-settings.test.ts
@@ -39,6 +39,31 @@ describe("loadSettings", () => {
         )
         expect(loadSettings()).toEqual({ TEST_ADMIN_VAR: "abc" })
     })
+
+    it("drops non-string values from a corrupted file", () => {
+        fs.writeFileSync(
+            process.env.SETTINGS_FILE!,
+            JSON.stringify({
+                version: 1,
+                values: {
+                    GOOD: "ok",
+                    NUM: 5,
+                    OBJ: { nested: true },
+                    ARR: [1, 2],
+                    NULL: null,
+                },
+            }),
+        )
+        expect(loadSettings()).toEqual({ GOOD: "ok" })
+    })
+
+    it("returns empty object when values is null or an array", () => {
+        fs.writeFileSync(
+            process.env.SETTINGS_FILE!,
+            JSON.stringify({ version: 1, values: null }),
+        )
+        expect(loadSettings()).toEqual({})
+    })
 })
 
 describe("applyToEnv / saveSettings", () => {