fix(security): inline shell variable indirection before dangerous-com…

[MorAlekss]

Summary

Fixes a gap where shell variable indirection lets the exact same
sensitive-path and hardline command shapes this file's patterns exist
to catch slip past detection entirely. sed -i s/a/b/ ~/.bashrc is
correctly flagged; H=~/.bashrc; sed -i s/a/b/ $H executes identically
in any POSIX shell but was not flagged at all, since the patterns match
literal text and the unresolved $H never appears in _SENSITIVE_WRITE_TARGET.

This isn't a narrow, one-off gap. The same unresolved-variable blind
spot applies uniformly across the sensitive-write-target layer: sed -i
on ~/.bashrc/~/.hermes/config.yaml/.env, tee/redirect writes to
~/.ssh/authorized_keys and similar paths, and even the command name
itself (a=rm; $a -rf / bypasses the hardline floor the same way). One
technique devalues the whole layer, not a single pattern.

It's also a lower-effort trigger than most of the interpreter-flag gaps
found elsewhere in this file recently. Those needed a specifically
constructed, unusual flag combination that requires deliberate intent
to produce. H=~/.bashrc; sed -i ... is an ordinary, everyday shell
idiom: assign a path to a variable, use it later. A request as mundane
as "add an alias to my bashrc" can plausibly produce a script shaped
exactly like the bypass, with no adversarial intent involved at all.

Root cause

_normalize_command_for_detection() already de-obfuscates several
shell mechanisms before pattern matching (backslash escapes, empty-
string literals, $IFS/${IFS} word-splitting, absolute home-path
prefixes) precisely because the patterns below it match literal text
and would otherwise miss the same command shape spelled differently.
Shell variable assignment/expansion (VAR=value; ...$VAR...) was not
among them, despite being handled by the exact same class of reasoning
already documented for the other cases in this function.

Behavioral change

Before: H=~/.bashrc; sed -i s/a/b/ $H (and the equivalent for tee,
redirects, and the command name itself) passed through undetected,
identical in shell semantics to a literal form that was correctly
caught.

After: _inline_simple_var_assignments() inlines simple NAME=value
assignments (including export/local/declare-prefixed forms) into
later $NAME/${NAME} references within the same command, run inside
_normalize_command_for_detection() alongside the existing $IFS
handling. Simple variable-to-variable chains (H2=$H referencing an
earlier H=...) resolve through a bounded number of passes, and a
circular reference terminates safely without hanging. Value-side
command substitution (H=$(echo ...)) does not resolve — see "What is
NOT changed" below.

Because _normalize_command_for_detection() sits inside the shared
_command_detection_variants() pipeline, this fix applies uniformly to
detect_dangerous_command(), detect_hardline_command(), and
_match_user_deny_rule() (the user-editable approvals.deny list) —
not just one of the three.

What changed

tools/approval.py:

• Added _inline_simple_var_assignments(), called from
  _normalize_command_for_detection() right after the $IFS collapse
• Handles quoted and unquoted assignment values, export/local/
  declare (with flags) prefixes, and multi-level variable chains
• Deliberately excludes IFS from generic inlining (handled
  separately, immediately above) and leaves values containing command
  substitution ($(...)) unresolved rather than guessing at their
  content

tests/tools/test_approval.py:

• New TestVariableIndirectionBypass class (21 tests) covering the
  sed -i/tee/redirect/command-name bypass shapes, chained and
  export/local/declare-prefixed assignments, a circular-reference
  safety check, benign variable usage that must stay unflagged, $HOME
  from the real environment correctly left untouched, and the
  command-substitution limitation pinned as an explicit, accepted
  non-goal rather than a silent gap

What is NOT changed

• Command substitution inside an assignment value (H=$(echo ...)) is
  not resolved. Statically resolving arbitrary command substitution
  would require actually executing code, which this detector will
  never do; leaving it unmatched is the safe failure mode; it does not
  widen any existing bypass, it simply doesn't help with this specific
  case. Simple variable-to-variable chains resolve; value-side command
  substitution does not
• The interpreter-flag detection work in a separate, currently open PR
  is unrelated to this fix; verified no merge conflicts between the two
• Full local test run: 319 passed, 0 failed (existing suite + the 21
  new tests above)

[alt-glitch]

This was generated by AI during triage.

Related to the approval-gate hardening family: sibling open PR #57666 (interpreter-name/flag obfuscation in the same tools/approval.py heuristic) and #36859. This is a distinct bypass class (shell variable indirection), not a duplicate. Note for triage: per SECURITY.md §3.2 the dangerous-command approval gate is an in-process heuristic, not the OS-isolation boundary, so this is defense-in-depth hardening (P3) rather than a boundary-crossing fix.

[MorAlekss]

Follow-up commit: resolves empty-value assignments (C=) and broadens coverage to concatenated multi-variable forms (${X}${Y}, $CMD$SUFFIX), variables holding flags rather than paths/commands, &&/|| separators, and applies the same resolution across other existing patterns (curl-pipe-to-shell, killall signal argument), not just the sed-i/tee/rm cases from the original commit. 32 new tests total, 330 passed.

[teknium1]

Thanks for isolating a real approval-gate bypass class. Current main still returns from _normalize_command_for_detection() immediately after $IFS handling (tools/approval.py:852-853), so simple variable indirection is not already covered.

Problems

