From 194170e3cb8fb7e95f81b4d6f28d03f589829af4 Mon Sep 17 00:00:00 2001 From: Bruno Azoulay Date: Mon, 3 Aug 2026 13:33:15 +0200 Subject: [PATCH 1/2] fix(cli): close shell injection via crafted staged filenames (security) stagedContent() built a shell command string via execSync(`git show ":${path}"`) with path taken directly from stagedFiles() -- i.e. the literal name of a staged file, fully controlled by whoever authored the commit. git's core.quotepath escapes an embedded `"` or `\`, but not backticks or $(...), so a filename like `` x`touch pwned.txt`.ts `` ran as shell syntax. Anyone running `harness check` as a pre-commit hook (the README's own documented usage) against an untrusted staged file list -- e.g. right after `git add .` on a checked-out contributor branch -- got arbitrary command execution. Both stagedContent() and stagedFiles() now use execFileSync with an argv array; the filename reaches `git show` as a single literal argument and is never parsed by a shell, regardless of its content. stagedFiles() had no interpolated input (its args were already literal) but is switched for consistency and defense in depth. test/staged-content-shell-injection.test.ts exercises the real stagedContent() against an isolated git repo (mkdtempSync, cleaned up in a finally block) with a backtick-laden staged filename -- proven discriminating: it fails against the vulnerable execSync form (the shell eats the backticks before git ever sees them) and passes against the fix. All 9 execution sites in src/ were audited for the same shape; this was the only exploitable one. Reported in #87 by @VikramNehreTR, with reproduction and root-cause analysis (including why core.quotepath doesn't help) and the exact fix adopted here. --- src/cli/run.ts | 15 ++++-- test/staged-content-shell-injection.test.ts | 51 +++++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) create mode 100644 test/staged-content-shell-injection.test.ts diff --git a/src/cli/run.ts b/src/cli/run.ts index 529d7c2..1406e6b 100644 --- a/src/cli/run.ts +++ b/src/cli/run.ts @@ -1,17 +1,24 @@ -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { evaluate } from "../policy/evaluate"; import { formatPrompt } from "../prompt/types"; import { isCodeFile } from "../util/project-root"; /** Staged files (Added/Copied/Modified/Renamed). Uses `node:child_process` (Bun shell can hang on `git show`). */ export function stagedFiles(): string[] { - const out = execSync("git diff --cached --name-only --diff-filter=ACMR", { encoding: "utf8" }); + const out = execFileSync("git", ["diff", "--cached", "--name-only", "--diff-filter=ACMR"], { + encoding: "utf8", + }); return out.trim().split("\n").filter(Boolean); } -/** Read a file's staged (index) content — not the working-tree version. */ +/** + * Read a file's staged (index) content — not the working-tree version. + * Uses `execFileSync` with an argv array (never a shell), so a staged + * filename containing shell metacharacters (backticks, `$(...)`, quotes) + * reaches `git show` as a single literal argument — see issue #87. + */ export function stagedContent(path: string): string { - return execSync(`git show ":${path}"`, { encoding: "utf8" }); + return execFileSync("git", ["show", `:${path}`], { encoding: "utf8" }); } /** diff --git a/test/staged-content-shell-injection.test.ts b/test/staged-content-shell-injection.test.ts new file mode 100644 index 0000000..449199c --- /dev/null +++ b/test/staged-content-shell-injection.test.ts @@ -0,0 +1,51 @@ +import { test, expect } from "bun:test"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { stagedContent } from "../src/cli/run"; + +/** + * Regression test for the shell-injection reported in issue #87: a staged + * filename containing a command substitution (`` `...` `` / `$(...)`) must + * NOT be interpreted by a shell when `stagedContent` reads its staged blob + * via `git show`. Exercises the real entry point (`stagedContent`) against + * an isolated temporary git repo — never the real repo — and cleans up in + * `finally` regardless of outcome. + */ +test("stagedContent: a filename with shell metacharacters is never executed by a shell", () => { + const repo = mkdtempSync(join(tmpdir(), "harness-issue-87-")); + const marker = join(repo, "pwned.txt"); + try { + execFileSync("git", ["init", "-q"], { cwd: repo }); + execFileSync("git", ["config", "user.email", "test@example.com"], { cwd: repo }); + execFileSync("git", ["config", "user.name", "test"], { cwd: repo }); + + // Filename carries a command substitution that would create `marker` if + // ever handed to a shell (`sh -c "git show \":${path}\""`). Kept relative + // (no `/`) since a filename component cannot embed a path separator — + // `stagedContent` runs with `cwd` pinned to `repo` (see below). + const evilName = "x`touch pwned.txt`.ts"; + writeFileSync(join(repo, evilName), "content\n"); + execFileSync("git", ["add", "--", evilName], { cwd: repo }); + + const content = stagedContentAt(repo, evilName); + + expect(content).toBe("content\n"); + expect(existsSync(marker)).toBe(false); + } finally { + rmSync(repo, { recursive: true, force: true }); + rmSync(marker, { force: true }); + } +}); + +/** Runs the real `stagedContent` implementation with `cwd` pinned to `repo`. */ +function stagedContentAt(repo: string, path: string): string { + const cwd = process.cwd(); + process.chdir(repo); + try { + return stagedContent(path); + } finally { + process.chdir(cwd); + } +} From e902272cdaf11f5c815312ab105b7e9bcf817b38 Mon Sep 17 00:00:00 2001 From: Bruno Azoulay Date: Mon, 3 Aug 2026 13:34:01 +0200 Subject: [PATCH 2/2] chore: update CHANGELOG to 0.1.89 --- CHANGELOG.md | 6 ++++++ package.json | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d1ab4b..32af419 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to `@fusengine/harness`. Format: [Keep a Changelog](https:// ## [Unreleased] +## [0.1.89] - 2026-08-03 + +### Security + +- **Arbitrary command execution via a crafted staged filename in `harness check`** (`src/cli/run.ts`) — `stagedContent()` built a shell command string via `execSync(\`git show ":${path}"\`)`, with `path` taken directly from the literal name of a staged file, fully controlled by whoever authored the commit. Git's `core.quotepath` escapes an embedded `"` or `\`, but not backticks or `$(...)`, so a filename like `` x`touch pwned.txt`.ts `` ran as shell syntax. Anyone running `harness check` as a pre-commit hook (the README's own documented usage) against an untrusted staged file list — e.g. right after `git add .` on a checked-out contributor branch — got arbitrary command execution. Both `stagedContent()` and `stagedFiles()` now use `execFileSync` with an argv array; the filename reaches `git show` as a single literal argument and is never parsed by a shell. Reported in [#87](https://github.com/fusengine/harness/issues/87) by @VikramNehreTR, with reproduction, root-cause analysis, and the exact fix adopted here. A non-regression test exercises the real `stagedContent()` against an isolated repo with a backtick-laden filename, proven discriminating against the vulnerable form. All 9 execution sites in `src/` were audited for the same shape; this was the only exploitable one. + ## [0.1.88] - 2026-08-03 ### Added diff --git a/package.json b/package.json index 9fa4ba9..0823d34 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@fusengine/harness", - "version": "0.1.88", + "version": "0.1.89", "description": "Harness-agnostic toolkit for AI coding agents: runtime harness detection (Claude Code, Codex, Cursor, Cline, Gemini, Aider...), pure policy core (env config, project/framework detection, SOLID/file-size limits, APEX freshness, guard patterns, portable prompts), cache, project memory, ref routing, state/locks, statusline, per-harness adapters (Claude/Cursor/Cline/Gemini) and a cli-mode harness-check binary. Bun-native, with a built dist for Node + bundlers.", "type": "module", "module": "src/index.ts",