diff --git a/js/packages/cli/src/sinks.ts b/js/packages/cli/src/sinks.ts index e2ce475b..c22a20a7 100644 --- a/js/packages/cli/src/sinks.ts +++ b/js/packages/cli/src/sinks.ts @@ -28,9 +28,16 @@ const PASSTHROUGH_ENV = ["PATH", "HOME", "LANG", "LC_ALL", "TERM", "TZ", "TMPDIR * Build the environment for a sink child: the allowlist above, never the * caller's full environment. */ -export function sinkEnv(extra: Record = {}): Record { +export function sinkEnv( + extra: Record = {}, + // The allowlist is a parameter purely so the SEEDPASS_ backstop below can + // be tested. No entry in PASSTHROUGH_ENV starts with SEEDPASS_ today, so + // the check cannot fire against the real list -- which is what makes it a + // backstop, and also what made removing it invisible to the suite. + passthrough: readonly string[] = PASSTHROUGH_ENV, +): Record { const env: Record = {}; - for (const key of PASSTHROUGH_ENV) { + for (const key of passthrough) { // Backstop, not the control: nothing SEEDPASS-prefixed rides along even // if a future edit adds one to the allowlist. `extra` is exempt — the // exec sink's whole job is injecting SEEDPASS_SECRET. diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index b21ac8b8..1614775a 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -594,6 +594,74 @@ async function daemonAt(now: () => number): Promise<{ d: AgentDaemon; sock: stri return { d, sock }; } +describe("token-issue validates before it mints", () => { + // Everything here is reachable over the wire by a process that already + // holds the owner socket, and none of it was asserted: the unlocked check + // and both shape checks could be removed with the suite still green. + let ownerCap: string; + beforeAll(async () => { + ownerCap = (await readFile(daemon.capabilityPath, "utf8")).trim(); + }); + + it("refuses to mint a token for a profile it is not holding", async () => { + // A token is an authorization to read a specific vault. Minting one for a + // profile the agent never unlocked hands out authority nobody proved they + // had — the seed being resident IS the proof. + const other = generateFingerprint(mnemonics["zoo24"]!); + const r = await rawRequest({ + op: "token-issue", + cap: ownerCap, + fingerprint: other, + name: "for-a-vault-i-do-not-hold", + scopes: ["read"], + ttl: 600, + uses: 1, + }); + expect(r["ok"]).toBe(false); + expect(String(r["error"])).toContain("not unlocked"); + expect(r["token"]).toBeUndefined(); + }); + + it("refuses a bare string where an array is required", async () => { + // `"read"` and `["read"]` are easy to confuse when hand-writing the + // protocol, and the difference is not cosmetic: a string `kinds` turns + // the exact kind match in tokenMaySee into String.includes, so a token + // declared for one kind starts matching any kind whose name is a + // substring of it. Refuse the shape rather than discovering it later. + const base = { + op: "token-issue", + cap: ownerCap, + fingerprint: FINGERPRINT, + name: "wrong-shape", + ttl: 600, + uses: 1, + }; + const badScopes = await rawRequest({ ...base, scopes: "read" }); + expect(badScopes["ok"]).toBe(false); + expect(String(badScopes["error"])).toContain("scopes must be an array"); + expect(badScopes["token"]).toBeUndefined(); + + const badKinds = await rawRequest({ + ...base, + scopes: ["read"], + kinds: "password", + }); + expect(badKinds["ok"]).toBe(false); + expect(String(badKinds["error"])).toContain("kinds must be an array"); + expect(badKinds["token"]).toBeUndefined(); + + // The same request with both shapes correct does mint, so these tests + // cannot pass by token-issue being broken outright. + const good = await rawRequest({ + ...base, + scopes: ["read"], + kinds: ["password"], + }); + expect(good["ok"]).toBe(true); + expect(String(good["token"]).length).toBeGreaterThan(0); + }); +}); + describe("put refuses a half-supplied identity", () => { // `put` is the only door a seed comes through, and everything after it // trusts that both halves arrived. Mutation testing turned the `||` in its diff --git a/js/packages/cli/test/api.test.ts b/js/packages/cli/test/api.test.ts index c15cb94e..6be6d185 100644 --- a/js/packages/cli/test/api.test.ts +++ b/js/packages/cli/test/api.test.ts @@ -452,6 +452,60 @@ describe("profiles and vault state", () => { expect(existsSync(app.profileDir(OTHER))).toBe(false); }); + it("refuses to select a profile that does not exist", async () => { + // Without this the caller's string becomes ctx.fingerprint, which every + // later request joins onto a path — so the refusal is what keeps + // "../../etc" out of the profile directory. Asserted on the shared + // server precisely because a refusal must leave its state untouched. + const before = ctx.fingerprint; + for (const bogus of [generateFingerprint(mnemonics["zoo24"]!), "../../etc"]) { + const res = await call("POST", "/api/v1/fingerprint/select", { + body: { fingerprint: bogus }, + }); + // 404 specifically. AppDir.switchProfile repeats both checks, so + // dropping this one still refuses — as a raw 500 from an unhandled + // error, telling a caller the server broke when it simply named a + // profile that is not there. + expect(res.status).toBe(404); + expect(ctx.fingerprint).toBe(before); + } + }); + + it("selects a real profile, dropping the seed on the way", async () => { + // The positive half, on its own context so the shared server keeps + // serving the profile the rest of this file expects. + const second = generateFingerprint(mnemonics["zoo24"]!); + await app.mutateFingerprints((data) => { + if (!data.fingerprints.includes(second)) data.fingerprints.push(second); + }); + await mkdir(app.profileDir(second), { recursive: true }); + + const srv = new ApiServer({ host: "127.0.0.1", port: 0, token: TOKEN }); + const ctx2 = buildContext({ app, fingerprint: FINGERPRINT, mnemonic: MNEMONIC }); + registerRoutes(srv, ctx2); + const bound = await srv.listen(); + try { + const res = await fetch( + `http://${bound.host}:${bound.port}/api/v1/fingerprint/select`, + { + method: "POST", + headers: { + authorization: `Bearer ${TOKEN}`, + "content-type": "application/json", + }, + body: JSON.stringify({ fingerprint: second }), + }, + ); + expect(res.status).toBe(200); + expect(ctx2.fingerprint).toBe(second); + // The held seed belongs to the profile being left; carrying it across + // would read one vault while the caller believed they selected another. + expect(ctx2.mnemonic).toBeNull(); + } finally { + await srv.close(); + } + }); + it("reports lock state that follows the actual seed", async () => { expect((await call("GET", "/api/v1/vault/status")).json.locked).toBe(false); @@ -935,6 +989,48 @@ describe("structural invariants of the route table", () => { expect(mismatched).toEqual([]); }); + it("refuses a flagged route whose handler forgot to check", async () => { + // The behavioural half of the pairing test above. The flag is redundant + // for every route that exists today, which is why disabling the server's + // check changed nothing — but its entire purpose is the route that does + // NOT exist yet, written by someone who set the flag and skipped the + // call. So register exactly that route and confirm the server layer + // stops it before the handler ever runs. + const srv = new ApiServer({ host: "127.0.0.1", port: 0, token: TOKEN }); + let handlerRan = false; + srv.route( + "GET", + "/api/v1/test-forgot-to-check", + async () => { + handlerRan = true; + return { json: { secret: "leaked-plaintext" } }; + }, + { requiresPassword: true }, + ); + const bound = await srv.listen(); + try { + const url = `http://${bound.host}:${bound.port}/api/v1/test-forgot-to-check`; + const res = await fetch(url, { headers: { authorization: `Bearer ${TOKEN}` } }); + expect(res.status).toBe(401); + expect(await res.text()).not.toContain("leaked-plaintext"); + // Not merely refused after the fact — never invoked. + expect(handlerRan).toBe(false); + + // With the password it goes through, so the test cannot pass by the + // route being broken. + const ok = await fetch(url, { + headers: { + authorization: `Bearer ${TOKEN}`, + "x-seedpass-password": PASSWORD, + }, + }); + expect(ok.status).toBe(200); + expect(handlerRan).toBe(true); + } finally { + await srv.close(); + } + }); + it("password-gates every route that can return plaintext", async () => { // The failure this guards against is a NEW secret-returning route with // neither the flag nor the call — which the pairing test above cannot diff --git a/js/packages/cli/test/highRisk.test.ts b/js/packages/cli/test/highRisk.test.ts index 9da16cd2..078b44bf 100644 --- a/js/packages/cli/test/highRisk.test.ts +++ b/js/packages/cli/test/highRisk.test.ts @@ -131,6 +131,21 @@ describe("the factor", () => { expect(String((second.error as Error).message)).toContain("already configured"); }); + it("cannot be empty, and leaves nothing behind when refused", async () => { + // An empty factor would wrap the partition key under nothing: the + // envelope unwraps for anyone who supplies "", while `configured` still + // reports true. So the vault would show a second factor that is not one. + // Python raises on the same falsy check, so this is parity as well. + const fresh = await mkdtemp(join(tmpdir(), "seedpass-empty-factor-")); + await expect(setFactor(fresh, "")).rejects.toThrow(/cannot be empty/); + // Refused BEFORE the envelope is written — a half-configured factor would + // be worse than none, because factorConfigured() would start saying yes. + expect(factorConfigured(fresh)).toBe(false); + // And a real factor still works in the same directory, so the guard is + // rejecting the empty string rather than the directory. + await expect(setFactor(fresh, "a-real-factor")).resolves.toMatch(/^[0-9a-f]{64}$/); + }); + it("is never read from the command line", async () => { const r = await run({ ...asOwner, SEEDPASS_HIGH_RISK_FACTOR: undefined }, "agent", "high-risk", "unlock"); diff --git a/js/packages/cli/test/sinks.test.ts b/js/packages/cli/test/sinks.test.ts index 53b4a034..041ce78f 100644 --- a/js/packages/cli/test/sinks.test.ts +++ b/js/packages/cli/test/sinks.test.ts @@ -167,3 +167,43 @@ describe("command spec parsing keeps every argument", () => { expect(() => parseCommandSpec([""])).toThrow(/empty command/); }); }); + +import { sinkEnv } from "../src/sinks.js"; + +describe("the environment a sink child receives", () => { + it("passes only the allowlist through, and never the caller's own env", async () => { + process.env["SEEDPASS_TEST_LEAK"] = "must-not-appear"; + try { + const env = sinkEnv(); + expect(env["SEEDPASS_TEST_LEAK"]).toBeUndefined(); + // PATH is on the allowlist and the child genuinely needs it, so its + // presence is what proves the allowlist is being applied rather than + // the whole environment simply being empty. + expect(env["PATH"]).toBe(process.env["PATH"]); + } finally { + delete process.env["SEEDPASS_TEST_LEAK"]; + } + }); + + it("drops a SEEDPASS_ variable even if the allowlist grows one", async () => { + // The backstop. It cannot fire against today's allowlist — which is why + // deleting it changed nothing anywhere — so the allowlist is injected + // here to stand in for the future edit that adds one by mistake. + process.env["SEEDPASS_SNEAKY"] = "must-not-appear"; + try { + const env = sinkEnv({}, ["PATH", "SEEDPASS_SNEAKY"]); + expect(env["SEEDPASS_SNEAKY"]).toBeUndefined(); + expect(env["PATH"]).toBeDefined(); + } finally { + delete process.env["SEEDPASS_SNEAKY"]; + } + }); + + it("still injects the exec sink's own secret variable", async () => { + // `extra` is deliberately exempt from the SEEDPASS_ rule: handing the + // secret to the child through the environment is the exec sink's entire + // job. A backstop that swallowed it would break the feature. + const env = sinkEnv({ SEEDPASS_SECRET: "the-secret" }); + expect(env["SEEDPASS_SECRET"]).toBe("the-secret"); + }); +}); diff --git a/js/packages/cli/test/tokens.test.ts b/js/packages/cli/test/tokens.test.ts index 125d8e43..e345c129 100644 --- a/js/packages/cli/test/tokens.test.ts +++ b/js/packages/cli/test/tokens.test.ts @@ -111,6 +111,52 @@ describe("token issuance", () => { * issued. These tests pin the semantics so the surprise stays documented * rather than becoming an accidental behavior change later. */ +describe("the kind restriction on a token", () => { + /** + * `--kind` is half of a token's reach; `--label-regex` is the other half. + * The header of this file claimed both were covered and only the label side + * was — removing the kind check entirely left every test here green, which + * means a token scoped to one kind could read every other kind's secrets + * and nothing would have said so. + */ + it("hides other kinds from listings and refuses their secrets", async () => { + await run(asOwner, "entry", "add", "key-value", "kv-only", "k", "kv-secret"); + await run(asOwner, "entry", "add", "password", "pw-only"); + + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "kv-scoped", + "--scope", "read", + "--kind", "key_value", + "--label-regex", "only", + "--ttl", "600", + )).stdout, + ); + + // The label pattern matches BOTH entries, so anything visible here is the + // kind check's doing and not the label's. + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + expect(rows.map((x) => x.label)).toEqual(["kv-only"]); + + // And listing is not the only door: the secret itself must be refused, + // not merely omitted from the index the token can see. + const pwRow = JSON.parse( + (await run(asOwner, "entry", "list")).stdout, + ) as { id: string | number; label: string }[]; + const pwId = String(pwRow.find((r) => r.label === "pw-only")!.id); + const denied = await run( + asTokenHolder(issued.token), + "entry", "get", pwId, "--secret", + ); + expect(denied.error).toBeDefined(); + expect(denied.stdout).not.toContain("pw-only"); + }); +}); + describe("label_regex matches anywhere, matching Python's re.search", () => { it("an unanchored pattern reaches labels that merely contain it", async () => { await run(asOwner, "entry", "add", "key-value", "not-prod-db", "k", "adjacent-secret"); diff --git a/scripts/mutation_test.py b/scripts/mutation_test.py index bfb24922..8fcf60b3 100644 --- a/scripts/mutation_test.py +++ b/scripts/mutation_test.py @@ -114,7 +114,12 @@ SECURITY_HINTS = re.compile( r"\b(" r"auth|token|cap|capab|scope|permit|allow|deny|refus|valid|verif|" r"expire|expires|ttl|limit|rate|uses|remaining|owner|secret|password|" - r"factor|unlock|lock|held|fingerprint|hash|equal|match|forbidden" + r"factor|unlock|lock|held|fingerprint|hash|equal|match|forbidden|" + # `kind` is an authorization dimension in its own right -- a token's + # `kinds` list is half of what it may reach -- and leaving it out hid the + # shape check that stops a string `kinds` becoming a substring match. + # `mnemonic`/`seed` guard the material everything else protects. + r"kind|mnemonic|seed" r")", re.IGNORECASE, ) @@ -173,6 +178,7 @@ def generate(source: str) -> list[Mutant]: # Force a guard to always allow. This is the single most informative # mutation for security code: if no test notices a check being removed # entirely, that check is unverified. + # Block form: `if (cond) {` guard = re.match(r"^(\s*)if \((.+)\) \{\s*$", line) if guard and SECURITY_HINTS.search(guard.group(2)): mutants.append( @@ -183,6 +189,28 @@ def generate(source: str) -> list[Mutant]: "guard disabled (always allow)", ) ) + # Single-statement form: `if (cond) throw ...;` / `if (cond) return ...;` + # Missed for a long time, and it is the shape a short refusal takes -- + # src/highRisk.ts generated NO mutants at all because its only guard + # is written this way. + # Greedy, so the condition runs to the LAST `) ` on the line -- a + # non-greedy match splits `if (a(b)) return x;` in the wrong place and + # produces garbage that only shows up as "invalid". Lines ending in + # `{` are the block form, already handled above. + inline = re.match(r"^(\s*)if \((.+)\) (\S.*)$", line) + if ( + inline + and not line.rstrip().endswith("{") + and SECURITY_HINTS.search(inline.group(2)) + ): + mutants.append( + Mutant( + i, + line, + f"{inline.group(1)}if (false) {inline.group(3)}", + "inline guard disabled (always allow)", + ) + ) return mutants @@ -217,11 +245,24 @@ def main() -> int: ), ) parser.add_argument("--timeout", type=int, default=420) + parser.add_argument( + "--kind", + default="", + help=( + "only run mutants whose description contains this substring, e.g. " + "--kind inline. Lets a newly added mutation rule be swept across " + "every target without re-running the ones already judged." + ), + ) args = parser.parse_args() targets = sorted(TARGETS) if args.target == "all" else [args.target] overall_survivors: list[tuple[str, Mutant]] = [] overall_counts = {"killed": 0, "survived": 0, "invalid": 0} + # A target the generator cannot reach scores 0 killed / 0 survived, which + # prints identically to a clean sweep. That is the one result that must + # never be mistaken for a pass: it means the file was NOT MEASURED. + unmeasured: list[str] = [] acquire_lock() print( @@ -234,11 +275,20 @@ def main() -> int: path = CLI / rel original = path.read_text() all_mutants = generate(original) + if args.kind: + all_mutants = [m for m in all_mutants if args.kind in m.description] mutants = all_mutants[args.offset :] if args.limit: mutants = mutants[: args.limit] span = f"{args.offset + 1}-{args.offset + len(mutants)} of {len(all_mutants)}" print(f"\n=== {name} ({rel}): mutants {span} ===") + if not all_mutants: + unmeasured.append(f"{name} ({rel})") + print( + " NOT MEASURED: this file produced no mutants. Its security\n" + " logic is not in comparisons the generator can reach (or the\n" + " generator has a blind spot). Do not read this as a pass." + ) # Restore the file whatever happens, including Ctrl-C. Leaving a # deliberate security hole in the tree would be a spectacular own goal. @@ -295,6 +345,14 @@ def main() -> int: if viable: print(f"mutation score: {overall_counts['killed'] / viable:.0%} of viable mutants caught") + if unmeasured: + print( + "\nNOT MEASURED — no mutants generated, so these files were not\n" + "tested at all by this run:\n" + ) + for where in unmeasured: + print(f" {where}") + if overall_survivors: print(f"\nSURVIVORS — each is either a gap or an equivalent mutant, and needs judging:\n") for where, mutant in overall_survivors: