diff --git a/.changeset/refuse-reload-flag-injection.md b/.changeset/refuse-reload-flag-injection.md new file mode 100644 index 000000000..732566983 --- /dev/null +++ b/.changeset/refuse-reload-flag-injection.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Reject session reload inputs whose VCS `range` or `ref` values look like command options. A caller reaching the session broker could otherwise inject `git` flags such as `--output=` through a `/session-api` reload request and make Hunk write diff output to an arbitrary path. diff --git a/src/app/session/reloadBounds.test.ts b/src/app/session/reloadBounds.test.ts index d3caa2ec7..2dd2f53dc 100644 --- a/src/app/session/reloadBounds.test.ts +++ b/src/app/session/reloadBounds.test.ts @@ -61,6 +61,61 @@ describe("session reload filesystem bounds", () => { } }); + test("rejects option-like VCS ranges and refs in daemon reloads", () => { + const repo = mkdtempSync(join(tmpdir(), "hunk-reload-bounds-options-")); + + try { + const bounds = createSessionReloadBounds( + bootstrapFor({ kind: "vcs", staged: false, options: {} }, repo), + { cwd: repo }, + ); + + expect(() => + validateSessionReloadWithinBounds(bounds, { + kind: "vcs", + range: "--output=/tmp/hunk-poc", + staged: false, + options: {}, + }), + ).toThrow("diff range that looks like a VCS option"); + expect(() => + validateSessionReloadWithinBounds(bounds, { + kind: "show", + ref: "--output=/tmp/hunk-poc", + options: {}, + }), + ).toThrow("show ref that looks like a VCS option"); + expect(() => + validateSessionReloadWithinBounds(bounds, { + kind: "stash-show", + ref: "-R", + options: {}, + }), + ).toThrow("stash-show ref that looks like a VCS option"); + + // Plain revisions and ranges still reload, including literal-dash pathspecs that + // bundled backends append after `--` so the VCS treats them as paths. + expect(() => + validateSessionReloadWithinBounds(bounds, { + kind: "vcs", + range: "main..feature", + staged: false, + pathspecs: ["--help"], + options: {}, + }), + ).not.toThrow(); + expect(() => + validateSessionReloadWithinBounds(bounds, { + kind: "stash-show", + ref: "stash@{1}", + options: {}, + }), + ).not.toThrow(); + } finally { + rmSync(repo, { force: true, recursive: true }); + } + }); + test("rejects daemon reload source paths outside the initial repo root", () => { const repo = mkdtempSync(join(tmpdir(), "hunk-reload-bounds-repo-")); const outside = mkdtempSync(join(tmpdir(), "hunk-reload-bounds-outside-")); diff --git a/src/app/session/reloadBounds.ts b/src/app/session/reloadBounds.ts index 73ed4dc52..ffc898845 100644 --- a/src/app/session/reloadBounds.ts +++ b/src/app/session/reloadBounds.ts @@ -22,6 +22,11 @@ import type { VcsCatalog } from "../../core/vcs/types"; * * All candidate paths are realpath-normalized through existing ancestors so symlinks cannot escape * the roots, including paths whose final file does not exist yet. + * + * Revision-shaped fields (`vcs` `range`, `show`/`stash-show` `ref`) must name revisions, not + * command options: a value beginning with `-` would let a daemon caller inject flags such as + * `--output=` into the VCS backend's spawned command. Pathspecs are exempt because every + * bundled backend appends them after `--`, where the VCS treats them as paths. */ export interface SessionReloadBounds { @@ -148,6 +153,13 @@ function assertReloadSourceWithinBounds(bounds: SessionReloadBounds, cwd: string return candidate; } +/** Reject daemon-supplied revision fields that a VCS backend would parse as command options. */ +function assertReloadRevisionNotOptionLike(description: string, value: string) { + if (value.startsWith("-")) { + throw new Error(`Session reload refused ${description} that looks like a VCS option: ${value}`); + } +} + /** Validate common reload options that may cause filesystem reads. */ function validateCommonReloadOptions( bounds: SessionReloadBounds, @@ -199,8 +211,15 @@ export function validateSessionReloadWithinBounds( } break; case "vcs": + if (nextInput.range) { + assertReloadRevisionNotOptionLike("diff range", nextInput.range); + } + break; case "show": case "stash-show": + if (nextInput.ref) { + assertReloadRevisionNotOptionLike(`${nextInput.kind} ref`, nextInput.ref); + } break; } diff --git a/src/extensions/default/vcs/git/commands.test.ts b/src/extensions/default/vcs/git/commands.test.ts index c773065b3..5a47d19be 100644 --- a/src/extensions/default/vcs/git/commands.test.ts +++ b/src/extensions/default/vcs/git/commands.test.ts @@ -4,10 +4,13 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { buildGitDiffArgs, + buildGitDiffNumstatArgs, buildGitIgnoredDirectoryArgs, + buildGitShowArgs, buildGitStashShowArgs, buildGitStatusArgs, listGitIgnoredDirectoryRoots, + listGitUntrackedFiles, parseGitIgnoredDirectoryRoots, resolveGitDiffEndpoints, parseGitNumstat, @@ -101,6 +104,27 @@ describe("git command helpers", () => { expect(buildGitDiffArgs(makeGitInput())).toContain("core.quotePath=true"); }); + test("refuses option-like revision values before spawning Git", () => { + expect(() => buildGitDiffArgs(makeGitInput({ range: "--output=/tmp/hunk-poc" }))).toThrow( + "looks like a Git option", + ); + expect(() => buildGitDiffNumstatArgs(makeGitInput({ range: "-R" }))).toThrow( + "looks like a Git option", + ); + expect(() => + buildGitShowArgs({ kind: "show", ref: "--output=/tmp/hunk-poc", options: {} }), + ).toThrow("looks like a Git option"); + expect(() => + buildGitStashShowArgs({ kind: "stash-show", ref: "--output=/tmp/hunk-poc", options: {} }), + ).toThrow("looks like a Git option"); + // Guarded revision helpers must also refuse ranges before any rev-parse spawn. + expect(() => listGitUntrackedFiles(makeGitInput({ range: "--output=/tmp/hunk-poc" }))).toThrow( + "looks like a Git option", + ); + + expect(buildGitDiffArgs(makeGitInput({ range: "main..feature" }))).toContain("main..feature"); + }); + test("disables external diff tools for stash patches", () => { const args = buildGitStashShowArgs({ kind: "stash-show", diff --git a/src/extensions/default/vcs/git/commands.ts b/src/extensions/default/vcs/git/commands.ts index 3526c7eee..42487e991 100644 --- a/src/extensions/default/vcs/git/commands.ts +++ b/src/extensions/default/vcs/git/commands.ts @@ -56,6 +56,23 @@ export function appendGitPathspecs(args: string[], pathspecs?: string[]) { args.push("--", ...pathspecs); } +/** + * Return one caller-supplied revision or range argument, refusing option-like values. + * Revisions and ranges never start with `-`, so a leading dash is injected content (for + * example `--output=`) that Git would parse as a flag; it fails closed here instead + * of reaching the spawned command. + */ +export function requireGitRevisionArg(input: GitBackedInput, value: string) { + if (value.startsWith("-")) { + throw new HunkExtensionUserError( + `\`${formatGitCommandLabel(input)}\` refused revision \`${value}\` because it looks like a Git option.`, + { suggestions: ["Pass a plain revision or range, such as `HEAD` or `main..feature`."] }, + ); + } + + return value; +} + // @pierre/diffs currently assumes git-style a/ and b/ prefixes when parsing patch headers. // Force byte-safe path quoting and canonical prefixes so user/repo git config cannot replace raw // non-UTF-8 bytes during stdout decoding or break the shared parser's expected side prefixes. @@ -130,7 +147,7 @@ export function buildGitDiffArgs( } if (input.range) { - args.push(input.range); + args.push(requireGitRevisionArg(input, input.range)); } if (excludedPathspecs.length > 0) { @@ -155,7 +172,7 @@ export function buildGitDiffNumstatArgs(input: ExtensionVcsDiffInput) { } if (input.range) { - args.push(input.range); + args.push(requireGitRevisionArg(input, input.range)); } appendGitPathspecs(args, input.pathspecs); @@ -237,7 +254,7 @@ export function buildGitShowArgs( ]; if (input.ref) { - args.push(input.ref); + args.push(requireGitRevisionArg(input, input.ref)); } appendGitPathspecs(args, input.pathspecs); @@ -259,7 +276,7 @@ export function buildGitStashShowArgs( ]; if (input.ref) { - args.push(input.ref); + args.push(requireGitRevisionArg(input, input.ref)); } return withNormalizedDiffPrefixes(withGitMovedLineColorConfig(args, colorMoved)); @@ -564,7 +581,7 @@ function isWorkingTreeGitDiffInput( const revs = runGitText({ input, - args: ["rev-parse", "--revs-only", input.range], + args: ["rev-parse", "--revs-only", requireGitRevisionArg(input, input.range)], cwd, gitExecutable, preventOptionalLocks, @@ -825,7 +842,7 @@ function resolveRangeRevisions( ): { positives: string[]; negatives: string[] } { const revs = runGitText({ input, - args: ["rev-parse", "--revs-only", range], + args: ["rev-parse", "--revs-only", requireGitRevisionArg(input, range)], cwd: repoRoot ?? cwd, gitExecutable, }) diff --git a/test/session/cli.test.ts b/test/session/cli.test.ts index c506e277b..981c6bbe9 100644 --- a/test/session/cli.test.ts +++ b/test/session/cli.test.ts @@ -1,5 +1,5 @@ import { afterAll, afterEach, describe, expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { createServer } from "node:net"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -491,6 +491,56 @@ sessionDescribe("session CLI integration", () => { } }, 20_000); + test("reload refuses option-like VCS ranges sent directly to the session API", async () => { + const port = await reserveLoopbackPort(); + const fixture = createFixtureFiles( + "reload-injection", + ["export const visible = 1;"], + ["export const visible = 2;"], + ); + mkdirSync(join(fixture.dir, ".git")); + const session = spawnHunkSession(fixture, port); + + try { + const listed = await waitForRegisteredSessions(port); + + const sessionId = listed[0]!.sessionId; + // Bypass the CLI parser on purpose: the raw daemon surface is the attacker-controlled + // path, so reproduce the injected flag exactly as a hostile /session-api caller would. + const sentinel = join(fixture.dir, "hunk-poc"); + const response = await fetch(`http://127.0.0.1:${port}/session-api`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + action: "reload", + selector: { sessionId }, + nextInput: { + kind: "vcs", + range: `--output=${sentinel}`, + staged: false, + options: {}, + }, + }), + }); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ + error: expect.stringContaining("looks like a VCS option"), + }); + expect(existsSync(sentinel)).toBe(false); + + const get = runSessionCli(["get", sessionId, "--json"], port); + expect(get.proc.exitCode).toBe(0); + expect(JSON.parse(get.stdout)).toMatchObject({ + session: { + files: [{ path: fixture.afterName }], + }, + }); + } finally { + await cleanupHunkSession(session, fixture, port); + } + }, 20_000); + test("navigate works, and comment add only focuses the session when --focus is passed", async () => { const port = await reserveLoopbackPort(); const fixture = createFixtureFiles(