diff --git a/.changeset/agile-elks-sing.md b/.changeset/agile-elks-sing.md new file mode 100644 index 000000000..d24a37418 --- /dev/null +++ b/.changeset/agile-elks-sing.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3650 +--- +**Running a capability's own test suite no longer silently deactivates it** — `bundleContentHash` digested every entry under a capability bundle with no exclusions, so ordinary Python bytecode caching (`__pycache__/*.pyc`, written by any plain `python3` run) changed the consent-binding hash. The capability then reported `inactive` with no error and no warning, and `loop render-hooks` quietly dropped its step and gate — indistinguishable from never having installed it. An *empty* `__pycache__` directory was enough to trigger it, since the digest binds directory existence. Only a `*.pyc`/`*.pyo` file sitting directly inside a `__pycache__` directory is now excluded from the digest; a `.pyc`/`.pyo` file anywhere else stays bound, since a sourceless legacy `.pyc` there is still importable and executable. A `__pycache__`/`.pytest_cache` directory has only its own marker suppressed — its contents still bind the digest normally. `node_modules` and other executable content stay bound, excluded entries still count toward the walk's caps, and the filter runs after the symlink rejection so it cannot smuggle one past. (#3631) diff --git a/CONTEXT.md b/CONTEXT.md index 74ecc90fe..00c968fda 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -326,7 +326,7 @@ ADR-1244 D3 fetch-and-stage seam (`gsd-core/bin/lib/capability-source.cjs`). Pri ADR-1244 D4 per-runtime install manifest (`gsd-core/bin/lib/capability-ledger.cjs`). Leaf module (only `node:fs`/`node:path` plus `shell-command-projection`'s `platformWriteSync`). Records `{ id, version, source, integrity, files[], sharedEdits[{file,marker}] }` per installed capability in `.gsd-capabilities.json` at the runtime config dir root. Exports: `readLedger` (structural-validated, never throws), `writeLedger` (atomic via `platformWriteSync`), `recordInstall` (idempotent, prototype-pollution-guarded), `removeEntry`, and `reconcile` (reports orphans whose `files[]` are missing on disk; hardened against non-string/`..` members; never mutates). Serves as the atomic commit point for Phase-4 upgrade/remove and the reconciliation basis for detecting stale entries after out-of-band deletions. ### Capability Consent Store -Issue #1459 user-owned consent seam (`gsd-core/bin/lib/capability-consent.cjs`). Leaf module (`node:fs`/`node:path`/`node:os`/`node:crypto` + the ledger's shared bounded `readSmallRegularFile`/`readSmallRegularFileBuffer` + the shared `capability-lock` primitive). Stores `{ version:"1", records: { "": { projectRoot, id, scope:'project', integrity, disclosureSignature, contentHash, consentedAt } } }` at `${GSD_HOME||homedir()}/.gsd/consent.json` — a USER-OWNED file OUTSIDE any repository. Exports: `consentStorePath(gsdHome?)`, `readConsentStore(gsdHome?)` (bounded via `readSmallRegularFile` + 8 MiB cap, NON-THROWING — missing/corrupt/oversized/FIFO/wrong-shape → empty `{records:{}}`; caps records at `MAX_RECORDS=4096`), `bundleContentHash(capDir)` (THE security binding — a `sha512-` over a DETERMINISTIC, INJECTIVE, LOSSLESS serialization of EVERY regular file AND directory under the bundle: length-FRAMED entry COUNT + per-entry TYPE tag + uint32 path-byte-len + RAW path bytes from a `{encoding:'buffer'}` dir walk [finding 4] + for files uint64 content-byte-len + RAW content bytes via `readSmallRegularFileBuffer` [finding 1b], plus typed DIR markers binding empty directories [finding 2]; symlinks/non-regular rejected; size+count bounded), `hasProjectConsent({gsdHome,projectRoot,id,contentHash})` (true iff a record for `${realpath(projectRoot)}\x00` exists AND its stored `contentHash` equals the supplied recomputed hash — the binding is `contentHash`, NOT `integrity` and NOT `disclosureSignature` (those remain on the record purely for the human disclosure + re-consent-on-executable-change UX); unsafe ids → false; prototype-pollution-safe NUL-joined keys + `Object.prototype.hasOwnProperty`), `recordProjectConsent({gsdHome,projectRoot,id,integrity,disclosureSignature,contentHash})` (LOCKED, atomic+durable write — tmp `wx`/fsync/rename/dir-fsync mirroring `writeLedger`; enforces the record cap at write time) and `revokeProjectConsent({gsdHome,projectRoot,id})` (LOCKED atomic delete, no-op if absent) — BOTH **THROW** rather than perform an UNLOCKED read-modify-write when the consent-store lock cannot be acquired (finding 3; the lifecycle treats a consent-write failure as non-fatal, and the `trust revoke` CLI catches the throw and emits a clean error). This is the authoritative consent signal the loader recomputes (`bundleContentHash(capDir)`) and checks at load before activating a PROJECT-scope third-party overlay (declarative surfaces AND command dispatch): a forged/cloned in-repo project ledger, OR any post-consent tamper (swapped declarative manifest, edited hook script, empty-integrity local install — all change the recomputed hash), leaves the cap DISCOVERED-BUT-INACTIVE until the user consents on THIS machine to the EXACT bundle (the lifecycle records the consent on a consented project install/upgrade and revokes it on remove; install/lookup/revoke share one canonical `consentProjectRoot` root key). GLOBAL-scope overlays (under the user's own home) need no record; and when `GSD_HOME` resolves (via realpath, defeating symlink aliasing — finding 1) to a genuine project root the in-repo bundle still requires a record. The consent lock is the SHARED hardened primitive (below), so it never stale-steals a slow-but-live writer (finding 4). See `docs/explanation/capability-trust-model.md` "project-scope trust boundary". +Issue #1459 user-owned consent seam (`gsd-core/bin/lib/capability-consent.cjs`). Leaf module (`node:fs`/`node:path`/`node:os`/`node:crypto` + the ledger's shared bounded `readSmallRegularFile`/`readSmallRegularFileBuffer` + the shared `capability-lock` primitive). Stores `{ version:"1", records: { "": { projectRoot, id, scope:'project', integrity, disclosureSignature, contentHash, consentedAt } } }` at `${GSD_HOME||homedir()}/.gsd/consent.json` — a USER-OWNED file OUTSIDE any repository. Exports: `consentStorePath(gsdHome?)`, `readConsentStore(gsdHome?)` (bounded via `readSmallRegularFile` + 8 MiB cap, NON-THROWING — missing/corrupt/oversized/FIFO/wrong-shape → empty `{records:{}}`; caps records at `MAX_RECORDS=4096`), `bundleContentHash(capDir)` (THE security binding — a `sha512-` over a DETERMINISTIC, INJECTIVE, LOSSLESS serialization of every regular file AND directory under the bundle, with two NARROW exclusions from the DIGEST ONLY (#3631, amended after two orthogonal reviews found the original wholesale directory-skip unsafe): a `__pycache__`/`.pytest_cache` DIRECTORY's own marker is suppressed but the directory is ALWAYS recursed into (so no unbounded unhashed region — H1), and a `.pyc`/`.pyo` FILE is excluded only when its immediate parent basename is exactly `__pycache__` (a `.pyc` elsewhere, e.g. bundle root or `scripts/`, stays hashed — H2); `.DS_Store` is deliberately NOT excluded (stays hashed like any other file — an excluded filename is a permanently unhashed name a declared hook could still point at, and it is unrelated to #3631's Python-bytecode symptom); excluded entries still count toward the walk's size/count caps, and the filter runs AFTER the symlink/non-regular rejection so it can never be used to smuggle one past; manifest-declared hook `script` paths naming a `__pycache__`/`.pytest_cache` segment or a `.pyc`/`.pyo` basename are rejected by `isSafeHookScriptPath` (`capability-lifecycle.cts`/`capability-validator.cjs`), but that guard is NOT containment — it only inspects the declared `script` path string, so a hashed `hooks/run.js` doing `require('../__pycache__/mod.pyc')` reaches the excluded region in one hop via Node's default `.js` handler, after which that file is free to change post-consent with the digest unmoved; ACCEPTED RESIDUAL RISK: CPython's default timestamp-based `.pyc` invalidation checks only the source's mtime+size (both attacker-forgeable by whoever can already write the bundle), so a forged `__pycache__/*.pyc` matching an unmodified source executes without moving the digest — bounded only by requiring post-consent write access and by everything outside `__pycache__/*.pyc` staying hashed (the declared-surface guard is NOT a bound, per the indirection above); `.pytest_cache` CONTENTS still change the digest (only its directory marker is suppressed); `node_modules`/`dist`/`build` are deliberately NOT excluded because their contents are executed at runtime): length-FRAMED entry COUNT + per-entry TYPE tag + uint32 path-byte-len + RAW path bytes from a `{encoding:'buffer'}` dir walk [finding 4] + for files uint64 content-byte-len + RAW content bytes via `readSmallRegularFileBuffer` [finding 1b], plus typed DIR markers binding empty directories [finding 2]; symlinks/non-regular rejected; size+count bounded), `hasProjectConsent({gsdHome,projectRoot,id,contentHash})` (true iff a record for `${realpath(projectRoot)}\x00` exists AND its stored `contentHash` equals the supplied recomputed hash — the binding is `contentHash`, NOT `integrity` and NOT `disclosureSignature` (those remain on the record purely for the human disclosure + re-consent-on-executable-change UX); unsafe ids → false; prototype-pollution-safe NUL-joined keys + `Object.prototype.hasOwnProperty`), `recordProjectConsent({gsdHome,projectRoot,id,integrity,disclosureSignature,contentHash})` (LOCKED, atomic+durable write — tmp `wx`/fsync/rename/dir-fsync mirroring `writeLedger`; enforces the record cap at write time) and `revokeProjectConsent({gsdHome,projectRoot,id})` (LOCKED atomic delete, no-op if absent) — BOTH **THROW** rather than perform an UNLOCKED read-modify-write when the consent-store lock cannot be acquired (finding 3; the lifecycle treats a consent-write failure as non-fatal, and the `trust revoke` CLI catches the throw and emits a clean error). This is the authoritative consent signal the loader recomputes (`bundleContentHash(capDir)`) and checks at load before activating a PROJECT-scope third-party overlay (declarative surfaces AND command dispatch): a forged/cloned in-repo project ledger, OR any post-consent tamper (swapped declarative manifest, edited hook script, empty-integrity local install — all change the recomputed hash), leaves the cap DISCOVERED-BUT-INACTIVE until the user consents on THIS machine to the EXACT bundle (the lifecycle records the consent on a consented project install/upgrade and revokes it on remove; install/lookup/revoke share one canonical `consentProjectRoot` root key). GLOBAL-scope overlays (under the user's own home) need no record; and when `GSD_HOME` resolves (via realpath, defeating symlink aliasing — finding 1) to a genuine project root the in-repo bundle still requires a record. The consent lock is the SHARED hardened primitive (below), so it never stale-steals a slow-but-live writer (finding 4). See `docs/explanation/capability-trust-model.md` "project-scope trust boundary". ### Capability Lock Issue #1459 finding 4 shared cross-process lock primitive (`gsd-core/bin/lib/capability-lock.cjs`). Leaf module (`node:fs`/`node:path`/`node:os`/`node:crypto` + the ledger's bounded `readSmallRegularFile` + `shell-command-projection`'s `execTool` for the rare start-time shell-out). THE single hardened lockfile protocol shared by BOTH `capability-lifecycle` (the `.gsd/capabilities/.lock` mutation lock) and `capability-consent` (the consent-store `.consent.lock`) — extracted so the two locks cannot diverge (mirrors the shared-validator / shared bounded-reader lessons). Exports: `acquireLock(lockPath, opts?)` (O_EXCL create with a JSON `{token,pid,hostname,startTime,ts}` body; steal protocol binds age to the body's own `ts`, never stale-steals a VERIFIED-LIVE same-host holder — pid alive AND recorded start-time matches the pid's current start-time, defeating pid-reuse without ever stealing a live holder — and reclaims only a dead/unverifiable holder via the dead-pid fast path or the hard `LOCK_DEADMAN_MS` deadman; `opts.maxAttempts` raises the bounded retry budget and `opts.waitForFresh` makes a contended fresh/live holder be WAITED FOR rather than failed-fast so genuinely-racing consent writers serialize), `releaseLock(handle)` (token + inode owner-safe — never deletes a successor's lock), `getProcessStartTime`, and the `_setLockProbes`/`_resetLockProbes` test seams. Carries the #1462 lifecycle-lock invariants (process-start-time liveness, TOCTOU-safe pre-rename identity recheck, bounded iterative loop). diff --git a/docs/adr/2363-capability-instruction-surface-trust.md b/docs/adr/2363-capability-instruction-surface-trust.md index 24c4cbd4a..dd27dad31 100644 --- a/docs/adr/2363-capability-instruction-surface-trust.md +++ b/docs/adr/2363-capability-instruction-surface-trust.md @@ -155,3 +155,81 @@ The README's first ratification trap is *"shipped code is necessary, not suffici **Full consent parity with hooks** — require the same ceremony for a skill contribution as for a hook. Rejected as consent fatigue. [`capability-trust-model.md`](../explanation/capability-trust-model.md) already rejects a per-run egress prompt on exactly these grounds: inflating every contribution to hook-level ceremony trains users to click through, degrading the prompt that matters. **Docs correction with no ADR** — rejected: `CONTRIBUTING.md` requires an ADR for an architectural decision, and a docs edit with no recorded decision reproduces the unrecorded posture this ADR exists to end. + +## Amendment (2026-08-18): `bundleContentHash` is no longer exclusion-free + +The residual-gap section above states that `bundleContentHash` "walks every entry under the +bundle with no exclusions." As of #3631 that is no longer literally true, so the sentence is +corrected here rather than edited in place. + +The trigger was a usability defect, not a security one — running a Python-backed capability's +own test suite wrote `__pycache__` inside the bundle (an *empty* `__pycache__` directory was +enough, because the walk emits a typed DIR marker per directory), which changed the recomputed +hash and silently deactivated the capability. The FIRST fix shipped for this (basename-excluded +`__pycache__`/`.pytest_cache`/`.DS_Store`, plus any `.pyc`/`.pyo` file anywhere, with an excluded +DIRECTORY skipped from recursion entirely) was itself found UNSAFE by two orthogonal reviews and +was corrected before merge to next. That draft is not described further here; this section +describes the shipped exclusion. `.DS_Store` was part of that first draft and was later removed +from the exclusion set entirely (not merely narrowed) — see the amendment below. + +`bundleContentHash` now excludes from the DIGEST ONLY: +- A `__pycache__` or `.pytest_cache` DIRECTORY's own marker (its bare existence no longer moves + the hash) — but the directory is ALWAYS recursed into; every non-excluded child underneath is + still hashed. (Skipping recursion was the unsafe draft's hole: an excluded directory became an + unbounded, permanently-unhashed region a manifest hook `script` could point into — + `__pycache__/run.js` — ship benign, get consent, then rewrite freely afterward.) +- A `.pyc`/`.pyo` FILE, but ONLY when its immediate parent directory's basename is exactly + `__pycache__`. A `.pyc`/`.pyo` anywhere else (bundle root, `scripts/`, a directory literally + named `cache.pyc`, etc.) stays hashed, because a sourceless legacy `.pyc` there is genuinely + importable/executable. (The unsafe draft matched the suffix anywhere in the tree.) + +`.DS_Store` is deliberately NOT excluded (the first draft excluded it; that exclusion was removed +entirely, not narrowed). An excluded filename is a permanently unhashed name that a declared hook +`script` could still be pointed at — e.g. `hooks/.DS_Store` — and `isSafeHookScriptPath` (below) +was hardened only for the `__pycache__`/`.pytest_cache`/`.pyc`/`.pyo` shapes, not for `.DS_Store`. +It was also unrelated to #3631's reported symptom (Python bytecode caching from a test run), so it +was not worth carrying as a permanently unhashed name. `.DS_Store` now stays bound like any other +file. + +`isSafeHookScriptPath` (`src/capability-lifecycle.cts` and its mirror +`gsd-core/bin/lib/capability-validator.cjs`) rejects any DECLARED script path containing a +`__pycache__`/`.pytest_cache` path segment, or whose basename ends `.pyc`/`.pyo` — a file named +e.g. `x.pyc` can contain perfectly valid JavaScript and would be executed by `node` regardless of +extension. This raises the bar for a manifest-declared hook `script`, but it is NOT a containment +bound on the excluded region: it only ever inspects the declared `script` path string itself, not +what that script `require`s/`import`s at runtime. A hashed, consent-covered `hooks/run.js` +containing `require('../__pycache__/mod.pyc')` reaches the excluded region in one hop — Node loads +an unregistered extension through its default `.js` handler — and the validator never sees that +reference. Once loaded that way, the referenced `.pyc` is free to be rewritten post-consent with +the digest unmoved. See the corrected bound below. + +**D4's argument is unaffected.** The claim this ADR rests on is that a single changed byte in a +skill body deactivates a project-scoped capability until re-consent. Skill bodies are `.md` files +and are not in the exclusion set, so that still holds exactly as written. + +**ACCEPTED RESIDUAL RISK — stated plainly, not glossed over.** An earlier version of this section +claimed CPython "validates [a cached `.pyc`] against its sibling source" before trusting it. That +claim is FALSE and was disproven by execution: CPython's default (timestamp-based) invalidation +compares the cached `.pyc` header's stored mtime and size against the CURRENT source file's mtime +and size — it does NOT check source content. Both mtime and size are ordinary file metadata an +attacker who can already write to the bundle can forge. A forged `__pycache__/mod.cpython-3XX.pyc` +whose header mtime/size were copied from an unmodified, still-hashed `mod.py` executes without +moving this digest. Before this change, any write under `__pycache__` (even an empty directory) +was detected; after it, a `__pycache__/*.pyc` matching that narrow shape is not. This is accepted +deliberately — it stops routine bytecode caching from silently deactivating capabilities, which is +the usability defect this exclusion exists to fix — and it is bounded by: (1) the attacker must +already have POST-CONSENT write access to the bundle (this is not a remote-exploit surface); (2) +everything outside `__pycache__/*.pyc` — including sourceless legacy `.pyc`/`.pyo` files anywhere +else in the bundle — remains hashed. NOT a bound: the excluded region IS reachable by indirection +from any hashed, consent-covered script — a `require`/`import` of a `__pycache__/*.pyc` path is one +hop, not only CPython's own bytecode loading path described above — so `isSafeHookScriptPath` +raises the bar for a DECLARED hook `script` surface but does not contain the risk. KNOWN +LIMITATION: `.pytest_cache`'s CONTENTS still change the digest as ordinary hashed files — only its +directory marker is suppressed, so this residual risk does not extend to `.pytest_cache`. + +Two properties were preserved deliberately and are pinned by tests: the exclusion is applied +AFTER the symlink/non-regular fail-closed rejection (so a symlink named `x.pyc` still throws +rather than being silently skipped), and excluded entries still count toward the walk's +size/count caps. `node_modules`, `dist`, and `build` were considered and deliberately NOT +excluded: their contents are required/executed at runtime, so dropping them from the digest +would stop consent binding executable content. diff --git a/gsd-core/bin/lib/capability-validator.cjs b/gsd-core/bin/lib/capability-validator.cjs index b6e187aa4..fa196ce97 100644 --- a/gsd-core/bin/lib/capability-validator.cjs +++ b/gsd-core/bin/lib/capability-validator.cjs @@ -210,10 +210,35 @@ const SHA512_INTEGRITY_RE = /^sha512-[A-Za-z0-9+/]{86}==$/; // hard validation error — fail closed so the capability install/load is rejected loudly. const SAFE_HOOK_SCRIPT_RE = /^[A-Za-z0-9._/-]+$/; +// #3631 (defense-in-depth, mirrors capability-lifecycle.cts — KEEP BOTH IN SYNC): a declared script +// path must not point into the space bundleContentHash (capability-consent.cts) excludes from the +// consent-binding digest. A file whose basename ends `.pyc`/`.pyo` can contain perfectly valid +// JavaScript and would be executed by `node` regardless of extension, and a `__pycache__`/ +// `.pytest_cache` segment marks a directory whose digest marker is suppressed — so a MANIFEST-DECLARED +// executable surface must never be able to reach either, or the exclusion becomes reachable from a +// path an attacker fully controls at declare-time rather than only via post-consent tamper. +// Regex asymmetry is DELIBERATE, KEEP BOTH RULES IN SYNC WITH capability-lifecycle.cts (byte-identical +// text, verified by the isSafeHookScriptPath parity test in tests/capability-registry.test.cjs): +// (i) PYCACHE_SUFFIX_RE is case-INSENSITIVE (`/i`) on purpose — a validator should be STRICTER than +// the digest it defends, so it rejects `x.PYC` too even though bundleContentHash's own suffix +// match (hasPycacheFileSuffix, capability-consent.cts) is byte-exact and would still hash it. +// (ii) The __pycache__/.pytest_cache SEGMENT match is case-SENSITIVE to match the digest's own +// byte-exact, case-sensitive directory-basename comparison (CPython always writes a lowercase +// `__pycache__`) — a validator segment match looser than the digest here would reject paths the +// digest would still hash, which is over-strict in the wrong direction for a defense-in-depth +// check layered on top of an already-correct digest. +// (iii) The `[/\\]` backslash alternations in both regexes are defensive/UNREACHABLE in practice: +// SAFE_HOOK_SCRIPT_RE (above) already rejects any backslash character outright, so a script +// string containing `\` never reaches either PYCACHE_*_RE check. +const PYCACHE_SEGMENT_RE = /(?:^|[/\\])(__pycache__|\.pytest_cache)(?:[/\\]|$)/; +const PYCACHE_SUFFIX_RE = /\.(pyc|pyo)$/i; + /** * #1460 (R): true when a relative hook-script path is shell-safe (see SAFE_HOOK_SCRIPT_RE). * Rejects absolute paths, `..` segments, a leading `-` on any path segment, and any char - * outside the allowlist (whitespace / shell metacharacters / control / NUL). + * outside the allowlist (whitespace / shell metacharacters / control / NUL). #3631: also rejects a + * path with a `__pycache__`/`.pytest_cache` segment or a `.pyc`/`.pyo` basename suffix (defense in + * depth — keeps a declared executable surface out of the digest-excluded space). */ function isSafeHookScriptPath(script) { if (typeof script !== 'string' || script.length === 0) return false; @@ -225,6 +250,8 @@ function isSafeHookScriptPath(script) { for (const seg of segments) { if (seg.startsWith('-')) return false; } + if (PYCACHE_SEGMENT_RE.test(script)) return false; + if (PYCACHE_SUFFIX_RE.test(path.basename(script))) return false; return true; } diff --git a/src/capability-consent.cts b/src/capability-consent.cts index 262ea7f31..f77df56e3 100644 --- a/src/capability-consent.cts +++ b/src/capability-consent.cts @@ -15,8 +15,9 @@ * project ledger — `'' === ''` is no binding) NOR to the `disclosureSignature` alone (which covers * only executable surfaces, so a declarative-only cap has a constant signature and a repo-write * attacker could swap `capability.json` for a malicious gate/contribution while consent still - * matched). `bundleContentHash` is recomputed by the loader at load over EVERY file in the bundle - * (manifest AND artifacts AND identity), so any tamper — declarative-only swap, hook-script edit, + * matched). `bundleContentHash` is recomputed by the loader at load over the bundle (manifest AND + * artifacts AND identity), excluding only derived Python bytecode/cache noise (#3631 — see + * `bundleContentHash`'s own doc comment), so any tamper — declarative-only swap, hook-script edit, * empty-integrity local install — changes the hash and leaves the cap inactive. `integrity` and * `disclosureSignature` remain on the record for the human disclosure + re-consent-on-executable- * change UX (TRUST-2); they are NO LONGER the security binding. @@ -265,10 +266,31 @@ function normalizeSepBytes(rel: Buffer): Buffer { /** * Recursively collect every REGULAR file AND every DIRECTORY under `absDir` as RAW-BYTE POSIX-relative - * paths (`rel`, relative to the bundle root), refusing to follow symlinks out of the bundle. Bounded: - * throws if the entry count or total byte size exceeds the caps (fail closed — a hostile/runaway tree - * never hashes unbounded content). A non-regular entry encountered IN the tree (FIFO/device) is a - * fail-closed throw — a bundle must be plain files and directories. + * paths (`rel`, relative to the bundle root), refusing to follow symlinks out of the bundle. Two + * NARROW, RECURSION-PRESERVING exclusions apply (#3631, amended after two orthogonal reviews found the + * original wholesale-skip UNSAFE — see the doc comment on `bundleContentHash` for the full rationale + * and the accepted residual risk): + * + * 1. A DIRECTORY whose basename is exactly `__pycache__` or `.pytest_cache`: its own TAG_DIR marker + * is SUPPRESSED (not pushed), but it is STILL RECURSED INTO — every child that is not itself + * excluded is still hashed. Suppressing only the marker is what keeps an empty/all-excluded such + * directory from moving the digest; recursing is what closes the original hole (H1): skipping + * recursion made an excluded directory an unbounded, permanently-unhashed region a manifest could + * point a hook `script` at (`__pycache__/run.js`) to install benign, then rewrite freely post-consent. + * 2. A FILE whose name ends `.pyc` or `.pyo` is excluded ONLY when its PARENT directory's basename is + * exactly `__pycache__` (checked byte-exact, never `.pytest_cache`) — a `.pyc`/`.pyo` anywhere else + * (bundle root, `scripts/`, a directory literally named `cache.pyc`, etc.) is hashed normally, + * because a sourceless legacy `.pyc` there IS importable and executable (H2). + * + * A regular FILE literally named `__pycache__` or `.pytest_cache` is NOT excluded (only a DIRECTORY of + * that basename gets marker-suppression) — it is hashed like any other file. A DIRECTORY literally + * named `x.pyc` is NOT excluded either — the suffix rule only ever applies to files. All exclusion + * checks run strictly AFTER the lstat-backed symlink/non-regular rejections below, so a symlink or + * device masquerading under an excluded name is still fail-closed rejected rather than silently + * skipped. Bounded: throws if the entry count or total byte size exceeds the caps (fail closed — a + * hostile/runaway tree never hashes unbounded content); an excluded entry still counts toward BOTH + * caps (the caps guard the walk itself, not the digest). A non-regular entry encountered IN the tree + * (FIFO/device) is a fail-closed throw — a bundle must be plain files and directories. * * #1459 finding 2 (MED/HIGH, ROUND 6): the enumeration ITSELF is bounded. Instead of * `fs.readdirSync` (which loads + sorts a WHOLE directory before the count cap — so a malicious bundle @@ -287,11 +309,76 @@ function normalizeSepBytes(rel: Buffer): Buffer { * abs/rel paths are concatenated at the BYTE level, so an invalid-UTF-8 filename is never lossily * decoded — two filenames that differ only in invalid bytes produce distinct rel byte strings. * - * @param absDir the absolute directory to scan, as RAW BYTES (Buffer). - * @param relDir the relpath of `absDir` from the bundle root, as RAW BYTES (Buffer; empty at the root). - * @param count the CUMULATIVE entry counter shared across the whole recursive walk (fail-closed at the cap). + * @param absDir the absolute directory to scan, as RAW BYTES (Buffer). + * @param relDir the relpath of `absDir` from the bundle root, as RAW BYTES (Buffer; empty at the root). + * @param dirBasename the basename of `absDir` itself, as RAW BYTES (Buffer) — the PARENT basename every + * entry collected at this level shares. Threaded down so a `.pyc`/`.pyo` FILE can be + * excluded only when ITS parent is exactly `__pycache__` (empty at the bundle root). + * @param count the CUMULATIVE entry counter shared across the whole recursive walk (fail-closed at the cap). */ -function collectBundleEntries(absDir: Buffer, relDir: Buffer, acc: BundleEntry[], total: { bytes: number }, count: { n: number }): void { +// #3631 (amended — see bundleContentHash's doc comment for the H1/H2 rationale and accepted residual +// risk): basename rules for what is excluded FROM THE DIGEST. They are NOT excluded from the walk's +// resource caps — see the count.n / total.bytes accounting below, which still sees every excluded +// entry. +// +// Exact byte comparison, case-sensitive, deliberately: CPython always writes a lowercase +// `__pycache__` directory, so byte-exact matching keeps the digest identical across case-insensitive +// filesystems (e.g. default macOS/Windows) instead of varying with how a name happens to be spelled +// on disk. +// +// DELIBERATELY NOT on this list: node_modules, dist, build, or similar. Those hold code that is +// actually required/executed at runtime, so excluding them from the digest would stop consent from +// binding executable content — turning a usability bug (noisy re-consent prompts) into a +// supply-chain hole (a swapped dependency that never re-triggers consent). + +/** Directory basenames whose TAG_DIR marker is suppressed — but the directory is STILL RECURSED INTO. */ +const PYCACHE_DIR_BASENAMES: readonly Buffer[] = [ + Buffer.from('__pycache__'), + Buffer.from('.pytest_cache'), +]; +/** FILE-name suffixes excluded ONLY when the file's parent directory basename is `__pycache__` (see below). */ +const PYCACHE_FILE_SUFFIXES: readonly Buffer[] = [ + Buffer.from('.pyc'), + Buffer.from('.pyo'), +]; +/** The one parent directory basename that gates the `.pyc`/`.pyo` FILE-suffix exclusion — never `.pytest_cache`. */ +const PYCACHE_PARENT_BASENAME = Buffer.from('__pycache__'); + +/** + * True when `name` (a raw-byte dirent basename) is a DIRECTORY whose TAG_DIR marker must be + * suppressed — `__pycache__` or `.pytest_cache`, exact byte match. The caller still recurses into it; + * this predicate answers ONLY "skip the marker", never "skip the subtree". + */ +function isPycacheDirBasename(name: Buffer): boolean { + for (const exact of PYCACHE_DIR_BASENAMES) { + if (Buffer.compare(name, exact) === 0) return true; + } + return false; +} + +/** True when raw-byte basename `name` ends in `.pyc` or `.pyo` (byte-suffix match, never utf8-decoded). */ +function hasPycacheFileSuffix(name: Buffer): boolean { + for (const suffix of PYCACHE_FILE_SUFFIXES) { + if (name.length >= suffix.length && Buffer.compare(name.subarray(name.length - suffix.length), suffix) === 0) { + return true; + } + } + return false; +} + +/** + * True when a FILE with basename `name`, inside a directory whose OWN basename is `dirBasename`, must + * be excluded from the digest: a `.pyc`/`.pyo` suffix whose parent directory basename is exactly + * `__pycache__` (byte-exact — never `.pytest_cache`, so a `.pyc` sitting directly inside a + * `.pytest_cache` dir is still hashed). A `.pyc`/`.pyo` file anywhere else — bundle root, `scripts/`, + * a directory literally named `cache.pyc`, etc. — is NOT excluded (H2): a sourceless legacy `.pyc` + * there is importable and executable, so it must stay bound to consent. + */ +function isExcludedFileBasename(name: Buffer, dirBasename: Buffer): boolean { + return hasPycacheFileSuffix(name) && Buffer.compare(dirBasename, PYCACHE_PARENT_BASENAME) === 0; +} + +function collectBundleEntries(absDir: Buffer, relDir: Buffer, dirBasename: Buffer, acc: BundleEntry[], total: { bytes: number }, count: { n: number }): void { let dir: fs.Dir; try { // RAW-BYTE streaming open: dirent names are Buffers (encoding: 'buffer'), so an invalid-UTF-8 @@ -344,19 +431,34 @@ function collectBundleEntries(absDir: Buffer, relDir: Buffer, acc: BundleEntry[] throw new Error(`bundleContentHash: refusing to hash a symlink in the bundle: "${abs.toString('utf8')}"`); } if (st.isDirectory()) { + // #3631 (amended, H1): exclusion is checked HERE — after the symlink fail-closed throw above — + // so a symlink named e.g. "__pycache__" or "x.pyc" is never silently skipped; only a REAL + // (lstat-confirmed) dir/file can be excluded. An excluded directory's own dirent was already + // counted toward count.n above (the cap guards the walk itself). Only the TAG_DIR MARKER is + // suppressed for `__pycache__`/`.pytest_cache` — the directory is ALWAYS recursed into so every + // non-excluded child underneath is still hashed (closing H1: no unbounded unhashed region). + if (isPycacheDirBasename(name)) { + collectBundleEntries(abs, rel, name, acc, total, count); + continue; + } // Emit a typed DIR marker for THIS directory (so an empty dir is bound), then recurse into it. acc.push({ abs, rel, kind: 'dir' }); - collectBundleEntries(abs, rel, acc, total, count); + collectBundleEntries(abs, rel, name, acc, total, count); continue; } if (!st.isFile()) { throw new Error(`bundleContentHash: refusing to hash a non-regular file in the bundle: "${abs.toString('utf8')}"`); } - acc.push({ abs, rel, kind: 'file' }); + // #3631: an excluded FILE (a __pycache__/*.pyc or *.pyo) still counts its bytes toward the + // total-size cap below — exclusion answers "does this bind the digest?", not "is this safe to + // read unbounded?" — so it must never become a way to smuggle unbounded bytes past + // BUNDLE_MAX_TOTAL_BYTES. total.bytes += st.size; if (total.bytes > BUNDLE_MAX_TOTAL_BYTES) { throw new Error(`bundleContentHash: bundle size exceeds ${BUNDLE_MAX_TOTAL_BYTES} bytes (refusing)`); } + if (isExcludedFileBasename(name, dirBasename)) continue; + acc.push({ abs, rel, kind: 'file' }); } } @@ -383,8 +485,39 @@ const TAG_DIR = Buffer.from([0x02]); /** * The recomputed full-bundle content hash (#1459 CB-1/CB-2/TRUST2-5) — the SECURITY BINDING. A - * `sha512-` over a DETERMINISTIC, INJECTIVE, LOSSLESS serialization of EVERY regular file - * AND directory under `capDir` (recursively). + * `sha512-` over a DETERMINISTIC, INJECTIVE, LOSSLESS serialization of every regular file + * AND directory under `capDir` (recursively), with two NARROW exclusions (#3631, amended after two + * orthogonal reviews found the original wholesale directory-skip UNSAFE): + * - A `__pycache__` or `.pytest_cache` DIRECTORY's own TAG_DIR marker is suppressed, but it is + * ALWAYS recursed into — every non-excluded child underneath is still hashed. + * - A `.pyc`/`.pyo` FILE is excluded ONLY when its immediate parent directory's basename is exactly + * `__pycache__`; a `.pyc`/`.pyo` anywhere else (bundle root, `scripts/`, etc.) is hashed normally, + * since a sourceless legacy `.pyc` there is importable and executable. + * `node_modules`, `dist`, `build`, and similar are deliberately NOT excluded: their contents are + * executed/required at runtime, so dropping them from the digest would stop consent from binding + * executable content. + * + * ACCEPTED RESIDUAL RISK (documented, not a defect to re-litigate): CPython's default `.pyc` + * invalidation (timestamp-based) validates a cached bytecode file against its SOURCE's mtime and size + * only — NOT against the source's content — and both mtime and size are attacker-settable by whoever + * can already write to the bundle. So a forged `__pycache__/mod.cpython-3XX.pyc` whose header mtime/size + * were copied from an unmodified, still-hashed `mod.py` will execute without moving this digest. Before + * this exclusion existed that write was detected (any file under `__pycache__` moved the hash); after + * it, it is not, for files matching the narrow `__pycache__/*.pyc` shape above. This is accepted + * deliberately — the alternative (H1's original wholesale skip, or hashing every regenerated bytecode + * file) either reopens an unbounded unhashed region or makes consent fire on routine bytecode caching — + * and is bounded by: (1) the attacker must already have POST-CONSENT write access to the bundle; (2) + * everything outside `__pycache__/*.pyc` — including sourceless legacy `.pyc`/`.pyo` files anywhere + * else — remains hashed. NOT a bound, despite `isSafeHookScriptPath` (capability-lifecycle.cts / + * capability-validator.cjs) rejecting a manifest-declared hook `script` that names a + * `__pycache__`/`.pytest_cache` segment or ends `.pyc`/`.pyo`: the excluded region is still reachable + * by ONE HOP of indirection from any hashed, consent-covered script — a `require`/`import` of a + * `__pycache__/*.pyc` module path is not itself a declared script and the validator never sees it — + * so a hashed `hooks/run.js` can load a `__pycache__/mod.pyc` whose bytes are then free to change + * post-consent with the digest unmoved. The validator guard raises the bar for DECLARED surfaces; it + * does not contain the risk. KNOWN LIMITATION: `.pytest_cache`'s CONTENTS still change the digest as + * normal files — only its directory marker is suppressed, so this residual risk does NOT extend to + * `.pytest_cache`. * * Canonicalization (#1459 findings 1 + 4 — the prior `relpath + NUL + content + NUL` over utf8-decoded * STRINGS was non-injective, lossy in CONTENT, AND lossy in the PATH component): @@ -412,8 +545,12 @@ const TAG_DIR = Buffer.from([0x02]); function bundleContentHash(capDir: string): string { // Resolve to an absolute path, then carry it as RAW BYTES so the walk never lossily decodes a name. const rootBytes = Buffer.from(path.resolve(capDir)); + // The root's own basename is threaded as the initial `dirBasename` so the parent-basename check for + // a `.pyc`/`.pyo` FILE applies even to a file placed DIRECTLY at the bundle root (root literally + // named `__pycache__` is the only case this matters for, and it is a correct, if exotic, match). + const rootBasename = Buffer.from(path.basename(path.resolve(capDir))); const entries: BundleEntry[] = []; - collectBundleEntries(rootBytes, Buffer.alloc(0), entries, { bytes: 0 }, { n: 0 }); + collectBundleEntries(rootBytes, Buffer.alloc(0), rootBasename, entries, { bytes: 0 }, { n: 0 }); // Sort by the raw-byte (separator-normalized) relpath so the digest is identical on Windows and POSIX, // and is independent of the on-disk creation/readdir order. Tie-break on kind so a (degenerate, never // produced on a real fs) file-and-dir same-relpath pair still has a stable order. diff --git a/src/capability-lifecycle.cts b/src/capability-lifecycle.cts index bddc1c9f8..4b6e915c3 100644 --- a/src/capability-lifecycle.cts +++ b/src/capability-lifecycle.cts @@ -416,6 +416,28 @@ function confinedSharedFile(runtimeDir: string, relFile: unknown): string | null // isSafeHookScriptPath; see confinedBundleScript for why). Only [A-Za-z0-9._/-], no leading // `-` segment, no `..`, not absolute. const SAFE_HOOK_SCRIPT_RE = /^[A-Za-z0-9._/-]+$/; +// #3631 (defense-in-depth, mirrors capability-validator.cjs — KEEP BOTH IN SYNC): a declared script +// path must not point into the space bundleContentHash (capability-consent.cts) excludes from the +// consent-binding digest. A file whose basename ends `.pyc`/`.pyo` can contain perfectly valid +// JavaScript and would be executed by `node` regardless of extension, and a `__pycache__`/ +// `.pytest_cache` segment marks a directory whose digest marker is suppressed — so a MANIFEST-DECLARED +// executable surface must never be able to reach either, or the exclusion becomes reachable from a +// path an attacker fully controls at declare-time rather than only via post-consent tamper. +// Regex asymmetry is DELIBERATE, KEEP BOTH RULES IN SYNC WITH capability-validator.cjs (byte-identical +// text, verified by the isSafeHookScriptPath parity test in tests/capability-registry.test.cjs): +// (i) PYCACHE_SUFFIX_RE is case-INSENSITIVE (`/i`) on purpose — a validator should be STRICTER than +// the digest it defends, so it rejects `x.PYC` too even though bundleContentHash's own suffix +// match (hasPycacheFileSuffix, capability-consent.cts) is byte-exact and would still hash it. +// (ii) The __pycache__/.pytest_cache SEGMENT match is case-SENSITIVE to match the digest's own +// byte-exact, case-sensitive directory-basename comparison (CPython always writes a lowercase +// `__pycache__`) — a validator segment match looser than the digest here would reject paths the +// digest would still hash, which is over-strict in the wrong direction for a defense-in-depth +// check layered on top of an already-correct digest. +// (iii) The `[/\\]` backslash alternations in both regexes are defensive/UNREACHABLE in practice: +// SAFE_HOOK_SCRIPT_RE (above) already rejects any backslash character outright, so a script +// string containing `\` never reaches either PYCACHE_*_RE check. +const PYCACHE_SEGMENT_RE = /(?:^|[/\\])(__pycache__|\.pytest_cache)(?:[/\\]|$)/; +const PYCACHE_SUFFIX_RE = /\.(pyc|pyo)$/i; function isSafeHookScriptPath(script: string): boolean { if (typeof script !== 'string' || script.length === 0) return false; if (!SAFE_HOOK_SCRIPT_RE.test(script)) return false; @@ -425,6 +447,8 @@ function isSafeHookScriptPath(script: string): boolean { for (const seg of segments) { if (seg.startsWith('-')) return false; } + if (PYCACHE_SEGMENT_RE.test(script)) return false; + if (PYCACHE_SUFFIX_RE.test(path.basename(script))) return false; return true; } diff --git a/tests/capability-consent.test.cjs b/tests/capability-consent.test.cjs index fbdab56c4..2fd50483b 100644 --- a/tests/capability-consent.test.cjs +++ b/tests/capability-consent.test.cjs @@ -985,4 +985,337 @@ test('WIN-3: roots containing spaces are keyed unambiguously on disk (no collisi } }); +// --------------------------------------------------------------------------- +// #3631: bundleContentHash excludes Python bytecode-cache noise from the DIGEST, so that +// running a Python-backed capability's own test suite — which writes __pycache__ inside +// the bundle — does not flip the recomputed hash and silently deactivate consent. +// +// The exclusion is deliberately NARROW, because this digest is a consent binding and every +// excluded byte is a byte that can change post-consent without detection: +// - a DIRECTORY named __pycache__ / .pytest_cache has only its TAG_DIR marker suppressed; +// the walk STILL RECURSES and hashes every non-excluded child (so __pycache__/run.js +// stays bound). Skipping recursion would make it a permanently unhashed region that a +// declared hook script could point into. +// - a FILE ending .pyc/.pyo is excluded ONLY when its parent basename is exactly +// __pycache__. Elsewhere it stays bound: a legacy sourceless .pyc IS importable. +// - a regular FILE named __pycache__, and a DIRECTORY named x.pyc, are ordinary content +// and stay bound — marker suppression is directory-only, the suffix rule file-only. +// Exclusion applies AFTER the lstat/symlink fail-closed rejection, and excluded entries +// still count toward BUNDLE_MAX_FILES / BUNDLE_MAX_TOTAL_BYTES. +// +// Deliberately NOT excluded: node_modules, dist, build — their contents ARE executed, so +// excluding them would break the consent binding for real executable surfaces. +// ACCEPTED RESIDUAL RISK (see the ADR-2363 amendment): CPython's default timestamp +// invalidation checks a cached pyc only against its source's mtime+size, both forgeable by +// anyone who can already write to the bundle — so a forged __pycache__/mod.pyc matching an +// unmodified mod.py executes without moving the digest. Accepted knowingly; NOT excused by +// any claim that CPython validates bytecode against source content (it does not). +// --------------------------------------------------------------------------- + +test('#3631: an EMPTY __pycache__/ directory does not change the bundle hash', (t) => { + // This is the TAG_DIR-marker trigger: collectBundleEntries pushes a {kind:'dir'} entry for EVERY + // directory (including empty ones) and bundleContentHash emits a TAG_DIR marker for it. A fix that + // only filters *.pyc file CONTENT and still emits the DIR marker for an empty __pycache__/ leaves + // this red — the exclusion must be by basename, applied to the dir entry itself, not just its files. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '__pycache__'), { recursive: true }); // EMPTY — no .pyc inside yet + const after = consent.bundleContentHash(dir); + assert.strictEqual(after, before, 'an empty __pycache__ directory must not change the hash'); +}); + +test('#3631: hooks/__pycache__/check.cpython-313.pyc does not change the hash', (t) => { + const dir = makeBundle({ manifest: { id: 'cap', role: 'feature', version: '1.0.0', hooks: [{ event: 'PostToolUse', script: 'hooks/check.js' }] }, script: 'console.log(1)' }); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, 'hooks', '__pycache__'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'hooks', '__pycache__', 'check.cpython-313.pyc'), Buffer.from([5, 6, 7])); + const after = consent.bundleContentHash(dir); + assert.strictEqual(after, before, '__pycache__ nested under hooks/ must not change the hash'); +}); + +test('#3631: __pycache__ under an existing tests/ dir does not change the hash (isolated from the tests/-dir-creation variable)', (t) => { + // Creating the NEW tests/ dir itself legitimately changes the hash (it is not excluded), so tests/ is + // pre-created WITH a real file BEFORE the baseline snapshot — only the __pycache__ add is under test. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, 'tests'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'tests', 'test_x.py'), 'def test_x():\n assert True\n', 'utf8'); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, 'tests', '__pycache__'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'tests', '__pycache__', 'test_x.pyc'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.strictEqual(after, before, '__pycache__ under an already-present tests/ dir must not change the hash'); +}); + +test('#3631 (post-hardening): .pytest_cache/CACHEDIR.TAG DOES change the hash — only the dir MARKER is suppressed', (t) => { + // Hardened semantics: exclusion suppresses the TAG_DIR marker for a __pycache__/.pytest_cache + // directory, but the walk still RECURSES into it and hashes every non-excluded child. Suppressing + // the whole subtree would create an unhashed region a declared surface could point into (the HIGH + // finding closed below) — this is a deliberate, documented limitation, not a gap. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '.pytest_cache'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.pytest_cache', 'CACHEDIR.TAG'), 'Signature: 8a477f597d28d172789f06886806bc55\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, '.pytest_cache/CACHEDIR.TAG content is still bound to the hash even though the dir marker is suppressed'); +}); + +test('#3631 (post-hardening): .DS_Store at the bundle root DOES change the hash — deliberately NOT excluded', (t) => { + // .DS_Store is deliberately NOT on the exclusion list. Excluding a filename means a permanently + // unhashed name that a declared hook `script` could point into (e.g. `hooks/.DS_Store`, which the + // validator's __pycache__/.pytest_cache/.pyc/.pyo rejection does not cover), and it is unrelated to + // #3631's reported symptom (Python bytecode caching from a test run). Stays bound like any other file. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, '.DS_Store'), Buffer.from([0, 0, 0, 1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, '.DS_Store must still change the hash — it is not excluded'); +}); + +test('#3631 (post-hardening): a REGULAR FILE literally named __pycache__ at the bundle root DOES change the hash', (t) => { + // Marker suppression applies to DIRECTORIES only. A file wearing the __pycache__ name is ordinary + // content — the walk must still bind it, and must not choke on the kind mismatch. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, '__pycache__'), 'not actually a directory', 'utf8'); + let after; + assert.doesNotThrow(() => { after = consent.bundleContentHash(dir); }, 'a file named __pycache__ must not throw'); + assert.notStrictEqual(after, before, 'a regular FILE named __pycache__ is ordinary content and must change the hash'); +}); + +test('#3631 (post-hardening): stray.pyc at the bundle ROOT (not under any __pycache__) DOES change the hash', (t) => { + // A legacy sourceless .pyc outside __pycache__ IS importable by CPython, so it must stay bound to + // the digest. Only __pycache__-resident bytecode (whose parent dir basename is exactly + // __pycache__) is excluded by suffix; a stray .pyc elsewhere is hashed like any other file. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, 'stray.pyc'), Buffer.from([9, 9, 9])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a root-level .pyc file (not under __pycache__) must still change the hash — it is legacy-importable content'); +}); + +test('#3631: a bundle whose ONLY added content is __pycache__/mod.pyc hashes IDENTICALLY to before that dir existed, and does not throw', (t) => { + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '__pycache__'), { recursive: true }); + fs.writeFileSync(path.join(dir, '__pycache__', 'mod.pyc'), Buffer.from([1, 2, 3, 4])); + let after; + assert.doesNotThrow(() => { after = consent.bundleContentHash(dir); }, 'adding only __pycache__/mod.pyc must not throw'); + assert.strictEqual(after, before, 'adding only __pycache__/mod.pyc must be a full no-op on the digest'); +}); + +test('#3631 (GREEN — must stay green: anti-regression control): modifying a REAL source file still changes the hash', (t) => { + // Proves the fix NARROWS the hash rather than gutting it — an actual code-content edit must still be + // observable to the binding once the __pycache__/.pyc noise is excluded. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, 'scripts'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'scripts', 'm.py'), 'print("v1")\n', 'utf8'); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, 'scripts', 'm.py'), 'print("v2")\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a real source-file content edit must still change the hash'); +}); + +test('#3631 (GREEN — must stay green): adding node_modules/pkg/index.js changes the hash (node_modules is deliberately NOT excluded)', (t) => { + // node_modules holds code that is actually executed/required at runtime — excluding it would break the + // consent binding for real executable content. Only __pycache__/.pytest_cache/*.pyc/*.pyo (the latter + // two only when nested directly under __pycache__) are excluded; node_modules, dist, and build are NOT + // on that list and must keep binding the hash. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, 'node_modules', 'pkg'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'node_modules', 'pkg', 'index.js'), 'module.exports = 1;\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'node_modules content must still bind the hash'); +}); + +test('#3631 (GREEN — must stay green, boundary): excluded .pyc entries still count toward BUNDLE_MAX_FILES (limit-1 / limit / limit+1)', (t) => { + // The digest excludes __pycache__/*.pyc CONTENT, but the walk's entry-count cap must still see every + // entry (excluded or not) BEFORE exclusion is applied — the caps guard the walk itself (DoS/memory + // bound), the digest answers a separate question. Padding must be GENUINELY excluded to prove that + // property: a ROOT-level f{i}.pyc is NOT excluded (its parent is not __pycache__ — see the sibling + // "stray.pyc at the bundle ROOT" test above), so it would prove nothing about excluded entries. The + // padding here lives under __pycache__/f{i}.pyc, which the digest never binds, while the count cap + // still sees it. The real cap is BUNDLE_MAX_FILES (default BUNDLE_MAX_FILES_DEFAULT = 100_000 in + // src/capability-consent.cts:117) — too large to materialize cheaply in a test, so this drives the + // exported `_setBundleMaxFilesForTest` seam (the same seam the existing "finding 2" cap tests above + // use) down to a small CAP and proves the exact boundary. + // + // Entry arithmetic (confirmed by executing the built lib, not by reasoning): a bundle built from + // capability.json + an empty __pycache__/ dir + N excluded .pyc files inside it has + // 1 [capability.json] + 1 [the __pycache__ DIRECTORY dirent itself — its marker is suppressed but the + // walk still counts the dirent and still recurses] + N [f{i}.pyc files] = N + 2 total entries. + const CAP = 5; + const restore = consent._setBundleMaxFilesForTest(CAP); + t.after(restore); + + const build = (pycCount) => { + const bdir = makeBundle({}); + fs.mkdirSync(path.join(bdir, '__pycache__'), { recursive: true }); + for (let i = 0; i < pycCount; i++) { + fs.writeFileSync(path.join(bdir, '__pycache__', `f${i}.pyc`), Buffer.from([i])); + } + return bdir; + }; + + const dirBelow = build(CAP - 3); // total entries = 1 + 1 + (CAP-3) = CAP-1 + t.after(() => cleanup(dirBelow)); + assert.doesNotThrow(() => consent.bundleContentHash(dirBelow), 'limit-1 total entries (capability.json + __pycache__ dir + genuinely-excluded .pyc padding) must not throw'); + + const dirAt = build(CAP - 2); // total entries = 1 + 1 + (CAP-2) = CAP + t.after(() => cleanup(dirAt)); + assert.doesNotThrow(() => consent.bundleContentHash(dirAt), 'exactly-limit total entries (capability.json + __pycache__ dir + genuinely-excluded .pyc padding) must not throw'); + + const dirOver = build(CAP - 1); // total entries = 1 + 1 + (CAP-1) = CAP+1 + t.after(() => cleanup(dirOver)); + assert.throws( + () => consent.bundleContentHash(dirOver), + /exceeds|refusing/i, + 'limit+1 total entries must still throw even though every .pyc padding entry is excluded from the digest — caps guard the WALK, not the digest', + ); +}); + +test('#3631 (GREEN — must stay green, boundary): an EXCLUDED __pycache__/*.pyc file still trips BUNDLE_MAX_TOTAL_BYTES', (t) => { + // Exclusion answers "does this bind the digest?", never "is this safe to read unbounded?" — an + // excluded file's bytes must still count toward BUNDLE_MAX_TOTAL_BYTES (src/capability-consent.cts:105, + // 16 MiB), or exclusion becomes an unbounded-bytes DoS hole. Uses a SPARSE file (fs.truncateSync) well + // beyond any plausible cap so the assertion never hardcodes the exact byte constant — it costs no real + // disk or CPU time (no 32 MiB buffer is ever written). + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, '__pycache__'), { recursive: true }); + const bigPath = path.join(dir, '__pycache__', 'big.pyc'); + fs.writeFileSync(bigPath, Buffer.alloc(0)); + fs.truncateSync(bigPath, 32 * 1024 * 1024); // sparse — well past the 16 MiB cap, no real bytes written + assert.throws( + () => consent.bundleContentHash(dir), + /bundle size exceeds \d+ bytes \(refusing\)/, + 'an EXCLUDED __pycache__/*.pyc file must still trip BUNDLE_MAX_TOTAL_BYTES even though its content never reaches the digest', + ); +}); + +test('#3631 (security pin — parent-threading precision): __pycache__/sub/x.pyc stays HASHED — its parent is "sub", not "__pycache__"', (t) => { + // The single most valuable missing pin (isolated review finding): the .pyc/.pyo suffix exclusion is + // gated on the IMMEDIATE parent directory basename, threaded down one level at a time through the + // recursive walk. A .pyc two levels under __pycache__ must NOT be excluded — only a .pyc whose direct + // parent is __pycache__ itself is. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '__pycache__', 'sub'), { recursive: true }); + fs.writeFileSync(path.join(dir, '__pycache__', 'sub', 'x.pyc'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, '__pycache__/sub/x.pyc must still change the hash — its parent basename is "sub", not "__pycache__"'); +}); + +test('#3631 (security pin): .pytest_cache/y.pyc stays HASHED — the .pyc suffix rule never gates on .pytest_cache', (t) => { + // The PYCACHE_PARENT_BASENAME gate for the .pyc/.pyo suffix rule is exactly "__pycache__", never + // ".pytest_cache" — a .pyc sitting directly inside a .pytest_cache dir is ordinary content. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, '.pytest_cache'), { recursive: true }); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, '.pytest_cache', 'y.pyc'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, '.pytest_cache/y.pyc must still change the hash — the .pyc suffix rule only ever gates on a __pycache__ parent'); +}); + +test('#3631 (security pin — closes the untested .pyo mutant): __pycache__/m.pyo is excluded exactly like a .pyc sibling', (t) => { + // .pyo is on PYCACHE_FILE_SUFFIXES alongside .pyc but was never independently exercised anywhere in + // the suite — a surviving mutant could delete the ".pyo" entry with nothing failing. Pins both halves: + // excluded under __pycache__/, and still bound everywhere else. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, '__pycache__'), { recursive: true }); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, '__pycache__', 'm.pyo'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.strictEqual(after, before, '__pycache__/m.pyo must be excluded from the digest exactly like __pycache__/m.pyc'); +}); + +test('#3631 (security pin — closes the untested .pyo mutant): a root-level m.pyo DOES change the hash', (t) => { + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, 'm.pyo'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a root-level m.pyo (not under __pycache__) must still change the hash — it is legacy-importable content, same as a stray .pyc'); +}); + +test('#3631 (GREEN — already passes today, pins ordering post-fix): a SYMLINK named x.pyc still makes bundleContentHash THROW (fail-closed)', { skip: process.platform === 'win32' }, (t) => { + // Pins that the exclusion match must be applied AFTER the existing lstat/symlink rejection, never + // before — a symlinked *.pyc is exactly the shape a naive "skip by suffix before lstat" fix would + // silently pass through instead of rejecting. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.symlinkSync('/etc/passwd', path.join(dir, 'x.pyc')); + assert.throws(() => consent.bundleContentHash(dir), /symlink/i, 'a symlinked *.pyc must still be rejected fail-closed'); +}); + +// --------------------------------------------------------------------------- +// #3631 hardening — security regression pins. Two independent reviews found holes in the original +// exclusion: (HIGH) suppressing recursion into an excluded dir left it a permanently-unhashed region a +// declared surface could point into, and (the sourceless-legacy-.pyc vector) a bare .pyc anywhere was +// excluded by suffix alone even though CPython can import a sourceless .pyc outside __pycache__. Both +// holes are now closed: marker suppression is directory-only and never stops recursion, and the .pyc/ +// .pyo suffix exclusion applies ONLY when the parent directory basename is exactly __pycache__. +// --------------------------------------------------------------------------- + +test('#3631 (security pin — closes HIGH: excluded-dir recursion skip): __pycache__/run.js DOES change the hash', (t) => { + // Pins the HIGH finding: skipping recursion into __pycache__ made it a permanently-unhashed region + // that a declared hook script could point into. A non-.pyc file inside __pycache__ must still bind. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '__pycache__'), { recursive: true }); + fs.writeFileSync(path.join(dir, '__pycache__', 'run.js'), 'module.exports = 1;\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a non-.pyc file inside __pycache__ must still change the hash'); +}); + +test('#3631 (security pin — closes sourceless-legacy-.pyc vector): scripts/x.pyc DOES change the hash', (t) => { + // A .pyc whose parent is NOT __pycache__ is a legacy sourceless bytecode file CPython can still + // import directly — it must stay bound to the digest, regardless of how deep it is nested. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, 'scripts'), { recursive: true }); + const before = consent.bundleContentHash(dir); + fs.writeFileSync(path.join(dir, 'scripts', 'x.pyc'), Buffer.from([1, 2, 3])); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'scripts/x.pyc (parent is not __pycache__) must still change the hash'); +}); + +test('#3631 (security pin — suffix rule is file-only): a DIRECTORY named cache.pyc/ containing inner.js DOES change the hash', (t) => { + // Pins that the .pyc/.pyo suffix rule never applies to directories — a directory literally named + // cache.pyc gets no marker suppression and no exclusion; its contents bind normally. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, 'cache.pyc'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'cache.pyc', 'inner.js'), 'module.exports = 1;\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a directory named cache.pyc must be hashed normally (marker + recursion), not excluded'); +}); + +test('#3631 (security pin — recursion is unbounded depth): __pycache__/sub/deep.js DOES change the hash', (t) => { + // Proves recursion into an excluded dir goes all the way down, not just one level — a nested + // non-.pyc file several directories under __pycache__ must still bind. + const dir = makeBundle({}); + t.after(() => cleanup(dir)); + const before = consent.bundleContentHash(dir); + fs.mkdirSync(path.join(dir, '__pycache__', 'sub'), { recursive: true }); + fs.writeFileSync(path.join(dir, '__pycache__', 'sub', 'deep.js'), 'module.exports = 1;\n', 'utf8'); + const after = consent.bundleContentHash(dir); + assert.notStrictEqual(after, before, 'a nested non-.pyc file under __pycache__ must still change the hash, at any depth'); +}); + void crypto; // reserved import; keep explicit. diff --git a/tests/capability-registry.test.cjs b/tests/capability-registry.test.cjs index 8d1c242a9..ceceab16f 100644 --- a/tests/capability-registry.test.cjs +++ b/tests/capability-registry.test.cjs @@ -1918,6 +1918,82 @@ describe('C4: description and hooks validation', () => { assert.deepEqual(hookErrors, [], 'Expected a normal nested relative script to be accepted, got: ' + JSON.stringify(hookErrors)); }); + // ─── #3631 (defense-in-depth): a declared hook script path must not point into the space + // bundleContentHash excludes from the consent digest. A file named `x.pyc` can contain valid + // JavaScript and would be executed by `node` regardless of extension, and a __pycache__/ + // .pytest_cache path segment marks digest-excluded space — so a manifest-declared executable + // surface must never be able to reach either. ─── + for (const [label, script] of [ + ['__pycache__ path segment', '__pycache__/run.js'], + ['.pytest_cache path segment', '.pytest_cache/run.js'], + ['.pyc basename suffix', 'hooks/x.pyc'], + ]) { + test(`hook script pointing into digest-excluded space is rejected (${label})`, () => { + const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script }] }; + const errors = validateCapability(cap, 'ui'); + const hookErrors = errors.filter((e) => e.includes('hooks[0].script')); + assert.ok( + hookErrors.length > 0, + `Expected a hooks[0].script rejection for ${label} (script=${JSON.stringify(script)}), got: ` + JSON.stringify(errors), + ); + }); + } + + test('hook script not pointing into digest-excluded space is still accepted (hooks/check.js)', () => { + const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script: 'hooks/check.js' }] }; + const errors = validateCapability(cap, 'ui'); + const hookErrors = errors.filter((e) => e.includes('hooks[0].script')); + assert.deepEqual(hookErrors, [], 'Expected a normal .js hook script to be accepted, got: ' + JSON.stringify(hookErrors)); + }); + + // ─── Generative-fix-divergence parity: `isSafeHookScriptPath` is duplicated hand-maintained + // logic in src/capability-lifecycle.cts (via `confinedBundleScript`, the nearest exported + // consumer) and gsd-core/bin/lib/capability-validator.cjs (via `validateCapability`, the + // nearest exported consumer). There is no src/capability-validator.cts — the .cjs is hand + // -maintained — so this drives BOTH real copies behaviorally (never source-greps either file) + // and asserts they agree on every path, catching the two implementations drifting apart. #3631 + // finding 1 removed the `.DS_Store` DIGEST exclusion, but the validator never rejected + // `.DS_Store` on either side — `hooks/.DS_Store` is expected to be ACCEPTED by both. + test('isSafeHookScriptPath parity: capability-lifecycle.cts and capability-validator.cjs agree on every path', () => { + const { confinedBundleScript } = require('../gsd-core/bin/lib/capability-lifecycle.cjs'); + // A capDir that does not exist on disk: confinedBundleScript falls into its lexical + // (does-not-exist-yet) branch, so the verdict reflects ONLY isSafeHookScriptPath — never a + // realpath/confinement side effect unrelated to what is under test here. + const fakeCapDir = path.join(os.tmpdir(), 'gsd-parity-probe-nonexistent-cap-dir'); + + const cases = [ + ['hooks/check.js', true], + ['__pycache__/run.js', false], + ['hooks/__pycache__/run.js', false], + ['.pytest_cache/run.js', false], + ['hooks/x.pyc', false], + ['x.pyo', false], + ['hooks/x.PYC', false], + ['hooks/.DS_Store', true], + ['hooks/../__pycache__/run.js', false], + ['hooks/__pycache__./run.js', true], + ['__PYCACHE__/run.js', true], + ['hooks\\__pycache__\\run.js', false], + ]; + + for (const [script, expectedAccept] of cases) { + const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script }] }; + const errors = validateCapability(cap, 'ui'); + const cjsAccept = errors.filter((e) => e.includes('hooks[0].script')).length === 0; + const ctsAccept = confinedBundleScript(fakeCapDir, script) !== null; + assert.strictEqual( + cjsAccept, + ctsAccept, + `capability-validator.cjs (accept=${cjsAccept}) and capability-lifecycle.cts (accept=${ctsAccept}) disagree on ${JSON.stringify(script)}`, + ); + assert.strictEqual( + cjsAccept, + expectedAccept, + `expected accept=${expectedAccept} for ${JSON.stringify(script)}, both copies returned accept=${cjsAccept}`, + ); + } + }); + test('description present in UI_CAP passes validation', () => { const errors = validateCapability(UI_CAP, 'ui'); const descErrors = errors.filter((e) => e.includes('description'));