diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index 1749cfc8..98b2e210 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -65,8 +65,8 @@ function measureCombinedGitOutputBytes(cwd, argSets, maxBytes) { return totalBytes; } -function buildBranchComparison(cwd, baseRef) { - const mergeBase = gitChecked(cwd, ["merge-base", "HEAD", baseRef]).stdout.trim(); +function buildBranchComparison(cwd, baseRef, _gitCheckedImpl = gitChecked) { + const mergeBase = _gitCheckedImpl(cwd, ["merge-base", "HEAD", "--end-of-options", baseRef]).stdout.trim(); return { mergeBase, commitRange: `${mergeBase}..HEAD`, @@ -101,11 +101,11 @@ export function detectDefaultBranch(cwd) { const candidates = ["main", "master", "trunk"]; for (const candidate of candidates) { - const local = git(cwd, ["show-ref", "--verify", "--quiet", `refs/heads/${candidate}`]); + const local = git(cwd, ["show-ref", "--verify", "--quiet", "--end-of-options", `refs/heads/${candidate}`]); if (local.status === 0) { return candidate; } - const remote = git(cwd, ["show-ref", "--verify", "--quiet", `refs/remotes/origin/${candidate}`]); + const remote = git(cwd, ["show-ref", "--verify", "--quiet", "--end-of-options", `refs/remotes/origin/${candidate}`]); if (remote.status === 0) { return `origin/${candidate}`; } @@ -262,9 +262,9 @@ function collectBranchContext(cwd, baseRef, options = {}) { const includeDiff = options.includeDiff !== false; const comparison = options.comparison ?? buildBranchComparison(cwd, baseRef); const currentBranch = getCurrentBranch(cwd); - const changedFiles = gitChecked(cwd, ["diff", "--name-only", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean); - const logOutput = gitChecked(cwd, ["log", "--oneline", "--decorate", comparison.commitRange]).stdout.trim(); - const diffStat = gitChecked(cwd, ["diff", "--stat", comparison.commitRange]).stdout.trim(); + const changedFiles = gitChecked(cwd, ["diff", "--name-only", "--end-of-options", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean); + const logOutput = gitChecked(cwd, ["log", "--oneline", "--decorate", "--end-of-options", comparison.commitRange]).stdout.trim(); + const diffStat = gitChecked(cwd, ["diff", "--stat", "--end-of-options", comparison.commitRange]).stdout.trim(); return { mode: "branch", @@ -275,7 +275,7 @@ function collectBranchContext(cwd, baseRef, options = {}) { formatSection("Diff Stat", diffStat), formatSection( "Branch Diff", - gitChecked(cwd, ["diff", "--binary", "--no-ext-diff", "--submodule=diff", comparison.commitRange]).stdout + gitChecked(cwd, ["diff", "--binary", "--no-ext-diff", "--submodule=diff", "--end-of-options", comparison.commitRange]).stdout ) ].join("\n") : [ @@ -321,11 +321,11 @@ export function collectReviewContext(cwd, target, options = {}) { diffBytes <= maxInlineDiffBytes); details = collectWorkingTreeContext(repoRoot, state, { includeDiff }); } else { - const comparison = buildBranchComparison(repoRoot, target.baseRef); - const fileCount = gitChecked(repoRoot, ["diff", "--name-only", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean).length; + const comparison = buildBranchComparison(repoRoot, target.baseRef, options._gitCheckedImpl); + const fileCount = gitChecked(repoRoot, ["diff", "--name-only", "--end-of-options", comparison.commitRange]).stdout.trim().split("\n").filter(Boolean).length; diffBytes = measureGitOutputBytes( repoRoot, - ["diff", "--binary", "--no-ext-diff", "--submodule=diff", comparison.commitRange], + ["diff", "--binary", "--no-ext-diff", "--submodule=diff", "--end-of-options", comparison.commitRange], maxInlineDiffBytes ); includeDiff = options.includeDiff ?? (fileCount <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); diff --git a/tests/git.test.mjs b/tests/git.test.mjs index 14ff2576..7e838faa 100644 --- a/tests/git.test.mjs +++ b/tests/git.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import assert from "node:assert/strict"; import { collectReviewContext, resolveReviewTarget } from "../plugins/codex/scripts/lib/git.mjs"; +import { runCommandChecked } from "../plugins/codex/scripts/lib/process.mjs"; import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; test("resolveReviewTarget prefers working tree when repo is dirty", () => { @@ -181,3 +182,90 @@ test("collectReviewContext keeps untracked file content in lightweight working t assert.match(context.content, /## Untracked Files/); assert.match(context.content, /UNTRACKED_RISK_MARKER/); }); + +// --- option-injection hardening tests --- + +test("resolveReviewTarget with a flag-shaped --base value does not pass it as a git option", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + run("git", ["checkout", "-b", "feature/test"], { cwd }); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v2');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "change"], { cwd }); + + // Sentinel file that --output= would create if option-parsing succeeded. + const sentinel = path.join(cwd, "sentinel-must-not-exist.txt"); + assert.equal(fs.existsSync(sentinel), false, "sentinel must not exist before the call"); + + const target = resolveReviewTarget(cwd, { base: `--output=${sentinel}` }); + + // resolveReviewTarget itself just records the baseRef; the hostile value + // only reaches git when collectReviewContext calls buildBranchComparison. + assert.equal(target.mode, "branch"); + assert.equal(target.baseRef, `--output=${sentinel}`); + + // collectReviewContext must throw because git rejects the value as a bad ref, + // NOT silently succeed (which would indicate option-injection worked). + // Note: git merge-base does not support --output, so in practice the current + // code also errors — but without --end-of-options the boundary is unguarded + // if a future subcommand does accept such an option. + assert.throws( + () => collectReviewContext(cwd, target), + /unknown revision|bad revision|did not match|ambiguous argument|unknown option|not a valid object name/i + ); + + // The sentinel must still not exist — git did NOT treat the value as --output=. + assert.equal(fs.existsSync(sentinel), false, "sentinel must remain absent after the call"); +}); + +test("resolveReviewTarget accepts refs with embedded dashes", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + run("git", ["checkout", "-b", "feature/has-dashes-in-name"], { cwd }); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v2');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "change"], { cwd }); + run("git", ["checkout", "main"], { cwd }); + + const target = resolveReviewTarget(cwd, { base: "feature/has-dashes-in-name" }); + const context = collectReviewContext(cwd, target); + + assert.equal(target.mode, "branch"); + assert.equal(target.baseRef, "feature/has-dashes-in-name"); + assert.match(context.content, /Branch Diff|Diff Stat|Changed Files/); +}); + +test("buildBranchComparison passes --end-of-options before user-controlled baseRef in merge-base argv", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + run("git", ["checkout", "-b", "feature/test"], { cwd }); + fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v2');\n"); + run("git", ["add", "app.js"], { cwd }); + run("git", ["commit", "-m", "change"], { cwd }); + + const capturedArgvs = []; + const spyGitChecked = (wd, args, opts = {}) => { + capturedArgvs.push([...args]); + return runCommandChecked("git", args, { cwd: wd, ...opts }); + }; + + const target = resolveReviewTarget(cwd, { base: "main" }); + collectReviewContext(cwd, target, { _gitCheckedImpl: spyGitChecked }); + + const mergeBaseArgv = capturedArgvs.find((args) => args[0] === "merge-base"); + assert.ok(mergeBaseArgv, "expected a merge-base call to be captured"); + + const eooIdx = mergeBaseArgv.indexOf("--end-of-options"); + const baseRefIdx = mergeBaseArgv.indexOf("main"); + assert.ok(eooIdx !== -1, `--end-of-options must appear in merge-base argv; got: ${JSON.stringify(mergeBaseArgv)}`); + assert.ok(eooIdx < baseRefIdx, `--end-of-options must come before baseRef; got: ${JSON.stringify(mergeBaseArgv)}`); +});