From 2cc64bbe5f620a5c85ad621621bcfe54665f6f6d Mon Sep 17 00:00:00 2001
From: "dayuan.jiang" <jdy.toh@gmail.com>
Date: Mon, 15 Jun 2026 00:16:01 +0900
Subject: [PATCH] fix(admin): address follow-up Copilot findings on the prior
 fixes

- loadAdminProviders now validates against a stored-shape schema where
  secrets are plain strings, so a hand-edited ADMIN_PROVIDERS holding an
  {isSet} marker is dropped instead of later crashing maskSecret().
- loadSettings guards against array values (typeof [] === 'object'),
  which would otherwise overlay numeric keys onto process.env.
- Admin SecretInput uses the bare id so the shared component's
  <Label htmlFor> stays associated (only one ProviderDetail mounts).
- Add tests: marker-secret rejection, array-values guard, bedrock
  multi-secret round-trip.
---
 app/[lang]/admin/models-section.tsx |  4 +++-
 lib/admin/providers.ts              | 12 ++++++++++-
 lib/admin/settings.ts               |  9 ++++++---
 tests/unit/admin-providers.test.ts  | 31 +++++++++++++++++++++++++++++
 tests/unit/admin-settings.test.ts   | 11 +++++++++-
 5 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/app/[lang]/admin/models-section.tsx b/app/[lang]/admin/models-section.tsx
index 00c66d8..4f293c7 100644
--- a/app/[lang]/admin/models-section.tsx
+++ b/app/[lang]/admin/models-section.tsx
@@ -182,8 +182,10 @@ function ProviderDetail({
                 disabled={disabled}
                 onChange={(field, value) => onUpdate({ [field]: value })}
                 renderSecret={({ field, id }) => (
+                    // Bare id keeps the shared component's <Label htmlFor={id}>
+                    // associated; only one ProviderDetail is mounted at a time.
                     <SecretInput
-                        id={`${id}-${provider.id}`}
+                        id={id}
                         keepOnEmpty
                         value={provider[field]}
                         disabled={disabled}
diff --git a/lib/admin/providers.ts b/lib/admin/providers.ts
index 8caacbb..67d5034 100644
--- a/lib/admin/providers.ts
+++ b/lib/admin/providers.ts
@@ -45,6 +45,16 @@ export const AdminProviderSchema = z.object({
 
 export const AdminProvidersSchema = z.array(AdminProviderSchema)
 
+// Stored shape: secrets are plain strings (never {isSet} markers, which
+// only exist in transit). Used to validate ADMIN_PROVIDERS on load so a
+// hand-edited/corrupted value can't slip a marker object past maskSecret.
+const StoredAdminProviderSchema = AdminProviderSchema.extend({
+    apiKey: z.string().optional(),
+    awsAccessKeyId: z.string().optional(),
+    awsSecretAccessKey: z.string().optional(),
+    vertexApiKey: z.string().optional(),
+})
+
 export type AdminProviderInput = z.infer<typeof AdminProviderSchema>
 
 // Stored form: secrets are plain strings
@@ -95,7 +105,7 @@ export function loadAdminProviders(): StoredAdminProvider[] {
         // 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)
+            const result = StoredAdminProviderSchema.safeParse(entry)
             return result.success ? [result.data as StoredAdminProvider] : []
         })
     } catch {
diff --git a/lib/admin/settings.ts b/lib/admin/settings.ts
index 88f6988..664c370 100644
--- a/lib/admin/settings.ts
+++ b/lib/admin/settings.ts
@@ -33,11 +33,14 @@ export function loadSettings(): Record<string, string> {
         // 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
+        const rawValues =
+            parsed &&
+            typeof parsed.values === "object" &&
+            parsed.values &&
+            !Array.isArray(parsed.values)
                 ? parsed.values
                 : {}
-        for (const [key, value] of Object.entries(raw_values)) {
+        for (const [key, value] of Object.entries(rawValues)) {
             if (typeof value === "string") values[key] = value
         }
         cachedSettings = values
diff --git a/tests/unit/admin-providers.test.ts b/tests/unit/admin-providers.test.ts
index 6a3978a..9a552d4 100644
--- a/tests/unit/admin-providers.test.ts
+++ b/tests/unit/admin-providers.test.ts
@@ -7,6 +7,7 @@ import {
     adminProvidersToConfig,
     deriveEnvUpdates,
     loadAdminProviders,
+    maskAdminProviders,
     mergeSecrets,
     type StoredAdminProvider,
     validateAdminProviders,
@@ -341,6 +342,22 @@ describe("loadAdminProviders", () => {
         expect(loadAdminProviders()).toHaveLength(1)
     })
 
+    it("round-trips a bedrock provider with multiple string secrets", () => {
+        const bedrock = provider({
+            provider: "bedrock",
+            apiKey: undefined,
+            awsAccessKeyId: "AKIA123",
+            awsSecretAccessKey: "secret",
+            awsRegion: "us-west-2",
+            models: ["claude-x"],
+        })
+        saveSettings({ [ADMIN_PROVIDERS_KEY]: JSON.stringify([bedrock]) })
+        const loaded = loadAdminProviders()
+        expect(loaded).toHaveLength(1)
+        expect(loaded[0].awsAccessKeyId).toBe("AKIA123")
+        expect(loaded[0].awsSecretAccessKey).toBe("secret")
+    })
+
     it("drops malformed entries and keeps valid ones", () => {
         saveSettings({
             [ADMIN_PROVIDERS_KEY]: JSON.stringify([
@@ -364,4 +381,18 @@ describe("loadAdminProviders", () => {
         saveSettings({ [ADMIN_PROVIDERS_KEY]: "{ broken" })
         expect(loadAdminProviders()).toEqual([])
     })
+
+    it("drops entries whose secret is an {isSet} marker, not a string", () => {
+        // A hand-edited file could hold a transit-only marker object; if it
+        // slipped through, maskSecret() would throw on a non-string value.
+        saveSettings({
+            [ADMIN_PROVIDERS_KEY]: JSON.stringify([
+                { ...provider(), apiKey: { isSet: true, hint: "…1234" } },
+            ]),
+        })
+        const loaded = loadAdminProviders()
+        expect(loaded).toEqual([])
+        // Masking the loaded list must not throw
+        expect(() => maskAdminProviders(loaded)).not.toThrow()
+    })
 })
diff --git a/tests/unit/admin-settings.test.ts b/tests/unit/admin-settings.test.ts
index 1a73949..8610751 100644
--- a/tests/unit/admin-settings.test.ts
+++ b/tests/unit/admin-settings.test.ts
@@ -57,13 +57,22 @@ describe("loadSettings", () => {
         expect(loadSettings()).toEqual({ GOOD: "ok" })
     })
 
-    it("returns empty object when values is null or an array", () => {
+    it("returns empty object when values is null", () => {
         fs.writeFileSync(
             process.env.SETTINGS_FILE!,
             JSON.stringify({ version: 1, values: null }),
         )
         expect(loadSettings()).toEqual({})
     })
+
+    it("returns empty object when values is an array (no numeric keys)", () => {
+        fs.writeFileSync(
+            process.env.SETTINGS_FILE!,
+            JSON.stringify({ version: 1, values: ["a", "b"] }),
+        )
+        // Without the Array.isArray guard this would yield { "0": "a", ... }
+        expect(loadSettings()).toEqual({})
+    })
 })
 
 describe("applyToEnv / saveSettings", () => {