From 17b94d3e26ec65079e636c5ffaba2b45ec58267f Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Wed, 19 Aug 2026 16:07:25 -0400 Subject: [PATCH] fix(agent): an empty exec allowlist now fails closed; test the allowlist at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation slice 2 scored 0% before this, and the reason was blunt: **the exec allowlist had no test whatsoever.** The clipboard restriction could be inverted, weakened, or deleted outright and the suite stayed green. That control matters because the clipboard is readable by every process in the session. A token restricted to `cat` that may still reach the clipboard is not restricted to anything — the holder simply copies the secret out, and the allowlist looks enforced while containing nothing. Now tested: a command-restricted token is refused the clipboard, a token with no allowlist is not (so the rule is conditional rather than a blanket ban passing as one), an allowlisted command is permitted, and one outside the list is refused without the sink ever running. **A fail-open found while triaging one of the survivors.** An empty `exec_allowlist` was DROPPED at issue time, which made the token unrestricted. Automation that computes an allowlist and arrives at zero entries means "permit nothing" and was silently handed "permit everything" — a fail-open in the one field whose entire purpose is restriction. It is now refused at issue time with an error saying which of the two readings the caller probably wanted. Reachable only over the raw socket protocol, since commander requires at least one value. **An audit-accuracy gap.** A denial and a missing entry deliberately return the SAME message, so the response cannot be used to enumerate ids — the audit log is where the difference is supposed to be recorded, and nothing read it. The two reasons could be swapped with every test passing. Testing it needed two corrections of my own: the CLI resolves references locally so the request never reaches the daemon, and the shared token in that file is revoked by an earlier test, so both calls were refused at the token check instead. It also needed a stronger assertion than the one I first wrote — checking that both reasons merely APPEAR passes when they are swapped, which is exactly the mutation. It now asserts the pairing: which id got which reason. Three survivors are recorded as genuinely unkillable with reasons, rather than chased: the two `length > 0` comparisons cannot differ from `>= 0` now that an empty allowlist cannot be stored, and the capability early-return is redundant with the length check below it. agent slices 1-2 (mutants 1-16): 100% of viable mutants caught. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- js/packages/cli/src/agent.ts | 23 ++- js/packages/cli/test/agentSecurity.test.ts | 36 +++++ js/packages/cli/test/tokens.test.ts | 162 ++++++++++++++++++++- 3 files changed, 219 insertions(+), 2 deletions(-) diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index d0491a15..876be4ef 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -375,6 +375,10 @@ export class AgentDaemon { if (sinkSpec) { const allowed = auth.token.exec_allowlist; + // mutation-equivalent: `length > 0` cannot differ from `length >= 0` + // here. An empty allowlist is refused at issue time, so a stored token + // either has no allowlist (undefined, short-circuiting the `&&`) or a + // non-empty one. if (allowed && allowed.length > 0 && sinkSpec.sink === "clipboard") { // The clipboard is readable by every process in the session, so it // defeats an allowlist just as thoroughly as an arbitrary command. @@ -387,6 +391,8 @@ export class AgentDaemon { error: { ok: false, error: "denied: clipboard not permitted by this token" }, }; } + // mutation-equivalent: as above, an empty allowlist never reaches a + // stored token, so `> 0` and `>= 0` decide identically here. if (sinkSpec.sink !== "clipboard" && allowed && allowed.length > 0) { // The allowlist constrains the command WORD only. The token holder // still chooses every argument, and the child receives the secret in @@ -591,6 +597,21 @@ export class AgentDaemon { const uses = positiveIntField(msg["uses"], 1); if (uses === null) return { ok: false, error: "uses must be a positive integer" }; const secret = randomBytes(24).toString("base64url"); + // An empty exec_allowlist used to be DROPPED, which turned it into an + // unrestricted token — a fail-open. Automation that computes an + // allowlist and arrives at zero entries meant "permit nothing" and + // silently received "permit everything". Refusing is the only reading + // that cannot be a surprise: a token permitting no command is useless, + // so asking for one is a mistake worth reporting rather than + // reinterpreting. + if (Array.isArray(msg["exec_allowlist"]) && (msg["exec_allowlist"] as string[]).length === 0) { + return { + ok: false, + error: + "exec_allowlist is empty, which would permit no command at all. " + + "Omit it for an unrestricted token, or name the commands to allow.", + }; + } const record: TokenRecord = { id: `tok-${randomBytes(6).toString("hex")}`, name: String(msg["name"] ?? "agent"), @@ -602,7 +623,7 @@ export class AgentDaemon { expires_at: Math.floor(this.now() + ttl), uses_remaining: uses, revoked: false, - ...(Array.isArray(msg["exec_allowlist"]) && (msg["exec_allowlist"] as string[]).length + ...(Array.isArray(msg["exec_allowlist"]) ? { exec_allowlist: msg["exec_allowlist"] as string[] } : {}), }; diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index acd6dbf1..8e03bac5 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -452,6 +452,42 @@ describe("malformed daemon input cannot corrupt held state", () => { expect(JSON.stringify(owner)).not.toContain("abandon"); }); + it("refuses an empty exec allowlist rather than making the token unrestricted", async () => { + // Found by mutation testing. An empty allowlist used to be dropped, so a + // caller computing an allowlist that came out empty — meaning "permit + // nothing" — silently received a token permitting EVERY command. A + // fail-open in the one field whose entire job is restriction. + const r = await rawRequest({ + op: "token-issue", + cap, + fingerprint: FINGERPRINT, + scopes: ["use"], + ttl: 600, + uses: 1, + exec_allowlist: [], + }); + expect(r["ok"]).toBe(false); + expect(String(r["error"])).toContain("permit no command"); + expect(r["token"]).toBeUndefined(); + }); + + it("still accepts an allowlist with entries", async () => { + // The refusal above must be specific to the empty case; rejecting real + // allowlists would push callers toward omitting the field entirely, which + // is the permissive option. + const r = await rawRequest({ + op: "token-issue", + cap, + fingerprint: FINGERPRINT, + scopes: ["use"], + ttl: 600, + uses: 1, + exec_allowlist: ["cat"], + }); + expect(r["ok"]).toBe(true); + expect((r["record"] as Record)["exec_allowlist"]).toEqual(["cat"]); + }); + it("token-issue refuses a bare-string scope and a non-numeric ttl without crashing", async () => { const strScope = await rawRequest({ op: "token-issue", cap, fingerprint: FINGERPRINT, scopes: "reveal", ttl: 600, uses: 1, diff --git a/js/packages/cli/test/tokens.test.ts b/js/packages/cli/test/tokens.test.ts index dc31dccf..125d8e43 100644 --- a/js/packages/cli/test/tokens.test.ts +++ b/js/packages/cli/test/tokens.test.ts @@ -7,12 +7,19 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { mkdtemp, readFile, writeFile, appendFile } from "node:fs/promises"; +import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import process from "node:process"; import { mnemonics } from "@seedpass/test-vectors"; import { generateFingerprint } from "@seedpass/core"; -import { buildProgram, AgentDaemon, agentSocketPath, type ProgramIo } from "../src/index.js"; +import { + buildProgram, + AgentDaemon, + AgentClient, + agentSocketPath, + type ProgramIo, +} from "../src/index.js"; const MNEMONIC = mnemonics["abandon12"]!; const FINGERPRINT = generateFingerprint(MNEMONIC); @@ -263,3 +270,156 @@ describe("audit chain", () => { expect(String((r.error as Error).message)).toContain("audit chain broken"); }); }); + +describe("the exec allowlist", () => { + /** + * Mutation testing found this control had NO test at all: the clipboard + * check could be inverted, weakened, or removed entirely and the suite + * stayed green. + * + * It matters because the clipboard is readable by every process in the + * session. A token restricted to `cat` that may still reach the clipboard + * is not restricted to anything — the holder just copies the secret out. + * The allowlist would look enforced and contain nothing. + */ + let restricted: string; + let unrestricted: string; + + beforeAll(async () => { + restricted = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "restricted", + "--scope", "read", "use", + "--kind", "key_value", + "--ttl", "600", + "--uses", "20", + "--exec-allowlist", "cat", + )).stdout, + ).token; + unrestricted = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "unrestricted", + "--scope", "read", "use", + "--kind", "key_value", + "--ttl", "600", + "--uses", "20", + )).stdout, + ).token; + }); + + it("refuses the clipboard to a command-restricted token", async () => { + const r = await run(asTokenHolder(restricted), "use", "api-token", "--clipboard"); + expect(String((r.error as Error).message)).toContain("clipboard not permitted"); + expect(r.stdout).not.toContain("sinkable-secret"); + }); + + it("permits the clipboard to a token with no allowlist", async () => { + // The restriction has to be conditional on there BEING an allowlist, + // otherwise it is just a blanket clipboard ban wearing a disguise, and + // the test above would pass for the wrong reason. + const r = await run(asTokenHolder(unrestricted), "use", "api-token", "--clipboard"); + // Clipboard tooling is absent in CI; either it worked or it failed for a + // clipboard reason — what matters is that it was not refused by policy. + const message = r.error ? String((r.error as Error).message) : ""; + expect(message).not.toContain("clipboard not permitted"); + }); + + it("permits an allowlisted command and refuses one outside the list", async () => { + const dir = await mkdtemp(join(tmpdir(), "seedpass-allow-")); + const capture = join(dir, "out.txt"); + const script = join(dir, "notcat.sh"); + await writeFile(script, `#!/bin/sh\nprintf '%s' "$SEEDPASS_SECRET" > "${capture}"\n`, { + mode: 0o755, + }); + + // `cat` is allowlisted, so the delivery is permitted. + const allowed = await run(asTokenHolder(restricted), "use", "api-token", "--exec", "cat"); + expect(allowed.error).toBeUndefined(); + + // The script is not, however harmless it looks. + const denied = await run(asTokenHolder(restricted), "use", "api-token", "--exec", script); + expect(String((denied.error as Error).message)).toContain("not permitted by this token"); + expect(existsSync(capture)).toBe(false); + }); +}); + +describe("the audit log distinguishes what the response deliberately does not", () => { + /** + * A denial and a missing entry return the SAME message on purpose, so the + * response cannot be used to enumerate which ids exist. The audit log is + * where the difference is supposed to be recorded — that is the whole + * reason two reason strings exist. + * + * Mutation testing found the two could be swapped with every test passing, + * because nothing read the log. An audit trail that records the wrong + * reason is worse than one that records nothing: it is evidence, and it + * would be wrong. + */ + it("records why access was denied, even though the caller is not told", async () => { + // Straight to the daemon, by id. The CLI resolves references against the + // entries the token may already see, so it refuses locally and the + // request never arrives — which is correct behaviour and useless for + // testing what the DAEMON records. + // + // Entry 9999 does not exist; entry 2 (a password) exists but is outside + // this token's key_value/^api- constraints. The caller cannot tell them + // apart, and the operator must be able to. + // A fresh token: the shared one is revoked by an earlier test in this + // file, and a revoked token is refused at the token check — long before + // the entry-resolution step whose reasons this is about. + const fresh = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "audit-reasons", + "--scope", "read", "reveal", + "--kind", "key_value", + "--label-regex", "^api-", + "--ttl", "600", + "--uses", "10", + )).stdout, + ).token as string; + + const client = new AgentClient(agentSocketPath(appDir)); + await expect( + client.secret({ fingerprint: FINGERPRINT, id: "9999", token: fresh }), + ).rejects.toThrow(); + await expect( + client.secret({ fingerprint: FINGERPRINT, id: "2", token: fresh }), + ).rejects.toThrow(); + + // Both refusals must read the same to the caller, or the response itself + // becomes the enumeration oracle the audit split exists to avoid. + const [a, b] = await Promise.all([ + client.secret({ fingerprint: FINGERPRINT, id: "9999", token: fresh }).catch((e) => String(e.message)), + client.secret({ fingerprint: FINGERPRINT, id: "2", token: fresh }).catch((e) => String(e.message)), + ]); + expect(String(a).replace("9999", "ID")).toBe(String(b).replace("2", "ID")); + + const log = await readFile(join(appDir, FINGERPRINT, "audit.log"), "utf8"); + const denials = log + .split("\n") + .filter((line) => line.includes("access_denied")) + // The reason lives under `details`, alongside the action and entry id. + .map( + (line) => + JSON.parse(line) as { details?: { reason?: string; entry_id?: string } }, + ); + + const reasonFor = (entryId: string): string => + String( + denials.filter((d) => d.details?.entry_id === entryId).at(-1)?.details?.reason ?? + "", + ); + + // Asserting that both reasons merely APPEAR is not enough: swapping them + // leaves both present and the log confidently wrong. The pairing is the + // property — which id got which reason. + expect(reasonFor("9999")).toContain("no such entry"); + expect(reasonFor("2")).toContain("outside token constraints"); + }); +});