From 8b46b745ee866329071503aafef8806a1e8091c5 Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 30 Oct 2026 02:47:10 +0000 Subject: [PATCH] fix(ship-bins): gh gets the branch as argv, never a shell; a local-path origin skips gh; RUNTIME_LINES leaves data files out /ship review of #3633 (security CRITICAL x2): gh pr list --head "$branch" ran through sh(), so a branch named x$(cmd) executed. Replay 3 of gstack-evals #34: a local bare origin under GH_HOST failed the carry check, and RUNTIME_LINES counted JSON. The fixture's origin is now a GitHub URL mapped to the bare repo by insteadOf. Failing-first tests. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ship-bin-common.ts | 9 +++++++++ lib/ship-preflight.ts | 19 ++++++++++++++----- lib/ship-release.ts | 5 +++-- test/helpers/ship-bin-fixture.ts | 12 ++++++++---- test/ship-preflight.test.ts | 30 ++++++++++++++++++++++++++++++ test/ship-release.test.ts | 15 +++++++++++++++ 6 files changed, 79 insertions(+), 11 deletions(-) diff --git a/lib/ship-bin-common.ts b/lib/ship-bin-common.ts index d59cf10ef..c9a13ff4a 100644 --- a/lib/ship-bin-common.ts +++ b/lib/ship-bin-common.ts @@ -19,6 +19,15 @@ export function git(cwd: string, args: string[], timeout = 120_000): Run { export function bin(cwd: string, name: string, args: string[], timeout = 600_000): Run { return done(runScriptSync(path.join(BIN_DIR, name), args, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024 })); } +/** Whether origin's configured URL names a host (https, ssh or scp form), before any insteadOf rewrite; a local path or file:// remote has none, so gh cannot map it to a repository. */ +export function hostedOrigin(cwd: string): boolean { + const url = git(cwd, ['config', '--get', 'remote.origin.url']).stdout.trim(); + return /^(?:[a-z][a-z0-9+.-]*:\/\/(?:[^@/\s]+@)?[^/:\s]+[:/]|[^@/\s]+@[^/:\s]+:)\S+$/i.test(url) && !/^file:/i.test(url); +} +/** gh with argv, never a shell, so a branch name cannot change what runs. */ +export function gh(cwd: string, args: string[], timeout = 60_000): Run { + return done(spawnSync('gh', args, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024, env: { ...process.env, GIT_TERMINAL_PROMPT: '0' } })); +} export function sh(cwd: string, command: string, timeout = 1_800_000): Run { return done(runBashSync(command, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024 })); } diff --git a/lib/ship-preflight.ts b/lib/ship-preflight.ts index 777eb545f..820bfcd85 100644 --- a/lib/ship-preflight.ts +++ b/lib/ship-preflight.ts @@ -7,7 +7,9 @@ */ import * as fs from 'node:fs'; import * as path from 'node:path'; -import { bin, firstLine, flagReader, git, laneLine, lanes, Report, runLanes, shipPolicy, short, sh } from './ship-bin-common'; +import { auditCodePaths } from './review-packet'; +import { matchesAny } from './ship-policy'; +import { bin, firstLine, flagReader, gh, git, hostedOrigin, laneLine, lanes, Report, runLanes, shipPolicy, short } from './ship-bin-common'; export const PREFLIGHT_HELP = `gstack-ship-preflight — /ship Steps 1-3 and Step 5's pregate and lanes in one call (ship/SKILL.md) @@ -34,9 +36,10 @@ function integrate(cwd: string, base: string, r: Report): boolean { const lines = [...stat.matchAll(/(\d+) (insertion|deletion)/g)].reduce((n, m) => n + Number(m[1]), 0); const commits = git(cwd, ['rev-list', '--count', `origin/${base}..HEAD`]).stdout.trim(); r.add(`DIFF: ${stat || 'no changes'}; commits=${commits || 0}${lines > 200 ? '; over 200 lines: recommend /plan-eng-review or /autoplan' : ''}`); - const pr = sh(cwd, `gh pr list --head ${JSON.stringify(branch)} --state open --json number --jq '.[0].number // empty'`, 60_000); - const n = pr.stdout.trim(); - if (pr.status !== 0) r.add(`CARRY: not checked (gh pr list failed: ${firstLine(pr.stderr)})`); + const pr = hostedOrigin(cwd) ? gh(cwd, ['pr', 'list', '--head', branch, '--state', 'open', '--json', 'number', '--jq', '.[0].number // empty']) : null; + const n = pr?.stdout.trim() ?? ''; + if (!pr) r.add('PR: not checked (origin is a local path, not a hosted remote gh can map; Step 0 uses git-native commands)'); + else if (pr.status !== 0) r.add(`CARRY: not checked (gh pr list failed: ${firstLine(pr.stderr)})`); else if (/^\d+$/.test(n)) { const carry = bin(cwd, 'gstack-ship-receipt', ['carry', '--pr', n, '--base', base, '--dry-run'], 120_000); const line = (carry.stdout.match(/^CARRY: .*$/m) ?? [`CARRY: check failed (${firstLine(carry.stderr)})`])[0]; @@ -86,7 +89,13 @@ function merge(cwd: string, base: string, r: Report): boolean { r.add(`POLICY: ${['bump', 'queue_mode', 'review_scope', 'history'].map(k => `${k}=${policy?.[k] ?? '?'}`).join(' ')} (binds Steps 12, 16 and history)`); const plan = bin(cwd, 'gstack-review-plan', ['--base', base, '--json'], 180_000); let runtime = 'unknown'; - try { runtime = String(JSON.parse(plan.stdout).runtime_lines ?? 'unknown'); } catch { /* unknown */ } + try { + const j = JSON.parse(plan.stdout); + const outputs: string[] = j.registry?.outputs ?? []; + const counted = (j.per_path as { path: string; lines: number; binary: boolean }[]).filter(e => !e.binary && !(j.stamp_only ?? []).includes(e.path) + && !(outputs.length && matchesAny(outputs, e.path)) && auditCodePaths([e.path], cwd).length); + runtime = String(counted.reduce((n, e) => n + e.lines, 0)); + } catch { /* unknown */ } r.add(`RUNTIME_LINES: ${runtime} (gstack-ship-budget check --runtime-lines at the Step 7 and 12 boundaries)`); return true; } diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 62616842e..f538534e8 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -14,7 +14,7 @@ import * as path from 'node:path'; import { classifyPath } from './review-depth'; import { resolveStateRoot } from './state-root'; import { bumpVersion, cmpVersion, fmtVersion, parseVersion, versionWidth, type Bump } from './version-source'; -import { bin, firstLine, flagReader, git, laneLine, lanes, releasePaths, Report, runLanes, sh, shipPolicy, short } from './ship-bin-common'; +import { bin, firstLine, flagReader, gh, git, hostedOrigin, laneLine, lanes, releasePaths, Report, runLanes, sh, shipPolicy, short } from './ship-bin-common'; export const RELEASE_HELP = `gstack-ship-release — /ship Steps 12 and 15-17 (ship/SKILL.md) @@ -202,7 +202,8 @@ export function push(argv: readonly string[], cwd: string): Report { if (guard !== 'true' && !prompted) return r.stop('Step 17\'s one-time pre-push guard offer is unanswered: run the guard block and ask it, then rerun push'); const gateOpen = Number(flag('--gate-open') ?? '0'); if (gateOpen > 0) { - const pr = sh(cwd, `gh pr list --head ${JSON.stringify(branch)} --state open --json number,isDraft --jq '[.[] | select(.isDraft | not) | .number] | first // empty'`, 60_000); + if (!hostedOrigin(cwd)) return r.stop('open-PR lookup unavailable: origin is a local path, not a hosted remote gh can map; it must succeed before pushing open gate findings'); + const pr = gh(cwd, ['pr', 'list', '--head', branch, '--state', 'open', '--json', 'number,isDraft', '--jq', '[.[] | select(.isDraft | not) | .number] | first // empty']); if (pr.status !== 0) return r.stop(`open-PR lookup failed (${firstLine(pr.stderr)}): it must succeed before pushing open gate findings`); if (pr.stdout.trim() && !has('--gate-open-approved')) return r.stop(`${gateOpen} open gate finding(s) and ready PR #${pr.stdout.trim()}: spawned/headless sessions stop here; otherwise ask (resolve now, or publish with the banner and rerun with --gate-open-approved)`); } diff --git a/test/helpers/ship-bin-fixture.ts b/test/helpers/ship-bin-fixture.ts index 4a0818e68..2db3045dd 100644 --- a/test/helpers/ship-bin-fixture.ts +++ b/test/helpers/ship-bin-fixture.ts @@ -21,10 +21,13 @@ export interface ShipFx { /** Push to origin/main from a second clone, moving the base under the feature branch. */ advanceBase(files: Record): void; gh(script: string): void; - run(bin: string, args: string[]): { status: number | null; stdout: string; stderr: string }; + run(bin: string, args: string[], extraEnv?: Record): { status: number | null; stdout: string; stderr: string }; cleanup(): void; } +/** origin's URL as a GitHub remote; the clone's insteadOf maps it to the bare repository, so gh calls see a GitHub repo and git still works offline. */ +export const GITHUB_URL = 'https://github.com/fx/fx.git'; + const IDENTITY = ['-c', 'user.email=fixture@example.com', '-c', 'user.name=Fixture', '-c', 'commit.gpgsign=false']; export function shipFixture(base: Record = {}, feat: Record = { 'src/b.ts': 'export const b = 2;\n' }): ShipFx { @@ -49,6 +52,7 @@ export function shipFixture(base: Record = {}, feat: Record = {}, feat: Record writeIn(work, files), commit: msg => { must(gitIn(work, ['add', '-A']), 'add'); must(gitIn(work, ['commit', '-qm', msg]), msg); }, advanceBase: files => { - const other = path.join(root, `other-${Date.now()}`); + const other = fs.mkdtempSync(path.join(root, 'other-')); must(gitIn(root, ['clone', '-q', origin, other]), 'clone other'); writeIn(other, files); must(gitIn(other, ['add', '-A']), 'add'); must(gitIn(other, ['commit', '-qm', 'base moves']), 'base moves'); must(gitIn(other, ['push', '-q', 'origin', 'main']), 'push'); }, gh: script => { fs.writeFileSync(path.join(stub, 'gh'), `#!/bin/sh\n${script}\n`, { mode: 0o755 }); }, - run: (bin, args) => { - const r = spawnSync(bin, args, { cwd: work, encoding: 'utf8', timeout: 300_000, env }); + run: (bin, args, extraEnv = {}) => { + const r = spawnSync(bin, args, { cwd: work, encoding: 'utf8', timeout: 300_000, env: { ...env, ...extraEnv } }); return { status: r.status, stdout: r.stdout ?? '', stderr: r.stderr ?? '' }; }, cleanup: () => fs.rmSync(root, { recursive: true, force: true }), diff --git a/test/ship-preflight.test.ts b/test/ship-preflight.test.ts index 9e2e227d5..e78bd9e17 100644 --- a/test/ship-preflight.test.ts +++ b/test/ship-preflight.test.ts @@ -120,6 +120,36 @@ describe('gstack-ship-preflight', () => { expect(r.stdout).toContain('NEXT: Step 4'); }); + // Value: protects=the branch name reaches gh as one argv item, never through a shell; fails_when=a branch named with $(...) runs a command on the shipper's machine; + // why_new=GSTA-167 /ship review of #3633 (security, CRITICAL); seam=gh; no_claim=gh's own handling of the name + test('a branch name with shell syntax is never executed', () => { + const f = fx(); + const name = 'x$(touch${IFS}PWNED)'; + expect(f.git('checkout', '-qb', name).status).toBe(0); + f.gh('echo 7'); + f.run(PREFLIGHT, ['--base', 'main']); + expect(fs.existsSync(path.join(f.work, 'PWNED'))).toBe(false); + }); + + // Value: protects=a local-path origin skips the open-PR and carry checks with a reason instead of asking gh, which cannot map it to a repository (and fails outright under a GH_HOST); fails_when=gh runs against a local remote and CARRY reports a GH_HOST failure; + // why_new=GSTA-167 replay 3 of gstack-evals #34 (CARRY: not checked, GH_HOST); seam=gh; no_claim=hosted remotes, which gh resolves itself + test('a local-path origin is not checked by gh', () => { + const f = fx(); + f.gh('echo "none of the git remotes configured for this repository correspond to the GH_HOST environment variable" >&2; exit 1'); + f.git('config', 'remote.origin.url', f.origin); + const local = f.run(PREFLIGHT, ['--base', 'main'], { GH_HOST: 'ghe.example.com' }); + expect(lineOf(local.stdout, 'PR')).toMatch(/^PR: not checked \(origin is a local path/); + expect(local.stdout).not.toContain('CARRY:'); + expect(local.status).toBe(0); + }); + + // Value: protects=RUNTIME_LINES (the small-run delegation input) counts code lines the way the packets classify paths, leaving data files out; fails_when=a JSON attachment inflates the runtime count; + // why_new=GSTA-167 replay 3 of gstack-evals #34 (RUNTIME_LINES 6687 counted JSON); seam=gstack-review-plan --json per_path; no_claim=the review depth's own count + test('RUNTIME_LINES leaves data files out', () => { + const f = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'data/rows.json': Array.from({ length: 100 }, (_, i) => `{"i":${i}}`).join('\n') + '\n' }); + expect(lineOf(f.run(PREFLIGHT, ['--base', 'main']).stdout, 'RUNTIME_LINES')).toMatch(/^RUNTIME_LINES: 1 /); + }); + test('usage: --base is required and --tests-only needs a lane', () => { const f = fx(); expect(f.run(PREFLIGHT, []).status).toBe(2); diff --git a/test/ship-release.test.ts b/test/ship-release.test.ts index ae33fb0f7..b00bc9e32 100644 --- a/test/ship-release.test.ts +++ b/test/ship-release.test.ts @@ -154,3 +154,18 @@ describe('push (Step 17)', () => { expect(r.stdout).toContain('never force-push'); }); }); + +describe('gstack-ship-release push: branch names', () => { + // Value: protects=push's open-PR lookup passes the branch to gh as one argv item; fails_when=a branch named with $(...) runs a command before the push guards; + // why_new=GSTA-167 /ship review of #3633 (security, CRITICAL); seam=gh; no_claim=gh's own handling of the name + test('a branch name with shell syntax is never executed by the gate lookup', () => { + const f = fx(); + const name = 'x$(touch${IFS}PWNED)'; + expect(f.git('checkout', '-qb', name).status).toBe(0); + fs.mkdirSync(f.state, { recursive: true }); + fs.writeFileSync(path.join(f.state, '.redact-prepush-prompted'), ''); + f.gh('echo 12'); + f.run(RELEASE, ['push', '--branch', name, '--gate-open', '1']); + expect(fs.existsSync(path.join(f.work, 'PWNED'))).toBe(false); + }); +});