From cf18ab8091ca2b51ec3006b3fa435c2f6f10c217 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Wed, 19 Aug 2026 13:41:55 -0400 Subject: [PATCH] test: close the gaps mutation testing found in approvals, sinks, and a flaky test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approvals scored 58%. Five survivors, all genuine: - `ttlSeconds` and `uses` validation was unguarded by any test. An approval issued with ttl 0 is expired the instant it exists; one with uses 0 can never be consumed. Either would look like a granted authorization to whoever asked for it and silently authorize nothing — the confusing failure rather than the safe one. - The expiry boundary (`<=` vs `<`) was unpinned. It now asserts that an approval expiring exactly now is refused, and that one millisecond earlier it still works, so the boundary is where it claims to be rather than approximately there. Python uses `<=`, so this is parity as well as the safer direction. Approvals now 100%. Sinks scored 67%. The survivor turned `&&` into `||` in the command-spec parser, which 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, and dropping a `--dry-run` or a target path is exactly the kind of silent change that matters for a sink being handed a secret. Sinks now 100%. Separately, the full suite caught a flaky test of my own: the recovery-split tamper check rewrote the payload's tail and, depending on the payload's padding, sometimes reproduced the original string exactly — asserting nothing. It only failed when a randomized share happened to end the wrong way. Now it flips a character and asserts the result actually differs before expecting the throw, plus a second case that tampers at four positions rather than trusting one. Five consecutive runs are clean. The agent's mapping was also wrong: agent.ts holds the high-risk session, whose tests live in highRisk.test.ts, so leaving that suite out scored the agent at 38% and blamed the code for a gap that was really a gap in the harness. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- js/packages/cli/src/agent.ts | 2 +- js/packages/cli/test/highRisk.test.ts | 46 ++++++++++++++++++++ js/packages/cli/test/sinks.test.ts | 26 +++++++++++ js/packages/core/test/recoverySplit.test.ts | Bin 8015 -> 9155 bytes scripts/mutation_test.py | 8 +++- 5 files changed, 80 insertions(+), 2 deletions(-) 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 e4dc6dab95d8bd55e29babcdce89975167c38c1e..a4f7634eb731efb0b611972195c7050632389506 100644 GIT binary patch delta 1163 zcmah}OK#La5M6_W*pP??5*4$+CdT;)1u19(iI^ae!Xjih%A&Dr;)Zs&)m@Xxz+$+= za1A6Rj=%xfaRBy69D(Y#dxBtrS5|k`t5@$m``g~vXP@t^%uAykK`Um;hzbV$X333q zPS+k?1%5U*;84jHs74q|`$uV63oA-O3uHjR)8~hW!{g!06L>8g?mU=S1SFTSrloL5L(>3~GjV$hMru*g1o4&BJ)+C2CgTWrW zkHVaAjLI6zw{FrvX?zAcm1?6fQVfdV1=>MD x227EnUTR)Sad>7)hK7=uqmrhAy@HaHl7f|j5{RR%pk%8w*-<=nbEA|KCjd*JGb;c9 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"]),