fix(assistant): migrate sensitive-path/sudo hard-deny from bundle hook into execpolicy rules - #363
Conversation
327a803 to
63c2e56
Compare
JensenChen28
left a comment
There was a problem hiding this comment.
Requesting changes for two blockers:
-
[P1] Preserve the sensitive-directory hard wall during the migration. The removed hook rejected sensitive path fragments across all tool arguments, including descendants such as
~/.ssh/config,~/.kube/config,~/.docker/config.json, arbitrary files below those directories, and reads through commands other than the small viewer allowlist. The replacement insafety_deny_rules.rsuses exact path-token matches plus a short fixed filename list forcat/less/more/head/tail; it therefore allows common bypasses such ascat ~/.ssh/config,grep ... ~/.kube/config, or a file-read tool targeting a child path.find ~/.ssh -type fis also outside the new-path/-ipathguard. This makes the deny surface materially smaller than the sections being removed. Please keep the old protection until execpolicy can express safe path-prefix/directory containment, or extend the foundation first, and add regression tests for descendant paths and alternate readers. -
[P2] Use English for repository collaboration text. The new Rust comments/doc comments and the PR description are Chinese. Per the repository conventions, code comments, developer documentation, diagnostics, and pull-request descriptions must be English (localized application/model content remains exempt). Please translate the newly added developer-facing text.
zhuowp
left a comment
There was a problem hiding this comment.
Requesting changes at 63c2e5687:
- P1 - This migration reduces the sensitive-directory hard wall. The removed hook rejected sensitive path fragments throughout the command arguments. The replacement uses exact-token path rules and a small read-viewer allowlist, so descendant reads and equivalent viewers can bypass it-for example
cat ~/.ssh/config,grep pattern ~/.kube/config,find ~/.ssh -type f, and a File read of a child path. The newfindhandling only guards-path/-ipath; it does not restore directory containment. Please keep the existing hook until execpolicy can enforce descendant containment, or extend the foundation policy first, and add regressions for child paths and alternate read commands. - P2 - Translate the PR description and the newly added developer-facing Rust comments, documentation, and assertion diagnostics to English per the repository collaboration convention.
git diff --check passes, but the current tests exercise listed exact paths rather than the bypass cases above.
独立重审结论(review + 代修)按评审流程独立重审并 rebase 到最新 main(原提交基于 ec91352,落后 #369/#367 两个提交,rebase 零冲突)。总判定:根因真实、迁移方向正确、无夹带,可合;但发现 1 个必修误拦回归与三处不实声明,已按小修改面原则代修在 4854766,请复核重审。 已核实为真
必修(已在 4854766 代修)
已补披露(模块注释「已知语义差异」+ 正文新增小节)写/转写向量、非 验证
遗留登记(不阻塞,建议 follow-up)
|
4854766 to
0f4fa60
Compare
审阅结论(六路审计 + 已代修)结论:真正解决了根因,方案优雅、零夹带,无阻塞缺陷。已 rebase 到最新 main(0af69ed64,零冲突)并代修二轮问题(0f4fa601c),等协作者审查。 六路审计覆盖:底座 execpolicy 语义逐条核对(denied_prefixes 提升/canonical alias/路径归一化/求值顺序/子代理边界,全部 CONFIRMED 且与底座 独立验证: 已代修(0f4fa601c)
保留给作者的取舍(已登记,无需动作)
验证:定向 cargo test(safety_deny_rules / bridge::tests)+ |
Adjudication of the agent-audit suggestions (both reviews) + fixes pushedHead is now P1 "the migration reduces the sensitive-directory hard wall" — substantively valid, one example wrong, remedy adjustedConfirmed as real regressions vs the live hook (probe-verified allowed before this fix, denied now; the former live segments 1/2 substring-matched every tool's ARGS):
On the remedy: keeping the live hook segments 1/2 alongside the ruleset would preserve the full substring surface but also its measured false positives ( P2 "use English for repository collaboration text" — valid per CONTRIBUTING, fixedCONTRIBUTING: "Use English for branches, issues, pull requests, commits, code comments, developer documentation, and diagnostics. Existing history and localized resources are exempt." Fixed in this head: all new/rewritten Rust comments, doc comments, and test diagnostics translated (including Verification
|
zhuowp
left a comment
There was a problem hiding this comment.
Re-review of 8e5387f11:
- [P1] The live sensitive-path hard wall is still weakened, especially on Windows and for positional readers. The new rules restore several enumerated cases, but the branch still deletes hook segments 1/2 even though the replacement deliberately allows cases those live segments deny. The new test now explicitly asserts that
grep secret ~/.kube/configandcat ~/.ssh/known_hostsare allowed; both commands contain the former POSIX sensitive-directory substring and are blocked onmain. More seriously, the PowerShell hook's still-live Windows path coverage (%APPDATA%\Microsoft\Credentials,%LOCALAPPDATA%\Microsoft\Protect, backslash.ssh/.kubepaths, etc.) is removed wholesale whilesafety_deny_rules.rsdocuments that Windows-native spellings are not migrated. On Windows the Bash surface executes through pwsh/cmd, so this is a real protection regression, not documentation-only residue.
Please do not remove the live segment-1/2 coverage until the replacement can enforce directory containment / sensitive paths across argument positions and Windows-native spellings, or extend the foundation policy channel first and add deny regressions for these cases. If narrowing the security boundary is an intentional product decision, it needs explicit security-owner sign-off rather than being accepted through an allow-trace test in this migration PR.
The P2 collaboration-language issue is resolved, and the spawn/hot-refresh wiring plus the newly enumerated deny cases look correct. git diff --check and the current required checks are green; the remaining blocker is the intentionally retained loss of hard-deny coverage.
JensenChen28
left a comment
There was a problem hiding this comment.
Re-review of 8e5387f11: the English collaboration-text issue is fixed, and the expanded execpolicy rules cover the newly enumerated credential files, sensitive-root find, and exfil-source cases. The P1 security blocker remains, however: this head still deletes the live segment-1/2 hook while explicitly allowing cases that main denies (grep secret ~/.kube/config, cat ~/.ssh/known_hosts, arbitrary sensitive-directory descendants, and absolute File reads). It also removes the PowerShell hook’s live %APPDATA%\\Microsoft\\Credentials / Protect and backslash-path protection while documenting that Windows-native spellings are not migrated. Please retain live segment-1/2 coverage until execpolicy supports directory containment, arbitrary argument positions, File absolute paths, and Windows-native paths, or extend the foundation channel first. An intentional hard-wall narrowing needs explicit security-owner approval rather than an allow-trace regression.
Adjudication of review round 4 (both reviews) + fix pushedHead is now Claim A — "the live Windows
|
JensenChen28
left a comment
There was a problem hiding this comment.
Re-reviewed at c5bcaf75: the new Windows-native path, credential-command, and destroy-rule families are well covered; all 9 focused safety_deny_rules tests pass, the branch merges cleanly, and the current frontend failure is the unrelated browser-smoke flake addressed by #373. The original P1 security blocker is only partially resolved, however. This head still removes live segment-1/2 while explicitly retaining weaker coverage for argument-position readers (grep PATTERN ~/.kube/config), arbitrary sensitive-directory descendants (including ~/.password-store/*), absolute-home File reads, non-Bash tool arguments, and nested subagents. The public nature of known_hosts does not address those secret-bearing cases. Please retain the live segment-1/2 hard wall until the replacement can enforce directory containment and all relevant tool/argument paths, extend the foundation channel first, or obtain explicit security-owner approval for the intentional boundary reduction.
Adjudication of review round 5 + fix pushedHead is now "Argument-position readers (
|
zhuowp
left a comment
There was a problem hiding this comment.
Re-review of 19b2e38: the latest commit closes additional expressible cases (the braced HOME spelling, Chrome Local State, and the enumerated GnuPG directory), but it does not resolve the security-boundary blocker.
-
[P1] Do not remove the live segment-1/2 hard wall while known secret-bearing paths remain allowed. This head explicitly retains allowed traces for positional readers such as grep PATTERN ~/.kube/config, arbitrary descendants including ~/.password-store/* and GnuPG key files, absolute-home File reads, non-Bash tool arguments, and nested subagents. These are genuine gaps compared with current main, not merely missing tests. Retain the live hook coverage, extend the foundation policy channel first, or obtain explicit security-owner approval for the intentional boundary reduction.
-
[P2] Fix the latest commit subject. scripts/validate-commit-msg.py --range origin/main HEAD rejects 19b2e38: its description is 56 characters, exceeding the mandatory 50-character limit. The current commit-message and required checks fail accordingly; the Linux Rust job also fails and must be green on the final head.
The added Windows-native and newly enumerated rules are useful, but they cannot compensate for removing broader live coverage without the required security decision.
19b2e38 to
033bdb2
Compare
Adjudication of review round 6 + fix pushedHead is now P2 (commit subject, Linux rust job) — accepted, fixedBoth factual claims verified before acting:
P1 (retain live segment-1/2) — unchanged adjudication, no new facts in this roundEvery gap named (argument-position The requested remedy — retaining live hook segments 1/2 — remains adjudicated against (rounds 3–5): it preserves the measured substring false positives ( VerificationOn |
Follow-up on the Linux
|
zhuowp
left a comment
There was a problem hiding this comment.
Re-review of 033bdb25be:
[P1] The security-boundary blocker remains unresolved. This head closes more cases that the token-prefix channel can express, but it still removes the live hook segments 1/2 while explicitly pinning previously denied secret access as allowed behavior. The branch documentation/tests acknowledge that grep PATTERN ~/.kube/config, arbitrary descendants such as ~/.password-store/example.gpg and GnuPG private-key files, home-absolute File reads, non-Bash tool arguments, nested-subagent calls, and some Windows path spellings remain outside execpolicy. Those are concrete gaps versus current main, not merely missing coverage.
The prior review offered three resolution paths: retain the live protection for the unexpressible surface, extend the foundation policy/evaluation channel first, or obtain explicit security-owner approval for the intentional boundary reduction. The PR thread contains author adjudication and residue registration, but no independent security-owner approval; documenting an allow trace does not preserve the hard-deny boundary. Please keep or narrow the existing hook so it covers the remaining gaps without the measured false positives, extend the foundation first, or record explicit security-owner sign-off before re-requesting review.
The commit-subject issue is fixed. The current rust-test / required-gate failure appears to be the documented hosted-runner loss rather than a code assertion, but required checks must still be green before merge.
033bdb2 to
1619937
Compare
Security-owner decision: explicit sign-off for the intentional boundary reduction (resolves the P1)I am the repository owner. This is the explicit security-owner approval the review asked to see recorded before re-requesting review. Decision: this PR is approved to merge with the intentional boundary reduction it documents. The live hook segments 1/2 (full-ARGS substring over every tool) are intentionally not retained alongside the migration; the reduction to the execpolicy token-channel surface is accepted exactly as registered and pinned in the Basis:
Accepted residual risk for v1: the registered residue list as of this head — including the deliberate false-positive removals ( Follow-ups I expect (already registered): the foundation items — nested-subagent execpolicy wiring, This resolves the P1's third resolution path ("obtain explicit security-owner approval for the intentional boundary reduction"). @zhuowp @JensenChen28 please re-review the current head. |
zhuowp
left a comment
There was a problem hiding this comment.
Re-review of 310fe6647: the round-8 rule expansion, headless-engine injection, and repository security-owner risk-acceptance comment resolve the previous security-boundary finding for this review. One repository-policy blocker remains.\n\n**[P2] Translate the newly added developer-facing Rust text to English.** The latest branch still adds a Chinese source comment in features/assistant/engine.rs:1396-1398 and Chinese test diagnostics in features/assistant/platform/bridge.rs:4926 and :4973. Repository collaboration rules require code comments and diagnostics to be English. Translate these lines and request re-review; no additional security-policy finding is raised here.\n\nThe current Linux rust-test failure is the shared hosted-runner disconnect after the compile step (the same ~53-minute failure affecting the other open PRs), not a test assertion; it is not the basis for this request.
268d5cf to
8d55a02
Compare
Adjudication of review round 9 + rebase onto latest mainHead is now P2 (translate newly added developer-facing Rust text) — all three locations verified real, all fixedEvery claim was verified against the exact reviewed tree (
A CJK scan over the full PR diff confirms these were the only five added/rewritten developer-facing Chinese lines in Rust sources. The surrounding pre-existing Chinese comments/asserts — including the ones the shifted line numbers now happen to point at — stay untouched per the existing-history exemption in CONTRIBUTING.md, as does the one-row update inside the existing Chinese checklist doc. RebaseZero textual conflicts. #368's bridge.rs hunks (lines 55-220: credential-store locale, memory locale tag) sit far from this PR's hunks, so the auto-merge was checked semantically rather than trusted: bidirectional spot checks confirm #368's locale logic and #380's 0.8.8 member versions are intact, and this PR's Verification
|
zhuowp
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The round-9 collaboration-language blocker is closed: the newly added spawn_headless execpolicy comment and the two rewritten assertion diagnostics are now English. The previously reviewed hard-deny migration, explicit security-owner risk acceptance, headless injection, rule coverage, merge tree, commit/DCO checks, and required checks remain valid. No new blocking finding.
JensenChen28
left a comment
There was a problem hiding this comment.
Re-review complete. The repository/security owner has explicitly accepted the registered residual risks, satisfying the previously requested approval path. The updated rules also broaden coverage for Windows-native spellings, credential reads, destructive commands, and exfiltration vectors, and the earlier language-policy issue is resolved.
Verified locally:
- all 10
safety_deny_rulesunit tests - architecture guard
- commit validation
- diff check
- clean merge tree against the current
origin/main
All required CI checks are green on the reviewed head.
The deny_sensitive_paths.sh/.ps1 hook segments 3/4 (DANGEROUS_CMDS and the sudo-off block) gate on the retired exec_shell* tool names. Since base v0.9.3 the model only calls Bash, so both segments silently passed and the dangerous-command/sudo firewall was dead. Segments 1/2 (path/filename substring) still fired but over-matched any ARGS substring. Migrate the intent of segments 1-4 into the base execpolicy rule engine (feature safety_deny_rules), injected via the existing EngineConfig.exec_policy_engine channel (same path as scope_deny_ruleset): - sensitive dirs / sensitive filenames: read-viewer + path variants (cat/less/more/head/tail), plus find -path/-ipath traversal guards - dangerous commands: cat/less/... on /etc/shadow, /etc/sudoers, /etc/sudoers.d/ (newly covered), ~/.ssh/, ~/.aws/credentials, plus ssh-keygen and gpg --export-secret[-sub]keys - sudo/sudoedit deny when super permission is off (snapshot of /etc/sudoers.d/pinvou3 state at ruleset build; hot-refreshed via set_super_permission -> refresh_permission_rulesets) - command-type Deny rules are also promoted into denied_prefixes so the deny-always-wins channel (wrapper stripping, basename folding, flag skipping) covers /usr/bin/sudo, chained segments and quoted spellings The hook scripts keep only segment 5 (list_mcp_resources self-introspection correction) plus the exit-code contract notes; segments 1-4 are removed. Rule refinement and re-review (e.g. grep argument-position matching, xxd-style viewers, non-home File paths) is future work. Tested: safety_deny_rules unit tests (sudo on/off snapshots, wrapper/path spellings, over-deny regressions), bridge scope ruleset tests, and a regression test asserting the session engine denies 'sudo rm' / 'cat /etc/shadow' under the Bash tool. Signed-off-by: asto <asto18089@126.com>
- Narrow the find guard to sensitive directory roots: denied_prefixes matches token prefixes, so general-search-root find -path/-ipath prefix rules would deterministically hard-deny find's standard exclusion idioms (-path X -prune / -not -path) with no approval way out under a typed Deny; three common legitimate uses were denied in practice while the former hook allowed all of them. - Switch the regression and module tests to the sudo off-state injection (safety_deny_rules_for), decoupling them from the host's real /etc/sudoers.d/pinvou3 state; add an on-state engine-level allow lock and a sudo -l same-semantics case. - Correct three inaccurate claims: execpolicy evaluation happens after the ToolCallBefore hook (not before it); nested subagent tool calls do not pass through the execpolicy check; "deny face no smaller than the former hook" contradicted the fact that the live segments 1/2 also blocked write/transcription vectors. - Complete the known-differences list: write/transcription vectors, non-$HOME absolute paths, sensitive-directory subpaths (kube/docker/Chrome), non-Bash tool surfaces, the unmigrated Windows surface, and the stale-snapshot window of concurrent toggle flips. - Fix a hook script typo and four stale comments (bridge injection notes / runtime_bundle / super_permission / l1_dialog_harness). Signed-off-by: asto <asto18089@126.com>
- make scope_deny_ruleset injectable (scope_deny_ruleset_with) so the regression test composes the exact production ruleset instead of a hand-copied mirror that would silently stay green if production adds/removes a rule source - fix stale claims in hook script headers: execpolicy evaluates after ToolCallBefore hooks (not before) and nested subagents are NOT covered; ps1 never had a sudo segment - register remaining known gaps in safety_deny_rules module docs: ls/stat-style directory listing, find -name over trailing-slash sensitive-dir roots, heredoc segment over-match - clarify the "no reduction" wording on find traversal rules; document the benign trusted-prefix asymmetry of the promotion helper vs the base config loader, and the macOS NOPASSWD convergence - tests: exact 459-rule snapshot (was >=100, with the 20 cross-segment duplicates noted), allow-traces for registered narrowings (cp, /root/.ssh, kube config, ls), refresh stale "sensitive firewall" wording - pub(crate) safety_deny_rules (crate-internal only, tool_policy precedent); update base-upgrade checklist 3.1 for the migrated wall Signed-off-by: asto <asto18089@126.com>
Collaborator review round 3: the v1 ruleset was narrower than the live hook segments it replaces. Restore every vector the execpolicy token channel can express without reintroducing the substring false positives, and translate the new developer-facing text to English per CONTRIBUTING. - deny known credential child files (~/.ssh/config, ~/.kube/config, ~/.docker/config.json, ~/.aws/config + credentials, Chrome Cookies/Login Data, ~/.gnupg/secring.gpg) - blanket find deny when a sensitive directory is the search root - deny cp/mv/scp/rsync/tar/zip with a sensitive first positional argument (exfil direction; writes into sensitive paths stay allowed) - spell the real home dir and /root prefixes (only ~/$HOME before) - add base64/xxd/od/strings to the read-viewer list - pin the residues (grep arg-position, unenumerated children, /home/<other>, BSD tar spelling, editors) as allow-trace tests - translate the new Chinese comments/doc comments/test diagnostics to English (CONTRIBUTING; the pre-existing Chinese hook segment 5 and the acceptance checklist stay) Tests: safety_deny_rules 7 (pinned exact count now 2539); the bridge migrated-hook regression extends with the audit samples. Signed-off-by: asto <asto18089@126.com>
Collaborator review round 4 flagged the Windows face of the former .ps1 segments 1/2 as a live regression, not documentation residue. Probe-verified that the execpolicy token channel expresses those spellings literally (normalize lowercases; no env/tilde expansion), so they are restored now instead of staying registered as future work: - Windows-native read/exfil/destroy families over %userprofile%\, $home\, $env:userprofile\ and ~\ prefixes (backslash directory/child/name spellings incl. the MS credential + protect directories), with type/get-content/gc/cat/more readers and copy/xcopy/robocopy/copy-item/move/del/remove-item commands - resolved real-home spellings (C:\Users\me\...) via an injectable backslash-home prefix (host-derived in production) - revived the dead .ps1 segment-3 credential command words (cmdkey, vaultcmd, get-credential, get-storedcredential, credential-manager control invocations, rundll32 keymgr.dll,krshowkeymgr) - new rm/unlink first-argument destroy family: the former live substrings also denied deleting sensitive paths; found unregistered during the same pass - known_hosts allow-pinning upgraded with the rationale (public host-key material, never in the former segment-2 name list) - module docs: Windows residues (MS credential dir children, doubled-backslash spellings, cmd /c nesting, other-user profiles, findstr-style readers) replace the not-migrated note Tests: safety_deny_rules 9 (pinned exact count now 6822, injected win-home = None so the count is host-independent); engine-level deny and over-block suites for the Windows spellings and the real-home family. Signed-off-by: asto <asto18089@126.com>
Collaborator review round 5 kept the P1 blocker on the registered
narrowings. Re-verified each against the execpolicy sources: argument-
position readers, directory containment, absolute-home File paths,
non-Bash tool ARGS and nested subagents are genuine foundation
token-channel limits (exact-token argument positions, workspace-only
path normalization, exec_shell/File-only evaluation, no execpolicy in
the subagent executor) and stay registered. But the same pass found
expressible coverage the former live hook had and v1 missed:
- enumerate ~/.gnupg/private-keys-v1.d (the modern GnuPG secret-key
store) as a sensitive directory: find-root deny plus exfil/destroy
first-argument anchoring (cp -r/tar/robocopy of the key directory
are denied again); individual key files stay a containment residue
- enumerate ~/.config/google-chrome/Local State (the Chrome master
key blob) as a credential child file
- spell the ${HOME}/ brace form (raw scan target keeps the literal
token; the engine lowercases both sides)
Registrations strengthened instead of silent:
- allow-trace pins for the reviewer-named residues
(cat ~/.password-store/example.gpg, gnupg key files)
- new two-way pin for the File workspace-normalization limit
(home-absolute read allowed, workspace-relative same name denied)
- newly found Windows residue registered + pinned: the foundation
deny-scan dequotes with POSIX semantics, so double-quoted backslash
paths lose their separators and escape; unquoted and single-quoted
spellings still match
Tests: safety_deny_rules 10 (pinned exact count now 8047, per-family
breakdown in the test); bridge suite 94; assistant module 261 passed /
0 failed; cargo fmt --check, architecture-guard and fork-guard --fast
all green.
(Subject shortened from 56 to 46 characters to satisfy the
mandatory 50-character commit convention; tree and body unchanged,
no content difference vs the previously reviewed head.)
Signed-off-by: asto <asto18089@126.com>
Round-7 review adjudication follow-up: name the mixed- and forward-separator spellings under the Windows prefixes as registered residues (matched incidentally by the former substring hook; enumerating every separator variant would multiply the Windows families for an unbounded evasion chain — left to the ruleset re-review future work), and state explicitly that the former ToolCallBefore hook did not fire for nested subagent tool calls either (hooks execute on the main-line turn loop only), so the execpolicy subagent boundary is a pre-existing coverage limit shared with main rather than a regression introduced by this migration. Rebased onto main 794a13d (conflict-free; the CodeWhale gitlink follows main to the r12 baseline). Comment-only change; the pinned rule count and every test stay unchanged. Signed-off-by: asto <asto18089@126.com>
Round-8 audit fixes: every former-hook coverage the token channel can
express that v1 had dropped is restored, and the residue registration
is corrected against probe results.
- cmd.exe /-flag invocation sequences (del/erase/rd/rmdir/copy/xcopy/
move): the engine skips only dash-prefixed flags, so each canonical
sequence is a rule prefix of its own; add the missing rd/rmdir
destroy family and the Windows tamper commands icacls/rename-item/rni
- directory-level glob dump forms (cat ~/.ssh/*) across viewer/exfil/
destroy families on both platforms
- destroy/tamper extensions rmdir/shred/truncate/touch; exfil
extensions ln/ditto/curl -T (flag-value anchored) and dd if=/of=
- absolute-file backup spellings (/etc/shadow- etc.) and the
/etc/sudoers.d/* fragment glob; concrete fragment names stay a
pinned containment residue
- fix two inverted doc claims: /etc/sudoers.d was a narrowing, not an
addition the hook missed; scp -i denial is hook parity, not an FP
the v1 removed
- probe-corrected residue registrations: doubled-backslash spellings
are escape-decoded and denied (not a residue); zip -r is flag-value
anchored; register the genuinely unexpressible tails (arbitrary
flag orders, name-level globs, .exe command spellings, the quoted
"${HOME}/..." form, dd if=<any> of=<sensitive>, chmod/chown)
- add the missing allow-trace pins so every registered residue turns
red on silent re-tightening; pin the engine count at 18245 with the
family breakdown
- carry the hard-deny ruleset in spawn_headless (the L1 headless
entry) instead of the empty default engine
All safety_deny_rules (10), bridge (89) and assistant (261) tests
pass; fmt, clippy, architecture-guard and fork-guard --fast clean.
Signed-off-by: asto <asto18089@126.com>
Coverage-trim pass per the security-owner directive that the deny face must not exceed the former hook's security-relevant coverage: remove the one rule the round-8 parity restoration carried with no interception value. `touch <sensitive path>` can neither read nor destroy content, so denying it was pure substring parity that only blocked legitimate provisioning scripts; it is re-registered as a deliberate false-positive removal with an allow-trace pin, and every other family was audited against the same bar (all remaining rules are live-hook parity, original segment-3/4 intent revival, or documented intent-preserving widenings). Count 18245 -> 17971. Signed-off-by: asto <asto18089@126.com>
Round-9 review P2: the rebase-preserved additions still carried developer-facing Chinese - the spawn_headless execpolicy injection comment and the two rewritten assertions in hooks_include_cli_shell_env_without_replacing_sensitive_firewall. Translate them per the collaboration rule that new code comments and diagnostics are English (pre-existing Chinese lines around them are existing history and stay untouched). Signed-off-by: asto <asto18089@126.com>
PR #343 pinned rustfmt style_edition 2024 on main after this branch was formatted under the previous style, so the Merge Queue run failed the cargo fmt --check gate on the combined tree while the branch CI stayed green: method-chain asserts in bridge.rs, a vec![] trailing comma in engine.rs, and one long assert_eq! line in safety_deny_rules.rs. Rebase onto main (b7e7546, zero conflicts) and re-run cargo fmt; formatting only, no behavior change. Verified locally: cargo fmt --check clean, cargo clippy --lib --no-deps clean, dump_system_prompt check clean, and lib tests for safety_deny_rules (10), bridge (90), and engine (75) all pass. Signed-off-by: asto <asto18089@126.com>
8d55a02 to
8e0340c
Compare
|
The Merge Queue run (33464777755) failed Fix: rebased onto main (b7e7546, zero conflicts) and re-ran |
Background
Segments 3 (DANGEROUS_CMDS) and 4 (sudo block while super permission is off) of the
deny_sensitive_paths.sh/.ps1ToolCallBefore hook are gated on$TOOL == "exec_shell"*. Since foundation v0.9.3 the model/execution surface only exposes theBashtool (exec_shell*spellings moved intoRETIRED_TOOL_NAMES), so the hook receives the raw nameBashand both segments silently pass — the dangerous-command and sudo firewalls are dead (segments 1/2, the path/filename substring match over the full ARGS of every tool, still fire but with a large false-positive surface). Per the agreed decision we do not patch the hook's tool-name matching (no stopgap); the policy moves into the foundation execpolicy rule engine.Injection point
EngineConfig.exec_policy_engine(initial value injected bybuild_engine_config_for_session_roots; the headless L1 harness entryspawn_headlesscarries the same ruleset), the same channel and computation as the existingscope_deny_ruleset(connector/skill gating); the hot refresh after a toggle reusesEnginePool::refresh_permission_rulesets(Op::SetPermissionRuleset). Rationale: a typedDenyshort-circuits every approval mode (including YOLO/Never); evaluation happens after the ToolCallBefore hook and before approval, so the two defense lines are independent. Coverage is bounded to main-line sessions: nested subagent tool calls do not pass through execpolicy yet (known boundary; foundation upstream item).commanddenies are also promoted intodenied_prefixes(same semantics as the foundation config loader), activating the deny-always-wins channel with wrapper stripping / basename folding / flag-aware matching.v1 rules vs original hook semantics
~,$HOME,${HOME}, the real home,/root× bare/trailing-slash) + blanketfind <sensitive-dir>search-root deny + known credential child files (~/.ssh/config,~/.kube/config,~/.docker/config.json,~/.aws/config,~/.aws/credentials, ChromeCookies/Login Data/Local State,~/.gnupg/secring.gpg) + the enumerated secret-bearing child directory~/.gnupg/private-keys-v1.d(find-root + exfil/destroy first-argument anchoring) + directory-level glob dump forms (cat ~/.ssh/*,type %userprofile%\.ssh\*)ssh -i ~/.ssh/id_rsa host); general search roots stay allowed (a prefix rule there deterministically denies find's-prune/-not -pathidioms)~/project/secrets) are no longer over-blocked by substring, and not covered (registered)cat/less/more/head/tail/base64/xxd/od/strings) × sensitive absolute files incl. the-/.bakbackup spellings and the/etc/sudoers.d/*fragment glob +ssh-keygen/gpg --export-secret-keys[-subkeys]sudo/sudoeditcommand-word deny (covers/usr/bin/sudo,sudo -u root, chained segments,sudoedit)cp/mv/scp/rsync/tar/zip/ln/ditto/curldeny when the FIRST positional (or flag-value) argument is a sensitive path, plusdd if=(read direction;of=covers the reversed overwrite order)cp ~/.ssh/id_rsa /tmp/x,curl -T ~/.ssh/id_rsa <url>) incl. flag-prefixed forms (rsync -av ~/.ssh/ host:,zip -r a.zip ~/.ssh/— the engine's flag-value skipping anchors both); writes INTO a sensitive path stay allowed (key rotation); flag-less BSD spelling (tar czf …) and dest-first forms without anchorable positions (7z a …,aws s3 cp …,curl --form,wget --post-file=) are registered residuesrm/unlink/rmdir/shred/truncatedeny when the FIRST positional argument is a sensitive path (Windows:del/erase/remove-item/ri/rm/rd/rmdir/icacls/rename-item/rni+ canonical cmd.exe/-flag sequences such asdel /f /s /q …)rm a bcovers only the first target andchmod/chownput mode/owner before the path (both argument-position residues); cmd.exe flag orders beyond the canonical sequences are a registered residue.ps1segments 1/2)%userprofile%\/$home\/$env:userprofile\/~\prefixes, backslash directory/child/name spellings and…\dir\*globs, the%appdata%/%localappdata%/$env:Microsoft credential & protect directories, and the resolved real home on Windows hosts (type/get-content/gc/cat/morereaders;copy/xcopy/robocopy/copy-item/move/del/remove-item/rd/rmdir/icacls/rename-itemcommands)cmd /cnesting, other-user profiles,findstr/Invoke-WebRequest-style readers, double-quoted backslash paths (POSIX-style dequoting strips the separators),attrib +h …plus-flag-first forms,.exe-suffixed POSIX command spellings (MSYS). Doubled-backslash JSON-escaped spellings are NOT a residue: the deny-scan escape decoding folds\\into\(probe-verified).ps1segment-3 credential command words (was already dead)cmdkey/vaultcmd/get-credential/get-storedcredential/ credential-managercontrolinvocations /rundll32 keymgr.dll,krshowkeymgrThe hook scripts keep only segment 5 (the
list_mcp_resourcesconnector introspection correction) and the exit-2 contract comment; everything else is removed with a migration note.sudo snapshot limitation
Rule 4 snapshots the existence of
/etc/sudoers.d/pinvou3(super_permission::is_enabledreads the disk live) when the ruleset is built: spawn injects the initial value; a mid-session toggle triggersset_super_permission→refresh_permission_rulesets, hot-refreshing running engines so it applies from the next turn (same semantics as the connector/skill toggles). macOS/Windows are permanently in the off state.Known narrowing (registered in v1, see the safety_deny_rules module docs)
grep PATTERN ~/.kube/configstays allowed (foundation token-channel limit); the same limit applies to multi-argument removals (rm a bcovers only the first target),chmod/chown(mode/owner precedes the path), Windowsfindstr/Invoke-WebRequestreaders, dest-first archive/upload forms without an anchorable position (7z a …,aws s3 cp …,curl --form,wget --post-file=),dd if=<any> of=<sensitive>(the varyingif=blocks the prefix match), concrete sudoers fragment names, andfindwith the sensitive directory NOT as the first path token…/dir/*glob forms (the glob token is exact at the engine — the shell expands it); arbitrary children stay allowed: the token channel has no directory-containment primitive.~/.ssh/known_hostsis deliberately NOT enumerated (public host-key material)~/.ssh/id_*— would over-block publicid_rsa.pub),.exe-suffixed MSYS command spellings (cat.exe),attrib +h …, the double-quoted"${HOME}/…"spelling (the expansion drops the brace form from the word), sensitive directories nested at arbitrary depth (~/projects/.ssh/…), prefix-agnostic\microsoft\credentialslocations outside the profile prefixes/home/other/.ssh/…) are not enumerated; only~,$HOME,${HOME}, the real home and/rootare spelled outexec_shellcommands and File read-family path rules.Filepath rules are workspace-relative only (foundation workspace normalization)touch <sensitive path>(can neither read nor destroy content) stays allowed, allow-trace pinned; re-adding such a rule requires a deliberate decisionFuture work
find -name <sensitive name>under general search roots/-style cmd.exe flags (collapses the slash-flag enumeration) and to skip--style flags without consuming the next token; nested-subagent execpolicy wiring (foundation upstream);set_super_permissionserializationReview fixes
History note: the branch was rewritten into a clean commit series on the latest main; per-round SHAs below refer to their round content, not to stable SHAs. Commit subjects/bodies follow the commit-message convention (English, ≤50-character descriptions, DCO).
round 1: fixed a deterministic false-positive regression (
find . -pathidioms), de-hosted the regression tests (injectable sudo state), corrected three inaccurate claimsround 2: same-source regression entry
scope_deny_ruleset_with, exact rule-count pinning, allow-trace for registered narrowings, doc correctionsround 3: restored the live-hook deny coverage the token channel can express — credential child files, blanket sensitive-root
find, exfil-source commands, real-home//rootspellings, extended viewersround 4: probe-verified that the token channel expresses the former
.ps1live surface literally and restored it — Windows-native read/exfil/destroy families, revived.ps1segment-3 credential command words, a newrm/unlinkdestroy family, and theknown_hostspinning rationaleround 5: re-verified every retained narrowing against the execpolicy sources; closed the expressible gaps found in the same pass (
~/.gnupg/private-keys-v1.d, ChromeLocal State, the${HOME}/spelling) and pinned/registered the reviewer-named residuesround 6 (commit subject reshortened to satisfy the 50-character convention, tree unchanged): Linux
rust-testred was the documented hosted-runner link-phase loss, not a test failureround 7: rebased onto latest main (conflict-free; the CodeWhale gitlink follows main) and completed the residue registration in the module docs
round 8 (
77f83bf69, this head): closed the expressible coverage gaps found by a fresh five-way audit, all former hook coverage the token channel can express — cmd.exe/-flag invocation sequences (del /f /s /q …), the missingrd/rmdirdestroy family, directory-level glob dump forms (cat ~/.ssh/*), the destroy/tamper extensions (rmdir/shred/truncate/touch+ Windowsicacls/rename-item/rni), the exfil extensions (ln/ditto/curl -T/dd if=), and the/etc/shadow[-|.bak]//etc/sudoers[-|.bak]//etc/sudoers.d/*absolute spellings; fixed two inverted doc claims (sudoers.dwas a narrowing not an addition;scp -iis hook parity not an FP regression); probe-corrected two residue registrations (doubled-backslash spellings are decoded and denied, not a residue;zip -ris flag-value anchored) and re-registered the genuinely unexpressible tails (arbitrary flag orders, name-level globs,.execommand spellings,"${HOME}"quoted form,dd if=<any> of=<sensitive>, chmod/chown arg-position); added the missing allow-trace pins so every registered residue turns red on silent re-tightening; closed thespawn_headlessruleset bypass; corrected the commit-message convention violations in the rewritten history (English bodies, consistent DCO identity)round 9 (
310fe6647, this head): coverage-trim pass per the security-owner directive "the deny face must not exceed the former hook's security-relevant coverage": removed the one zero-value rule the round-8 parity restoration had carried (touch <sensitive path>— it can neither read nor destroy content; blocking it was pure substring parity with no interception value), registered it as a deliberate false-positive removal with an allow-trace pin, and audited every other family against the same bar (all remaining rules are either live-hook parity, original segment-3/4 intent revival, or documented intent-preserving widenings). Count 18245 → 17971Tests
safety_deny_rules(10 tests): two-state sudo snapshot; sudo wrapper/path-spelling coverage and word boundaries; sensitive reads across spellings incl. the glob dump and absolute-backup forms; exfil/destroy/dd/ln/ditto/curl source vectors; cmd.exe/-flag sequences,rd/rmdir/icacls/rename-itemand Windows glob forms; ordinary commands not over-denied incl. the full pinned residue set (grep arg-position,known_hosts,/home/<other>,~/.password-store/*, gnupg key files,ls ~/.aws/,tar czf,vi, multi-targetrm,chmod/chown,7z/aws s3 cp,cat.exe,touch(deliberate FP removal), non-firstfindroot,"${HOME}"quoted,dd if=… of=…, sudoers fragment names,findstr, mixed separators,cmd /c,attrib +h, non-canonical flag orders); two-way pin of the File workspace-normalization limit; exact ruleset count 17971 with a per-family breakdown (injected win-homeNone, host-independent)session_exec_policy_denies_migrated_hook_targets_under_bash_tool— falsified dead-path regression (ruleset injected viascope_deny_ruleset_withwith sudo off, decoupled from the host's/etc/sudoers.d/pinvou3;sudo rm/cat /etc/shadow/cat ~/.ssh/config/cp ~/.ssh/id_rsa /tmp/x→ Forbidden,cat README.mdpasses); scope-ruleset tests assert safety-net rules are always presentcargo test(assistant suite incl. bridge, safety_deny_rules 10/10),cargo fmt --check,python3 scripts/architecture-guard.py,./scripts/fork-guard.sh --fastall pass locally on this head