fix(apodex): prevent saved Bash allow from bypassing typed confirm via substitution

[Samurai007AK]

Summary

• Hardens PermissionStore._matches against unquoted $(...)/backtick substitution: nested payloads must independently match a saved Bash(...) prefix, otherwise the command is not authorized.
• Stops assess_with_rules from downgrading CONFIRM to SAFE when the call carries a danger label, so dep-installs and force-pushes still require typed yes.
• Adds regression tests for echo $(pip install x), backticks, git push --force, and single-quoted literals.

Validation

• Repro script from Saved Bash allow-rule bypass via command substitution (docstring safety contract disproven) #39: git push --force stays confirm with danger='git force-push'; echo $(pip install evil-pkg) stays confirm with danger='installs dependencies'; single-quoted stays allowed.
• uv run pytest apodex/tests/test_features.py -k "saved_allow or single_quoted or permission_store or assess_with_rules": 5 passed.
• Full test_features.py: 84 passed, 4 failed — identical 4 fail on unmodified main (Windows path expectations), no new failures.
• uv run ruff check apodex/permissions.py apodex/agent_tools.py apodex/tests/test_features.py: passes.
• uv run pyright apodex/permissions.py apodex/agent_tools.py: 0 errors.
• uv run python tools/import_smoke.py --stage 1: 288/289 (only fcntl missing on Windows, pre-existing).

Root cause

1. _matches compared raw &&/|/; segments by prefix, never unwrapping substitution, so echo $(pip install x) looked like "just an echo".
2. assess_with_rules returned SAFE with the danger label dropped. Since observers.py skips confirm() for SAFE, the typed gate never fired. The hard denylist did not help because both repros are confirm, not deny.

Closes #39

[Samurai007AK]

Hey @dq-ai-dev just put this one up, would love your eyes on it when you get a sec!

This was a sneaky one: a saved Bash(echo) style allow was quietly green-lighting stuff like echo $(pip install evil-pkg) and even git push --force, which is exactly what the permissions docstring said could never happen. Turns out the prefix check never looked inside $(...)/backticks, and the danger label got dropped on the downgrade so the typed-yes gate never fired.

I tightened both layers nested shell now needs its own allow, and anything with a danger tag stays at confirm plus added regression tests next to the existing permission tests. ruff/pyright are clean and the repros from the issue now stay at confirm as they should.

Happy to tweak the approach if you'd rather handle the nested case differently!

[zhanghanduo]

Thanks for putting this together and adding regression tests! The confirmation-gate change looks good. While checking the substitution handling, I found a few edge cases that would be helpful to cover before merging.

apodex/permissions.py:217–219

Since _matches is used for both allow and deny rules, this check seems to weaken existing deny rules as well. With allow Bash(*) and deny Bash(echo), echo $(touch /tmp/marker) changes from deny on the parent commit to safe here, because the nested touch doesn't match the deny prefix. Could we separate the allow-authorization check from deny matching and add a regression test for this combination?

apodex/permissions.py:210

It looks like substitutions in helper segments can still slip through, since those segments are filtered out before this loop. For example, with only Bash(python) allowed, echo $(touch /tmp/marker) && python -V still returns safe. Could we check nested payloads in all original segments before applying the helper filter?

apodex/permissions.py:56

There’s also a quoting edge case in the extractor: a single quote inside double quotes is treated as starting a literal span, but Bash still expands substitutions there. With Bash(echo) allowed, echo "'$(touch /tmp/marker)'" returns safe even though the nested command executes. Could we account for double-quote and escape state in both the shared extractor and fallback scanner, with a regression test for this case?

[zhanghanduo]

Thanks for following up and adding tests for all three cases! I verified that the original examples are now handled correctly.
I found one more edge case in the updated substitution scanner. With only Bash(echo) allowed, echo $(echo "$(echo ")'")" $(touch /tmp/marker)) now returns safe, whereas the previous revision returned confirm. Bash does execute the touch; I verified this using a temporary file.

It looks like _substitution_end shares one quote state across nested substitutions, so the inner quotes cause it to stop early and miss the remaining payload. Could we track quote state separately for each nested substitution and add this case to the regression tests? The fallback scanner appears to have the same issue.

[Samurai007AK]

Good catch @zhanghanduo, and thanks for checking it against a real file. You were right: _substitution_end used one quote state for every level, so the quotes in the inner $(...) paired up with the outer ones and the scan stopped before the touch.

I pushed a follow-up commit. Each nested $( now starts with its own quote state, in both the shared extractor and the fallback scanner, and your example is in the regression tests.

While testing that, I wrote a small fuzzer that mutates tricky quoting and substitution commands, runs every one the matcher allows under Bash(echo) in real bash, and checks whether touch ran. On the previous commit it found 98 bypasses. It also turned up a few more holes, which I fixed in the same commit:

• The nested check split a snippet on ; before reading its inner payloads, so a quoted ; could hide one: echo $(echo "a;echo '" $(touch x) "'"). Payloads now come from the whole snippet.
• A single &, a newline and <(...)/>(...) weren't treated as running a separate command, so echo hi & touch x passed Bash(echo). They are now, and 2>&1, >&2 and &>file still work.
• bash expands the target of >&word a second time after quote removal, so echo x >&'$(touch m)' runs touch despite the single quotes. Both scanners now read that word without its quotes. The bash policy uses the same extractor, so enforce mode now denies it too.
• Very deep nesting raised RecursionError. The matcher now stops after 16 levels, where allow returns False and deny returns True.

With these changes the same fuzz run finds no bypasses. Every new test fails on the previous commit and passes on this one, the rest of the suite is unchanged, CI is green, and ruff and pyright are clean on the touched files.

[zhanghanduo]

LGTM now