From 12b945ca7ee99b50c8cd4df8f9509792e98f6831 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Wed, 19 Aug 2026 15:47:12 -0400 Subject: [PATCH] test(agent): pin token expiry at its exact boundary; give the daemon a clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation testing found `token.expires_at <= now` could become `<` with every test still passing. The suite checked well before and well after expiry, which cannot tell the two apart — so a token was free to be usable for one more request than it claimed, which is the wrong direction for a credential. The daemon now takes an injectable clock, so the boundary can be asserted exactly rather than raced against wall time: at T-1 the token works, at exactly T it is refused. Every expiry decision AND every expiry assignment goes through that clock — a token issued against real time while checked against test time would never expire, which would have made the test meaningless. The TOTP timestamp-skew window uses it too, since a clock that moved one but not the other is a difference no production path should have either. Also pins the owner capability: an empty or absent capability is refused, and so is one of the right length but wrong content — the latter proving the constant-time comparison is what rejects it rather than the length check. One survivor is now recorded as genuinely unkillable rather than chased. `!presented || !this.capability` cannot change behaviour when flipped to `&&`, because the length comparison below rejects an empty string either way. The harness learned a `mutation-equivalent:` marker that must carry a REASON, so future survivor lists mean "look at this" instead of "this again" — and the judgement stays re-checkable rather than merely trusted. The harness also gained `--offset`, so a long target is measured in foreground-sized slices. That is a safety property, not a convenience: this script writes broken security code into the working tree, and it must never run unattended alongside commits again. agent slice 1 (mutants 1-6): 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 | 28 ++++-- js/packages/cli/test/agentSecurity.test.ts | 102 ++++++++++++++++++++- scripts/mutation_test.py | 53 +++++++++-- 3 files changed, 167 insertions(+), 16 deletions(-) diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index 9d478271..d0491a15 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -173,6 +173,13 @@ export class AgentDaemon { private readonly defaultTtl = DEFAULT_TTL_SECONDS, /** Profile root; required for token-mode vault/secret serving. */ private readonly appDir?: string, + /** + * Seconds since the epoch. Injectable so expiry can be tested at its + * exact boundary rather than approximately — "expires at T" has to mean + * denied AT T, and a test that only checks T-1 and T+1 cannot tell + * `<=` from `<`. + */ + private readonly now: () => number = () => Date.now() / 1000, ) {} /** @@ -203,6 +210,10 @@ export class AgentDaemon { */ private isOwner(msg: Record): boolean { const presented = String(msg["cap"] ?? ""); + // mutation-equivalent: turning this `||` into `&&` cannot change the + // outcome — an empty string on either side fails the length comparison + // below, so the early return is a statement of intent rather than the + // thing doing the rejecting. Kept because the rule is worth stating. if (!presented || !this.capability) return false; const a = Buffer.from(presented); const b = Buffer.from(this.capability); @@ -261,7 +272,7 @@ export class AgentDaemon { ); if (!token) return { deny: "unknown token" }; if (token.revoked) return { deny: "token revoked" }; - if (token.expires_at <= Date.now() / 1000) return { deny: "token expired" }; + if (token.expires_at <= this.now()) return { deny: "token expired" }; if (token.uses_remaining <= 0) return { deny: "token exhausted" }; if (!token.scopes.includes(action)) return { deny: `scope '${action}' not granted` }; // "read" is not consumption; secret-bearing actions decrement uses @@ -408,7 +419,10 @@ export class AgentDaemon { let ts: number | undefined; if (msg["timestamp"] !== undefined) { const requested = Number(msg["timestamp"]); - const now = Math.floor(Date.now() / 1000); + // Same clock as expiry: a test clock that moved one but not the other + // would let a token expire while the skew window still measured real + // time, which is a difference no production path should have either. + const now = Math.floor(this.now()); if (!Number.isFinite(requested) || Math.abs(requested - now) > TIMESTAMP_SKEW_SECONDS) { return { error: { @@ -502,7 +516,7 @@ export class AgentDaemon { } private expire(): void { - const now = Date.now() / 1000; + const now = this.now(); for (const [fp, held] of this.held) { if (held.expiresAt <= now) this.forget(fp); } @@ -523,7 +537,7 @@ export class AgentDaemon { private highRiskTag(fingerprint: string): string { const session = this.highRisk.get(fingerprint); if (!session) return ""; - if (session.expiresAt <= Date.now() / 1000) { + if (session.expiresAt <= this.now()) { this.highRisk.delete(fingerprint); return ""; } @@ -585,7 +599,7 @@ export class AgentDaemon { scopes: scopes as TokenScope[], kinds: (kinds as string[] | undefined) ?? null, label_regex: String(msg["label_regex"] ?? ".*"), - expires_at: Math.floor(Date.now() / 1000 + ttl), + expires_at: Math.floor(this.now() + ttl), uses_remaining: uses, revoked: false, ...(Array.isArray(msg["exec_allowlist"]) && (msg["exec_allowlist"] as string[]).length @@ -738,7 +752,7 @@ export class AgentDaemon { if (generateFingerprint(mnemonic) !== fingerprint) { return { ok: false, error: "fingerprint does not match the supplied seed" }; } - const expiresAt = Math.floor(Date.now() / 1000 + ttl); + const expiresAt = Math.floor(this.now() + ttl); // The audit key is derived from the held seed, so the seed must go in // before the record is written — but an audit failure must then undo // it. Otherwise a `put` that reports ok:false can leave the seed @@ -774,7 +788,7 @@ export class AgentDaemon { } const ttl = positiveIntField(msg["ttl"], 300); if (ttl === null) return { ok: false, error: "ttl must be a positive integer" }; - const expiresAt = Math.floor(Date.now() / 1000 + ttl); + const expiresAt = Math.floor(this.now() + ttl); this.highRisk.set(fingerprint, { tag, expiresAt }); try { // Records THAT it was unlocked and for how long — never the tag. diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index 40f01607..acd6dbf1 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -8,14 +8,22 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { createConnection } from "node:net"; -import { mkdtemp, readFile, writeFile, stat } from "node:fs/promises"; +import { mkdtemp, readFile, writeFile, stat, mkdir } from "node:fs/promises"; import { join as joinPath } from "node:path"; import { tmpdir } from "node:os"; import { join } from "node:path"; import process from "node:process"; import { mnemonics } from "@seedpass/test-vectors"; +import { encryptV3, deriveIndexKeyBytes, utf8 } from "@seedpass/core"; import { generateFingerprint } from "@seedpass/core"; -import { buildProgram, AgentDaemon, agentSocketPath, type ProgramIo } from "../src/index.js"; +import { + buildProgram, + AgentDaemon, + AgentClient, + agentSocketPath, + INDEX_FILENAME, + type ProgramIo, +} from "../src/index.js"; const MNEMONIC = mnemonics["abandon12"]!; const FINGERPRINT = generateFingerprint(MNEMONIC); @@ -496,3 +504,93 @@ describe("a use-scoped token cannot crash the agent", () => { expect(status.map((p: { fingerprint: string }) => p.fingerprint)).toContain(FINGERPRINT); }); }); + +describe("the owner capability", () => { + it("refuses an empty or absent capability", async () => { + // Mutation testing showed `!presented || !this.capability` could become + // `&&` without any test noticing: the length comparison below it happens + // to reject an empty string too, so the early return is redundant. It is + // still the line that STATES the rule, and an empty credential being + // refused is worth asserting outright rather than relying on a length + // check further down to imply it. + for (const cap of ["", undefined]) { + const r = await rawRequest({ + op: "status", + ...(cap !== undefined && { cap }), + }); + expect(r["ok"]).toBe(false); + expect(String(r["error"])).toMatch(/owner/i); + } + }); + + it("refuses a capability of the right length but wrong content", async () => { + const real = (await readFile(daemon.capabilityPath, "utf8")).trim(); + // Same length, so the length check cannot be what rejects it — this is + // the constant-time comparison doing its job. + const forged = "x".repeat(real.length); + expect(forged).toHaveLength(real.length); + const r = await rawRequest({ op: "status", cap: forged }); + expect(r["ok"]).toBe(false); + }); +}); + +describe("expiry is denied AT the boundary, not after it", () => { + /** + * Mutation testing found `token.expires_at <= now` could become `<` with + * every test still passing: the suite checked well before and well after + * expiry, which cannot tell the two apart. "Expires at T" has to mean + * denied AT T — otherwise a token is usable for one more request than it + * claims, which is the wrong direction for a credential. + * + * The daemon takes an injectable clock so this can be pinned exactly + * rather than raced against wall time. + */ + const FROZEN = 1_800_000_000; + + async function daemonAt(now: () => number): Promise<{ d: AgentDaemon; sock: string }> { + const dir = await mkdtemp(join(tmpdir(), "seedpass-expiry-")); + // `put` writes an audit record, which needs the profile directory and an + // index to serve from. + const profile = join(dir, FINGERPRINT); + await mkdir(profile, { recursive: true }); + await writeFile( + join(profile, INDEX_FILENAME), + await encryptV3( + deriveIndexKeyBytes(MNEMONIC), + utf8(JSON.stringify({ schema_version: 4, entries: {} })), + ), + ); + const sock = join(dir, "agent.sock"); + const d = new AgentDaemon(sock, 900, dir, now); + await d.start(); + return { d, sock }; + } + + it("denies a token at the exact instant it expires", async () => { + let clock = FROZEN; + const { d, sock } = await daemonAt(() => clock); + try { + const client = new AgentClient(sock); + await client.put(FINGERPRINT, MNEMONIC, 900); + const issued = await client.tokenIssue({ + fingerprint: FINGERPRINT, + name: "boundary", + scopes: ["read"], + ttl: 60, + uses: 5, + }); + + // One second before expiry: usable. + clock = FROZEN + 59; + await expect(client.vaultEntries(FINGERPRINT, issued.token)).resolves.toBeDefined(); + + // EXACTLY at expiry: denied. This is the assertion `<` would fail. + clock = FROZEN + 60; + await expect(client.vaultEntries(FINGERPRINT, issued.token)).rejects.toThrow( + /expired/, + ); + } finally { + await d.stop(); + } + }); +}); diff --git a/scripts/mutation_test.py b/scripts/mutation_test.py index b3e80ae8..bfb24922 100644 --- a/scripts/mutation_test.py +++ b/scripts/mutation_test.py @@ -121,6 +121,14 @@ SECURITY_HINTS = re.compile( SKIP_LINE = re.compile(r"^\s*(//|\*|/\*)") +# A line preceded by this marker has been triaged as an EQUIVALENT mutant: +# the mutation provably cannot change behaviour, so no test can kill it and +# chasing it is wasted effort. Marking it keeps future survivor lists +# meaningful -- a survivor should mean "look at this", not "this again". +# The marker must say WHY, so the judgement can be re-checked rather than +# trusted. +EQUIVALENT_MARKER = "mutation-equivalent:" + @dataclass class Mutant: @@ -130,11 +138,29 @@ class Mutant: description: str +def _marked_equivalent(lines: list[str], line_no: int) -> bool: + """Is the code at `line_no` (1-based) preceded by an equivalence marker?""" + idx = line_no - 2 # the line directly above, 0-based + while idx >= 0 and SKIP_LINE.match(lines[idx]): + if EQUIVALENT_MARKER in lines[idx]: + return True + idx -= 1 + return False + + def generate(source: str) -> list[Mutant]: mutants: list[Mutant] = [] - for i, line in enumerate(source.splitlines(), start=1): + lines = source.splitlines() + for i, line in enumerate(lines, start=1): if SKIP_LINE.match(line) or not SECURITY_HINTS.search(line): continue + # Skip lines a human has already judged unkillable, with a reason. + # Scans the whole contiguous comment block above, not just the line + # immediately before: the marker belongs at the START of an + # explanation, and requiring it on the last line would mean writing + # the reason upside down. + if _marked_equivalent(lines, i): + continue for old, new in OPERATORS: if old not in line: continue @@ -179,6 +205,17 @@ def main() -> int: parser.add_argument( "--limit", type=int, default=0, help="cap mutants per target (0 = all)" ) + parser.add_argument( + "--offset", + type=int, + default=0, + help=( + "skip the first N mutants. With --limit this runs a slice, so a " + "long target can be measured in foreground-sized pieces rather " + "than backgrounded -- this script writes broken security code " + "into the tree, and must not run unattended alongside commits." + ), + ) parser.add_argument("--timeout", type=int, default=420) args = parser.parse_args() @@ -196,10 +233,12 @@ def main() -> int: rel, tests = TARGETS[name] path = CLI / rel original = path.read_text() - mutants = generate(original) + all_mutants = generate(original) + mutants = all_mutants[args.offset :] if args.limit: mutants = mutants[: args.limit] - print(f"\n=== {name} ({rel}): {len(mutants)} mutants ===") + span = f"{args.offset + 1}-{args.offset + len(mutants)} of {len(all_mutants)}" + print(f"\n=== {name} ({rel}): mutants {span} ===") # Restore the file whatever happens, including Ctrl-C. Leaving a # deliberate security hole in the tree would be a spectacular own goal. @@ -211,7 +250,7 @@ def main() -> int: ) try: - for idx, mutant in enumerate(mutants, start=1): + for idx, mutant in enumerate(mutants, start=args.offset + 1): lines = original.splitlines(keepends=True) ending = "\n" if lines[mutant.line_no - 1].endswith("\n") else "" lines[mutant.line_no - 1] = mutant.mutated + ending @@ -222,7 +261,7 @@ def main() -> int: ) if code != 0: overall_counts["invalid"] += 1 - print(f" [{idx}/{len(mutants)}] line {mutant.line_no}: invalid (does not typecheck)") + print(f" [{idx}/{len(all_mutants)}] line {mutant.line_no}: invalid (does not typecheck)") continue code, _ = run( @@ -230,11 +269,11 @@ def main() -> int: ) if code != 0: overall_counts["killed"] += 1 - print(f" [{idx}/{len(mutants)}] line {mutant.line_no}: killed ({mutant.description})") + print(f" [{idx}/{len(all_mutants)}] line {mutant.line_no}: killed ({mutant.description})") else: overall_counts["survived"] += 1 overall_survivors.append((f"{rel}:{mutant.line_no}", mutant)) - print(f" [{idx}/{len(mutants)}] line {mutant.line_no}: SURVIVED ({mutant.description})") + print(f" [{idx}/{len(all_mutants)}] line {mutant.line_no}: SURVIVED ({mutant.description})") finally: restore() # Prove the restore worked rather than assuming it: a mutant left