fix(git): run git through argv instead of a shell string

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 [];

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,20 +16,19 @@ 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 {
   const diffs: 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;

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

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');
+  });
+});