Skip to content

feat(execpolicy): builtin sensitive-path safety ruleset - #87

Open
asto18089 wants to merge 3 commits into
pinvou3-cleanfrom
upstream/u2-sensitive-path-inventory
Open

asto18089 wants to merge 3 commits into
pinvou3-cleanfrom
upstream/u2-sensitive-path-inventory

Conversation

@asto18089

@asto18089 asto18089 commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

execpolicy gains a canonical sensitive-path inventory and a builder that populates the previously unused RulesetLayer::BuiltinDefault, so embedders derive policy from the engine instead of hand-maintaining parallel inventories.

  • New module crates/execpolicy/src/sensitive_paths.rs:
    • SENSITIVE_DIRECTORY_NAMES / SENSITIVE_FILE_NAMES / SENSITIVE_ABSOLUTE_PREFIXES — the deduplicated union of the inventories embedders otherwise keep in parallel (an ingest-gate component blocklist, a generated hard-deny ruleset, and the migrated hook), each entry annotated with a one-line reason. Two entries (.dws, .tmeet) are embedder-contributed and split out at upstreaming time.
    • SENSITIVE_COMPONENT_NAMES — the ingest gate's component-anywhere face carried in full (bare directory names, SSH key basenames, credentials.json, .env, and the embedder stores .dws/.tmeet, which the migrated hook matched anywhere), so an adopter can re-derive the gate's ~/keys/id_rsa blocking that home-rooted spellings cannot express. .azure is deliberately ruleset-face-only: the gate's component blocklist does not name it. A cross-list test pins the two faces' agreement in both directions. has_sensitive_component(path) is the check.
    • contains(dir, candidate) — strict component-boundary containment (/home/u does NOT contain /home/user/...). Reuses the crate's existing path normalization (separator/case folding per platform, . collapse) and fails closed on .., empty, and drive-relative paths. Windows verbatim forms are folded first (\\?\-prefixed and \\?\UNC\ spellings compare equal to their plain forms), so a std::fs::canonicalize result and its plain spelling match; the limits lexical comparison cannot see (Win32 trailing dots/spaces, 8.3 short names, ASCII-only case folding, non-UTF-8 lossy collisions) are documented.
    • builtin_safety_ruleset() — the first producer of RulesetLayer::BuiltinDefault; hard-deny prefix rules derived from the inventory (a * <path> tail-anchored rule per exact-token spelling, plus trailing-slash and glob spellings for directories). cat /etc/shadow and rm -rf ~/.ssh are denied under every approval mode; ls /tmp runs cleanly; /var/log/auth.log is covered by an explicit rule.
  • The module docs scope the claim honestly: they name the in-tree predecessor (crates/tui/src/sandbox/read_guard.rs keeps a deliberately broader sandbox-face inventory that is not consolidated here), disclose the token channel's open bypass families in full — kernel-normalized spellings like cat /etc//shadow, glued tokens like dd if=/etc/shadow (including redirect and interpreter glue), remote specs like scp host:~/.ssh/id_rsa ., variable indirection like x=/etc/shadow; cat $x, command-side glob expansion like cat /etc/shado[w], cwd-relative composition like cd /etc && cat shadow, and aliased home spellings like cat ~root/.bashrc, with path-free command-native reads (getent shadow) scoped out — register the Windows DPAPI credential dirs as staying in the embedder's Windows-native list, state the deny face's unconditional ASCII case fold, and reword the posture so adopters know no cross-layer allowance can un-deny and which legitimate workflows (key rotation, kubectl --kubeconfig, textual mentions) are denied by construction.

Why the containment helper is the deliverable

The deny-rule token channel matches whole command tokens exactly: a rule can name ~/.ssh, but no rule can say "anything under ~/.ssh" — ~/.ssh/config is a different token. That gap is why embedders ship a generated corpus of thousands of enumerated rules today. contains() and has_sensitive_component() give callers that hold real paths (ingest gates, file-tool checks) the missing primitives, and the module docs record that the corpus collapses once rule evaluation gains containment matching. Until then, builtin_safety_ruleset() deliberately anchors only the exact-token faces and stays inventory-derived rather than reproducing the corpus.

The ruleset is hard deny (deny-always-wins across layers, pinned by test); the docs register that this also denies key-rotation vocabulary and agent-operated config reads so adopters make the posture trade-off knowingly.

