From 8653ee8006d6ccecc1dd2c94c81aa60b8f2799ee Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Tue, 18 Aug 2026 21:54:21 -0400 Subject: [PATCH] fix(agent): validate put at the daemon boundary; state the two scope semantics (L-5, L-6, L-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit L-7 — `put` is the only way a fingerprint enters `held`, and every later handler joins that string onto a path (the vault it opens, the audit log it writes). The CLI validated the format first, which is exactly why the daemon could not rely on it: agent.ts's own header says the CLI is untrusted, and the browser extension speaks this protocol next. `put` now requires 16 uppercase hex, and additionally that the fingerprint actually belongs to the supplied seed — a mismatch would unlock a vault the caller did not name and file its audit records under the wrong profile. Validation order matters and is now deliberate: cheap format check, then ttl, then the seed match last, because deriving a fingerprint runs the BIP-39 KDF and a malformed ttl should not pay for it. This keeps the existing non-numeric-ttl test exercising the ttl path rather than short-circuiting on the new check. L-5 — `label_regex` is unanchored, so a token issued for `prod` also reaches `not-prod-db`. Left as-is on purpose: Python matches with `re.search`, so anchoring the TypeScript side alone would make one token mean two different things depending on which implementation holds it, and would silently narrow every token already issued. The audit asked for whichever behavior Python has, documented and in the parity matrix — so the semantics are now stated in --label-regex help, reported machine-readably as capabilities().tokens.label_regex_semantics, explained at the enforcement point, recorded in the compatibility matrix, and pinned by tests that assert BOTH labels are visible to an unanchored token. L-6 — the exec allowlist constrains the command word, never its arguments. The token holder writes the argv and the child gets the secret in its environment, so an allowlisted binary with an output-file or network flag hands the secret straight back. Documented in the same three places rather than changed: argument-level containment means allowlisting full argv templates instead of command words, which is a token-format change that has to land in both implementations together. Tests: malformed and mismatched fingerprints refused with nothing held (path traversal, wrong case, wrong length, trailing component); unanchored vs anchored label_regex; capabilities reporting both semantics. Mutation-verified — disabling either put guard turns the matching tests red. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- docs/typescript_port_compatibility_matrix.md | 12 ++++ js/packages/cli/src/agent.ts | 52 +++++++++++++- js/packages/cli/src/appDir.ts | 2 +- js/packages/cli/src/capabilities.ts | 11 +++ js/packages/cli/src/program.ts | 12 +++- js/packages/cli/test/agentSecurity.test.ts | 35 +++++++++ js/packages/cli/test/tokens.test.ts | 76 ++++++++++++++++++++ 7 files changed, 194 insertions(+), 6 deletions(-) diff --git a/docs/typescript_port_compatibility_matrix.md b/docs/typescript_port_compatibility_matrix.md index 6c5b924b..0e846dd0 100644 --- a/docs/typescript_port_compatibility_matrix.md +++ b/docs/typescript_port_compatibility_matrix.md @@ -116,6 +116,18 @@ need a versioned protocol change in both implementations: numbers (or an info string) and a compatibility version, since existing keys must keep deriving as they do. +**Deliberately identical, and wider than it reads: token `label_regex`.** +Both implementations match a token's `label_regex` as a *search*, not a full +match — Python with `re.search` (`src/seedpass/cli/agent.py`), TypeScript with +`RegExp.test` (`js/packages/cli/src/agent.ts`). A token issued for `prod` +therefore also reaches `not-prod-db`. Anchoring the TypeScript side alone +would make one token mean two different things depending on which +implementation holds it, and would silently narrow the scope of every token +already issued, so the semantics stay as they are and are stated instead: in +`--label-regex` help, in `capabilities().tokens.label_regex_semantics`, and +pinned by tests in `js/packages/cli/test/tokens.test.ts`. Operators wanting an +exact match anchor the pattern themselves (`^prod$`). + **Resolved:** TOTP codes for entries with a non-default period/digits used to differ — Python built `pyotp.TOTP(secret)` with library defaults and ignored the entry's recorded values, so an imported 8-digit/45s secret diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index 8b78572a..180ab585 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -29,7 +29,7 @@ import { tmpdir } from "node:os"; import { dirname } from "node:path"; import { join } from "node:path"; import process from "node:process"; -import { deriveKeyIndex, sha256Hex, utf8, type Entry } from "@seedpass/core"; +import { deriveKeyIndex, generateFingerprint, sha256Hex, utf8, type Entry } from "@seedpass/core"; import { openVault } from "./vaultFile.js"; import { materializeSecret } from "./secrets.js"; import { entryMetadata } from "./refs.js"; @@ -42,7 +42,7 @@ import { } from "./sinks.js"; import { AuditLog } from "./audit.js"; import { loadConfig, passwordPolicyFromConfig } from "./configFile.js"; -import { INDEX_FILENAME } from "./appDir.js"; +import { INDEX_FILENAME, FINGERPRINT_RE } from "./appDir.js"; export const DEFAULT_TTL_SECONDS = 900; @@ -245,7 +245,24 @@ export class AgentDaemon { return { token }; } - /** Would this token be allowed to act on this entry at all? */ + /** + * Would this token be allowed to act on this entry at all? + * + * `label_regex` is matched with `RegExp.test`, i.e. unanchored: it succeeds + * if the pattern matches ANYWHERE in the label, so a token issued for + * `prod` also reaches `not-prod-db`. That is wider than the operator + * issuing it is likely to assume, and it is deliberate: Python matches with + * `re.search` (cli/agent.py), and a token must not mean two different + * things depending on which implementation is holding it. Anchoring here + * would silently narrow every existing token's scope on upgrade. + * + * The semantics are stated in `--label-regex` help and reported in + * `capabilities()` under `label_regex_semantics`, so a caller can scope + * correctly without reading this code. + * + * A pattern that fails to compile denies rather than throws: an invalid + * constraint must never widen access. + */ private tokenMaySee(token: TokenRecord, entry: { kind: string; label: string }): boolean { if (token.kinds && !token.kinds.includes(entry.kind)) return false; try { @@ -336,6 +353,17 @@ export class AgentDaemon { }; } 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 + // its environment, so an allowlisted binary with an output-file or + // network flag hands the secret straight back to the holder. Choose + // allowlist entries as if the holder writes their argv, because they + // do. Containment here is "which binary", never "what it does". + // + // Argument-level containment would mean allowlisting full argv + // templates (["ssh-add", "-"]) rather than command words; that is a + // token-format change and has to land in both implementations at + // once, so it is tracked rather than done here. const [cmd] = parseCommandSpec(sinkSpec.command); if (!allowed.includes(cmd)) { await this.auditLog(fingerprint, "access_denied", { @@ -641,8 +669,26 @@ export class AgentDaemon { const fingerprint = String(msg["fingerprint"] ?? ""); const mnemonic = String(msg["mnemonic"] ?? ""); if (!fingerprint || !mnemonic) return { ok: false, error: "missing fields" }; + // `put` is the only way a fingerprint enters `held`, and every later + // handler joins that string onto a path (the vault, the audit log). + // The CLI validates the format before it gets here, which is exactly + // why the daemon must too: agent.ts's own header states the CLI is + // untrusted, and the browser extension will be the next thing + // speaking this protocol. + if (!FINGERPRINT_RE.test(fingerprint)) { + return { ok: false, error: "fingerprint must be 16 uppercase hex characters" }; + } const ttl = positiveIntField(msg["ttl"], this.defaultTtl); if (ttl === null) return { ok: false, error: "ttl must be a positive integer" }; + // Last of the validations because it is the expensive one: deriving a + // fingerprint runs the BIP-39 KDF, and a malformed ttl should not pay + // for it. A fingerprint that does not belong to this seed is not an + // 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) { + return { ok: false, error: "fingerprint does not match the supplied seed" }; + } const expiresAt = Math.floor(Date.now() / 1000 + 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 diff --git a/js/packages/cli/src/appDir.ts b/js/packages/cli/src/appDir.ts index 4e888ed8..9571e32c 100644 --- a/js/packages/cli/src/appDir.ts +++ b/js/packages/cli/src/appDir.ts @@ -79,7 +79,7 @@ export function resolveAppDir(override?: string): string { * containing `..` would otherwise steer a recursive delete out of the * profile directory. */ -const FINGERPRINT_RE = /^[0-9A-F]{16}$/; +export const FINGERPRINT_RE = /^[0-9A-F]{16}$/; export function assertValidFingerprint(fingerprint: string): string { if (!FINGERPRINT_RE.test(fingerprint)) { diff --git a/js/packages/cli/src/capabilities.ts b/js/packages/cli/src/capabilities.ts index 2469dd79..a440c8af 100644 --- a/js/packages/cli/src/capabilities.ts +++ b/js/packages/cli/src/capabilities.ts @@ -57,6 +57,17 @@ export function capabilities(): Record { commands: ["agent token-issue/token-list/token-revoke"], scopes: ["read", "use", "reveal"], constraints: ["kinds", "label_regex", "ttl", "uses", "exec_allowlist"], + // Automation issuing tokens has to know this to scope them correctly: + // label_regex is a SEARCH, not a full match, in both implementations + // (Python uses re.search). `prod` therefore also matches `not-prod-db`. + // Anchor it yourself -- `^prod$` -- to get an exact match. + label_regex_semantics: "search (substring); anchor with ^...$ for exact match", + // Automation that issues `use` tokens needs this to pick allowlist + // entries safely: the check covers the command word, not its arguments. + exec_allowlist_semantics: + "command word only; the token holder supplies all arguments, so an " + + "allowlisted binary that can write a file or open a socket can " + + "return the secret to its caller", token_env_var: "SEEDPASS_TOKEN", enforcement: "session agent (secrets materialized agent-side in token mode)", escalation: diff --git a/js/packages/cli/src/program.ts b/js/packages/cli/src/program.ts index 6c4e08b3..f0f914b8 100644 --- a/js/packages/cli/src/program.ts +++ b/js/packages/cli/src/program.ts @@ -1361,12 +1361,20 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .option("--name ", "token name", "agent") .option("--scope ", "read/use/reveal (repeatable)", ["read"]) .option("--kind ", "restrict to entry kinds (repeatable)") - .option("--label-regex ", "restrict to matching labels", ".*") + .option( + "--label-regex ", + "restrict to labels the pattern MATCHES ANYWHERE (like Python's " + + "re.search): 'prod' also matches 'not-prod-db'. Anchor it — '^prod$' " + + "— for an exact match", + ".*", + ) .option("--ttl ", "token lifetime", "300") .option("--uses ", "max secret deliveries", "1") .option( "--exec-allowlist ", - "restrict `use` to these commands (without it, a use-scoped holder can read the secret)", + "restrict `use` to these commands (without it, a use-scoped holder can " + + "read the secret). Matches the command WORD only — the holder still " + + "chooses the arguments, so pick binaries that stay safe with any argv", ) .action( async (o: { diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index da27b139..40f01607 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -409,6 +409,41 @@ describe("malformed daemon input cannot corrupt held state", () => { expect(JSON.stringify(owner)).not.toContain("abandon"); }); + /** + * `put` is the only way a fingerprint enters `held`, and every later handler + * joins that string onto a path — the vault it opens, the audit log it + * writes. The CLI validates the format before the daemon ever sees it, + * which is precisely why the daemon cannot rely on that: it treats every + * caller as untrusted, and the browser extension will speak this protocol + * next. + */ + it("refuses a fingerprint that is not 16 uppercase hex characters", async () => { + for (const bad of ["../../../etc", "bbbbbbbbbbbbbbbb", "BBBB", "BBBBBBBBBBBBBBBB/x"]) { + const put = await rawRequest({ + op: "put", cap, fingerprint: bad, mnemonic: MNEMONIC, ttl: 60, + }); + expect(put["ok"]).toBe(false); + expect(String(put["error"])).toContain("16 uppercase hex"); + const owner = await rawRequest({ op: "owner-mnemonic", cap, fingerprint: bad }); + expect(owner["ok"]).toBe(false); + expect(JSON.stringify(owner)).not.toContain("abandon"); + } + }); + + it("refuses a well-formed fingerprint that does not belong to the seed", async () => { + // Correct shape, wrong profile: this would unlock a vault the caller did + // not name and file its audit records under the wrong fingerprint. + const put = await rawRequest({ + op: "put", cap, fingerprint: SCRATCH_FP, mnemonic: MNEMONIC, ttl: 60, + }); + expect(put["ok"]).toBe(false); + expect(String(put["error"])).toContain("does not match"); + + const owner = await rawRequest({ op: "owner-mnemonic", cap, fingerprint: SCRATCH_FP }); + expect(owner["ok"]).toBe(false); + expect(JSON.stringify(owner)).not.toContain("abandon"); + }); + 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 e507c69c..dc31dccf 100644 --- a/js/packages/cli/test/tokens.test.ts +++ b/js/packages/cli/test/tokens.test.ts @@ -96,6 +96,82 @@ describe("token issuance", () => { }); }); +/** + * `label_regex` is a search, not a full match — in both implementations. + * Python enforces it with `re.search` (cli/agent.py), so anchoring on the TS + * side would mean the same token grants different access depending on which + * implementation holds it, and would silently narrow every token already + * issued. These tests pin the semantics so the surprise stays documented + * rather than becoming an accidental behavior change later. + */ +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"); + await run(asOwner, "entry", "add", "key-value", "prod", "k", "intended-secret"); + + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "substring-demo", + "--scope", "read", + "--kind", "key_value", + "--label-regex", "prod", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + // The point of the test: BOTH are visible, not just the exact match. + expect(rows.map((x) => x.label).sort()).toEqual(["not-prod-db", "prod"]); + }); + + it("anchoring the pattern is how an operator gets an exact match", async () => { + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "anchored-demo", + "--scope", "read", + "--kind", "key_value", + "--label-regex", "^prod$", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + expect(rows.map((x) => x.label)).toEqual(["prod"]); + }); + + it("reports the semantics in capabilities so automation can scope correctly", async () => { + const caps = JSON.parse((await run({}, "capabilities")).stdout); + expect(caps.tokens.label_regex_semantics).toContain("search"); + expect(caps.tokens.label_regex_semantics).toContain("^...$"); + // The exec allowlist's containment is likewise narrower than it reads: + // it names a binary, not what that binary may be asked to do. + expect(caps.tokens.exec_allowlist_semantics).toContain("command word only"); + }); + + it("an uncompilable pattern denies rather than throwing", async () => { + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "bad-regex", + "--scope", "read", + "--label-regex", "([unclosed", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + expect(rows).toEqual([]); + }); +}); + describe("token-mode access (no mnemonic anywhere)", () => { it("lists only the entries the token may act on, with no secret values", async () => { const r = await run(asTokenHolder(token), "entry", "list");