Security control bypass in @simple-git/argv-parser: Git's VISUAL editor fallback is not classified as allowUnsafeEditor

Report metadata

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).
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).
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).
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).

Vulnerable code

packages/argv-parser/src/env/parse-env.ts, lines 5-27 at c427fbad33f1f2b11341f1cf852eedecbb106400:

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:

#!/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:

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.

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.