• tools/approval.py:883-923 builds one command-wide assignment map and replaces all references globally. That changes shell semantics for scoped command-prefix assignments and unset; for example, unset H; H=~/.bashrc env true; sed -i s/a/b/ $H has no H after env true, but this pass rewrites the final $H as if it did.
• The assignment regex at tools/approval.py:883-887 cannot see an assignment immediately after $(, (, or {. Existing parsing recognizes those command starts later (tools/approval.py:1311-1324), so echo $(H=~/.bashrc; sed -i s/a/b/ $H) remains unresolved.
• tests/tools/test_approval.py:1355-1358 expects a quoted ~ assignment to behave as a home-path assignment; quoted tilde is literal in shell assignment syntax.

Suggested changes

• Make substitution scope- and position-aware using the existing quote-aware shell tokenizer, including grouped command contexts.
• Add regressions for command-prefix scope, unset, and $() / subshell / brace-group forms; remove the quoted-tilde expectation.

GitHub reports the branch MERGEABLE/CLEAN, so the corrected work remains salvageable. Automated hermes-sweeper review.

[MorAlekss]

Thanks @teknium1, all three addressed and verified against your exact examples:

• Scope-aware now: unset H; H=~/.bashrc env true; sed -i s/a/b/ $H → not flagged (command-prefix scope doesn't persist). Also found and closed the more serious inverse: H=~/.bashrc; H=safe env true; sed -i s/a/b/ $H was resolving to safe and silently missing a real sensitive-path write, not just producing an extra prompt like your example. Verified live that $H actually reverts to ~/.bashrc there, not safe.
• echo $(H=~/.bashrc; sed -i s/a/b/ $H) now resolves, and so does the bare (...) and brace-group { H=~/.bashrc; sed -i s/a/b/ $H; } form. Reused _iter_shell_command_starts (the tokenizer _mark_command_starts already uses elsewhere in this file) for command-position recognition instead of a new parser, per your suggestion.
• The quoted-tilde test is replaced, not just supplemented: H="~/.bashrc"; sed -i s/a/b/ $H is no longer flagged (verified live that bash doesn't tilde-expand a value arriving via substitution), with a new non-match regression plus one confirming quoted non-tilde values still resolve normally.

Added regressions for all of the above, including unset, both subshell forms, and the brace-group form. 339 passed locally.

[egilewski]

suggesting changes

The variable-indirection handling is incomplete: shell assignments in control contexts and unresolved variable composition can still bypass approval checks, allowing sensitive edits or destructive operations to run without the intended gate.

• [P1] Assignments in shell control contexts bypass sensitive-path checks
  The resolver only tracks assignments at selected command boundaries. Assignments after shell negation, conditionals, loops, redirections, or comments can persist a sensitive path for a later edit while remaining unresolved to the detector.
  Remediation: Use a quote-aware shell parser that records assignments in every shell grammar position, including redirection-only statements and loop variables, or fail closed when the context is ambiguous. Add detector tests for each form.

• [P1] Unresolved variable suffixes hide destructive command names
  When a known command value is concatenated with an unresolved variable reference, literal boundary matching can miss the reconstructed command. If the unknown variable is empty, the shell still runs the destructive operation while detection reports it as safe.
  Remediation: Treat unresolved references in command positions as tainted and fail closed, or use an explicit unknown-expansion sentinel before matching. Add tests for empty and unset suffix and prefix expansions.

Security evidence:

• trust boundary: Agent-supplied terminal text is normalized and matched before execution; the approval decision protects sensitive-file edits and destructive operations.
• source/sink/invariant: Variable assignments that affect command words or sensitive paths must be resolved or treated as unsafe before dangerous-operation matching.
• current-main reproduction: Current main misses a standalone variable-indirection case, while the reviewed change detects that case.
• PR-head or patch-replay validation: Focused variable-indirection checks passed on the reviewed change, while review confirmed misses in shell control contexts and unresolved command-name expansions.
• positive/negative cases: Checks covered literal, chained, braced, command-name, and benign variable use, plus quoted-tilde values and unresolved dynamic values.
• residual bypass search: Residual checks covered wrappers, separators, subshells, brace groups, quote handling, unset operations, dynamic values, loop variables, and unknown expansions; shell control-context assignments and unknown expansion composition remain actionable.
• reviewer validation: Source review and focused checks confirmed the two bypasses and their approval impact.

Not checked:

• Full approval module suite
• Dynamic command substitution execution

Signed: GPT-5.6-luna-max in Codex

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(security): inline shell variable indirection before dangerous-command detection

Excellent, carefully-scoped security fix with outstanding test coverage (scope-aware assignment semantics, chained/circular refs, separator contexts, negative controls). A few residual edges worth considering:

1. Partial command-substitution values create false positives: _VAR_ASSIGN_TOKEN_RE's value class (?!\$\()[^\s;&|]* only excludes a value starting with $(. A value like H=abc$(echo ~/.bashrc) captures abc$(echo ~/.bashrc) verbatim (tools/approval.py line 459-463), which then gets inlined containing the literal ~/.bashrc → a benign command is flagged (runtime $H = abc + echo output, not a real path). Consider stopping the value capture at any $(, $((, or ` marker, not just at position 0.
2. Assignment prefixes not exhaustive: export/local/declare are handled, but typeset H=~/.bashrc, readonly H=~/.bashrc, and declare -x H=...-style variants (the regex's -[A-Za-z]+ covers single flags but not e.g. -rx) are not captured — $H stays unresolved and the indirection bypasses the patterns. typeset/readonly are rarer in malicious one-liners, but they're one-word additions to the prefix alternation.
3. Performance: the inliner runs on every command normalization via _normalize_command_for_detection. The per-chunk regex work is bounded and small, but on a busy gateway this is per-command overhead on the approval hot path — consider a fast pre-filter (skip entirely if the command contains no $-reference to a name assigned in the same string) before doing the tokenizer/inlining pass.
4. The abc$(...) case above is also a correctness gap for the comment's own contract — the docstring says "no command substitution … in the value" is deliberately excluded, but only the leading $( form is actually excluded. Worth tightening to match the documented intent.