Verification

  • cargo fmt --all -- --check — clean.
  • cargo test -p codewhale-execpolicy — test result: ok. 128 passed; 0 failed (unit, including 9 new tests: inventory non-empty/deduped with spot vectors; containment boundary cases incl. /home/u vs /home/user, .. fail-closed, .-collapse and repeated-separator folds, drive-relative rejection, aligned mixed rel/abs, Windows drive AND verbatim/UNC forms, both arms of the case-fold branch pinned directly; the component-anywhere face incl. the .env and embedder-store entries; builder targets BuiltinDefault and hard-denies representative commands incl. the absolute trailing-slash and glob faces, /root/, the home-rooted file face, and auth.log; every inventory entry asserts its emitted token faces, so a generator regression (dropped ~/, lost slash, missing glob face) fails the suite; directory/component cross-list agreement; no cross-layer allowance can un-deny; the builtin denies are pinned against every approval mode; innocuous commands stay allowed), plus integration 1 passed and doc-test 1 passed.
  • cargo doc -p codewhale-execpolicy --no-deps and cargo clippy -p codewhale-execpolicy --all-targets — clean.

No-Issue: foundation-side consolidation ahead of the embedder adoption PR; the drift it removes is documented in the module docs.

Add crates/execpolicy/src/sensitive_paths.rs:

- Canonical sensitive-path inventory (directory names, credential file
  names, absolute prefixes) as the union of the inventories embedders
  otherwise hand-maintain in parallel, each entry annotated with its
  reason.
- contains(), a strict component-boundary containment helper (/home/u
  does not contain /home/user/...), the primitive the deny-rule token
  channel lacks; it reuses the crate's path normalization and fails
  closed on traversal, empty, and drive-relative paths.
- builtin_safety_ruleset(), the first producer of the previously
  unused RulesetLayer::BuiltinDefault, deriving hard-deny prefix rules
  from the inventory (exact-token faces; descendants still need the
  containment helper or corpus enumeration).

The module docs record that the generated embedder corpus collapses
once rule evaluation gains containment matching.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Stacked consumer: Pinvou/pinvou-agent#653 pins upstream/integration (= pinjou3-clean + these upstream contributions) and chops the app-side duplicates onto the new APIs. That parent PR is draft until this PR merges — its verify-public-submodule gate requires the parent gitlink to equal the public pinjou3-clean head. After merge, the pin re-points to the branch head and the chop lands.

@asto18089 asto18089 left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh from-scratch review — 7 lanes + executed probes (2026-10-06)

Rebase: head 9057261a35 sits exactly on the current pinvou3-clean tip 44921cf028 (merge-base = tip, 0 behind) — no rebase needed. Smuggle audit: clean — exactly one commit touching exactly crates/execpolicy/src/lib.rs (+1) and sensitive_paths.rs (+383); no Cargo/version/CI/docs changes; DCO signed; commit message convention-compliant.

Verdict: changes requested. The core consolidation is real — the union claim verifies entry-by-entry against the three embedder inventories, contains() is POSIX-tight, and every engine-facing claim I probed holds. But the module's own docs promise coverage its two faces do not deliver, one source-inventory face was silently dropped, a larger sensitive-path inventory inside this very repo is ignored, and the containment primitive fails open on Windows path forms. Everything below was verified by executed probes, not static reading.

