Terminal auto-approve: assignments and method invocations bypass sub-command checks
- Dominant language
- TypeScript
- Stars
- 193k
- Forks
- 42.4k
- PR merge metrics
- PR metrics pending
Description
Follow up to #328052. Found while reviewing the Agent Host PowerShell grammar work; this one is in the **workbench** `runInTerminal` path and is live on `main` today, independent of that PR.
## Problem
`TreeSitterCommandParser.extractSubCommands` collects `(command)` nodes only, and `CommandLineAutoApprover` then approves when every collected sub-command matches an allow rule. Executable constructs that are **not** `(command)` nodes are therefore invisible to rule matching, and the surrounding allow-listed command approves the whole line.
`transientEnvVarRegex` (`/^[A-Z_][A-Z0-9_]*=/i`) only catches the *inline prefix* form, because bash folds that into the command node. The statement form parses as a separate node type that is never captured.
## Evidence
Instantiated the real `CommandLineAutoApprover` against the real shipped default rule table (122 rules) and drove it with the same `'(command) @command'` query the parser uses:
```
!! APPROVED [bash ] NODE_OPTIONS="--require ./evil.js"; npm ls subs=["npm ls"]
!! APPROVED [bash ] export NODE_OPTIONS="--require ./evil.js"; npm ls subs=["npm ls"]
!! APPROVED [bash ] FOO=bar; git status subs=["git status"]
denied [bash ] NODE_OPTIONS=--require ./evil.js; npm ls subs=["NODE_OPTIONS=…","npm ls"]
denied [bash ] GIT_SSH_COMMAND=evil git status subs=["GIT_SSH_COMMAND=evil git status"]
!! APPROVED [powershell] $env:NODE_OPTIONS="--require ./evil.js"; npm ls subs=["npm ls"]
!! APPROVED [powershell] $env:GIT_SSH_COMMAND="evil"; git status subs=["git status"]
```
Note rows 1 and 4: whether the command is denied depends on **whether the value is quoted**. Unquoted-with-a-space parses as a command-with-prefix and trips the regex; quoted parses as a bare `variable_assignment` and vanishes. Security behaviour hinging on quoting indicates the guard is at the wrong layer.
Node types that are missed:
| form | node type | captured? |
|---|---|---|
| `FOO=bar cmd` | folded into `(command)` | yes — regex fires |
| `FOO=bar; cmd` | `variable_assignment` | **no** |
| `export FOO=bar; cmd` | `declaration_command` | **no** |
| `$env:FOO="bar"; cmd` | `assignment_expression` | **no** |
| `[System.IO.File]::Delete("x")` | `invokation_expression` | **no** |
| `$obj.Delete()` | `invokation_expression` | **no** |
The last two matter most: a .NET invocation is arbitrary code execution, and because it is not a command it can hide **inside an allow-listed cmdlet's script block** — `Get-ChildItem | Where-Object { [System.IO.File]::Delete($_.FullName) }` captures `Where-Object { … }`, which matches the default `Where-Object` allow rule, so the whole line approves.
## Severity notes
- A *bare* bash assignment is shell-local, so `NODE_OPTIONS="…"; npm ls` only bites when the variable is already exported. The reliably exploitable bash forms are `export FOO=…; cmd` and reassignment of an already-exported variable (`PATH="./evil:$PATH"; ls`).
- PowerShell `$env:X=` writes the process environment block directly, so it always reaches children.
- Requires the model to emit such a command line — a prompt-injection / rogue-output concern, not a user-facing exploit. The terminal sandbox is the real containment layer where enabled.
## Fix applied in Agent Host (for reference)
The Agent Host fix captures the offending node types and fails closed:
- `invokation_expression` → always fail closed (arbitrary execution anywhere in the line, including nested).
- `assignment_expression` / `variable_assignment` / `declaration_command` → fail closed **unless** the node sits inside a captured command span, which is the inline prefix form already denied by `transientEnvVarRegex`.
Property access (`member_access`) is deliberately left alone, so `(Get-Content README.md).Length` still auto-approves.
## Suggested direction
Longer term the more robust shape is to invert the invariant: verify that captured spans account for the whole command line and fail closed on anything unaccounted for, rather than enumerating dangerous node types (which fails *open* on anything not enumerated). That would also subsume #328108.
/cc microsoft/vscode#326766
Contributor guide
Assessment
This issue has not been assessed yet.