From 7bfdc0bca6c33495cefbe00226189072e9ca5201 Mon Sep 17 00:00:00 2001 From: friendshipisover <290862769+friendshipisover@users.noreply.github.com> Date: Sun, 7 Jun 2026 06:54:13 +0300 Subject: [PATCH] fix(security): close env/config write-deny bypass via trailing arg or comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/tools/test_approval.py | 39 ++++++++++++++++++++++++++++++++++++ tools/approval.py | 18 +++++++++++++++-- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 134e7c870..70057953d 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -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 diff --git a/tools/approval.py b/tools/approval.py index bc1a607a3..11c33f9f7 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -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*["\']?{_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