Details
# Security control bypass in `@simple-git/argv-parser`: Git's `VISUAL` editor fallback is not classified as `allowUnsafeEditor`
## Report metadata
| Field | Value |
| --- | --- |
| Package | `@simple-git/argv-parser` (npm, `pkg:npm/%40simple-git/argv-parser`) |
| Repository | https://github.com/steveukx/git-js |
| Component | `parseEnv` (`packages/argv-parser/src/env/parse-env.ts`), reached via `vulnerabilityCheck(tokens, env)` |
| Vulnerability class | Security-control bypass — incomplete denylist in unsafe-editor detection |
| Verified against | `c427fbad33f1f2b11341f1cf852eedecbb106400` (`@simple-git/argv-parser` 1.1.1), plus the published npm artifact 1.1.1; `main` at `98864c6` observed still unpatched |
| API surface | Public documented API (`parseEnv(raw)` / `vulnerabilityCheck(tokens, env)`) |
| Affected in default configuration | Yes — reproduced with `blockUnsafeOperationsPlugin` under default options, with no unsafe allowances enabled |
## Summary
`GitEnvKeys` in `packages/argv-parser/src/env/parse-env.ts` maps only `editor`, `git_editor` and `git_sequence_editor` to the `allowUnsafeEditor` category. `prepareEnv` keeps an environment entry only when its lowercased name is a known `GitEnvKey` or starts with `git`, so `VISUAL` is discarded before `collectConfigVulnerabilities` ever inspects it. Git, however, falls back to `VISUAL` when resolving an editor, so `parseEnv({ VISUAL: '/tmp/evileditor' })` reports no vulnerability while an interactive Git operation will execute that binary.
In a consuming application the shape is: environment values derived from a request or job are forwarded into the child Git environment and classified by this parser before spawn. The parser exists to classify exactly such values, and the equivalent `EDITOR` or `GIT_EDITOR` value is rejected — so the attacker gains an editor substitution that the guard is specifically designed to block.
- `EDITOR` -> classified `allowUnsafeEditor` (`GitEnvKeys`, lines 5-27)
- `GIT_EDITOR` -> classified `allowUnsafeEditor` (`GitEnvKeys`, lines 5-27)
- `GIT_SEQUENCE_EDITOR` -> classified `allowUnsafeEditor` (`GitEnvKeys`, lines 5-27)
- `VISUAL` -> **absent from `GitEnvKeys`; dropped by `prepareEnv`, lines 60-68 — no vulnerability emitted**
## Impact
A consuming application that allows attacker-influenced environment values can have an attacker-selected executable launched by Git during operations such as `git commit --amend`, bypassing the parser's default unsafe-editor protection. Execution happens as the host user running the Git child process, with the attacker's binary invoked against the repository's editor file (for example `.git/COMMIT_EDITMSG`, or `.git/rebase-merge/git-rebase-todo` for `git rebase -i`).
The new capability is the bypass itself: without this gap, the same attacker-supplied value under `EDITOR`, `GIT_EDITOR` or `GIT_SEQUENCE_EDITOR` is refused unless the consumer explicitly opts in to `allowUnsafeEditor`. With `VISUAL`, the equivalent code execution proceeds with no opt-in and no reported vulnerability. `VISUAL` also takes precedence over `EDITOR`, the variable the parser does flag.
Scoring note: no CVSS vector or score is available for this finding, and one is not asserted here. Exploitability depends on the consuming application's data flow — specifically whether attacker-influenced environment entries reach the Git child environment. Consumers that never forward untrusted environment values into Git, or that always set a higher-priority `GIT_EDITOR` or `core.editor`, are not affected.
## Preconditions
1. The consumer forwards attacker-influenced environment entries into the environment passed to child Git (for example via simple-git's `.env()`), and classifies them with this parser before spawn.
2. The Git command opens an editor — for example `commit` without `-m`, `commit --amend`, or `rebase -i`.
3. No higher-priority editor setting overrides `VISUAL`: `GIT_EDITOR`, `core.editor` and `EDITOR` are absent (`GIT_EDITOR` and `core.editor` take precedence; `VISUAL` itself overrides `EDITOR`).
4. `TERM` is set to a value other than exactly `dumb` — Git consults `VISUAL` only then. `TERM` is neither a `GitEnvKey` nor `git`-prefixed, so an attacker who controls the environment object supplies it too and the parser reports nothing for it either.
5. The attacker-selected executable exists and is runnable on the host.
This requires no non-standard usage, no monkey-patching and no unusual configuration: the affected path is the documented, default-enabled guard. `docs/PLUGIN-UNSAFE-ACTIONS.md` ("Text editor") documents this control as covering editor environment variables that substitute an arbitrary binary, but lists only `EDITOR`, `GIT_EDITOR` and `GIT_SEQUENCE_EDITOR`; Git's `VISUAL` fallback is not mentioned, and the string `visual` does not appear anywhere in the repository at the verified commit. The docs do state that supplying environment values is the caller's responsibility, but they do not warn that `VISUAL` is outside the guard.
The feed payload's precondition list also carries entries relating to a separate `GIT_CONFIG_PARAMETERS` / `allowUnsafeConfigEnvCount` config-injection scenario. Those were not needed here: the bypass was reproduced with default options and no unsafe allowances enabled.
## Data flow
1. **Source** — the attacker-influenced `raw` environment object enters the public parser entry point: `parseEnv(raw)` ([`packages/argv-parser/src/env/parse-env.ts:70`](https://github.com/steveukx/git-js/blob/c427fbad33f1f2b11341f1cf852eedecbb106400/packages/argv-parser/src/env/parse-env.ts#L70)).
2. **Propagation** — `prepareEnv` lowercases keys and retains only known `GitEnvKeys` or names starting with `git`; `visual` is neither, so the entry is dropped before any analysis sees it ([`packages/argv-parser/src/env/parse-env.ts:60-68`](https://github.com/steveukx/git-js/blob/c427fbad33f1f2b11341f1cf852eedecbb106400/packages/argv-parser/src/env/parse-env.ts#L60-L68)).
3. **Sink** — `collectConfigVulnerabilities` therefore never emits `allowUnsafeEditor` for `VISUAL`, and `vulnerabilityCheck(tokens, env)` returns an empty list ([`packages/argv-parser/src/env/parse-env.ts:45-54`](https://github.com/steveukx/git-js/blob/c427fbad33f1f2b11341f1cf852eedecbb106400/packages/argv-parser/src/env/parse-env.ts#L45-L54)).
4. **Reachability** — simple-git's `blockUnsafeOperationsPlugin` passes its environment into `vulnerabilityCheck` before spawn; an empty vulnerability list means the Git child process is allowed to start ([`simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts:12-20`](https://github.com/steveukx/git-js/blob/c427fbad33f1f2b11341f1cf852eedecbb106400/simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts#L12-L20)).
## Vulnerable code
`packages/argv-parser/src/env/parse-env.ts`, lines 5-27 at `c427fbad33f1f2b11341f1cf852eedecbb106400`:
```ts
const GitEnvKeys = {
'editor': 'allowUnsafeEditor',
// ...
'git_editor': 'allowUnsafeEditor',
// ...
'git_sequence_editor': 'allowUnsafeEditor',
// VISUAL missing
} as const satisfies Record<string, VulnerabilityCategory>;
```
The three mapped keys are the safe siblings; the missing `visual` entry is the gap. Because `prepareEnv` (lines 60-68) filters on this map plus a `git` prefix, the omission is not merely a missing classification — the value never reaches the classifier at all.
## Reproduction
**Verified** — reproduced dynamically, end to end, against a checkout of `c427fbad33f1f2b11341f1cf852eedecbb106400` (`packages/argv-parser/package.json` = 1.1.1) and against the published npm artifact 1.1.1.
Observed at the parser level (vitest PoC run against the repo):
- `parseEnv({ EDITOR })`, `parseEnv({ GIT_EDITOR })` and `parseEnv({ GIT_SEQUENCE_EDITOR })` each yield one `allowUnsafeEditor` vulnerability.
- `parseEnv({ VISUAL: '/tmp/poc/evileditor' })` yields `[]` in every casing.
- `vulnerabilityCheck(['commit', '--amend'], { VISUAL })` — the exact call the spawn guard makes — returns `[]`.
- The published `dist/index.cjs` of 1.1.1 contains zero occurrences of `visual`; the same holds for the repository at the verified commit, including `docs/PLUGIN-UNSAFE-ACTIONS.md`.
Observed at the Git level (only `VISUAL` set, `EDITOR` and `GIT_EDITOR` unset): `git var GIT_EDITOR` returned the attacker path; `git commit --amend` executed the attacker script (marker written, commit subject rewritten); `git rebase -i` executed it for the rebase-todo as well. `VISUAL` also took precedence over `EDITOR` (`EDITOR=/bin/true VISUAL=evil` -> `evil`).
Observed end to end through the real spawn path (simple-git built from this commit, default options, no unsafe allowances): the `EDITOR` and `GIT_EDITOR` variants both threw `GitPluginError` — "Use of ... is not permitted without enabling allowUnsafeEditor" — with no execution. The `VISUAL` variant was **not** blocked: the plugin saw an empty vulnerability list, Git spawned, and the attacker-supplied editor executed as the host user against `.git/COMMIT_EDITMSG`, rewriting the commit message.
Minimal Git-level reproduction:
```sh
#!/bin/sh
set -e
rm -rf /tmp/visual-repo /tmp/GIT_VISUAL_POC /tmp/evileditor
mkdir /tmp/visual-repo && cd /tmp/visual-repo
git init -q
git config user.email a@a && git config user.name a
touch a && git add a && git commit -qm init
printf '#!/bin/sh\ntouch /tmp/GIT_VISUAL_POC\nexit 1\n' >/tmp/evileditor
chmod +x /tmp/evileditor
env -u EDITOR -u GIT_EDITOR VISUAL=/tmp/evileditor git commit --amend || true
test -e /tmp/GIT_VISUAL_POC && echo executed
```
Guard-level reproduction, using the public API:
```ts
import { vulnerabilityCheck } from '@simple-git/argv-parser';
import { spawnSync } from 'node:child_process';
const args = ['commit', '--amend'];
const env = { VISUAL: '/tmp/evileditor' };
if (vulnerabilityCheck(args, env).length === 0) spawnSync('git', args, { env });
```
- **Expected:** `parseEnv({ VISUAL: '/tmp/evileditor' }).vulnerabilities` contains `allowUnsafeEditor`, and the spawn guard refuses the operation unless the consumer has enabled `allowUnsafeEditor` — the behaviour already applied to `EDITOR`, `GIT_EDITOR` and `GIT_SEQUENCE_EDITOR`.
- **Actual:** no vulnerability is reported, the guard permits the spawn, and Git executes the attacker-selected binary.
## Suggested remediation
Treat `VISUAL` as an editor source, so Git's own editor-resolution precedence is fully covered by the denylist.
```ts
const GitEnvKeys = {
'editor': 'allowUnsafeEditor',
'visual': 'allowUnsafeEditor',
// ...
'git_editor': 'allowUnsafeEditor',
// ...
'git_sequence_editor': 'allowUnsafeEditor',
} as const satisfies Record<string, VulnerabilityCategory>;
```
Because `prepareEnv` filters on `GitEnvKeys` membership, this single entry is enough to make `visual` survive filtering and be classified; no change to `prepareEnv` or `collectConfigVulnerabilities` is required.
Notes:
- `TERM` is likewise neither a `GitEnvKey` nor `git`-prefixed, and it is the variable that decides whether Git consults `VISUAL` at all (`TERM=dumb` or unset means `VISUAL` is ignored). An attacker who controls the environment object supplies it alongside `VISUAL`; whether `TERM` warrants its own classification is a maintainer judgement call, but it is worth considering while fixing this.
- `git rebase -i` reaches the same sink: `git_sequence_editor` falls back to normal editor resolution, so the `VISUAL` path executes the attacker binary against the rebase-todo file as well.
- `docs/PLUGIN-UNSAFE-ACTIONS.md` ("Text editor") should list `VISUAL` alongside `EDITOR` / `GIT_EDITOR` / `GIT_SEQUENCE_EDITOR`, since the documented scope of the control is what consumers rely on.
- Already safe and needing no change: `EDITOR`, `GIT_EDITOR` and `GIT_SEQUENCE_EDITOR` are all correctly classified and enforced by the plugin, and the parser's handling of `git`-prefixed variables is unaffected.
Suggested regression test alongside `test/parse-env.spec.ts` (which currently covers `EDITOR` / `GIT_EDITOR` / `GIT_SEQUENCE_EDITOR` / `PAGER` but has no `VISUAL` case): assert that `parseEnv({ VISUAL: '/tmp/evileditor' })` yields one `allowUnsafeEditor` vulnerability in every casing, and that `vulnerabilityCheck(['commit', '--amend'], { VISUAL: '/tmp/evileditor' })` returns that vulnerability rather than `[]`. A precedence case is worth adding too: `VISUAL` set together with `EDITOR` must still be flagged, since `VISUAL` wins in Git's resolution order.