Verified sound (no action needed)

  • Union claim (path identities): TRUE. All 13 directory + 26 file + 15 absolute entries trace to at least one of the three sources (pinvou3-app/src-tauri/src/platform/path_policy.rs:19-44, features/assistant/safety_deny_rules.rs:718+, the migrated deny_sensitive_paths.sh/.ps1 segments in git history); nothing invented; the live drift between the three sources is real and correctly described. All ~12 spot-checked annotations are factually accurate (gpg 2.1 key storage, Firefox key4.db, Azure MSAL/ADAL spellings, Chrome Local State, shadow aging backups, …).
  • Engine claims: confirmed in code + probes. * <path> is a token-level middle wildcard with exact whole-token tails (cat /etc/shadowrc NOT matched — no token-interior false positives found); lowercased on both sides; compound segments covered (ls && cat /etc/shadow denied); deny-always-wins (a User-layer trusted prefix AND a typed Allow rule both fail to un-deny — probed); cat /etc/shadow Forbidden under all five approval modes including Never; "first producer of RulesetLayer::BuiltinDefault" is accurate in-tree (Ruleset::builtin_default() has zero callers; nothing populated the layer).
  • contains() on POSIX: zero false negatives across ~75 adversarial probes (.. both sides, . collapse, ~ literal, //, trailing separators, mixed rel/abs, NUL, invalid UTF-8); fails closed exactly as documented; 1.3 µs/call — performance a non-issue.
  • Builder: 89 rules, sorted, deduped, documented faces in documented order; the spaced Chrome names ("Login Data") tokenize into two-token tails with no plausible two-word false positive (probed cp login data, grep login data . — clean).
  • Pure addition, no new deps, no existing behavior change.

Required changes

R1 — the /var/log/auth doc claims are false on both faces this module ships. sensitive_paths.rs:126-127 says the slash-less entry "also catches the auth.log rotation spellings on the ingest face", echoed at :145. Probed: cat /var/log/auth.log → allow=true (the emitted rule * /var/log/auth is exact-token), and contains("/var/log/auth", "/var/log/auth.log") → false (auth.log is one component; strict boundary). The primary auth-log spelling is covered by neither face. Fix the comment and either enumerate the rotation spellings as entries or drop the claim.

R2 — the ingest gate's id_* component-anywhere face is dropped, and the doc misdescribes the source. sensitive_paths.rs:75-77 claims only "the last two" (credentials.json, .env) are "sensitive as a path component ANYWHERE", but the actual ingest gate (path_policy.rs:19-32) matches id_rsa/id_ed25519/id_ecdsa/id_dsa as components anywhere — that is how ~/keys/id_rsa gets blocked today. The PR carries them only at the .ssh/id_* home spelling, so an adopter re-deriving the gate from this inventory silently loses that face. Restore the face (e.g. a component-anywhere list) or state explicitly that the inventory cannot re-derive it.

R3 — undisclosed bypass faces on the token channel; "read, destroy, copy, and upload faces alike" (:216) overclaims. All probed allow=true under every mode while opening/moving the real file: lexical same-path spellings cat /etc//shadow, cat /etc/./shadow, cat /etc/ssh/../shadow (the kernel normalizes; contains() folds these, so the token channel is strictly weaker than its sibling primitive); remote-spec embedding scp user@host:~/.ssh/id_rsa .; glued tokens dd if=/etc/shadow of=/tmp/exfil, curl -F file=@/etc/shadow …. The disclosure section covers enumeration and $HOME gaps but none of these. Minimum: disclose them; better: normalize deny targets through parse_path_for_matching before matching. At :216 scope the sentence to positional tokens (adjacent sentence is already accurate).

R4 — contains() fails open on Windows path forms, and the doc's own advice triggers it. Probed: contains("\\?\C:\Users\u", "C:\Users\u\x") = false (and the reverse); \\?\UNC\… vs \\… likewise; trailing-dot (C:\u\bar vs C:\u\bar.) and 8.3 (USER~1) spellings compare unequal — Win32 opens all of them for the same file. The doc (:179-180) tells callers to "canonicalize first", but on Windows fs::canonicalize returns \\?\-prefixed paths, so canonicalizing only the candidate side silently disables the check. Also contains_with_case(.., true) (the case-fold branch) is never executed by any test on Linux — the sole case row uses platform_paths_are_case_insensitive() as the expected value, so a fold regression ships green. Minimum: strip verbatim/UNC prefixes in the parse (or prominently document the both-sides-identical-form contract) + test the fold branch directly via contains_with_case.

R5 — the in-repo predecessor is unnamed and unmigrated; "canonical inventory" is incomplete inside this repo. crates/tui/src/sandbox/read_guard.rs:701-714 (path_is_within) already implements case-folding component-boundary containment, and default_denied_subtrees()/machine_wide_denied_subtrees() (:418-557, ~40 entries: .oci, .pgpass, .my.cnf, .npmrc, .pypirc, .cargo/credentials*, .config/gh/hosts.yml, macOS keychains, …) is a larger live inventory the module doc never mentions — after this PR, 4+ sensitive-path inventories remain live across product+foundation. The repo's own AGENTS.md is explicit: rung 2 ("name the predecessor … otherwise edit the original") and "Migrate the last consumer, or do not start" (with check-dead-code-budget.py as the receipt). Nothing consumes the new API today. Name read_guard in the module doc, state the relationship (and why its entries are not in the union — they currently cannot migrate without losing coverage), and either plan the consumer migration or say why not.

R6 — .dws/.tmeet (:67-69) are Pinvou-product entries inside a foundation const. Per the workspace fork policy (§1.1 "底座只提供通用配置入口和执行期硬约束"; §2 所有-embedder-benefit → upstream; §5 upstream scan for product traces), embedder-specific entries belong at the app boundary or as embedder-supplied input (builtin_safety_ruleset(extra: &[&str])-style parameterization) — the module doc's own logic for allowances ("keep them outside this ruleset") applies symmetrically. If they stay, the parent gitlink PR must carry the fork-modifications.md topic, fingerprints, a forkguard_* test, and a justification for exceeding the 200-line per-file soft cap (this file is 383). State which path you're taking.

R7 — make CI tell the truth. The link required check is red: the body needs a Closes #N or No-Issue: <why> line. The body's verification claim says "122 passed" — actual is 123 (base 119 + 4 new; re-run on both commits). The check/gate reds are a flake in the untouched crates/cli/tests/telemetry_kill_switch_dispatch.rs (passes locally 5/5) and its aggregate gate; Test (macos) was killed at the runner time limit mid-suite with zero test failures; everything else red is repo-level (red on the base commit too) except "Lint & Type Check", which fails on stale release data (latest-published-release.json v0.9.12 vs published v0.10.0) that any PR would trip.

R8 — pin the unpinned faces (each a 3-6 line addition to the existing tests). Mutation-verified unpinned: absolute trailing-slash face (removing * {prefix} → suite stays green; the doc's own rm -rf /etc/ssh/ example), absolute glob face (removing * {prefix}* → green; doc's rm /etc/ssh/* example), and the module's headline security claim "deny always wins across layers" has no test at module level (verified true by probe — one engine construction with a User-layer trusted prefix pins it). Also add an aligned mixed rel/abs row for contains (mutating the roots guard to permissive passes the shipped table; e.g. (".ssh", "/.ssh/id_rsa") must be false).

Non-blocking

  • set_ruleset(Ruleset::builtin_default()) after installing the safety ruleset silently evicts all 89 denies (probed) — no in-tree reachability today, but the doc advertises the layer to embedders; one doc sentence ("install via with_rulesets/add_ruleset") or merge-on-install would do.
  • Posture disclosure: kubectl --kubeconfig ~/.kube/config get pods, cat ~/.docker/config.json are hard-denied with no un-deny path (deny-first beats any allowance) — the "keep allowances outside this ruleset" sentence reads like an escape hatch that does not exist; and quoted-string mentions (echo 'cat /etc/shadow', grep "cat /etc/shadow" notes.txt) hard-deny via quote-stripping. One sentence each in the docs.
  • The three accessor fns (:153-165) verbatim re-export the already-pub consts — redundant dual API surface; the builder's struct literal (:249-254) could use ..Ruleset::builtin_default(); #[must_use] on the pure fns.
  • Latent: an inventory entry of / would emit rule * which denies every command (probed) — a debug_assert!(!base.is_empty()) makes the invariant local.
  • contains() over-blocks some odd spellings (leading/trailing-space trim, rooted-no-drive vs UNC, invalid-UTF-8 lossy collisions) — fail-closed direction, fine to leave documented.

The direction and the inventory work are worth landing — this round is about making the docs stop promising more than the two faces deliver, restoring the one dropped face, and closing/contracting the Windows fail-open. Happy to re-review on push.

Review round-1 findings on the sensitive-path module:

- Enumerate /var/log/auth.log and correct the inventory comments: the
  source ingest gate's string-prefix behavior does not carry over to
  this crate's exact-token channels, so the primary auth.log spelling
  is now covered by a real rule and higher rotations are disclosed as
  remaining enumeration.
- Carry the ingest gate's component face in full: new
  SENSITIVE_COMPONENT_NAMES (bare dirs, SSH key basenames,
  credentials.json, .env) plus has_sensitive_component(), so an
  adopter can re-derive the gate's ~/keys/id_rsa blocking that
  home-rooted spellings cannot express.
- Fold Windows verbatim forms in contains(): \\?\ prefixes strip and
  \\?\UNC\ becomes \\\\server, so a std::fs::canonicalize result and its
  plain spelling compare equal; document the limits lexical comparison
  still has (Win32 trailing dots/spaces, 8.3 short names, ASCII-only
  case folding, non-UTF-8 lossy collisions).
- Scope the module doc honestly: name the in-tree predecessor
  (read_guard's broader sandbox inventory) as not replaced, and record
  the embedder-contributed entries (.dws, .tmeet) that split out at
  upstreaming time.
- Disclose the token channel's open bypass families (kernel-normalized
  spellings, glued tokens like dd if=, remote specs like scp
  host:~/.ssh/) and scope the deny-face sentence to positional tokens;
  reword the allowance paragraph to say no cross-layer allowance can
  un-deny, with the config-read and textual-mention vocabulary named.
- Pin what was unpinned: absolute trailing-slash and glob faces, the
  auth.log rule, cross-layer un-deny, both arms of the case-fold
  branch, aligned mixed rel/abs containment, and the new component
  face. Skip an inventory entry that normalizes to no path instead of
  emitting a deny-everything rule; drop the redundant accessor fns in
  favor of the already-public consts; #[must_use] on the pure fns;
  install via with_rulesets/add_ruleset noted against set_ruleset's
  whole-layer replacement.

Signed-off-by: asto18089 <asto18089@126.com>
@asto18089

asto18089 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed as fc7ffd5, addressing all required findings from the round-1 review:

  • R1 (auth.log claims): /var/log/auth.log is now an inventory entry, so the primary rotation spelling gets a real deny rule (pinned by the deny test); both comments now say the source ingest gate's string-prefix behavior does not carry over and higher rotations (auth.log.N) remain enumeration.
  • R2 (component face): new SENSITIVE_COMPONENT_NAMES + has_sensitive_component() carry the ingest gate's component-anywhere face in full (bare dirs, id_rsa family basenames, credentials.json, .env); the file-list sentence now cross-references it instead of misstating the source.
  • R3 (undisclosed bypasses): the builder docs name the three open families — kernel-normalized spellings, glued tokens (dd if=, curl -F file=@), remote specs (scp host:…) — and the "read, destroy, copy, and upload faces alike" sentence is scoped to bare positional tokens. The allowance paragraph now says no cross-layer allowance can un-deny and names the config-read and textual-mention vocabulary.
  • R4 (Windows fail-open): contains() strips \\?\ and folds \\?\UNC\ before parsing, so canonicalize output and plain spellings compare equal (pinned by three new table rows); the remaining lexical limits (trailing dots/spaces, 8.3, ASCII-only case fold, lossy UTF-8) are documented; the case-fold branch is pinned on both arms directly.
  • R5 (predecessor): the module doc names read_guard's broader sandbox inventory as the in-tree predecessor this module does not replace, and scopes the union claim to the three embedder inventories.
  • R6 (embedder entries): .dws/.tmeet are annotated as embedder-contributed, to be split to embedder-supplied input at upstreaming time; fork registration lands with the parent gitlink PR.
  • R7: body now carries the No-Issue: line and the corrected counts.
  • R8 (pins): absolute trailing-slash face (rm -rf /etc/ssh/), absolute glob face (rm /etc/ssh/*), auth.log rule, cross-layer un-deny (trusted prefix AND typed allow both present), aligned mixed rel/abs row (.ssh vs /.ssh/id_rsa), and both case-fold arms.

Non-blocking nits also taken: redundant accessor fns dropped (consts are already pub), builder now constructs via ..Ruleset::builtin_default(), #[must_use] on the pure fns, an inventory entry that normalizes to no path is skipped (with debug_assert) instead of emitting a deny-everything rule, and the docs note set_ruleset replaces a whole layer.

Verification: cargo fmt --check clean; cargo test -p codewhale-execpolicy 125 passed / 0 failed (+2 tests), integration 1, doc-test 1; cargo clippy --all-targets and cargo doc --no-deps clean.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-2 fresh review — 7 lanes from scratch + executed probes and mutations (2026-10-06)

Rebase: base pinvou3-clean is unchanged at 44921cf028 and the head fc7ffd599 sits exactly on it (merge-base = tip) — no rebase needed. Scope/smuggle audit: clean — still exactly crates/execpolicy/src/lib.rs (+1 registration) and the new module; every hunk of both commits serves the story; the commit-1/commit-2 split matches each message; DCO and subject lengths compliant. CI: 10/10 green. Local suite: 125 unit + 1 integration + 1 doctest, green.

Overall: the substance stands after a fully independent re-derivation. The union claim verifies entry-by-entry against the three embedder inventories at parent origin/main (13/13 dirs, 26/26 files, 12/12 components — 0 missing, 0 extra, 0 face-mismatched on the POSIX faces); the containment primitives are bug-free across every edge I could construct (all documented residuals reproduce); deny-always-wins, the * <path> middle-wildcard any-token match, set_ruleset eviction, and every load-bearing engine claim check out against the matcher code and probes; builtin_safety_ruleset() = exactly 90 rules (10 slash-less absolutes + 5 slash-terminated ×3 + 13 dirs ×3 + 26 files, dedup a no-op); performance is fine at this scale (34 µs per check with 90 rules; 3.5 µs to build). Zero new over-blocks: every usability cost (stat/test -f, textual mentions, kubectl --kubeconfig, key-rotation vocabulary) is pre-confessed in the docs. Duplicating containment next to read_guard::path_is_within is justified — the two have different semantics (lexical vs canonicalizing) and the predecessor is named, not hidden.

What remains is disclosure completeness and pins — for a module whose documentation is the safety deliverable, an enumeration presented as complete must be complete, and a face with no pin does not exist as far as CI can tell.

Required

R1 — the "three bypass families" enumeration (sensitive_paths.rs:321-329) is incomplete; four more families are equally open and probed. All allowed under AskForApproval::Never while the real file is read: variable indirection (x=/etc/shadow; cat $x, env VAR=/etc/shadow sh -c 'cat $VAR' — the expander deliberately leaves $VAR literal, shell_expand.rs:23-24); command-side glob expansion (cat /etc/shado[w], bash -c 'cat /etc/shado[w]' — a real shell glob-resolves before exec); cwd-relative composition (cd /etc && cat shadow, tar -C /etc -cf x.tar shadow — no token ever matches an inventory spelling); and aliased/path-free spellings (cat ~root/.ssh/id_rsa defeats the /root/ face whose own comment says root's whole home is privileged ground; getent shadow prints the very hashes with zero path tokens). Also worth naming under the existing glued family: redirect glue (done </etc/shadow) and interpreter payload (python3 -c "open('/etc/shadow').read()"). Fix: extend the paragraph (and its root cause: the expander models quoting/substitution but not variable, glob, tilde-user, or cwd resolution), and state the getent-class boundary explicitly.

R2 — three faces have no pin at any level (mutation-verified, re-confirmed independently). (a) The home-rooted FILE face: mutating the generator to format!("* ~{file}") breaks all 26 file rules — cat ~/.netrc, cp k ~/.ssh/authorized_keys would run — and the suite stays green; no ~/<file> command appears in any test. (b) The .env/credentials.json component face: removing .env from SENSITIVE_COMPONENT_NAMES is invisible (the component test covers id_rsa/.ssh/credentials.json spellings but never .env; the inventory spot-check asserts .env only in SENSITIVE_FILE_NAMES). (c) /root/ and 10 of 15 absolute prefixes have no entry-level pin at all — only /etc/shadow, /proc/, /var/log/auth.log are spot-checked. Fix: per-entry emitted-rule membership assertions (catches generator mutations for every entry in one loop) plus engine-level deny cases for a file face and the /root/ faces (rm -rf /root/, rm /root/*).

R3 — the union claim overclaims in two places. (a) Windows DPAPI credential dirs are carried by source inventory (b) but by none of the four consts in any face: WIN_MS_CREDENTIAL_DIRS (parent origin/main safety_deny_rules.rs:940-951: %appdata%\microsoft\credentials/protect, %localappdata%\…\credentials/protect/vault, plus PowerShell spellings) — so "the union of those lists lives here once" is false for five credential subtrees. Fix: register the exclusion explicitly (the way read_guard.rs is registered), or carry them. (b) .dws/.tmeet lost the hook's component-anywhere face (the hook matched /.dws/ anywhere; the PR carries them only as ~/ tokens) while SENSITIVE_DIRECTORY_NAMES' doc (lines 53-55) says the single-component entries "also form the directory half of SENSITIVE_COMPONENT_NAMES" — false for these two of the eight. Fix: add both to SENSITIVE_COMPONENT_NAMES (restores the source face), or correct the sentence and register the reduction.

R4 — false citation in the code: "per the fork's upstream-hygiene scan" (line 81) — no such mechanism, script, or doc section exists in either repo. Name the real anchor (parent docs/fork-policy.md upstream-decision table) or drop the proper-noun phrasing; same for the "embedder-supplied input" forward reference, which promises a parameterization that doesn't exist yet.

Non-blocking (no action demanded, listed for the record)

  • Case-fold asymmetry between channels is disclosed only in a test comment: the deny face folds ASCII case unconditionally on every platform (so rm -rf ~/.SSH is hard-denied even on ext4 where .SSH is a different, non-sensitive directory), while contains/has_sensitive_component fold only on Windows/macOS. One sentence in the builder doc would do.
  • SENSITIVE_ABSOLUTE_PREFIXES' const docs read as if the subtree is protected on the ruleset channel; it is not (cat /proc/self/environ, cat /root/.ssh/id_rsa allowed — descendants need contains() or the corpus). Cross-reference the containment gap from the const docs.
  • strip_verbatim_prefix's UNC\ strip is case-sensitive, so \\?\unc\… (which Windows folds) is not folded — unreachable from fs::canonicalize output, and the doc's canonicalize-both-sides advice covers it, but the doc says "verbatim forms are folded" without the qualifier.
  • Undocumented fold collision: \\server\share parses identically to /server/share (root /), so contains("/server/share", r"\\server\share\x") is true on every platform. Over-block direction; one doc sentence.
  • "fails closed" (lines 215-216, test comment 490) inverts the direction for a deny-side predicate — the primitive refuses to answer; the caller must fail closed. has_sensitive_component's doc (277-279) already words it right.
  • Pins that would be nice but aren't load-bearing: approval-mode sweep (deny is pinned only under Never; structurally mode-independent per lib.rs's early Forbidden return); contains doc behaviors without cases (.-collapse, repeated separators, drive-relative rejection, U+FFFD collision, the .-dir over-block); the tautological case row (expectation computed from platform_paths_are_case_insensitive() itself — mitigated by the two direct contains_with_case arms); cross-list invariants ("directory half of…", "last two are also members") are doc-only — that seam is exactly how R2(b) slipped.
  • Engine-side, pre-existing and out of scope here, but now quantified: denied_prefix_matches re-tokenizes rule and command per (rule, target) pair, ~380 ns/rule linear — trivial at 90 rules, ~3.3 ms per check if an embedder still installs an 8.7k generated corpus. Worth a follow-up engine PR (hoist command tokenization; tokenize rules at install); the module doc already frames corpus collapse into containment matching as the real fix.

Parent-side follow-ups (required before codewhale-hq#653 finalizes; not defects of this PR)

  • codewhale-hq#653's forkguard_deny_inventory_anchored_to_engine_sensitive_paths iterates all engine SENSITIVE_ABSOLUTE_PREFIXES with deliberate_absence = ("/etc/ssh/" | "/root/" | "/var/log/auth" | "/proc/" | "/sys/") — the round-1-added /var/log/auth.log is in neither arm, so that test fails against this head. After merge, codewhale-hq#653 rebases and adds the entry (or enumerates it).
  • codewhale-hq#653's app_only_disjoint_from_engine checks only SENSITIVE_DIRECTORY_NAMES/SENSITIVE_FILE_NAMES, so its "bare id_* have no engine counterpart" premise silently rotted when round-1 added SENSITIVE_COMPONENT_NAMES (item-for-item identical to the app's BLOCKED_COMPONENTS). The chop should migrate to component-face anchoring.

Fix wave for R1-R4 follows in this branch.

Round-2 review fixes. The token-channel disclosure now names every
bypass family the raw matcher leaves open - variable indirection
(x=/etc/shadow; cat $x), command-side glob expansion (cat /etc/shado[w]),
cwd-relative composition (cd /etc && cat shadow), aliased home spellings
(~root), redirect and interpreter glue - scopes path-free command-native
reads (getent shadow) out of a path inventory, and states the deny
face's unconditional ASCII case fold. The source ruleset's Windows DPAPI
credential dirs are registered as staying in the embedder's Windows-native
list, and the absolute-const subtree promise is scoped to the containment
channel.

Pins: every inventory entry asserts its emitted token faces, the
engine-level deny list covers the home-rooted file face and the /root/
slash and glob faces, the component face covers .env and the embedder
stores (restoring the hook's component-anywhere face for .dws/.tmeet),
the builtin denies are pinned against every approval mode, and the
directory/component cross-list invariant is pinned in both directions -
which surfaced .azure as ruleset-face-only (the ingest gate's component
blocklist does not name it); documented as the one exception. The stale
"upstream-hygiene scan" citation is replaced with the real fork-policy
anchor.

Verification: cargo test -p codewhale-execpolicy 128 passed / 0 failed
(+3 tests); integration 1, doc-test 1; cargo fmt --check, clippy
--all-targets and cargo doc --no-deps clean; the previously unpinned
generator mutations (dropped ~/, lost slash, removed component entry,
removed /root/) now all fail the suite.

No-Issue: foundation-side consolidation ahead of the embedder adoption
PR; the drift it removes is documented in the module docs.

Signed-off-by: asto18089 <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed as 5fe1117, addressing all four required findings from the round-2 review:

  • R1 (bypass enumeration): the builder doc now names every family the raw token matcher leaves open — variable indirection (x=/etc/shadow; cat $x), command-side glob expansion (cat /etc/shado[w]), cwd-relative composition (cd /etc && cat shadow), aliased home spellings (cat ~root/.bashrc), plus redirect glue (done </etc/shadow) and interpreter payloads under the glued family — with the root cause (the expander models quoting/substitution but not variable, glob, tilde-user, or cwd resolution) and getent-class path-free reads scoped out explicitly.
  • R2 (pins): new builtin_ruleset_enumerates_every_inventory_entry asserts each entry's emitted faces (mutation-verified: the dropped-~/, lost-slash, and removed-/root/ generator mutations that were green before now fail); engine-level denies added for the home-rooted file face (cat ~/.netrc, cat ~/.env) and the /root/ slash+glob faces; the component face pins .env and the embedder stores; a new test pins the builtin denies against all five approval modes.
  • R3 (union honesty): the Windows DPAPI credential dirs are registered as staying in the embedder's Windows-native list; .dws/.tmeet are added to SENSITIVE_COMPONENT_NAMES (restoring the hook's component-anywhere face), and the directory-const doc now states the real invariant — with .azure as the one ruleset-face-only exception. That last point is a bonus catch from the new cross-list test: .azure was a third single-component directory absent from the component face, and unlike .dws/.tmeet the ingest gate's blocklist genuinely does not name it, so it is documented rather than added (broadening the component face beyond the source union would silently change adopter ingest-gate behavior — that's a deliberate follow-up decision, not a review fix).
  • R4 (citation): the nonexistent "upstream-hygiene scan" reference is replaced with the fork policy's upstream-decision table; the "embedder-supplied input" forward reference now says what it means (a builder parameter that does not exist yet).

Non-blocking items also taken: the unconditional ASCII case fold of the deny face is now stated in the module docs; the absolute-prefix const doc scopes subtree protection to the containment channel (/proc/self/environ named); the contains case table pins .-collapse, repeated-separator folds, the .-dir over-block edge, and drive-relative rejection.

Verification: cargo fmt --check, cargo clippy --all-targets, cargo doc --no-deps clean; cargo test -p codewhale-execpolicy 128 passed / 0 failed (+3 tests), integration 1, doc-test 1.

For the record, the parent-side follow-ups from the round-2 review remain tracked on codewhale-hq#653: its forkguard_deny_inventory_anchored_to_engine_sensitive_paths needs /var/log/auth.log in its residual registration, and its app_only_disjoint_from_engine assert should migrate to component-face anchoring — both land when codewhale-hq#653 rebases onto the merged head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant