Skip to content

fix(agent): stop git_diff and git_commit arguments reaching the shell - #2

Merged
voidstackloop merged 1 commit into
voidstackloop:mainfrom
ConsultingFuture4200:fix/git-tool-shell-injection
Jul 25, 2026
Merged

fix(agent): stop git_diff and git_commit arguments reaching the shell#2
voidstackloop merged 1 commit into
voidstackloop:mainfrom
ConsultingFuture4200:fix/git-tool-shell-injection

Conversation

@ConsultingFuture4200

Copy link
Copy Markdown
Contributor

Problem

gitDiff builds git diff -- "<path>" and gitCommit builds git commit -m "<message>", interpolating a model-supplied value into a string that gitCommandrunCommand hands to promisify(exec), i.e. to sh -c.

Double quotes do not make that safe on a POSIX shell: command substitution and backticks are still expanded inside them. JSON.stringify escapes " and \ but not $ or backticks, so it looks like quoting without being shell quoting.

The reason this is worth a patch rather than a note: git_diff is classified read-only in the renderer (READ_ONLY_TOOLS, Chat.tsx), which is what makes the "Always allow this session" button appear for it. Granting that — a reasonable thing to do during a code-review task — converts a standing approval to read diffs into a standing approval to execute, with no further prompt. Agent mode reads file contents and web pages, so the value reaching path is not necessarily something the user typed.

Severity varies by platform: on Linux with a working bubblewrap, or on macOS, the command is confined to the workspace with network denied. Where no sandbox mechanism is available — Windows, and Linux where bubblewrap can't create user namespaces (the AppArmor restriction command-sandbox.ts already documents) — it is unconfined. findDangerousCommandReason sees the assembled string and catches some shapes, but it is a keyword blocklist and has no notion of substitution.

Fix

Both values now go through a shared quoting helper, exported from command-sandbox.ts rather than duplicated.

The helper is platform-aware, which matters here:

  • POSIX sh — single quotes, embedded quote written as '\''.
  • cmd.exe — double quotes, embedded quote written as "". Single quotes are not quote characters to cmd.exe, so quoting Windows arguments POSIX-style would have split ordinary paths and commit messages on their spaces. cmd.exe also has no command substitution to defend against.

applySandbox passes its platform argument through, so the existing wrapper-folding behaviour is unchanged.

Tests

Added to agent-tools.test.ts (git tools) and command-sandbox.test.ts:

  • git_diff with a path argument containing a substitution — asserts the side effect does not happen.
  • git_commit with a message containing a substitution — same.
  • git_commit with a message containing $, backticks, single and double quotes — asserts the whole subject survives intact, so the quoting can't pass by mangling the input.
  • shellQuote unit tests for both platforms, using the injectable-platform pattern already used in this file.

Control experiment: the two injection tests fail on main (the marker files are created) and pass with the patch.

npx tsc -p tsconfig.json --noEmit clean; npm test 232 passed.

gitDiff interpolated the model-supplied path into `git diff -- "<path>"`
and gitCommit interpolated the message into `git commit -m "<message>"`.
Both strings are executed via `sh -c`, and a POSIX shell still expands
`$(...)` and backticks inside double quotes, so either argument could run
arbitrary commands. JSON.stringify escapes `"` and `\` but not `$` or
backticks, so it is not a shell-quoting function.

git_diff is offered as a read-only tool the user can grant "always allow
this session", so this turned a standing approval for reading diffs into
one for arbitrary execution, with no further prompt.

Both values now go through a shared quoting helper in command-sandbox.ts,
which is platform-aware: single quotes for POSIX shells, double quotes for
cmd.exe (where `'` is not a quote character at all, and where there is no
`$(...)` or backtick expansion to defend against). Quoting Windows
arguments POSIX-style would have split ordinary paths and messages on
their spaces.
@voidstackloop
voidstackloop merged commit 54ffead into voidstackloop:main Jul 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants