mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-17 07:10:12 +02:00
test(ship): guard PR-title-version rule + pull_request_target safety
Two free gate tests so a future carve or workflow refactor can't silently
regress:
- ship-pr-title-version-always-loaded: asserts the invariant lives in the
always-loaded ship/SKILL.md skeleton (not only sections/), and that the
skeleton+sections union keeps BOTH the create and the existing-PR update
title paths. Modeled on test/auq-format-always-loaded.test.ts.
- pr-title-sync-workflow-safety: static tripwire that fails CI if
pr-title-sync.yml checks out PR-head code or inlines an attacker-controlled
${{ github.event.pull_request.* }} field inside a run: block (the two
pull_request_target footguns actionlint cannot catch).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
@@ -0,0 +1,135 @@
|
||||
/**
|
||||
* pr-title-sync.yml is a `pull_request_target` workflow — static injection
|
||||
* tripwire (gate, free).
|
||||
*
|
||||
* The anxiety this kills: `pull_request_target` runs with a WRITE token in the
|
||||
* base-repo context, even for fork PRs. That is what lets this workflow rewrite
|
||||
* fork-PR titles (the backstop). It is also the single most dangerous workflow
|
||||
* trigger in GitHub Actions. Two classic footguns turn it into remote code
|
||||
* execution / token theft, and `actionlint` catches NEITHER:
|
||||
*
|
||||
* 1. Checking out the PR head (`actions/checkout` with a `ref:` pointing at
|
||||
* `pull_request.head` / `head_ref`) and then running anything from it —
|
||||
* that executes attacker-controlled fork code with the write token.
|
||||
* 2. Interpolating an attacker-controlled `${{ github.event.pull_request.* }}`
|
||||
* field directly INSIDE a `run:` block — the title/body are attacker-
|
||||
* controlled and the `${{ }}` is expanded into the shell before execution,
|
||||
* so a crafted title runs as code. Those fields MUST arrive via `env:` and
|
||||
* be referenced as `"$VAR"` (shell-quoted), never inlined.
|
||||
*
|
||||
* This tripwire reads the workflow file directly and fails CI if either pattern
|
||||
* reappears. Mirrors the static-grep invariant tests in browse/test
|
||||
* (terminal-agent-pid-identity, server-sanitize-surrogates).
|
||||
*
|
||||
* Note: `gh api ... -q '.head.sha'` inside a run block is SAFE (reading PR
|
||||
* metadata as data via a jq filter string, not `${{ }}` interpolation), so we
|
||||
* ban the interpolation form specifically, not the literal substring `head.sha`.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
|
||||
const WORKFLOW = path.resolve(__dirname, '..', '.github', 'workflows', 'pr-title-sync.yml');
|
||||
|
||||
/** Indentation width (count of leading spaces) of a line. */
|
||||
function indent(line: string): number {
|
||||
const m = line.match(/^( *)/);
|
||||
return m ? m[1].length : 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the lines that live inside a `run:` block, each tagged with its 1-based
|
||||
* line number. Handles both `run: |` (multiline) and `run: <inline command>`.
|
||||
*/
|
||||
function runBlockLines(content: string): Array<{ n: number; text: string }> {
|
||||
const lines = content.split('\n');
|
||||
const out: Array<{ n: number; text: string }> = [];
|
||||
let inRun = false;
|
||||
let runIndent = -1;
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const line = lines[i];
|
||||
const n = i + 1;
|
||||
const inlineRun = line.match(/^(\s*)run:\s*(\S.*)$/); // `run: echo foo`
|
||||
const blockRun = /^(\s*)run:\s*(\|>?[+-]?)?\s*$/.test(line); // `run: |`
|
||||
if (inlineRun && !/^\|/.test(inlineRun[2])) {
|
||||
out.push({ n, text: inlineRun[2] });
|
||||
inRun = false;
|
||||
continue;
|
||||
}
|
||||
if (blockRun) {
|
||||
inRun = true;
|
||||
runIndent = indent(line);
|
||||
continue;
|
||||
}
|
||||
if (inRun) {
|
||||
if (line.trim() === '') {
|
||||
out.push({ n, text: line });
|
||||
continue;
|
||||
}
|
||||
// Block ends when a non-empty line is indented at or below the `run:` key.
|
||||
if (indent(line) <= runIndent) {
|
||||
inRun = false;
|
||||
} else {
|
||||
out.push({ n, text: line });
|
||||
}
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
describe('pr-title-sync.yml pull_request_target safety', () => {
|
||||
const content = fs.readFileSync(WORKFLOW, 'utf-8');
|
||||
|
||||
test('workflow file exists', () => {
|
||||
expect(fs.existsSync(WORKFLOW)).toBe(true);
|
||||
});
|
||||
|
||||
test('does NOT check out the PR head ref (no fork-code execution)', () => {
|
||||
const offenders: string[] = [];
|
||||
content.split('\n').forEach((line, i) => {
|
||||
// A checkout `ref:` (or any `ref:`) pointing at the PR head is the footgun.
|
||||
if (/ref:\s*\$\{\{[^}]*(pull_request\.head|head_ref)/.test(line)) {
|
||||
offenders.push(` L${i + 1}: ${line.trim()}`);
|
||||
}
|
||||
});
|
||||
if (offenders.length > 0) {
|
||||
throw new Error(
|
||||
`pr-title-sync.yml checks out the PR head under pull_request_target — that ` +
|
||||
`runs attacker-controlled fork code with a write token. Check out the base ` +
|
||||
`repo (no ref:) and read PR-head data via the API instead.\n` +
|
||||
offenders.join('\n'),
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('does NOT interpolate ${{ github.event.pull_request.* }} inside a run: block', () => {
|
||||
const offenders: string[] = [];
|
||||
for (const { n, text } of runBlockLines(content)) {
|
||||
if (/\$\{\{\s*github\.event\.pull_request/.test(text)) {
|
||||
offenders.push(` L${n}: ${text.trim()}`);
|
||||
}
|
||||
}
|
||||
if (offenders.length > 0) {
|
||||
throw new Error(
|
||||
`pr-title-sync.yml inlines an attacker-controlled PR field into a run: block ` +
|
||||
`— a crafted PR title/body executes as shell. Pass it via env: and ` +
|
||||
`reference "$VAR" (shell-quoted) instead.\n` +
|
||||
offenders.join('\n'),
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('uses pull_request_target (the hardening is actually present)', () => {
|
||||
// Positive assertion: if someone reverts to plain pull_request, the fork
|
||||
// backstop silently stops working (read-only token). Keep it intentional.
|
||||
expect(/^on:\s*$/m.test(content) || /\bpull_request_target\b/.test(content)).toBe(true);
|
||||
expect(content).toMatch(/\bpull_request_target\b/);
|
||||
});
|
||||
|
||||
test('passes the PR title through env:, not raw interpolation', () => {
|
||||
// The safe pattern: OLD_TITLE: ${{ github.event.pull_request.title }} in an
|
||||
// env: mapping, consumed as "$OLD_TITLE" in script.
|
||||
expect(content).toMatch(/env:/);
|
||||
expect(content).toMatch(/github\.event\.pull_request\.title/);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,92 @@
|
||||
/**
|
||||
* /ship PR-title-version rule is ALWAYS-LOADED — token-reduction safety net (gate, free).
|
||||
*
|
||||
* The anxiety this kills: the v1.54.0.0 carve ("carve /ship into skeleton +
|
||||
* on-demand sections, -59% always-loaded") moved the rule "PR title MUST start
|
||||
* with v$NEW_VERSION" out of the always-loaded monolith and entirely into the
|
||||
* lazily-loaded `ship/sections/pr-body.md`. The agent then sets the version
|
||||
* prefix only if it happens to read that section before creating the PR; when it
|
||||
* doesn't, PRs land with bare titles. This is the exact regression that shipped
|
||||
* — every recent open PR lacked a `v...` prefix until this guard + the skeleton
|
||||
* invariant restored it.
|
||||
*
|
||||
* This is the title-rule analogue of `test/auq-format-always-loaded.test.ts`,
|
||||
* which guards the AskUserQuestion format the same way. A carve that re-buries
|
||||
* the title rule fails here in milliseconds instead of surfacing weeks later as
|
||||
* a wave of version-less PR titles.
|
||||
*
|
||||
* The guarantee, made mechanical and per-PR:
|
||||
* 1. SKELETON PRESENCE — `ship/SKILL.md` (the always-loaded skeleton) carries
|
||||
* the invariant: the `v$NEW_VERSION` token, the single-source-of-truth
|
||||
* helper name, and a directive near a "PR title" mention. Present the
|
||||
* instant /ship reaches the push/PR steps, no section read required.
|
||||
* 2. UNION SURVIVAL (both paths) — the FULL procedure still exists somewhere
|
||||
* in skeleton+sections for BOTH the create path (`gh pr create --title
|
||||
* "v$NEW_VERSION ...`) AND the existing-PR update path (the `gh pr edit
|
||||
* --title` rewrite rule). A drop of either is the failure.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
|
||||
const ROOT = path.resolve(__dirname, '..');
|
||||
const SHIP_SKILL = path.join(ROOT, 'ship', 'SKILL.md');
|
||||
const SHIP_SECTIONS = path.join(ROOT, 'ship', 'sections');
|
||||
|
||||
function readUnion(): string {
|
||||
let union = fs.readFileSync(SHIP_SKILL, 'utf-8');
|
||||
for (const f of fs.readdirSync(SHIP_SECTIONS)) {
|
||||
if (f.endsWith('.md') && !f.endsWith('.md.tmpl')) {
|
||||
union += '\n' + fs.readFileSync(path.join(SHIP_SECTIONS, f), 'utf-8');
|
||||
}
|
||||
}
|
||||
return union;
|
||||
}
|
||||
|
||||
describe('/ship PR-title-version rule is always-loaded (token-reduction safety net)', () => {
|
||||
test('sanity: ship is a carved skill (has sections/*.md)', () => {
|
||||
// Guards against a path regression that would make the union/skeleton checks
|
||||
// vacuously pass against a non-carved skill.
|
||||
expect(fs.existsSync(SHIP_SECTIONS)).toBe(true);
|
||||
expect(fs.readdirSync(SHIP_SECTIONS).some(f => f.endsWith('.md'))).toBe(true);
|
||||
});
|
||||
|
||||
test('skeleton (ship/SKILL.md) carries the PR-title-version invariant', () => {
|
||||
const skeleton = fs.readFileSync(SHIP_SKILL, 'utf-8');
|
||||
// Robust independent markers, NOT one brittle full-sentence regex (so
|
||||
// rewording the prose doesn't false-fail). All three must be present in the
|
||||
// always-loaded skeleton.
|
||||
const markers: Array<{ name: string; re: RegExp }> = [
|
||||
{ name: 'v$NEW_VERSION token', re: /v\$NEW_VERSION/ },
|
||||
{ name: 'gstack-pr-title-rewrite helper reference', re: /gstack-pr-title-rewrite/ },
|
||||
{ name: 'a directive (MUST/always/never) near a PR-title mention', re: /(MUST|always|never)[^\n]{0,80}\btitle\b|\btitle\b[^\n]{0,80}(MUST|always|never)/i },
|
||||
];
|
||||
const missing = markers.filter(m => !m.re.test(skeleton));
|
||||
if (missing.length > 0) {
|
||||
throw new Error(
|
||||
`ship/SKILL.md (the always-loaded skeleton) is missing the PR-title-version ` +
|
||||
`invariant — a carve likely re-buried it in sections/. Missing:\n` +
|
||||
missing.map(m => ` - ${m.name} (${m.re.source})`).join('\n'),
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('union (skeleton+sections) keeps BOTH the create and the update title rules', () => {
|
||||
const union = readUnion();
|
||||
const paths: Array<{ name: string; re: RegExp }> = [
|
||||
// create path: `gh pr create --title "v$NEW_VERSION ...`
|
||||
{ name: 'PR create path prefixes the version', re: /pr create[^\n]*--title[^\n]*v\$NEW_VERSION/i },
|
||||
// update/idempotent path: the existing-PR `gh pr edit --title` rewrite rule
|
||||
{ name: 'PR update path rewrites the title', re: /pr edit[^\n]*--title/i },
|
||||
];
|
||||
const missing = paths.filter(p => !p.re.test(union));
|
||||
if (missing.length > 0) {
|
||||
throw new Error(
|
||||
`ship skeleton+sections dropped a PR-title-version code path. The update ` +
|
||||
`path is the more important idempotent /ship path — a create-only guard ` +
|
||||
`would miss its rot. Missing:\n` +
|
||||
missing.map(p => ` - ${p.name} (${p.re.source})`).join('\n'),
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user