Closed
Conversation
…mmand detection
DANGEROUS_PATTERNS ran against the literal command string, so four shell
tokenization tricks that the shell strips or substitutes at exec time would
silently bypass every detection rule. Reported in GHSA-xvmp-c27r-qw3c and
GHSA-pwx6-3q74-cxwr.
Bypasses closed:
1. Empty quote pairs: `r""m -rf /tmp` → shell collapses `r""m` to `rm`
2. ${IFS} / $IFS: `rm${IFS}-rf${IFS}/tmp` → expands to `rm -rf /tmp`
3. $(cmd) / `cmd` command substitution: `$(echo rm) -rf /tmp` → runs rm
4. ANSI-C quoting: `$'\\x72\\x6d' -rf /tmp` → decodes to `rm -rf /tmp`
Implementation: split normalization into two complementary passes.
_normalize_command_for_detection keeps the literal form (needed for
structural patterns like `kill $(pgrep ...)` that target the substitution
idiom itself). _expand_shell_obfuscations reverses the four tokenization
tricks. detect_dangerous_command matches every DANGEROUS_PATTERNS entry
against both forms and flags on either match.
Also strips quoted single-identifier tokens after command substitution
inlining so `$(echo 'rm') -rf` resolves correctly. The strip only fires
for shell-safe identifier content (no spaces, no metacharacters), so
legitimate quoted strings like `echo "hello world"` or
`git commit -m 'fix: update'` are unaffected.
Tests: 19 new regression tests covering all four bypass classes plus
false-positive guards for quoted log messages, grep patterns, commit
messages, curl headers, and \${PATH}-style variable expansion. The
structural `kill $(pgrep ...)` pattern added in aedf6c7 continues to
fire on the literal form.
teknium1
changed the title
latekvo
reviewed
Apr 21, 2026
Comment on lines
+850
to
+853
| def test_empty_double_quote_pair_bypass_detected(self): | ||
| cmd = 'r""m -rf /tmp/foo' | ||
| dangerous, _, _ = detect_dangerous_command(cmd) | ||
| assert dangerous is True |
There was a problem hiding this comment.
Isn't it better to deny access based on something like ptrace? Then you don't have to worry about complex regex or edge cases like this one.
It would be more pedantic than current approaches, but i'm not sure if that's a bad thing.
The user either allows for access to files outside the project or not, if it's the latter then i don't think every case can be handled perfectly, it could be better to default to rejecting such removals/edits.
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
2 participants
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.
No description provided.