Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions tests/tools/test_approval.py
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,111 @@ def test_normal_yaml_write_safe(self):
assert dangerous is False


class TestHermesConfigPerlAwkInPlace:
"""Sibling follow-up to #14639 / the sed-in-place pairing: `sed -i` is not
the only standard in-place editor that mutates ~/.hermes/config.yaml (or
.env) directly and so bypasses the redirection/tee/cp patterns. `perl -i`
(also `-pi`, `-i.bak`) and `awk -i inplace` / `gawk -i inplace` reach the
same security file via the same in-place-write path. Gating only `sed`
leaves the write_file/patch deny unpaired theater. These pin the remaining
in-place editors against the config/env files."""

def test_perl_in_place(self):
# The gap: perl -i mutates the file directly, like sed -i.
dangerous, key, desc = detect_dangerous_command(
"perl -i -pe 's/manual/off/' ~/.hermes/config.yaml"
)
assert dangerous is True
assert "hermes config" in desc.lower() or "in-place" in desc.lower()

def test_perl_combined_pi_flag(self):
dangerous, key, desc = detect_dangerous_command(
"perl -pi -e 's/manual/off/' ~/.hermes/config.yaml"
)
assert dangerous is True

def test_perl_in_place_with_backup_suffix_on_env(self):
# -i.bak (backup suffix) must still be caught, against .env.
dangerous, key, desc = detect_dangerous_command(
"perl -i.bak -pe 's/x/y/' ~/.hermes/.env"
)
assert dangerous is True

def test_awk_in_place(self):
dangerous, key, desc = detect_dangerous_command(
"awk -i inplace '{print}' ~/.hermes/config.yaml"
)
assert dangerous is True
assert "hermes config" in desc.lower() or "in-place" in desc.lower()

def test_gawk_in_place_on_env(self):
dangerous, key, desc = detect_dangerous_command(
"gawk -i inplace '{print}' ~/.hermes/.env"
)
assert dangerous is True

def test_custom_hermes_home(self):
# HERMES_HOME override form must be covered like the sed pairing.
dangerous, key, desc = detect_dangerous_command(
"perl -i -pe 's/x/y/' $HERMES_HOME/config.yaml"
)
assert dangerous is True

# --- No-regression negatives ---

def test_perl_without_in_place_safe(self):
# No -i flag, no config path — the new in-place pattern must not trip.
# (`perl -e` is independently gated by the -e/-c script pattern, so use
# a read-only invocation to isolate the in-place pairing.)
dangerous, key, desc = detect_dangerous_command("perl --version")
assert dangerous is False

def test_perl_in_place_non_sensitive_path_safe(self):
# In-place edit of a scratch file is not the security file.
dangerous, key, desc = detect_dangerous_command(
"perl -i -pe 's/x/y/' /tmp/scratch.yaml"
)
assert dangerous is False

def test_perl_include_path_flag_not_flagged(self):
# Regression for the Copilot finding on PR #37107: detection lowercases
# input and runs under re.IGNORECASE, so perl's include-path flag `-I`
# (uppercase, takes a directory arg) collapses to `-i`. A loose
# `[^\s]*i` match wrongly flagged `perl -Ilib script.pl <config>` as an
# in-place edit even though no in-place editing happens. The precise
# flag-grammar pattern must NOT fire here.
dangerous, key, desc = detect_dangerous_command(
"perl -Ilib script.pl ~/.hermes/config.yaml"
)
assert dangerous is False
# Multiple include paths bundled the same way must also stay safe.
dangerous, key, desc = detect_dangerous_command(
"perl -Ilib -Ivendor/lib run.pl ~/.hermes/.env"
)
assert dangerous is False

def test_perl_real_inplace_still_flagged_vs_include(self):
# Companion to the regression above: a genuine in-place edit that also
# passes an include path must STILL be gated. Pins that the fix narrows
# only the false positive, not real coverage.
dangerous, key, desc = detect_dangerous_command(
"perl -Ilib -i -pe 's/manual/off/' ~/.hermes/config.yaml"
)
assert dangerous is True

def test_read_only_awk_on_config_safe(self):
# awk without -i inplace only reads; must not trip.
dangerous, key, desc = detect_dangerous_command(
"awk '{print}' ~/.hermes/config.yaml"
)
assert dangerous is False

def test_cat_config_safe(self):
# Reading config is not a write.
dangerous, key, desc = detect_dangerous_command("cat ~/.hermes/config.yaml")
assert dangerous is False


class TestFindExecFullPathRm:
"""Detect find -exec with full-path rm bypasses."""

Expand Down
21 changes: 21 additions & 0 deletions tools/approval.py
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,27 @@ def _sudo_stdin_block_result(description: str) -> dict:
# the terminal side is not an open door. See #14639.
(rf'\bsed\s+-[^\s]*i.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env"),
(rf'\bsed\s+--in-place\b.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env (long flag)"),
# Same in-place-edit escalation as the sed pairing above, reached through
# the other standard in-place editors `sed -i` does not cover: `perl -i`
# (also `-pi`, `-i.bak`) and `awk -i inplace` / `gawk -i inplace`. Each
# 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.
#
# The `-i` token must be matched against perl's actual flag grammar, NOT a
# loose `[^\s]*i` that fires on any `i` in a flag bundle. Detection runs
# under re.IGNORECASE *and* lowercases input, so the include-path flag
# `-I` (which consumes its arg as a directory, e.g. `perl -Ilib script.pl`)
# is indistinguishable from `-i` by case alone — a loose match wrongly
# gates `perl -Ilib script.pl ~/.hermes/config.yaml`. Instead require `-i`
# to be a real in-place flag: an optional bundle of the boolean prefix
# flags perl allows before it (`-pi`, `-ni`, `-pi.bak`, …), then `i`, then
# either the flag boundary (whitespace) or a backup suffix introduced by a
# non-letter (`.bak`, `~`, `'*'`). `-Ilib` fails because `i` is followed by
# the letters of the include path, which is neither a boundary nor a
# suffix start. See #14639 and the Copilot finding on this PR.
(rf'\bperl\s+(?:[^\s]+\s+)*?-[pnaslwxcefutWCSU0-9]*i(?=\s|[.~\'"*])[^\s]*\s.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env (perl -i)"),
(rf'\b(?:g?awk)\s+-i\s+inplace\b.*(?:{_HERMES_CONFIG_PATH}|{_HERMES_ENV_PATH})', "in-place edit of Hermes config/env (awk -i inplace)"),
# Script execution via heredoc — bypasses the -e/-c flag patterns above.
# `python3 << 'EOF'` feeds arbitrary code via stdin without -c/-e flags.
(r'\b(python[23]?|perl|ruby|node)\s+<<', "script execution via heredoc"),
Expand Down
Loading