fix(security): close env/config write-deny bypass via trailing arg or comment
The dangerous-command approval gate has rules that flag a shell command
when it overwrites a project `.env` or `config.yaml` — these files hold
API keys, DB passwords, and (for `config.yaml`) the approval policy
itself, so a write to them should require user approval. The matching
`write_file`/`patch` deny on the file-tools side was paired with these
terminal-side rules so neither path is an open door.
The redirection and `tee` rules anchored the sensitive path with
`_COMMAND_TAIL` (`(?:\s*(?:&&|\|\||;).*)?$`), which only tolerates the
rest of the line being empty or a command separator. The problem: in
POSIX shell the redirection target is fixed regardless of what trails it.
`echo secret > .env extra` still truncates `.env` (the `extra` is just
another argument to `echo`), and `echo secret > .env # note` does too
(the `#` starts a comment). Because neither tail is a separator, the old
anchor failed to match and the command sailed through approval — a
prompt-injected step could overwrite a project `.env`/`config.yaml`
unprompted. The system-path redirection rule one line above never had
this restriction and already caught these forms.
The fix introduces `_WRITE_TARGET_BOUNDARY`, a lookahead that only
requires the path token to END at a shell word boundary (whitespace,
quote, separator, redirection operator, `#`, or EOL) rather than
demanding the rest of the line be empty. It is applied to the two
stream-write rules (redirection and `tee`) where the sensitive path is
always a write target. The `cp`/`mv`/`install` rule deliberately keeps
`_COMMAND_TAIL`: there the sensitive file is only a target when it is the
LAST argument (the destination), so requiring end-of-line is correct and
keeps `cp config.yaml backup.yaml` (config.yaml as the source) out of the
deny.
## What does this PR do?
Closes a bypass in the dangerous-command approval gate where a trailing
argument or `#` comment after a `>`/`>>`/`tee` write target let a command
overwrite a project `.env` or `config.yaml` without triggering approval,
even though the shell still overwrites the file.
## Related Issue
N/A
## Type of Change
- [x] 🔒 Security fix
## Changes Made
- `tools/approval.py`: add `_WRITE_TARGET_BOUNDARY` (a word-boundary
lookahead) and use it instead of `_COMMAND_TAIL` in the two
project-env/config stream-write patterns ("overwrite project env/config
via tee" and "via redirection"). `_COMMAND_TAIL` is kept and still used
by the `cp`/`mv`/`install` rule, where end-of-line anchoring is the
correct semantics.
- `tests/tools/test_approval.py`: add regression tests for
`> .env extra`, `> .env # note`, `>> config.yaml foo`, and
`tee .env backup` (now flagged), plus `> config.yaml.bak` (must stay
safe — different file).
## How to Test
1. Reproduce: before the fix,
`detect_dangerous_command("echo secret > .env extra")` returns
`(False, None, None)` — the overwrite is not flagged.
2. Apply the fix; the same call now returns the "overwrite project
env/config via redirection" detection.
3. Run `pytest tests/tools/test_approval.py -q` — the new cases pass and
the existing `cp config.yaml backup.yaml` / `config.yaml.bak`
false-positive guards still hold.
## Checklist
### Code
- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains only changes related to this fix
- [x] I've run the relevant tests and they pass
- [x] I've added tests for my changes
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)
### Documentation & Housekeeping
- [x] I've updated relevant documentation (README, docs/, docstrings) — or N/A
- [x] I've updated cli-config.yaml.example if I added/changed config keys — or N/A
- [x] I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) — or N/A
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
This commit is contained in:
parent
83ae65487e
commit
7bfdc0bca6
2 changed files with 55 additions and 2 deletions
|
|
@ -642,6 +642,37 @@ class TestSensitiveRedirectPattern:
|
|||
assert key is None
|
||||
assert desc is None
|
||||
|
||||
def test_redirect_to_dotenv_with_trailing_arg_requires_approval(self):
|
||||
# The redirection target is still `.env`; the trailing token is just an
|
||||
# extra argument to `echo`, so the file is overwritten. The old
|
||||
# _COMMAND_TAIL anchor required the rest of the line to be empty/a
|
||||
# separator and let this slip past the deny.
|
||||
dangerous, key, desc = detect_dangerous_command("echo secret > .env extra")
|
||||
assert dangerous is True
|
||||
assert key is not None
|
||||
assert "project env/config" in desc.lower()
|
||||
|
||||
def test_redirect_to_dotenv_with_trailing_comment_requires_approval(self):
|
||||
# A trailing `#` comment does not change the redirection target.
|
||||
dangerous, key, desc = detect_dangerous_command("echo secret > .env # note")
|
||||
assert dangerous is True
|
||||
assert key is not None
|
||||
assert "project env/config" in desc.lower()
|
||||
|
||||
def test_append_to_config_yaml_with_trailing_arg_requires_approval(self):
|
||||
dangerous, key, desc = detect_dangerous_command("echo mode: prod >> config.yaml foo")
|
||||
assert dangerous is True
|
||||
assert key is not None
|
||||
assert "project env/config" in desc.lower()
|
||||
|
||||
def test_redirect_to_config_yaml_backup_is_safe(self):
|
||||
# `config.yaml.bak` is a different file; the boundary must end the path
|
||||
# token at a word boundary so backup writes stay out of the deny.
|
||||
dangerous, key, desc = detect_dangerous_command("echo x > config.yaml.bak")
|
||||
assert dangerous is False
|
||||
assert key is None
|
||||
assert desc is None
|
||||
|
||||
|
||||
class TestProjectSensitiveCopyPattern:
|
||||
def test_cp_to_local_dotenv_requires_approval(self):
|
||||
|
|
@ -825,6 +856,14 @@ class TestProjectSensitiveTeePattern:
|
|||
assert key is not None
|
||||
assert "project env/config" in desc.lower()
|
||||
|
||||
def test_tee_to_dotenv_with_trailing_file_arg_requires_approval(self):
|
||||
# tee writes to every file argument, so `.env` is overwritten even when
|
||||
# another file follows it. The old _COMMAND_TAIL anchor missed this.
|
||||
dangerous, key, desc = detect_dangerous_command("printenv | tee .env backup")
|
||||
assert dangerous is True
|
||||
assert key is not None
|
||||
assert "project env/config" in desc.lower()
|
||||
|
||||
|
||||
class TestPatternKeyUniqueness:
|
||||
"""Bug: pattern_key is derived by splitting on \\b and taking [1], so
|
||||
|
|
|
|||
|
|
@ -258,7 +258,21 @@ _USER_SENSITIVE_WRITE_TARGET = (
|
|||
rf'{_CREDENTIAL_FILES})'
|
||||
)
|
||||
_PROJECT_SENSITIVE_WRITE_TARGET = rf'(?:{_PROJECT_ENV_PATH}|{_PROJECT_CONFIG_PATH})'
|
||||
# Anchor for the cp/mv/install rule, where the sensitive path is only a write
|
||||
# target when it is the LAST argument (the destination). Requiring end-of-line
|
||||
# (or a command separator) keeps `cp config.yaml backup.yaml` — config.yaml as
|
||||
# the SOURCE — out of the deny.
|
||||
_COMMAND_TAIL = r'(?:\s*(?:&&|\|\||;).*)?$'
|
||||
# Boundary for stream-write rules (`>`/`>>` redirection and `tee`), where the
|
||||
# sensitive path is ALWAYS a write target no matter what follows it. We only
|
||||
# need the path token to END at a shell word boundary — whitespace, a quote, a
|
||||
# command separator, a redirection operator, a `#` comment, or end-of-line.
|
||||
# Using _COMMAND_TAIL here was too strict: it required the rest of the line to
|
||||
# be empty or a command separator, so `echo x > .env extra` (extra arg to echo)
|
||||
# and `echo x > .env # note` (trailing comment) slipped past the deny even
|
||||
# though the shell still overwrites `.env`. Mirrors the looser system-path
|
||||
# redirection rule, which never had this restriction.
|
||||
_WRITE_TARGET_BOUNDARY = r'(?=[\s;&|<>#"\']|$)'
|
||||
|
||||
# =========================================================================
|
||||
# Hardline (unconditional) blocklist
|
||||
|
|
@ -487,8 +501,8 @@ DANGEROUS_PATTERNS = [
|
|||
(r'\b(bash|sh|zsh|ksh)\s+<\s*<?\s*\(\s*(curl|wget)\b', "execute remote script via process substitution"),
|
||||
(rf'\btee\b.*["\']?{_SENSITIVE_WRITE_TARGET}', "overwrite system file via tee"),
|
||||
(rf'>>?\s*["\']?{_SENSITIVE_WRITE_TARGET}', "overwrite system file via redirection"),
|
||||
(rf'\btee\b.*["\']?{_PROJECT_SENSITIVE_WRITE_TARGET}["\']?{_COMMAND_TAIL}', "overwrite project env/config via tee"),
|
||||
(rf'>>?\s*["\']?{_PROJECT_SENSITIVE_WRITE_TARGET}["\']?{_COMMAND_TAIL}', "overwrite project env/config via redirection"),
|
||||
(rf'\btee\b.*["\']?{_PROJECT_SENSITIVE_WRITE_TARGET}["\']?{_WRITE_TARGET_BOUNDARY}', "overwrite project env/config via tee"),
|
||||
(rf'>>?\s*["\']?{_PROJECT_SENSITIVE_WRITE_TARGET}["\']?{_WRITE_TARGET_BOUNDARY}', "overwrite project env/config via redirection"),
|
||||
(r'\bxargs\s+.*\brm\b', "xargs with rm"),
|
||||
# find -exec rm / -execdir rm — the -execdir variant (same semantics,
|
||||
# runs in the directory of each match) was previously missed. Claude
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue