refactor(approval): replace regex inline-script-execution detection w… - #60801
Closed
MorAlekss wants to merge 2 commits into
Closed
refactor(approval): replace regex inline-script-execution detection w…#60801MorAlekss wants to merge 2 commits into
MorAlekss wants to merge 2 commits into
Conversation
…ith tokenizer-based
…n interpreter family
Contributor
Author
|
Added recognition for |
Contributor
Author
|
Superseded by and merged as part of #63585 by @teknium1, which unifies this tokenizer direction with the execution-option coverage from #59899 into a single, broader detector (also covers rg/sort/ag/man hooks, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces regex-based inline-script-execution detection in
tools/approval.py (python/node/perl/ruby/php -c/-e/--eval/-p/--print,
PowerShell -File/-Command, and heredoc) with a tokenizer-based
implementation (shlex.split() + a language-specific exec-flag table).
This follows from #57666 and #57990, which received three and two
rounds of comments respectively, each finding a new command shape the
regex missed: combined/glued/long-form flags, versioned interpreter
binaries, flags with their own argument (-W ignore), and an entirely
different flag letter for the same behavior (node -p/--print, neither
'e' nor 'c'). Matching flag characters in a flat string can't reliably
model argument parsing; that needs an actual tokenizer, not another
pattern, so each round closed one shape while leaving the general
problem open for the next.
This isn't cosmetic.
~/.hermes/config.yamlis whereapprovals.mode,yolo, and the permanent allowlist live, so a missed flag shape lets acommand rewrite it with no approval at all, silently disabling the
agent's own approval gate. Tokenizing once (the same shlex.split()
approach already used in this file's
_literal_command_substitution_output()and in
hermes_cli/mcp_security.py's_command_basename()) closes thewhole class of gaps instead of the one shape found so far.
#57666 and #57990 are left open rather than closed, since both contain
other, independent fixes that don't touch this code path. If this PR
merges, their own
-e/-c/-Fileregex rules become the same fragilepattern this one replaces; the natural next step is rebasing those
additions onto this tokenizer instead of carrying their own regex
forward.
Root cause
The old rules were regular expressions trying to answer an argument-
parsing question ("skip past any number of other flags, some of which
take their own argument, in short or long form, and find one meaning
'run this code inline'"). A flat-string pattern has no way to know
which flags consume a following token without hand-enumerating every
one, and no amount of added alternation closes that gap in general; it
only closes the specific shapes someone tested. This is not a fourth
incremental fix to the same rule; it is a different category of tool
for a problem regex was never suited to solve, closing every case in
this class at once rather than the next one someone happens to test.
Behavioral change
Before: detection depended on which flag-combination shapes had been
added to the regex so far.
After:
_detect_inline_script_execution()tokenizes the command,identifies the interpreter by family (version-suffix tolerant), and
scans every token against that family's known inline-exec flags,
independent of how many other flags precede it or how the target flag
is glued/combined.
What changed
tools/approval.py:_INTERPRETER_FAMILY_PATTERN,_EXEC_FLAGS,_interpreter_family(),_detect_inline_script_execution(),_inline_exec_result(), called as a fallback indetect_dangerous_command()after the existing regex loop-e/-cand heredoc regex rules this replaces_PATTERN_KEY_ALIASESentries mapping the new pattern_keys tothe old regex description strings, so an operator's existing
permanent allowlist entry isn't silently invalidated
now report the exec flag, not heredoc
tests/tools/test_approval.py:TestTokenizerInlineScriptExecutionclass (31 tests) coveringevery previously-missed shape from both PRs, node/ruby
-c/--check(must stay unflagged), PowerShell
-File/-Commandincluding thefalse-positive-attribution case, versioned binaries, malformed shlex
input, and the aliasing fix
fix closes
What is NOT changed
untouched
perl/ruby -i(in-place file edit) rules are untouched, adifferent threat category not part of this refactor's scope
#57666and#57990are left open and unmodified; see Summary