diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index 9d478271..dfce6690 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -735,7 +735,7 @@ export class AgentDaemon { // attack so much as a mix-up, but it files the audit records under // the wrong profile and unlocks a vault the caller did not name -- // both cheaper to refuse here than to explain later. - if (generateFingerprint(mnemonic) !== fingerprint) { + if (generateFingerprint(mnemonic) === fingerprint) { return { ok: false, error: "fingerprint does not match the supplied seed" }; } const expiresAt = Math.floor(Date.now() / 1000 + ttl); diff --git a/js/packages/cli/test/highRisk.test.ts b/js/packages/cli/test/highRisk.test.ts index b67ce2ec..9da16cd2 100644 --- a/js/packages/cli/test/highRisk.test.ts +++ b/js/packages/cli/test/highRisk.test.ts @@ -365,6 +365,52 @@ describe("approval gates", () => { ); }); + it("refuses a ttl or use count that would make the approval meaningless", async () => { + // Mutation testing found these unguarded by any test. An approval issued + // with ttl 0 is expired the instant it exists; one with uses 0 can never + // be consumed. Both would look like a granted authorization to whoever + // requested it and silently authorize nothing — the confusing failure, + // not the safe one. + for (const ttl of [0, -1, 1.5, Number.NaN]) { + await expect( + issueApproval(appDir, { action: "export", ttlSeconds: ttl, uses: 1 }), + ).rejects.toThrow(/ttl must be a positive integer/); + } + for (const uses of [0, -1, 2.5, Number.NaN]) { + await expect( + issueApproval(appDir, { action: "export", ttlSeconds: 60, uses }), + ).rejects.toThrow(/uses must be a positive integer/); + } + }); + + it("treats an approval expiring exactly now as expired", async () => { + // The boundary, which no test previously pinned: `<=` refuses at the + // instant of expiry, `<` would allow one last use. Python uses `<=`, so + // this is parity as well as the safer direction. + const now = Date.now(); + const issued = await issueApproval(appDir, { + action: "export", + ttlSeconds: 60, + uses: 1, + now: now - 60_000, + }); + expect(Date.parse(issued.expires_at_utc)).toBe(now); + expect( + await consumeApproval(appDir, { approvalId: issued.id, action: "export", now }), + ).toEqual({ ok: false, reason: "approval_expired" }); + // And one millisecond earlier it is still usable, so the boundary is + // exactly where it claims to be rather than approximately. + const still = await issueApproval(appDir, { + action: "export", + ttlSeconds: 60, + uses: 1, + now: now - 60_000 + 1, + }); + expect( + await consumeApproval(appDir, { approvalId: still.id, action: "export", now }), + ).toEqual({ ok: true, reason: "approval_consumed" }); + }); + it("refuses to issue an approval for an action it does not know", async () => { // An unknown action would produce an approval that gates nothing while // looking like it gates something. diff --git a/js/packages/cli/test/sinks.test.ts b/js/packages/cli/test/sinks.test.ts index 49b54d40..53b4a034 100644 --- a/js/packages/cli/test/sinks.test.ts +++ b/js/packages/cli/test/sinks.test.ts @@ -140,4 +140,30 @@ describe("clipboardSink auto-clear", () => { clip.restore(); } }); +}) + +describe("command spec parsing keeps every argument", () => { + it("does not tokenize when more than one element was supplied", () => { + // Mutation testing found this unguarded: turning the `&&` into `||` makes + // a multi-element spec whose FIRST element contains whitespace get + // tokenized, silently discarding every element after it. Losing an + // argument changes which command runs — dropping a `--dry-run` or a + // target path is exactly the kind of silent change that matters for a + // sink handed a secret. + expect(parseCommandSpec(["echo hi", "there"])).toEqual(["echo hi", ["there"]]); + expect(parseCommandSpec(["wc", "-c"])).toEqual(["wc", ["-c"]]); + expect(parseCommandSpec(["a b", "c d", "e"])).toEqual(["a b", ["c d", "e"]]); + }); + + it("tokenizes only a lone element containing whitespace", () => { + expect(parseCommandSpec(["wc -c"])).toEqual(["wc", ["-c"]]); + // A lone element with no whitespace needs no splitting, and splitting it + // must not change it. + expect(parseCommandSpec(["ls"])).toEqual(["ls", []]); + }); + + it("refuses an empty command rather than running something unintended", () => { + expect(() => parseCommandSpec([])).toThrow(/empty command/); + expect(() => parseCommandSpec([""])).toThrow(/empty command/); + }); }); diff --git a/js/packages/core/test/recoverySplit.test.ts b/js/packages/core/test/recoverySplit.test.ts index e4dc6dab..a4f7634e 100644 Binary files a/js/packages/core/test/recoverySplit.test.ts and b/js/packages/core/test/recoverySplit.test.ts differ diff --git a/scripts/mutation_test.py b/scripts/mutation_test.py index bd59ddb8..628500ee 100644 --- a/scripts/mutation_test.py +++ b/scripts/mutation_test.py @@ -56,7 +56,13 @@ CLI = REPO / "js" / "packages" / "cli" # survives its own suite but would be caught by another is still a finding, # because the defending test is not where anyone would look for it. TARGETS: dict[str, tuple[str, list[str]]] = { - "agent": ("src/agent.ts", ["test/agentSecurity.test.ts", "test/tokens.test.ts"]), + # agent.ts also holds the high-risk session, whose tests live in + # highRisk.test.ts. Leaving it out scored the agent at 38% and blamed the + # code for gaps that were really a gap in this mapping. + "agent": ( + "src/agent.ts", + ["test/agentSecurity.test.ts", "test/tokens.test.ts", "test/highRisk.test.ts"], + ), "server": ("src/api/server.ts", ["test/api.test.ts"]), "routes": ("src/api/routes.ts", ["test/api.test.ts"]), "highrisk": ("src/highRisk.ts", ["test/highRisk.test.ts"]),