fix(exec-approvals): coerce bare string allowlist entries to objects (#9790)

src/infra/exec-approvals.test.ts

@@ -11,12 +11,14 @@ import {
   matchAllowlist,
   maxAsk,
   minSecurity,
+  normalizeExecApprovals,
   normalizeSafeBins,
   requiresExecApproval,
   resolveCommandResolution,
   resolveExecApprovals,
   resolveExecApprovalsFromFile,
   type ExecAllowlistEntry,
+  type ExecApprovalsFile,
 } from "./exec-approvals.js";
 
 function makePathEnv(binDir: string): NodeJS.ProcessEnv {
@@ -584,3 +586,98 @@ describe("exec approvals default agent migration", () => {
     expect(resolved.file.agents?.default).toBeUndefined();
   });
 });
+
+describe("normalizeExecApprovals handles string allowlist entries (#9790)", () => {
+  it("converts bare string entries to proper ExecAllowlistEntry objects", () => {
+    // Simulates a corrupted or legacy config where allowlist contains plain
+    // strings (e.g. ["ls", "cat"]) instead of { pattern: "..." } objects.
+    const file = {
+      version: 1,
+      agents: {
+        main: {
+          mode: "allowlist",
+          allowlist: ["things", "remindctl", "memo", "which", "ls", "cat", "echo"],
+        },
+      },
+    } as unknown as ExecApprovalsFile;
+
+    const normalized = normalizeExecApprovals(file);
+    const entries = normalized.agents?.main?.allowlist ?? [];
+
+    // Each entry must be a proper object with a pattern string, not a
+    // spread-string like {"0":"t","1":"h","2":"i",...}
+    for (const entry of entries) {
+      expect(entry).toHaveProperty("pattern");
+      expect(typeof entry.pattern).toBe("string");
+      expect(entry.pattern.length).toBeGreaterThan(0);
+      // Spread-string corruption would create numeric keys — ensure none exist
+      expect(entry).not.toHaveProperty("0");
+    }
+
+    expect(entries.map((e) => e.pattern)).toEqual([
+      "things",
+      "remindctl",
+      "memo",
+      "which",
+      "ls",
+      "cat",
+      "echo",
+    ]);
+  });
+
+  it("preserves proper ExecAllowlistEntry objects unchanged", () => {
+    const file: ExecApprovalsFile = {
+      version: 1,
+      agents: {
+        main: {
+          allowlist: [{ pattern: "/usr/bin/ls" }, { pattern: "/usr/bin/cat", id: "existing-id" }],
+        },
+      },
+    };
+
+    const normalized = normalizeExecApprovals(file);
+    const entries = normalized.agents?.main?.allowlist ?? [];
+
+    expect(entries).toHaveLength(2);
+    expect(entries[0]?.pattern).toBe("/usr/bin/ls");
+    expect(entries[1]?.pattern).toBe("/usr/bin/cat");
+    expect(entries[1]?.id).toBe("existing-id");
+  });
+
+  it("handles mixed string and object entries in the same allowlist", () => {
+    const file = {
+      version: 1,
+      agents: {
+        main: {
+          allowlist: ["ls", { pattern: "/usr/bin/cat" }, "echo"],
+        },
+      },
+    } as unknown as ExecApprovalsFile;
+
+    const normalized = normalizeExecApprovals(file);
+    const entries = normalized.agents?.main?.allowlist ?? [];
+
+    expect(entries).toHaveLength(3);
+    expect(entries.map((e) => e.pattern)).toEqual(["ls", "/usr/bin/cat", "echo"]);
+    for (const entry of entries) {
+      expect(entry).not.toHaveProperty("0");
+    }
+  });
+
+  it("drops empty string entries", () => {
+    const file = {
+      version: 1,
+      agents: {
+        main: {
+          allowlist: ["", "  ", "ls"],
+        },
+      },
+    } as unknown as ExecApprovalsFile;
+
+    const normalized = normalizeExecApprovals(file);
+    const entries = normalized.agents?.main?.allowlist ?? [];
+
+    // Only "ls" should survive; empty/whitespace strings should be dropped
+    expect(entries.map((e) => e.pattern)).toEqual(["ls"]);
+  });
+});

src/infra/exec-approvals.ts

@@ -132,6 +132,39 @@ function ensureDir(filePath: string) {
   fs.mkdirSync(dir, { recursive: true });
 }
 
+/**
+ * Coerce each allowlist item into a proper {@link ExecAllowlistEntry}.
+ * Older config formats or manual edits may store bare strings (e.g.
+ * `["ls", "cat"]`).  Spreading a string (`{ ..."ls" }`) produces
+ * `{"0":"l","1":"s"}`, so we must detect and convert strings first.
+ * Non-object, non-string entries and blank strings are dropped.
+ */
+function coerceAllowlistEntries(
+  allowlist: unknown[] | undefined,
+): ExecAllowlistEntry[] | undefined {
+  if (!Array.isArray(allowlist) || allowlist.length === 0) {
+    return allowlist as ExecAllowlistEntry[] | undefined;
+  }
+  let changed = false;
+  const result: ExecAllowlistEntry[] = [];
+  for (const item of allowlist) {
+    if (typeof item === "string") {
+      const trimmed = item.trim();
+      if (trimmed) {
+        result.push({ pattern: trimmed });
+        changed = true;
+      } else {
+        changed = true; // dropped empty string
+      }
+    } else if (item && typeof item === "object" && !Array.isArray(item)) {
+      result.push(item as ExecAllowlistEntry);
+    } else {
+      changed = true; // dropped invalid entry
+    }
+  }
+  return changed ? (result.length > 0 ? result : undefined) : (allowlist as ExecAllowlistEntry[]);
+}
+
 function ensureAllowlistIds(
   allowlist: ExecAllowlistEntry[] | undefined,
 ): ExecAllowlistEntry[] | undefined {
@@ -160,7 +193,8 @@ export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFi
     delete agents.default;
   }
   for (const [key, agent] of Object.entries(agents)) {
-    const allowlist = ensureAllowlistIds(agent.allowlist);
+    const coerced = coerceAllowlistEntries(agent.allowlist as unknown[]);
+    const allowlist = ensureAllowlistIds(coerced);
     if (allowlist !== agent.allowlist) {
       agents[key] = { ...agent, allowlist };
     }