Skip to content

Use --end-of-options before user-controlled refs in git invocations #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lohengrin332 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use --end-of-options before user-controlled refs in git invocations #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions plugins/codex/scripts/lib/git.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down Expand Up @@ -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}`;
}
Expand Down Expand Up @@ -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",
Expand All @@ -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")
: [
Expand Down Expand Up @@ -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);
Expand Down
88 changes: 88 additions & 0 deletions tests/git.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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=<file>.
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)}`);
});