fix(approval): hardline in-place edits of Hermes config/env - #60523
fix(approval): hardline in-place edits of Hermes config/env#60523qni-live wants to merge 1 commit into
Conversation
|
suggesting changes The new hardline entries still match these in-place-edit strings when they are quoted data inside another command, so normal repository operations such as Security evidence:
Signed: GPT-5.5-xhigh in Codex |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real approval-boundary gap. Current main still classifies in-place edits of Hermes config/env as dangerous (tools/approval.py:733-743) and sends smart-mode findings to _smart_approve() (tools/approval.py:2838-2862), so promoting actual editor commands to the hardline floor is directionally sound.
Problems
tools/approval.py:402-404uses unanchored searches across the entire command. The exact added regex matches quoted data such asgit commit -m "sed -i s/a/b/ ~/.hermes/config.yaml"andecho "perl -pi -e s/a/b/ ~/.hermes/.env"; hardline responses are unconditional (tools/approval.py:561-572). This regresses the existing quoted-data contract intests/tools/test_hardline_blocklist.py:242-274, added by7534b5be2.
Suggested changes
- Anchor each new rule at a real command position via
_CMDPOS, with the existing quote-aware_command_detection_variants()support (tools/approval.py:1401-1431). - Add quoted-data negative regressions for the new sed/perl/ruby patterns.
Automated hermes-sweeper review.
| @@ -399,11 +399,14 @@ def _hardline_rm_path(path_alt: str, tail: str = r'(?:\s|$|[)`;|&])') -> str: | |||
| (_CMDPOS + r'init\s+[06]\b', "init 0/6 (shutdown/reboot)"), | |||
| (_CMDPOS + r'systemctl\s+(poweroff|reboot|halt|kexec)\b', "systemctl poweroff/reboot"), | |||
| (_CMDPOS + r'telinit\s+[06]\b', "telinit 0/6 (shutdown/reboot)"), | |||
| (rf'\bsed\s+-[^\s]*i.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env (hardline)"), | |||
There was a problem hiding this comment.
This whole-command search also matches editor syntax inside quoted data, e.g. git commit -m "sed -i s/a/b/ ~/.hermes/config.yaml", making that unrelated command unconditionally hardline-blocked. Please anchor this and the sibling editor rules at an actual command position (the existing _CMDPOS plus quote-aware command variants provides that contract).
16ad3ac to
9b7bd72
Compare
|
Thanks @egilewski and @teknium1 — you were both right about the bare searches. Reworked in 9b7bd72:
One known limit, intentionally out of scope: indirect path delivery ( |
|
suggesting changes The The first form is valid GNU sed and performed the in-place edit in a focused probe. Please detect the actual command word and parse the sed option sequence rather than requiring one narrow spelling, then add regressions for these forms. There is also a remaining unconditional false-positive class because the protected path is searched anywhere after the sed options rather than as the target operand. Both Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
9b7bd72 to
66b596e
Compare
|
Thanks — both findings reproduced exactly as described, and both are now fixed by What was wrongThe regex approach was wrong in both directions, and no amount of pattern Bypasses. The pattern required literal False positives. The protected path was searched anywhere after the options What replaces it
Option parsing deliberately runs on original-case text: Two additions beyond the reported findings, both flagged here for scoping:
Indirect path delivery via Validation
|
fix(approval): hardline in-place edits of Hermes config/env The grammar-aware approach (resolve the real command word, walk the editor's option grammar, compare FILE operands as whole tokens) is a clear improvement over text-regex matching, and the test matrix is impressive. Observations, mostly about the boundary of what the grammar check can see:
|
|
suggesting changes Two P2 hardline bypasses remain in the protected-file edit guard:
Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
…onto origin/main) Collapse R1-R3 into one commit on current origin/main (36b0a96), replacing the ~2700-commits-stale base. Clean 3-way merge — no textual conflicts, no name collisions with upstream's a7aa814/8163c8731b; every piece of the R1-R3 work verified present in the merged tree. The fix (unchanged in content): - _wrapper_operand_span() + _leading_wrapper_indexes(): one shared wrapper model consumed by BOTH the floor resolver and the -c-payload stepper, so the two detection paths cannot drift (the R3 bypass class). - _flock_exec_payload(): flock's own -c (runs payload via sh -c). - _WrapperResolutionFailed + fail-closed path in _detect_hermes_inplace_edit(). - posixpath.normpath in _is_protected_hermes_operand() (path-alias canonicalization). - Complete _EDITOR_WRAPPERS / _WRAPPER_POSITIONAL / _WRAPPER_OPTS_WITH_ARG / _WRAPPER_NOARG_FLAGS tables (man-page verified). Verification (on the NEW base): targeted file: 354 passed, 0 failed reviewer probe: PASS (61/61 in-place, 13/13 unrelated, 23/23 allow) approval-domain: test_approval 2/106, test_execution_flag_detection 4/78/3sk, test_credential_files 3/39/1sk, test_approved_command_clean_slate 3/5 — all byte-identical to origin/main baseline
66b596e to
1f004a9
Compare
|
Thanks — both P2 findings reproduced as described and are now closed. Force-pushed [P2] Wrapper options can hide the real editorReproduced. The wrapper resolver skipped only option-shaped tokens, so a wrapper Rather than extend the tables again, the resolver now carries an explicit wrapper Auditing the existing table against the man pages turned up four entries that do [P2] Path aliases bypass protected-file matchingReproduced. Operands are now canonicalized before the filename-boundary Backup names stay out: Two structural changes, both prompted by this reviewFail closed on an unresolvable wrapper. Keeping two hand-maintained option One wrapper model, two call sites. The floor resolver and the payload stepper Validation
Known limits, unchanged in scopeIndirect target delivery still stays at the smart level, consistent with the |
What
Adds three patterns to
HARDLINE_PATTERNSintools/approval.pythat unconditionally block in-place edits of~/.hermes/config.yamland~/.hermes/.envviased -i,sed --in-place, andperl/ruby-i. They mirror the identical regexes that already exist inDANGEROUS_PATTERNS, reusing the existing_HERMES_CONFIG_PATH/_HERMES_ENV_PATHpath fragments (so$HOME,${HOME}, and$HERMES_HOMEspellings are covered too).Why
With
approvals.mode: smart, an aux LLM adjudicates commands that matchDANGEROUS_PATTERNS. Observed in the field (Windows install, July 2026): the adjudicator approvedsed -i 's/.../.../' ~/.hermes/config.yamleven though the matched rule's description ("in-place edit of Hermes config/env") was correctly surfaced to it.config.yamlis the security policy —approvals.mode, yolo, and the permanent-approval allowlist live there, and the mtime-keyed config cache means a write takes effect mid-session..envholds credentials and is already deny-listed on thefile_toolsside (write_file/patchrefuse it). One generous smart verdict on the terminal side therefore lets the agent rewrite its own guardrails, making the file-tool deny unpaired theater. That fits the hardline bar ("a floor below yolo"): the blast radius is every other guard.Scope / non-goals
cat,grep) andsedwithout-ion these files are unaffected.config.yaml/.envfiles keep their existing (smart-approvable) treatment via_PROJECT_CONFIG_PATH/_PROJECT_ENV_PATH.tee,>,cp) already have their own rules; this PR only promotes the in-place-editor family that was observed bypassing smart approval.Tests
tests/tools/test_hardline_blocklist.py(flag spellings-i,-i.bak,-ri,--in-place, macOS-i '',perl -pi/-i -pe,ruby -i -pe; path spellings~,$HOME,${HOME},$HERMES_HOME).HARDLINE_PATTERNSgrows 12 → 15, under thetest_hardline_list_is_smallcap of 20; the compile-cost comment inapproval.pyis updated to match.pytest tests/tools/test_hardline_blocklist.py tests/tools/test_approval.py tests/tools/test_shell_bypass_denylist.py: 535 passed.🤖 Generated with Claude Code