From c82f6fcef537f74929e6470a551163692f00d7bb Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Fri, 21 Aug 2026 10:59:22 +0200 Subject: [PATCH] fix(git): run git through argv instead of a shell string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit exec/execLarge/execWithStdin pass a string to execSync, which runs /bin/sh -c, and every caller built that string by interpolating a ref, a path or a search term. All of them are reachable unauthenticated over HTTP: /api/diff?ref=, /api/file/?ref=, /api/commits?search=, /api/revert-file. Quoting did not help - "$(…)" expands too. There is also a no-network trigger: the diff parser feeds every changed file name back into `git show :` for its line count, so opening a diff whose PR renames a file to `evil$(touch PWNED).txt` was enough to execute. Everything in the package now goes through argv-form git(), gitLines() and gitWithStdin(). revertFile uses `git clean --force --` for untracked files rather than `rm`, so git refuses paths outside the repository. getRecentCommits also drops the literal quotes it wrapped --format in, which were being parsed as part of the first and last field. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/git/src/commits.ts | 6 +- packages/git/src/diff.ts | 27 +++---- packages/git/src/exec.ts | 9 +++ packages/git/tests/shell-injection.test.ts | 94 ++++++++++++++++++++++ 4 files changed, 119 insertions(+), 17 deletions(-) create mode 100644 packages/git/tests/shell-injection.test.ts diff --git a/packages/git/src/commits.ts b/packages/git/src/commits.ts index b377487..2fc189c 100644 --- a/packages/git/src/commits.ts +++ b/packages/git/src/commits.ts @@ -1,4 +1,4 @@ -import { exec } from './exec.js'; +import { git } from './exec.js'; import type { Commit } from './types.js'; interface CommitQuery { @@ -10,12 +10,12 @@ interface CommitQuery { export function getRecentCommits(query: CommitQuery): Commit[] { const { count, skip = 0, search } = query; - const args = [`-n ${count}`, `--skip=${skip}`, '--format="%H|%h|%s|%cr"']; + const args = ['log', '-n', String(count), `--skip=${skip}`, '--format=%H|%h|%s|%cr']; if (search) { args.push(`--grep=${search}`, '-i'); } - const output = exec(`git log ${args.join(' ')}`); + const output = git(args); if (!output) { return []; diff --git a/packages/git/src/diff.ts b/packages/git/src/diff.ts index da1fce6..b2128b8 100644 --- a/packages/git/src/diff.ts +++ b/packages/git/src/diff.ts @@ -1,4 +1,4 @@ -import { exec, execLarge, execLines, execWithStdin, git } from './exec.js'; +import { git, gitLines, gitWithStdin } from './exec.js'; /** * Flags that neutralize user git config which would otherwise alter the diff @@ -16,12 +16,11 @@ const DIFF_FORMAT_ARGS = [ ]; export function getDiff(args: string[] = []): string { - const cmd = ['git', 'diff', ...DIFF_FORMAT_ARGS, ...args].join(' '); - return execLarge(cmd); + return git(['diff', ...DIFF_FORMAT_ARGS, ...args]); } export function getUntrackedFiles(): string[] { - return execLines('git ls-files --others --exclude-standard'); + return gitLines(['ls-files', '--others', '--exclude-standard']); } export function getUntrackedDiff(files: string[]): string { @@ -29,7 +28,7 @@ export function getUntrackedDiff(files: string[]): string { for (const file of files) { try { - execLarge(`git diff ${DIFF_FORMAT_ARGS.join(' ')} --no-index -- /dev/null "${file}"`); + git(['diff', ...DIFF_FORMAT_ARGS, '--no-index', '--', '/dev/null', file]); } catch (err: unknown) { const error = err as { stdout?: string; status?: number }; if (error.status === 1 && error.stdout) { @@ -77,7 +76,7 @@ export function resolveRef(ref: string, extraArgs: string[] = []): string { export function getDiffFiles(ref: string): string[] { const resolved = resolveDiffArgs(ref); - const tracked = execLines(`git diff ${DIFF_FORMAT_ARGS.join(' ')} --name-only ${resolved.args.join(' ')}`.trim()); + const tracked = gitLines(['diff', ...DIFF_FORMAT_ARGS, '--name-only', ...resolved.args]); if (resolved.includeUntracked) { const untracked = getUntrackedFiles(); return [...new Set([...tracked, ...untracked])]; @@ -86,9 +85,8 @@ export function getDiffFiles(ref: string): string[] { } export function getDiffStat(args: string[] = []): string { - const cmd = ['git', 'diff', ...DIFF_FORMAT_ARGS, '--stat', ...args].join(' '); try { - return execLarge(cmd); + return git(['diff', ...DIFF_FORMAT_ARGS, '--stat', ...args]); } catch { return ''; } @@ -106,18 +104,19 @@ export function getDiffStatForRef(ref: string): string { export function revertFile(filePath: string, isUntracked: boolean): void { if (isUntracked) { - exec(`rm "${filePath}"`); + // git refuses paths outside the repository, which a bare `rm` would not. + git(['clean', '--force', '--', filePath]); } else { - exec(`git checkout HEAD -- "${filePath}"`); + git(['checkout', 'HEAD', '--', filePath]); } } export function revertHunk(patch: string): void { - execWithStdin('git apply --reverse --unidiff-zero', patch); + gitWithStdin(['apply', '--reverse', '--unidiff-zero'], patch); } export function getMergeBase(a: string, b: string): string { - return exec(`git merge-base ${a} ${b}`); + return git(['merge-base', a, b]); } function isLocalBranch(ref: string): boolean { @@ -201,12 +200,12 @@ export function resolveBaseRef(ref: string): string { } export function getFileContent(path: string, ref = 'HEAD'): string { - return exec(`git show ${ref}:${path}`); + return git(['show', `${ref}:${path}`]); } export function getFileLineCount(path: string, ref = 'HEAD'): number | null { try { - const content = exec(`git show ${ref}:${path}`); + const content = git(['show', `${ref}:${path}`]); return content.split('\n').length; } catch { return null; diff --git a/packages/git/src/exec.ts b/packages/git/src/exec.ts index 29109ef..4a483ce 100644 --- a/packages/git/src/exec.ts +++ b/packages/git/src/exec.ts @@ -58,6 +58,15 @@ export function git(args: string[]): string { return execFileLarge('git', args); } +export function gitWithStdin(args: string[], input: string): string { + return execFileSync('git', args, { + encoding: 'utf-8', + stdio: STDIO, + input, + maxBuffer: MAX_BUFFER, + }); +} + export function gitLines(args: string[]): string[] { const output = git(args); if (!output) { diff --git a/packages/git/tests/shell-injection.test.ts b/packages/git/tests/shell-injection.test.ts new file mode 100644 index 0000000..3d6e58d --- /dev/null +++ b/packages/git/tests/shell-injection.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +// A pull request controls its file names, and a reviewer opening the diff is enough to reach +// every helper below. +const HOSTILE_NAME = 'evil$(touch PWNED).txt'; +// git itself rejects a branch name containing parentheses, but nothing stops this arriving +// as `/api/diff?ref=…`, and it reaches git without ever having to name a real branch. +const HOSTILE_REF = '$(touch PWNED_REF)'; + +let repoDir: string; +let origCwd: string; + +function git(args: string[]): string { + return execFileSync('git', args, { cwd: repoDir, encoding: 'utf-8', stdio: 'pipe' }).trim(); +} + +beforeAll(() => { + origCwd = process.cwd(); + repoDir = mkdtempSync(join(tmpdir(), 'diffity-injection-')); + + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + git(['config', 'user.email', 'test@test.com']); + git(['config', 'user.name', 'Test']); + + writeFileSync(join(repoDir, HOSTILE_NAME), 'one\n'); + git(['add', '.']); + git(['commit', '-m', 'hostile file name']); + + writeFileSync(join(repoDir, HOSTILE_NAME), 'two\n'); + git(['add', '.']); + git(['commit', '-m', 'change it']); + + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + rmSync(repoDir, { recursive: true, force: true }); +}); + +function assertNothingExecuted(): void { + expect(existsSync(join(repoDir, 'PWNED'))).toBe(false); + expect(existsSync(join(repoDir, 'PWNED_REF'))).toBe(false); +} + +describe('a file name containing a shell substitution', () => { + it('is read as a path, not executed', async () => { + const { getFileContent, getFileLineCount } = await import('../src/diff.js'); + + expect(getFileContent(HOSTILE_NAME, 'HEAD')).toContain('two'); + expect(getFileLineCount(HOSTILE_NAME, 'HEAD')).toBe(1); + assertNothingExecuted(); + }); + + it('survives the diff helpers', async () => { + const { getDiff, getDiffFiles, getDiffStat } = await import('../src/diff.js'); + + expect(getDiffFiles('HEAD~1')).toContain(HOSTILE_NAME); + expect(getDiff(['HEAD~1'])).toContain(HOSTILE_NAME); + expect(getDiffStat(['HEAD~1'])).toContain('PWNED'); + assertNothingExecuted(); + }); +}); + +describe('a ref containing a shell substitution', () => { + it('is rejected as an unknown ref, not executed', async () => { + const { getMergeBase, normalizeRef } = await import('../src/diff.js'); + + expect(() => getMergeBase(HOSTILE_REF, 'HEAD')).toThrow(); + expect(() => normalizeRef(HOSTILE_REF)).toThrow(); + assertNothingExecuted(); + }); +}); + +describe('a commit search term containing a shell substitution', () => { + it('is passed to --grep, not executed', async () => { + const { getRecentCommits } = await import('../src/commits.js'); + + expect(getRecentCommits({ count: 10, search: '$(touch PWNED)' })).toEqual([]); + assertNothingExecuted(); + }); + + it('parses commits without stray quotes in the format', async () => { + const { getRecentCommits } = await import('../src/commits.js'); + const [head] = getRecentCommits({ count: 1 }); + + expect(head.hash).toHaveLength(40); + expect(head.message).toBe('change it'); + }); +});