From bffc8d91bb0641709f57a64dfa9582549ef2f89f Mon Sep 17 00:00:00 2001 From: Arijit2916 <178023477+Samurai007AK@users.noreply.github.com> Date: Sat, 12 Sep 2026 13:10:13 +0530 Subject: [PATCH 1/4] fix(apodex): prevent saved Bash allow from bypassing typed confirm via substitution Root cause: PermissionStore._matches compared raw segments, ignoring unquoted command substitution, and assess_with_rules dropped the danger label on downgrade. Validation: repro script confirms git push --force and echo pip install payloads stay confirm with danger; 5 permission tests pass; ruff and pyright clean. Closes #39 --- apodex/agent_tools.py | 9 +++- apodex/permissions.py | 99 +++++++++++++++++++++++++++++++++-- apodex/tests/test_features.py | 38 ++++++++++++++ 3 files changed, 141 insertions(+), 5 deletions(-) diff --git a/apodex/agent_tools.py b/apodex/agent_tools.py index a6687a5..a959b3a 100644 --- a/apodex/agent_tools.py +++ b/apodex/agent_tools.py @@ -404,7 +404,9 @@ def assess_with_rules( 3. If ``auto_for_me`` is enabled (Docker / trusted env mode), any non-denied call is treated as safe. 4. If the user saved an explicit ``allow`` rule for this command/tool, downgrade - ``RISK_CONFIRM`` to ``RISK_SAFE``. + ``RISK_CONFIRM`` to ``RISK_SAFE`` — unless the call carries a ``danger`` + label (dep-install, force-push, delete, ...). A dangerous call never + downgrades: the typed-confirmation gate must still fire. """ base = assess_tool_risk(name, args, cwd) if rules is not None and rules.denies(name, args): @@ -413,6 +415,11 @@ def assess_with_rules( return base if auto_for_me: return ToolRisk(RISK_SAFE, "auto for me (docker/trusted env)", base.target) + if base.level == RISK_CONFIRM and base.danger: + # Saved allows only downgrade *plain* confirms. ``observers`` skips + # ``confirm()`` entirely when level is SAFE, so preserving ``danger`` + # on a SAFE result would still bypass the typed-yes gate. + return base if base.level == RISK_CONFIRM and rules is not None and rules.allows(name, args): return ToolRisk(RISK_SAFE, "allowed by a saved rule", base.target) return base diff --git a/apodex/permissions.py b/apodex/permissions.py index a593ab6..b106ab6 100644 --- a/apodex/permissions.py +++ b/apodex/permissions.py @@ -12,7 +12,10 @@ Safety contract: this store only ever *downgrades a plain confirm to safe*, or *forces a deny*. It is consulted in :func:`agent_tools.assess_tool_risk` AFTER danger detection and the hard denylist — so a saved ``Bash(git)`` allow can -never green-light a dangerous ``git push --force``. +never green-light a dangerous ``git push --force``. Unquoted ``$(...)`` and +backtick substitutions must be separately authorized against the same saved +prefixes, and a command carrying a ``danger`` label never downgrades (the +typed-confirmation gate still fires). """ from __future__ import annotations @@ -36,6 +39,84 @@ }) +def _nested_shell_snippets(cmd: str) -> list[str]: + """Shell-code strings nested in unquoted ``$(...)``/backticks. + + Reuses :func:`plugins.tools._bash_policy._extract_nested_shell` (stdlib-only, + no import cycle). Single-quoted spans are skipped — the shell does not expand + them, so ``echo '$(rm -rf /)'`` is a harmless literal. Falls back to a small + self-contained scanner when the import fails so matching never throws and + never silently allows. + """ + try: + from plugins.tools._bash_policy import ( # type: ignore + _extract_nested_shell as _extract, + ) + + return list(_extract(cmd or "")) + except Exception: + pass + out: list[str] = [] + s = cmd or "" + n = len(s) + i = 0 + sq = False + while i < n: + c = s[i] + if sq: + if c == "'": + sq = False + i += 1 + continue + if c == "'": + sq = True + i += 1 + continue + if c == "$" and i + 1 < n and s[i + 1] == "(": + depth, j = 1, i + 2 + start = j + while j < n and depth: + if s[j] == "(": + depth += 1 + elif s[j] == ")": + depth -= 1 + j += 1 + if depth == 0: + out.append(s[start : j - 1]) + i = j + continue + if c == "`": + j = i + 1 + while j < n and s[j] != "`": + j += 1 + out.append(s[i + 1 : j]) + i = j + 1 + continue + i += 1 + return out + + +def _nested_segments_authorized(nested: str, prefixes: set[str]) -> bool: + """True when every ``&&``/``|``/``;`` piece of a nested snippet matches. + + Each piece must itself satisfy the same ``seg == p or seg.startswith(p)`` + prefix rule, transitively (a nested snippet containing further substitution + must have that inner payload authorized too). Fail-closed: empty or + unmatched pieces return False. + """ + segs = [p.strip() for p in _SEGMENT_SPLIT.split(nested or "") if p.strip()] + if not segs: + return False + for seg in segs: + if not any(seg == p or seg.startswith(p + " ") for p in prefixes): + return False + # Transitive: ``echo $(foo $(bar))`` needs ``bar`` authorized as well. + for inner in _nested_shell_snippets(seg): + if not _nested_segments_authorized(inner, prefixes): + return False + return True + + def _extract_prefix_from_segment(seg: str) -> str: try: toks = shlex.split(seg) @@ -124,9 +205,19 @@ def _matches(rules: set[str], name: str, args: dict) -> bool: if _extract_prefix_from_segment(s).split()[0] not in _HELPER_CMDS ] check_segs = non_helpers if non_helpers else segs - return bool(check_segs) and all( - any(seg == p or seg.startswith(p + " ") for p in prefixes) for seg in check_segs - ) + if not check_segs: + return False + for seg in check_segs: + if not any(seg == p or seg.startswith(p + " ") for p in prefixes): + return False + # Nested-shell guard (issue #39): ``echo $(pip install x)`` is + # "just an echo" only on the raw string. Each unquoted nested + # payload must independently match a saved prefix, else the + # whole command is not authorized. + for nested in _nested_shell_snippets(seg): + if not _nested_segments_authorized(nested, prefixes): + return False + return True return False diff --git a/apodex/tests/test_features.py b/apodex/tests/test_features.py index 6615908..47b515d 100644 --- a/apodex/tests/test_features.py +++ b/apodex/tests/test_features.py @@ -1290,6 +1290,44 @@ def test_assess_with_rules_layering(tmp_path): assert r2.level == RISK_SAFE +def test_saved_allow_does_not_cover_substitution(tmp_path): + """Issue #39: ``Bash(echo)`` must not authorize ``echo $(pip install x)``.""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + + cwd = str(tmp_path) + rules = PermissionStore(allow={"Bash(echo)"}) + assert not rules.allows("bash", {"command": "echo $(pip install evil-pkg)"}) + assert not rules.allows("bash", {"command": "echo `pip install evil-pkg`"}) + r = assess_with_rules( + "bash", {"command": "echo $(pip install evil-pkg)"}, cwd, rules + ) + assert r.level == RISK_CONFIRM + assert r.danger == "installs dependencies" + + +def test_saved_allow_does_not_cover_force_push(tmp_path): + """Issue #39: ``Bash(git push)`` must not downgrade a force-push confirm.""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + + cwd = str(tmp_path) + rules = PermissionStore(allow={"Bash(git push)"}) + r = assess_with_rules( + "bash", {"command": "git push --force origin main"}, cwd, rules + ) + assert r.level == RISK_CONFIRM + assert r.danger == "git force-push" + + +def test_single_quoted_substitution_is_literal(tmp_path): + """Single-quoted ``$(...)`` is not expanded by the shell — still allowed.""" + from apodex.permissions import PermissionStore + + rules = PermissionStore(allow={"Bash(echo)"}) + assert rules.allows("bash", {"command": "echo '$(pip install x)'"}) + + def test_user_settings_save_and_load(tmp_path): from apodex.config import UserSettings p = str(tmp_path / "settings.json") From 743c26acef98b1888418ea7e9dd272459c0d76ed Mon Sep 17 00:00:00 2001 From: Arijit2916 <178023477+Samurai007AK@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:57:45 +0530 Subject: [PATCH 2/4] fix(apodex): check nested substitutions in deny rules, helper segments and double quotes Follow-up to the review on #42. - Allow and deny are now separate checks. An allow still needs every segment and every nested payload to match. A deny now fires when any segment matches, top-level or nested. With allow Bash(*) and deny Bash(echo), `echo $(touch x)` is denied again. - The allow check collects $(...) and backtick payloads from the whole command before the helper filter runs. With only Bash(python) allowed, `echo $(touch x) && python -V` now goes to confirm. - _extract_nested_shell and the fallback scanner in permissions.py track double quotes and backslash escapes. `echo "'$(touch x)'"` now yields `touch x`, while `echo '$(x)'` and `echo \$(x)` stay literal. A quoted ")" no longer ends a substitution early. The bash policy uses the same extractor, so enforce mode now denies `echo "'$(foobarcmd)'"` too. - A segment that is an empty word (`"" && python -V`) no longer raises IndexError. Tests: one regression test per case, plus a test that the two scanners return the same results. The new tests fail on the previous commit and pass on this one. The full pytest run has the same failing set before and after (Windows path and symlink tests). ruff is clean, and pyright reports 0 errors on the touched files. --- apodex/permissions.py | 202 ++++++++++++++++++++++------------ apodex/tests/test_features.py | 75 +++++++++++++ plugins/tools/_bash_policy.py | 74 +++++++++---- 3 files changed, 258 insertions(+), 93 deletions(-) diff --git a/apodex/permissions.py b/apodex/permissions.py index b106ab6..ee73644 100644 --- a/apodex/permissions.py +++ b/apodex/permissions.py @@ -5,17 +5,19 @@ ``npm test``", "never allow ``git push``" — matched by command prefix, so the gate stays livable without being all-or-nothing. -Rules are strings: ``Bash(npm test)`` / ``Bash(git push)`` for shell (matched by -prefix across every ``&&``/``|``/``;`` segment, fail-safe), or a bare tool name -(``write_file``) for everything else. +Rules are strings: ``Bash(npm test)`` / ``Bash(git push)`` for shell, or a bare +tool name (``write_file``) for everything else. Shell rules match by prefix per +``&&``/``|``/``;`` segment. An allow must cover every segment and a deny fires +on any one segment, so both fail safe. Safety contract: this store only ever *downgrades a plain confirm to safe*, or *forces a deny*. It is consulted in :func:`agent_tools.assess_tool_risk` AFTER danger detection and the hard denylist — so a saved ``Bash(git)`` allow can -never green-light a dangerous ``git push --force``. Unquoted ``$(...)`` and -backtick substitutions must be separately authorized against the same saved -prefixes, and a command carrying a ``danger`` label never downgrades (the -typed-confirmation gate still fires). +never green-light a dangerous ``git push --force``. Every ``$(...)`` or +backtick substitution the shell would run, unquoted or inside double quotes, +needs its own match against the saved allow prefixes. A deny prefix also fires +on a command nested inside one. A command carrying a ``danger`` label never +downgrades, so the typed-confirmation gate still fires. """ from __future__ import annotations @@ -40,13 +42,16 @@ def _nested_shell_snippets(cmd: str) -> list[str]: - """Shell-code strings nested in unquoted ``$(...)``/backticks. + """Shell-code strings nested in ``$(...)``/backticks the shell would run. Reuses :func:`plugins.tools._bash_policy._extract_nested_shell` (stdlib-only, - no import cycle). Single-quoted spans are skipped — the shell does not expand - them, so ``echo '$(rm -rf /)'`` is a harmless literal. Falls back to a small - self-contained scanner when the import fails so matching never throws and - never silently allows. + no import cycle). It reads quotes the way bash does. A single-quoted span is + skipped, so ``echo '$(rm -rf /)'`` is a harmless literal. A ``'`` inside + double quotes is an ordinary character, so ``echo "'$(rm -rf /)'"`` still + yields ``rm -rf /``. If the import fails it uses + :func:`_fallback_nested_shell`, so matching never throws and never silently + allows. ``test_nested_shell_extractors_agree`` checks the two give the same + results. """ try: from plugins.tools._bash_policy import ( # type: ignore @@ -55,40 +60,68 @@ def _nested_shell_snippets(cmd: str) -> list[str]: return list(_extract(cmd or "")) except Exception: - pass + return _fallback_nested_shell(cmd or "") + + +def _fallback_substitution_end(s: str, i: int) -> int: + """Index of the ``)`` closing a ``$(`` whose body starts at ``i`` (``len`` + when unterminated); quoted or escaped parens don't count.""" + depth, n = 1, len(s) + quote: str | None = None + while i < n: + c = s[i] + if quote == "'": + if c == "'": + quote = None + elif c == "\\": + i += 1 + elif c == '"': + quote = None if quote else '"' + elif quote is None: + if c == "'": + quote = "'" + elif c == "(": + depth += 1 + elif c == ")": + depth -= 1 + if depth == 0: + return i + i += 1 + return n + + +def _fallback_nested_shell(s: str) -> list[str]: + """Standalone copy of ``_extract_nested_shell`` for when its import fails. + + It tracks single quotes, double quotes and backslash escapes. An + unterminated substitution yields the rest of the string, which fails closed. + """ out: list[str] = [] - s = cmd or "" - n = len(s) - i = 0 - sq = False + i, n = 0, len(s) + quote: str | None = None while i < n: c = s[i] - if sq: + if quote == "'": if c == "'": - sq = False + quote = None i += 1 continue - if c == "'": - sq = True - i += 1 + if c == "\\": + i += 2 continue - if c == "$" and i + 1 < n and s[i + 1] == "(": - depth, j = 1, i + 2 - start = j - while j < n and depth: - if s[j] == "(": - depth += 1 - elif s[j] == ")": - depth -= 1 - j += 1 - if depth == 0: - out.append(s[start : j - 1]) - i = j + if c == "'" and quote is None: + quote = "'" + elif c == '"': + quote = None if quote else '"' + elif c == "$" and s.startswith("(", i + 1): + end = _fallback_substitution_end(s, i + 2) + out.append(s[i + 2 : end]) + i = end + 1 continue - if c == "`": + elif c == "`": j = i + 1 while j < n and s[j] != "`": - j += 1 + j += 2 if s[j] == "\\" else 1 out.append(s[i + 1 : j]) i = j + 1 continue @@ -104,11 +137,11 @@ def _nested_segments_authorized(nested: str, prefixes: set[str]) -> bool: must have that inner payload authorized too). Fail-closed: empty or unmatched pieces return False. """ - segs = [p.strip() for p in _SEGMENT_SPLIT.split(nested or "") if p.strip()] + segs = _segments(nested) if not segs: return False for seg in segs: - if not any(seg == p or seg.startswith(p + " ") for p in prefixes): + if not _seg_matches(seg, prefixes): return False # Transitive: ``echo $(foo $(bar))`` needs ``bar`` authorized as well. for inner in _nested_shell_snippets(seg): @@ -117,6 +150,27 @@ def _nested_segments_authorized(nested: str, prefixes: set[str]) -> bool: return True +def _segments(cmd: str) -> list[str]: + return [s.strip() for s in _SEGMENT_SPLIT.split(cmd or "") if s.strip()] + + +def _seg_matches(seg: str, prefixes: set[str]) -> bool: + return any(seg == p or seg.startswith(p + " ") for p in prefixes) + + +def _denied_anywhere(cmd: str, prefixes: set[str]) -> bool: + """True when any segment matches a deny prefix, at the top level or nested + in a substitution at any depth. + + This is the opposite of the allow check, which needs every segment to + match. A nested command that matches no rule never cancels a deny on its + parent, so ``Bash(echo)`` still denies ``echo $(touch x)``. + """ + if any(_seg_matches(seg, prefixes) for seg in _segments(cmd)): + return True + return any(_denied_anywhere(inner, prefixes) for inner in _nested_shell_snippets(cmd)) + + def _extract_prefix_from_segment(seg: str) -> str: try: toks = shlex.split(seg) @@ -176,10 +230,37 @@ def save(self) -> None: pass def allows(self, name: str, args: dict) -> bool: - return self._matches(self.allow, name, args) + """True only when every segment and every nested payload matches.""" + if _name_matches(self.allow, name): + return True + prefixes = _bash_rule_prefixes(self.allow) if name == "bash" else set() + if not prefixes: + return False + cmd = str(args.get("command", "")) + # Issue #39. On the raw string ``echo $(pip install x)`` looks like a + # plain echo, so every payload the shell would run has to match a saved + # prefix on its own. Payloads come from the whole command before the + # helper filter runs. Otherwise ``echo $(touch x) && python -V`` would + # pass, because the filter drops the ``echo`` segment. + for nested in _nested_shell_snippets(cmd): + if not _nested_segments_authorized(nested, prefixes): + return False + segs = _segments(cmd) + # Filter out helper segments (e.g. 'cd /foo') unless all segments are helpers + non_helpers = [ + s for s in segs + # ``or [""]`` stops an empty-word segment (``"" && ...``) raising IndexError. + if (_extract_prefix_from_segment(s).split() or [""])[0] not in _HELPER_CMDS + ] + check_segs = non_helpers if non_helpers else segs + return bool(check_segs) and all(_seg_matches(s, prefixes) for s in check_segs) def denies(self, name: str, args: dict) -> bool: - return self._matches(self.deny, name, args) + """True when any segment, top-level or nested, matches a deny rule.""" + if _name_matches(self.deny, name): + return True + prefixes = _bash_rule_prefixes(self.deny) if name == "bash" else set() + return bool(prefixes) and _denied_anywhere(str(args.get("command", "")), prefixes) def add_allow(self, name: str, args: dict) -> str: """Persist 'always allow' for this call; returns the rule added.""" @@ -188,37 +269,16 @@ def add_allow(self, name: str, args: dict) -> str: self.save() return rule - @staticmethod - def _matches(rules: set[str], name: str, args: dict) -> bool: - if name in rules: - return True - if name == "bash": - if "bash" in rules or "Bash" in rules or "Bash(*)" in rules: - return True - prefixes = {r[5:-1] for r in rules if r.startswith("Bash(") and r.endswith(")")} - if not prefixes: - return False - segs = [s.strip() for s in _SEGMENT_SPLIT.split(str(args.get("command", ""))) if s.strip()] - # Filter out helper segments (e.g. 'cd /foo') unless all segments are helpers - non_helpers = [ - s for s in segs - if _extract_prefix_from_segment(s).split()[0] not in _HELPER_CMDS - ] - check_segs = non_helpers if non_helpers else segs - if not check_segs: - return False - for seg in check_segs: - if not any(seg == p or seg.startswith(p + " ") for p in prefixes): - return False - # Nested-shell guard (issue #39): ``echo $(pip install x)`` is - # "just an echo" only on the raw string. Each unquoted nested - # payload must independently match a saved prefix, else the - # whole command is not authorized. - for nested in _nested_shell_snippets(seg): - if not _nested_segments_authorized(nested, prefixes): - return False - return True - return False + +def _name_matches(rules: set[str], name: str) -> bool: + """A bare tool-name rule, or a whole-tool bash rule (``bash``/``Bash(*)``).""" + return name in rules or ( + name == "bash" and ("Bash" in rules or "Bash(*)" in rules) + ) + + +def _bash_rule_prefixes(rules: set[str]) -> set[str]: + return {r[5:-1] for r in rules if r.startswith("Bash(") and r.endswith(")")} __all__ = ["PermissionStore", "rule_for"] diff --git a/apodex/tests/test_features.py b/apodex/tests/test_features.py index 47b515d..4bfb8a3 100644 --- a/apodex/tests/test_features.py +++ b/apodex/tests/test_features.py @@ -1326,6 +1326,81 @@ def test_single_quoted_substitution_is_literal(tmp_path): rules = PermissionStore(allow={"Bash(echo)"}) assert rules.allows("bash", {"command": "echo '$(pip install x)'"}) + assert rules.allows("bash", {"command": r"echo \$(pip install x)"}) # escaped, so literal + + +def test_deny_rule_still_matches_parent_of_substitution(tmp_path): + """A nested command that matches no rule must not cancel a deny (PR #42 review).""" + from apodex.agent_tools import RISK_DENY, assess_with_rules + from apodex.permissions import PermissionStore + + cwd = str(tmp_path) + rules = PermissionStore(allow={"Bash(*)"}, deny={"Bash(echo)"}) + cmd = {"command": "echo $(touch /tmp/marker)"} + assert rules.denies("bash", cmd) + assert assess_with_rules("bash", cmd, cwd, rules).level == RISK_DENY + # A deny prefix also fires on a command nested inside a substitution. + nested = PermissionStore(allow={"Bash(*)"}, deny={"Bash(touch)"}) + assert assess_with_rules("bash", cmd, cwd, nested).level == RISK_DENY + # It also fires on any one top-level segment, not only when all of them match. + assert PermissionStore(deny={"Bash(git push)"}).denies( + "bash", {"command": "git status && git push origin main"} + ) + + +def test_helper_segment_substitution_needs_authorization(tmp_path): + """The helper filter must not hide a payload inside an echo segment (PR #42 review).""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + + rules = PermissionStore(allow={"Bash(python)"}) + cmd = {"command": "echo $(touch /tmp/marker) && python -V"} + assert not rules.allows("bash", cmd) + assert assess_with_rules("bash", cmd, str(tmp_path), rules).level == RISK_CONFIRM + assert rules.allows("bash", {"command": "echo hi && python -V"}) # a plain helper is still skipped + assert not rules.allows("bash", {"command": '"" && python -V'}) # empty word returns False, no IndexError + + +def test_double_quoted_substitution_is_not_literal(tmp_path): + """A ``'`` inside ``"..."`` is an ordinary character, so ``$(...)`` still runs (PR #42 review).""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + from plugins.tools._bash_policy import assess_bash_command + + rules = PermissionStore(allow={"Bash(echo)"}) + for cmd in ( + "echo \"'$(touch /tmp/marker)'\"", + r"echo \' $(touch /tmp/marker) \'", # an escaped ' does not start a quoted span + r'''echo "a\"'$(touch /tmp/marker)'"''', # an escaped " does not end the string + 'echo $(echo ")"; touch /tmp/marker)', # a quoted ")" does not end the substitution + ): + assert not rules.allows("bash", {"command": cmd}), cmd + assert assess_with_rules("bash", {"command": cmd}, str(tmp_path), rules).level == RISK_CONFIRM + # The sandbox bash policy uses the same extractor. + assert assess_bash_command("echo \"'$(foobarcmd)'\"", mode="enforce").level == "deny" + + +def test_nested_shell_extractors_agree(): + """The fallback scanner in permissions.py must match the shared extractor.""" + from apodex.permissions import _fallback_nested_shell + from plugins.tools._bash_policy import _extract_nested_shell + + corpus = { + "echo $(a)": ["a"], + "echo `b`": ["b"], + "echo '$(c)'": [], + "echo \"'$(d)'\"": ["d"], + r"echo \$(e)": [], + r"echo \' $(f) \'": ["f"], + r'''echo "a\"'$(g)'"''': ["g"], + 'echo $(echo ")"; h)': ['echo ")"; h'], + "echo $(i $(j))": ["i $(j)"], + "echo $(unterminated": ["unterminated"], + r"echo \`k\`": [], + } + for cmd, want in corpus.items(): + assert _extract_nested_shell(cmd) == want, cmd + assert _fallback_nested_shell(cmd) == want, cmd def test_user_settings_save_and_load(tmp_path): diff --git a/plugins/tools/_bash_policy.py b/plugins/tools/_bash_policy.py index a01ade0..74698f8 100644 --- a/plugins/tools/_bash_policy.py +++ b/plugins/tools/_bash_policy.py @@ -798,41 +798,71 @@ def _split_top_level(command: str) -> list[str]: return [s.strip() for s in segs if s.strip()] +def _substitution_end(command: str, i: int) -> int: + """Index of the ``)`` closing a ``$(`` whose body starts at ``i``, or + ``len(command)`` when unterminated. Quoted or escaped parens don't count, + so ``$(echo ")"; rm x)`` closes at the last ``)``, not inside the quotes.""" + depth, n = 1, len(command) + quote: str | None = None + while i < n: + c = command[i] + if quote == "'": + if c == "'": + quote = None + elif c == "\\": + i += 1 + elif c == '"': + quote = None if quote else '"' + elif quote is None: + if c == "'": + quote = "'" + elif c == "(": + depth += 1 + elif c == ")": + depth -= 1 + if depth == 0: + return i + i += 1 + return n + + def _extract_nested_shell(command: str) -> list[str]: """Return shell-code strings nested in ``$(...)`` and backticks (which the - shell expands+executes). Single-quoted spans are skipped — the shell does - not expand them, so ``echo '$(rm -rf /)'`` is a harmless literal.""" + shell expands+executes). + + It reads quotes the way bash does. A single-quoted span is skipped, so + ``echo '$(rm -rf /)'`` is a harmless literal. Inside double quotes a ``'`` + is an ordinary character and substitution still runs, so + ``echo "'$(rm -rf /)'"`` yields ``rm -rf /``. A backslash outside single + quotes escapes the next character (``\\$(...)``, ``\\'``, ``\\"``). An + unterminated substitution yields the rest of the string, which fails closed. + """ out: list[str] = [] i, n = 0, len(command) - sq = False + quote: str | None = None while i < n: c = command[i] - if sq: + if quote == "'": if c == "'": - sq = False + quote = None i += 1 continue - if c == "'": - sq = True - i += 1 + if c == "\\": + i += 2 continue - if c == "$" and i + 1 < n and command[i + 1] == "(": - depth, j = 1, i + 2 - start = j - while j < n and depth: - if command[j] == "(": - depth += 1 - elif command[j] == ")": - depth -= 1 - j += 1 - if depth == 0: - out.append(command[start:j - 1]) - i = j + if c == "'" and quote is None: + quote = "'" + elif c == '"': + quote = None if quote else '"' + elif c == "$" and command.startswith("(", i + 1): + end = _substitution_end(command, i + 2) + out.append(command[i + 2:end]) + i = end + 1 continue - if c == "`": + elif c == "`": j = i + 1 while j < n and command[j] != "`": - j += 1 + j += 2 if command[j] == "\\" else 1 out.append(command[i + 1:j]) i = j + 1 continue From c2fd303f9fcca581ab6fcbe85e9ea4cd776572db Mon Sep 17 00:00:00 2001 From: Arijit2916 <178023477+Samurai007AK@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:04:08 +0530 Subject: [PATCH 3/4] fix(apodex): give each nested substitution its own quote state Follow-up to the second review. - _substitution_end shared one quote state across nested $(...), so the quotes in `echo $(echo "$(echo ")'")" $(touch x))` paired across levels and the scan stopped before `touch x`. Each nested $( now starts with its own quote state, kept on a list instead of the call stack. The fallback scanner in permissions.py has the same change. - The allow check split a nested snippet on ;/&&/| before reading its inner payloads. The split ignores quotes, so a quoted ";" could cut a string in half and hide $(...) in what then looked like a single-quoted span. Inner payloads now come from the whole snippet. - Allow and deny stop after 16 levels of nesting. Past that, allow returns False and deny returns True. Before, very deep nesting raised RecursionError. - Segments are also split on a single & and on newlines, without touching the & inside 2>&1, >&2 or &>file. `echo hi & touch x` no longer passes Bash(echo). - <(...) and >(...) count as nested commands, like $(...). - bash expands the target of >&word a second time after quote removal, so `echo x >&'$(touch m)'` runs touch. Both scanners now read that word with its quotes removed. The bash policy uses the same extractor, so enforce mode now denies `echo x >&'$(foobarcmd)'`. Tests: regression tests for each case, more cases in the scanner parity test, and a deep-nesting test. The new tests fail on the previous commit and pass on this one. A differential fuzz run against real bash (mutating tricky quoting and substitution commands, running every command the matcher allows under Bash(echo), and checking whether `touch` ran) finds 98 bypasses on the previous commit and none on this one. The full pytest failing set matches the previous commit apart from a timing-flaky TUI clipboard test that also fails there. ruff is clean, and pyright reports 0 errors on the touched files. --- apodex/permissions.py | 130 ++++++++++++++++++++++++++-------- apodex/tests/test_features.py | 62 ++++++++++++++++ plugins/tools/_bash_policy.py | 86 ++++++++++++++++++---- 3 files changed, 235 insertions(+), 43 deletions(-) diff --git a/apodex/permissions.py b/apodex/permissions.py index ee73644..d362f45 100644 --- a/apodex/permissions.py +++ b/apodex/permissions.py @@ -7,14 +7,15 @@ Rules are strings: ``Bash(npm test)`` / ``Bash(git push)`` for shell, or a bare tool name (``write_file``) for everything else. Shell rules match by prefix per -``&&``/``|``/``;`` segment. An allow must cover every segment and a deny fires -on any one segment, so both fail safe. +segment, where segments are split on ``&&``, ``||``, ``|``, ``;``, ``&`` and +newlines. An allow must cover every segment and a deny fires on any one +segment, so both fail safe. Safety contract: this store only ever *downgrades a plain confirm to safe*, or *forces a deny*. It is consulted in :func:`agent_tools.assess_tool_risk` AFTER danger detection and the hard denylist — so a saved ``Bash(git)`` allow can -never green-light a dangerous ``git push --force``. Every ``$(...)`` or -backtick substitution the shell would run, unquoted or inside double quotes, +never green-light a dangerous ``git push --force``. Every command the shell +would run from a ``$(...)``, backtick or ``<(...)``/``>(...)`` substitution needs its own match against the saved allow prefixes. A deny prefix also fires on a command nested inside one. A command carrying a ``danger`` label never downgrades, so the typed-confirmation gate still fires. @@ -35,14 +36,22 @@ "git", "npm", "pnpm", "yarn", "uv", "pip", "pip3", "cargo", "go", "docker", "poetry", "conda", "make", "apt", "apt-get", "brew", "kubectl", "gh", }) -_SEGMENT_SPLIT = re.compile(r"&&|\|\||\||;") +# Command separators: ``&&`` ``||`` ``|`` ``;``, a newline, and a single ``&`` +# (background). The ``&`` inside a redirection (``2>&1``, ``>&2``, ``&>log``) +# is not a separator, and the lookarounds skip it. +_SEGMENT_SPLIT = re.compile(r"&&|\|\||\||;|\n|(?])&(?![>&])") +# How many levels of nested ``$(...)`` the matcher follows. Real commands use +# one or two. Past this, allow fails closed and deny fires, instead of +# recursing until Python raises RecursionError. +_MAX_NEST_DEPTH = 16 _HELPER_CMDS = frozenset({ "cd", "pwd", "export", "set", "env", "echo", "mkdir", "clear", "true", "source", ".", }) def _nested_shell_snippets(cmd: str) -> list[str]: - """Shell-code strings nested in ``$(...)``/backticks the shell would run. + """Shell-code strings nested in ``$(...)``, backticks, ``<(...)`` and + ``>(...)``, which the shell runs as separate commands. Reuses :func:`plugins.tools._bash_policy._extract_nested_shell` (stdlib-only, no import cycle). It reads quotes the way bash does. A single-quoted span is @@ -65,8 +74,52 @@ def _nested_shell_snippets(cmd: str) -> list[str]: def _fallback_substitution_end(s: str, i: int) -> int: """Index of the ``)`` closing a ``$(`` whose body starts at ``i`` (``len`` - when unterminated); quoted or escaped parens don't count.""" - depth, n = 1, len(s) + when unterminated). Quoted or escaped parens don't count, and every nested + ``$(`` starts with its own quote state, as in bash.""" + n = len(s) + quotes: list[str | None] = [None] # quote state of each open $( level + depths = [1] # unquoted "(" nesting inside each open $( level + while i < n: + c = s[i] + quote = quotes[-1] + if quote == "'": + if c == "'": + quotes[-1] = None + elif c == "\\": + i += 1 + elif s.startswith("(", i + 1) and (c == "$" or (c in "<>" and quote is None)): + quotes.append(None) + depths.append(1) + i += 1 + elif c == "`": + i += 1 + while i < n and s[i] != "`": + i += 2 if s[i] == "\\" else 1 + elif c == '"': + quotes[-1] = None if quote else '"' + elif quote is None: + if c == "'": + quotes[-1] = "'" + elif c == "(": + depths[-1] += 1 + elif c == ")": + depths[-1] -= 1 + if depths[-1] == 0: + if len(depths) == 1: + return i + depths.pop() + quotes.pop() + i += 1 + return n + + +def _fallback_dup_redirect_word(s: str, i: int) -> str: + """Target word of a ``>&`` redirect starting at ``i``, quotes and + backslashes removed. bash expands it a second time after quote removal.""" + n = len(s) + while i < n and s[i] in " \t": + i += 1 + start = i quote: str | None = None while i < n: c = s[i] @@ -75,19 +128,21 @@ def _fallback_substitution_end(s: str, i: int) -> int: quote = None elif c == "\\": i += 1 + elif s.startswith("$(", i): + i = _fallback_substitution_end(s, i + 2) + elif c == "`": + i += 1 + while i < n and s[i] != "`": + i += 2 if s[i] == "\\" else 1 elif c == '"': quote = None if quote else '"' elif quote is None: if c == "'": quote = "'" - elif c == "(": - depth += 1 - elif c == ")": - depth -= 1 - if depth == 0: - return i + elif c.isspace() or c in ";&|<>()": + break i += 1 - return n + return re.sub(r"[\\'\"]", "", s[start:i]) def _fallback_nested_shell(s: str) -> list[str]: @@ -95,6 +150,8 @@ def _fallback_nested_shell(s: str) -> list[str]: It tracks single quotes, double quotes and backslash escapes. An unterminated substitution yields the rest of the string, which fails closed. + The target of ``>&`` is read with its quotes removed, because bash expands + it twice. """ out: list[str] = [] i, n = 0, len(s) @@ -113,7 +170,7 @@ def _fallback_nested_shell(s: str) -> list[str]: quote = "'" elif c == '"': quote = None if quote else '"' - elif c == "$" and s.startswith("(", i + 1): + elif s.startswith("(", i + 1) and (c == "$" or (c in "<>" and quote is None)): end = _fallback_substitution_end(s, i + 2) out.append(s[i + 2 : end]) i = end + 1 @@ -125,29 +182,34 @@ def _fallback_nested_shell(s: str) -> list[str]: out.append(s[i + 1 : j]) i = j + 1 continue + elif c == ">" and quote is None and s.startswith("&", i + 1): + out.extend(_fallback_nested_shell(_fallback_dup_redirect_word(s, i + 2))) i += 1 return out -def _nested_segments_authorized(nested: str, prefixes: set[str]) -> bool: - """True when every ``&&``/``|``/``;`` piece of a nested snippet matches. +def _nested_segments_authorized(nested: str, prefixes: set[str], depth: int = 0) -> bool: + """True when every segment of a nested snippet matches. Each piece must itself satisfy the same ``seg == p or seg.startswith(p)`` prefix rule, transitively (a nested snippet containing further substitution must have that inner payload authorized too). Fail-closed: empty or - unmatched pieces return False. + unmatched pieces return False, and so does nesting deeper than + ``_MAX_NEST_DEPTH``. """ + if depth >= _MAX_NEST_DEPTH: + return False segs = _segments(nested) - if not segs: + if not segs or not all(_seg_matches(seg, prefixes) for seg in segs): return False - for seg in segs: - if not _seg_matches(seg, prefixes): - return False - # Transitive: ``echo $(foo $(bar))`` needs ``bar`` authorized as well. - for inner in _nested_shell_snippets(seg): - if not _nested_segments_authorized(inner, prefixes): - return False - return True + # Transitive: ``echo $(foo $(bar))`` needs ``bar`` authorized as well. The + # inner payloads come from the whole snippet, not from each piece. The + # split ignores quotes, so a quoted ``;`` would cut a string in half and + # could hide ``$(...)`` inside what then looks like a single-quoted span. + return all( + _nested_segments_authorized(inner, prefixes, depth + 1) + for inner in _nested_shell_snippets(nested) + ) def _segments(cmd: str) -> list[str]: @@ -158,17 +220,23 @@ def _seg_matches(seg: str, prefixes: set[str]) -> bool: return any(seg == p or seg.startswith(p + " ") for p in prefixes) -def _denied_anywhere(cmd: str, prefixes: set[str]) -> bool: +def _denied_anywhere(cmd: str, prefixes: set[str], depth: int = 0) -> bool: """True when any segment matches a deny prefix, at the top level or nested in a substitution at any depth. This is the opposite of the allow check, which needs every segment to match. A nested command that matches no rule never cancels a deny on its - parent, so ``Bash(echo)`` still denies ``echo $(touch x)``. + parent, so ``Bash(echo)`` still denies ``echo $(touch x)``. Nesting deeper + than ``_MAX_NEST_DEPTH`` counts as denied, because the rest can't be checked. """ + if depth >= _MAX_NEST_DEPTH: + return True if any(_seg_matches(seg, prefixes) for seg in _segments(cmd)): return True - return any(_denied_anywhere(inner, prefixes) for inner in _nested_shell_snippets(cmd)) + return any( + _denied_anywhere(inner, prefixes, depth + 1) + for inner in _nested_shell_snippets(cmd) + ) def _extract_prefix_from_segment(seg: str) -> str: diff --git a/apodex/tests/test_features.py b/apodex/tests/test_features.py index 4bfb8a3..bdaaf4c 100644 --- a/apodex/tests/test_features.py +++ b/apodex/tests/test_features.py @@ -1397,12 +1397,74 @@ def test_nested_shell_extractors_agree(): "echo $(i $(j))": ["i $(j)"], "echo $(unterminated": ["unterminated"], r"echo \`k\`": [], + """echo $(echo "$(echo ")'")" $(l))""": ["""echo "$(echo ")'")" $(l)"""], + "cat <(m) >(n)": ["m", "n"], + 'echo "<(o)"': [], + "echo x >&'$(p)'": ["p"], + "echo x > '$(q)'": [], + "python x.py 2>&1": [], } for cmd, want in corpus.items(): assert _extract_nested_shell(cmd) == want, cmd assert _fallback_nested_shell(cmd) == want, cmd +def test_nested_quotes_do_not_end_the_outer_substitution(tmp_path): + """Each nested ``$(`` has its own quote state (PR #42 second review).""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + + rules = PermissionStore(allow={"Bash(echo)"}) + for cmd in ( + """echo $(echo "$(echo ")'")" $(touch /tmp/marker))""", + # A quoted ";" must not split the snippet before its payloads are read. + """echo $(echo "a;echo '" $(touch /tmp/marker) "'")""", + ): + assert not rules.allows("bash", {"command": cmd}), cmd + assert assess_with_rules("bash", {"command": cmd}, str(tmp_path), rules).level == RISK_CONFIRM + assert rules.allows("bash", {"command": """echo $(echo "$(echo ")'")")"""}) + + +def test_separators_and_process_substitution_need_authorization(): + """``&``, newlines and ``<(...)``/``>(...)`` all run another command.""" + from apodex.permissions import PermissionStore + + rules = PermissionStore(allow={"Bash(echo)", "Bash(python)", "Bash(cat)"}) + for cmd in ("echo hi & touch /tmp/marker", "echo hi\ntouch /tmp/marker", + "cat <(touch /tmp/marker)", "echo >(touch /tmp/marker)"): + assert not rules.allows("bash", {"command": cmd}), cmd + # The "&" in a redirection is not a separator. + for cmd in ("python x.py 2>&1", "python x.py >&2", "python x.py &> out.log", + "python x.py &", "cat <(echo a)", 'echo "<(touch /tmp/marker)"'): + assert rules.allows("bash", {"command": cmd}), cmd + assert PermissionStore(deny={"Bash(touch)"}).denies( + "bash", {"command": "echo hi & touch /tmp/marker"} + ) + + +def test_dup_redirect_target_is_expanded_twice(tmp_path): + """bash expands a ``>&word`` target again after quote removal, so + ``echo x >&'$(touch m)'`` runs ``touch`` despite the single quotes.""" + from apodex.agent_tools import RISK_CONFIRM, assess_with_rules + from apodex.permissions import PermissionStore + from plugins.tools._bash_policy import assess_bash_command + + rules = PermissionStore(allow={"Bash(echo)"}) + cmd = {"command": "echo x >&'$(touch /tmp/marker)'"} + assert not rules.allows("bash", cmd) + assert assess_with_rules("bash", cmd, str(tmp_path), rules).level == RISK_CONFIRM + assert rules.allows("bash", {"command": "echo x > '$(touch /tmp/marker)'"}) # plain > is literal + assert assess_bash_command("echo x >&'$(foobarcmd)'", mode="enforce").level == "deny" + + +def test_deep_nesting_fails_closed_without_recursion_error(): + from apodex.permissions import PermissionStore + + cmd = {"command": "echo " + "$(echo " * 2000 + "x" + ")" * 2000} + assert not PermissionStore(allow={"Bash(echo)"}).allows("bash", cmd) + assert PermissionStore(allow={"Bash(*)"}, deny={"Bash(rm)"}).denies("bash", cmd) + + def test_user_settings_save_and_load(tmp_path): from apodex.config import UserSettings p = str(tmp_path / "settings.json") diff --git a/plugins/tools/_bash_policy.py b/plugins/tools/_bash_policy.py index 74698f8..7ea70ca 100644 --- a/plugins/tools/_bash_policy.py +++ b/plugins/tools/_bash_policy.py @@ -801,8 +801,64 @@ def _split_top_level(command: str) -> list[str]: def _substitution_end(command: str, i: int) -> int: """Index of the ``)`` closing a ``$(`` whose body starts at ``i``, or ``len(command)`` when unterminated. Quoted or escaped parens don't count, - so ``$(echo ")"; rm x)`` closes at the last ``)``, not inside the quotes.""" - depth, n = 1, len(command) + so ``$(echo ")"; rm x)`` closes at the last ``)``, not inside the quotes. + + Like bash, every nested ``$(`` starts with its own quote state, so the + quotes in ``$(echo "$(echo ")'")" $(rm x))`` pair up inside the inner + substitution and the scan still reaches ``rm x``. The scan keeps the levels + on a list instead of the call stack, so deep nesting can't hit the + recursion limit. It skips a backtick span whole. + """ + n = len(command) + quotes: list[str | None] = [None] # quote state of each open $( level + depths = [1] # unquoted "(" nesting inside each open $( level + while i < n: + c = command[i] + quote = quotes[-1] + if quote == "'": + if c == "'": + quotes[-1] = None + elif c == "\\": + i += 1 + elif command.startswith("(", i + 1) and (c == "$" or (c in "<>" and quote is None)): + quotes.append(None) + depths.append(1) + i += 1 + elif c == "`": + i += 1 + while i < n and command[i] != "`": + i += 2 if command[i] == "\\" else 1 + elif c == '"': + quotes[-1] = None if quote else '"' + elif quote is None: + if c == "'": + quotes[-1] = "'" + elif c == "(": + depths[-1] += 1 + elif c == ")": + depths[-1] -= 1 + if depths[-1] == 0: + if len(depths) == 1: + return i + depths.pop() + quotes.pop() + i += 1 + return n + + +def _dup_redirect_word(command: str, i: int) -> str: + """The target word of a ``>&`` redirect that starts at ``i``, with its + quotes and backslashes removed. + + bash expands that word a second time after quote removal, so + ``echo x >&'$(id)'`` runs ``id``. The stripped text is roughly what the + second pass sees. Dropping every backslash can only expose more ``$(``, + never hide one. + """ + n = len(command) + while i < n and command[i] in " \t": + i += 1 + start = i quote: str | None = None while i < n: c = command[i] @@ -811,24 +867,26 @@ def _substitution_end(command: str, i: int) -> int: quote = None elif c == "\\": i += 1 + elif command.startswith("$(", i): + i = _substitution_end(command, i + 2) + elif c == "`": + i += 1 + while i < n and command[i] != "`": + i += 2 if command[i] == "\\" else 1 elif c == '"': quote = None if quote else '"' elif quote is None: if c == "'": quote = "'" - elif c == "(": - depth += 1 - elif c == ")": - depth -= 1 - if depth == 0: - return i + elif c.isspace() or c in ";&|<>()": + break i += 1 - return n + return re.sub(r"[\\'\"]", "", command[start:i]) def _extract_nested_shell(command: str) -> list[str]: - """Return shell-code strings nested in ``$(...)`` and backticks (which the - shell expands+executes). + """Return shell-code strings nested in ``$(...)``, backticks and unquoted + process substitution ``<(...)``/``>(...)`` (which the shell executes). It reads quotes the way bash does. A single-quoted span is skipped, so ``echo '$(rm -rf /)'`` is a harmless literal. Inside double quotes a ``'`` @@ -836,6 +894,8 @@ def _extract_nested_shell(command: str) -> list[str]: ``echo "'$(rm -rf /)'"`` yields ``rm -rf /``. A backslash outside single quotes escapes the next character (``\\$(...)``, ``\\'``, ``\\"``). An unterminated substitution yields the rest of the string, which fails closed. + The one exception to quoting is the target of ``>&``, which bash expands + twice (see :func:`_dup_redirect_word`). """ out: list[str] = [] i, n = 0, len(command) @@ -854,7 +914,7 @@ def _extract_nested_shell(command: str) -> list[str]: quote = "'" elif c == '"': quote = None if quote else '"' - elif c == "$" and command.startswith("(", i + 1): + elif command.startswith("(", i + 1) and (c == "$" or (c in "<>" and quote is None)): end = _substitution_end(command, i + 2) out.append(command[i + 2:end]) i = end + 1 @@ -866,6 +926,8 @@ def _extract_nested_shell(command: str) -> list[str]: out.append(command[i + 1:j]) i = j + 1 continue + elif c == ">" and quote is None and command.startswith("&", i + 1): + out.extend(_extract_nested_shell(_dup_redirect_word(command, i + 2))) i += 1 return out From eac35181496bbe6819c5474c72803e2bb995f498 Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Fri, 2 Oct 2026 17:22:48 +0800 Subject: [PATCH 4/4] fix(apodex): decode backtick escapes before permission checks --- apodex/permissions.py | 29 ++++++- plugins/tools/_bash_policy.py | 29 ++++++- tests/test_permissions_shell_expansion.py | 100 ++++++++++++++++++++++ 3 files changed, 150 insertions(+), 8 deletions(-) create mode 100644 tests/test_permissions_shell_expansion.py diff --git a/apodex/permissions.py b/apodex/permissions.py index d362f45..7168878 100644 --- a/apodex/permissions.py +++ b/apodex/permissions.py @@ -72,6 +72,29 @@ def _nested_shell_snippets(cmd: str) -> list[str]: return _fallback_nested_shell(cmd or "") +def _fallback_backtick_body(command: str, i: int) -> tuple[str, int]: + """Read a backtick body and apply bash's first-pass escape removal. + + Inside backticks, backslashes before $, `, a backslash or a newline are removed + before the body is parsed as shell code. Other backslashes are retained. + Return the decoded body and the closing backtick's index (len if absent). + """ + body: list[str] = [] + n = len(command) + while i < n: + c = command[i] + if c == "`": + break + if c == "\\" and i + 1 < n and command[i + 1] in "$`\\\n": + i += 1 + if command[i] != "\n": + body.append(command[i]) + else: + body.append(c) + i += 1 + return "".join(body), i + + def _fallback_substitution_end(s: str, i: int) -> int: """Index of the ``)`` closing a ``$(`` whose body starts at ``i`` (``len`` when unterminated). Quoted or escaped parens don't count, and every nested @@ -176,10 +199,8 @@ def _fallback_nested_shell(s: str) -> list[str]: i = end + 1 continue elif c == "`": - j = i + 1 - while j < n and s[j] != "`": - j += 2 if s[j] == "\\" else 1 - out.append(s[i + 1 : j]) + body, j = _fallback_backtick_body(s, i + 1) + out.append(body) i = j + 1 continue elif c == ">" and quote is None and s.startswith("&", i + 1): diff --git a/plugins/tools/_bash_policy.py b/plugins/tools/_bash_policy.py index 7ea70ca..a15c7f1 100644 --- a/plugins/tools/_bash_policy.py +++ b/plugins/tools/_bash_policy.py @@ -798,6 +798,29 @@ def _split_top_level(command: str) -> list[str]: return [s.strip() for s in segs if s.strip()] +def _backtick_body(command: str, i: int) -> tuple[str, int]: + """Read a backtick body and apply bash's first-pass escape removal. + + Inside backticks, backslashes before $, `, a backslash or a newline are removed + before the body is parsed as shell code. Other backslashes are retained. + Return the decoded body and the closing backtick's index (len if absent). + """ + body: list[str] = [] + n = len(command) + while i < n: + c = command[i] + if c == "`": + break + if c == "\\" and i + 1 < n and command[i + 1] in "$`\\\n": + i += 1 + if command[i] != "\n": + body.append(command[i]) + else: + body.append(c) + i += 1 + return "".join(body), i + + def _substitution_end(command: str, i: int) -> int: """Index of the ``)`` closing a ``$(`` whose body starts at ``i``, or ``len(command)`` when unterminated. Quoted or escaped parens don't count, @@ -920,10 +943,8 @@ def _extract_nested_shell(command: str) -> list[str]: i = end + 1 continue elif c == "`": - j = i + 1 - while j < n and command[j] != "`": - j += 2 if command[j] == "\\" else 1 - out.append(command[i + 1:j]) + body, j = _backtick_body(command, i + 1) + out.append(body) i = j + 1 continue elif c == ">" and quote is None and command.startswith("&", i + 1): diff --git a/tests/test_permissions_shell_expansion.py b/tests/test_permissions_shell_expansion.py new file mode 100644 index 0000000..4d4cdf7 --- /dev/null +++ b/tests/test_permissions_shell_expansion.py @@ -0,0 +1,100 @@ +"""Compare saved shell permissions with actual bash expansion semantics.""" + +from __future__ import annotations + +import itertools +import shlex +import shutil +import subprocess + +import pytest + +from apodex.agent_tools import RISK_CONFIRM, RISK_DENY, assess_with_rules +from apodex.permissions import PermissionStore, _fallback_nested_shell +from plugins.tools._bash_policy import _extract_nested_shell, assess_bash_command + + +def _backtick(code: str) -> str: + """Encode a shell body for one legacy command-substitution level.""" + escaped = "".join("\\" + c if c in "\\$`" else c for c in code) + return "`" + escaped + "`" + + +@pytest.mark.parametrize("body,expected", [ + (r"echo \$(touch marker)", "echo $(touch marker)"), + (r"echo \`touch marker\`", "echo `touch marker`"), + (r"echo \\$(touch marker)", r"echo \$(touch marker)"), + (r"echo \q", r"echo \q"), + ("echo to\\\nuch", "echo touch"), +]) +def test_backtick_scanners_apply_bash_escape_removal(body, expected): + command = "echo `" + body + "`" + assert _extract_nested_shell(command) == [expected] + assert _fallback_nested_shell(command) == [expected] + + +@pytest.mark.parametrize("command", [ + r"echo `echo \$(foobarcmd)`", + r"echo `echo \`foobarcmd\``", + 'echo "`echo \\$(foobarcmd)`"', +]) +def test_backtick_escape_removal_reaches_enforced_policy(command): + assert assess_bash_command(command, mode="enforce").level == "deny" + + +def test_saved_permissions_match_real_bash_nested_expansions(tmp_path): + """Every generated command actually runs touch; none may inherit echo's allow. + + Mix both substitution syntaxes and double-quoted variants at each level. + Execute only harmless commands that write one marker in this test's folder. + """ + bash = shutil.which("bash") + if bash is None: + pytest.skip("bash is needed for the shell-semantics comparison") + marker = tmp_path / "marker" + marker_word = shlex.quote(marker.as_posix()) + allow = PermissionStore(allow={"Bash(echo)"}) + deny = PermissionStore(allow={"Bash(*)"}, deny={"Bash(touch)"}) + wrappers = ( + lambda code: "echo $(" + code + ")", + lambda code: 'echo "$(' + code + ')"', + lambda code: "echo " + _backtick(code), + lambda code: 'echo "' + _backtick(code) + '"', + ) + commands = [] + for depth in range(1, 4): + for sequence in itertools.product(wrappers, repeat=depth): + command = "touch " + marker_word + for wrap in sequence: + command = wrap(command) + commands.append(command) + # The concrete review examples also include a second evaluation level. + commands.extend([ + r"echo `echo \$(touch " + marker_word + r")`", + r"echo `echo \`touch " + marker_word + r"\``", + 'echo $(echo "$(echo ")\'")" $(touch ' + marker_word + '))', + ]) + for command in commands: + result = subprocess.run( + [bash, "--noprofile", "--norc", "-c", command], + cwd=tmp_path, capture_output=True, text=True, timeout=5, + ) + assert result.returncode == 0, (command, result.stderr) + assert marker.exists(), command + marker.unlink() + args = {"command": command} + assert assess_with_rules("bash", args, str(tmp_path), allow).level == RISK_CONFIRM, command + assert assess_with_rules("bash", args, str(tmp_path), deny).level == RISK_DENY, command + assert _extract_nested_shell(command) == _fallback_nested_shell(command), command + + +@pytest.mark.parametrize("command", [ + "echo '$(touch marker)'", + "echo '`touch marker`'", + r"echo \`touch marker\`", + r"echo `echo '\$(touch marker)'`", + "echo $(echo literal)", + "echo `echo literal`", +]) +def test_literal_and_authorized_backtick_payloads_stay_allowed(command): + assert PermissionStore(allow={"Bash(echo)"}).allows("bash", {"command": command})