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"); + }); +});