Skip to content

fix(approval): gate perl -i and awk -i inplace edits of Hermes config/env - #37107

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/approval-perl-awk-inplace-14639
Closed

fix(approval): gate perl -i and awk -i inplace edits of Hermes config/env#37107
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/approval-perl-awk-inplace-14639

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

This is a sibling follow-up to commit 4e9d886d9 (fix(approval): pair terminal-side gate for ~/.hermes/config.yaml writes)

  • 4e9d886 covered: terminal-side in-place edits via sed -i / sed --in-place of ~/.hermes/config.yaml and .env (added _HERMES_CONFIG_PATH, folded into _SENSITIVE_WRITE_TARGET + sed DANGEROUS_PATTERNS).
  • 4e9d886 did NOT touch: the other standard in-place editors — perl -i (also -pi, -i.bak) and awk -i inplace / gawk -i inplace. Same in-place-mutation pattern, different editor verb, reaches the same security file.
  • This PR adds: the same in-place-edit deny for perl -i and awk -i inplace against ~/.hermes/config.yaml and .env, reusing the existing _HERMES_CONFIG_PATH / _HERMES_ENV_PATH fragments.

What does this PR do?

Pairs the remaining terminal-side in-place editors with the existing write_file/patch deny on ~/.hermes/config.yaml / .env. sed -i is already gated (#14639 / 4e9d886); perl -i -pe 's/.../approvals.mode: off/' ~/.hermes/config.yaml and awk -i inplace were not. Because the config cache is mtime-keyed, such an in-place write takes effect mid-session and the agent can flip approvals.mode=off to bypass the approval gate — so the sed-only pairing is incomplete ("theater" per SECURITY.md, the same argument the parent made for sed).

Issue lists 1 escalation vector at this code site (in-place edit of the Hermes security config). The parent closed it for sed; this PR closes the remaining editor verbs (perl -i, awk -i inplace) at the same site. No new path fragment — only the editor verb is widened.

Related Issue

Sibling follow-up to 4e9d886 / #14639 (no separate issue).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix

Changes Made

  • tools/approval.py: two new DANGEROUS_PATTERNS entries — perl -[..]i and (g)awk -i inplace targeting _HERMES_CONFIG_PATH / _HERMES_ENV_PATH.
  • tests/tools/test_approval.py: positive cases (perl -i / -pi / -i.bak, awk/gawk -i inplace, $HERMES_HOME form) + no-regression negatives (perl --version, in-place edit of /tmp, read-only awk, cat of config.yaml).

How to Test

uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/tools/test_approval.py -v

Regression guard verified both directions: with the prod hunk stashed, all 6 positive cases fail (perl -i/-pi/-i.bak, awk/gawk -i inplace, $HERMES_HOME); restored, the full file passes 207/207.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the focused suite and all tests pass (207/207 in tests/tools/test_approval.py)
  • I've added tests for my changes
  • I've tested on my platform: macOS 15.2

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact — the regexes are shell-syntax (not OS path) based and match the parent's sed pairing — or N/A

Audited siblings: confirmed sed (parent), perl -i, and awk -i inplace are the standard in-place editors; tee/>/cp/mv are already covered by _SENSITIVE_WRITE_TARGET. No further widening needed.

Copilot AI review requested due to automatic review settings June 2, 2026 01:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR expands dangerous-command detection to block additional in-place editors (Perl and Awk) from directly mutating Hermes config/env files, closing a bypass similar to the existing sed -i protection.

Changes:

  • Add dangerous-command patterns for perl -i* and awk/gawk -i inplace targeting Hermes config/env paths.
  • Add test coverage for the new Perl/Awk in-place patterns, including positive and negative cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tools/approval.py Adds regex detections for Perl/Awk in-place edits to Hermes config/env files.
tests/tools/test_approval.py Adds tests validating the new in-place editor detections and no-regression negatives.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/approval.py Outdated
# mutates ~/.hermes/config.yaml (or .env) directly, and the mtime-keyed
# config cache reloads it mid-session — so gating only `sed` leaves the
# write_file/patch deny unpaired. Sibling follow-up to #14639.
(rf'\bperl\s+-[^\s]*i.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env (perl -i)"),
@mxnstrexgl

Copy link
Copy Markdown

Hermes Agent auto-review — approval gate hardening: ✅ clean. Sibling follow-up to the sed -i pairing (#14639) — perl -i and awk -i inplace were real bypasses against ~/.hermes/config.yaml / .env. Test coverage is thorough (positive + negative + HERMES_HOME override). Approve.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management comp/tools Tool registry, model_tools, toolsets labels Jun 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #36894 - same approval-gate hardening for perl -i / awk -i inplace edits of hermes config.yaml and .env. #36894 covers perl/ruby -i; this PR covers perl/awk -i. Maintainer should consolidate.

@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot All findings addressed in commit 78ddc36ab:

  • The perl in-place regex no longer uses the loose -[^\s]*i match that fired on any 'i' in a flag bundle. Because detection lowercases input and runs under re.IGNORECASE, the include-path flag -I collapsed to -i, so perl -Ilib script.pl ~/.hermes/config.yaml was wrongly flagged. The pattern now requires a real in-place flag: an optional perl boolean-flag bundle, then 'i', then a flag boundary (whitespace) or a backup-suffix start (.bak / ~ / quote / *). -Ilib no longer matches because 'i' is followed by include-path letters.
  • Added regression tests: test_perl_include_path_flag_not_flagged asserts perl -Ilib script.pl ~/.hermes/config.yaml (and a multi--I variant) is NOT flagged, while test_perl_real_inplace_still_flagged_vs_include asserts perl -Ilib -i -pe ... ~/.hermes/config.yaml (include path + genuine -i) IS still flagged. Existing perl -i / -pi / -i.bak positives remain green (full file: 209 passed).

@briandevans
briandevans force-pushed the fix/approval-perl-awk-inplace-14639 branch 9 times, most recently from 1a8dceb to 105a7fd Compare June 3, 2026 22:16
…/env

The terminal-side pairing for ~/.hermes/config.yaml and .env (NousResearch#14639)
gated `sed -i` / `sed --in-place` but not the other standard in-place
editors that reach the same security file: `perl -i` (also `-pi`,
`-i.bak`) and `awk -i inplace` / `gawk -i inplace`. Each mutates the
file directly, bypassing the redirection/tee/cp patterns, and the
mtime-keyed config cache reloads it mid-session — so the agent can flip
approvals.mode=off and bypass the gate. Gating only sed left the
write_file/patch deny unpaired theater for these verbs.

Add two DANGEROUS_PATTERNS entries reusing the audited
_HERMES_CONFIG_PATH / _HERMES_ENV_PATH fragments (no new path fragment;
only the editor verb is widened). Regression tests cover perl -i / -pi /
-i.bak, awk/gawk -i inplace, the $HERMES_HOME override form, and
no-regression negatives (read-only awk, scratch-path in-place edit, cat).
… not gated

The perl in-place pattern used a loose `-[^\s]*i` match that fired on any
'i' in a flag bundle. Detection lowercases input and runs under
re.IGNORECASE, so perl's include-path flag `-I` (uppercase, takes a
directory arg) collapses to `-i` and `perl -Ilib script.pl
~/.hermes/config.yaml` was wrongly flagged as an in-place edit.

Require `-i` to be a genuine in-place flag: an optional bundle of perl's
boolean prefix flags, then 'i', then a flag boundary (whitespace) or a
backup suffix start (.bak / ~ / quote / *). `-Ilib` no longer matches
because 'i' is followed by include-path letters. Real in-place edits,
including when an include path precedes `-i`, still gate.
@briandevans
briandevans force-pushed the fix/approval-perl-awk-inplace-14639 branch from 105a7fd to 86b26ed Compare June 4, 2026 03:17
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing — superseded. The perl/ruby in-place approval gate landed on main (a6a4e6f + b04c6e9, the #36894 family alt-glitch flagged), and the remaining awk -i inplace coverage is now carried by #39597. Nothing left for this branch to add. Happy to reopen if a gap is found.

@briandevans briandevans closed this Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants