diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 6970b8e83..e1d196b48 100644 --- a/src/infra/exec-approvals.test.ts +++ b/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"]); + }); +}); diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 2d167631c..87b8c58c1 100644 --- a/src/infra/exec-approvals.ts +++ b/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 }; }