Skip to content

Commit c497c9c

Browse files
bradygasterCopilot
andauthored
fix(gh-aw): harden hosted canary contracts (#2120)
* fix(gh-aw): harden hosted canary contracts Validate bootstrap output in isolation before trusted writes, authorize mutating command dispatches, and bind review relays to the latest exact base-controlled run attempt. Closes #2103 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(gh-aw): close review authorization gaps Closes #2103 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: ignore CommonMark tab-indented commands Closes #2103 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent e77c5f2 commit c497c9c

26 files changed

Lines changed: 625 additions & 178 deletions

‎.changeset/secure-gh-aw-canary.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@bradygaster/squad-cli": patch
3+
"@bradygaster/squad-sdk": patch
4+
---
5+
6+
Harden Squad workflow bootstrap, command routing, review relays, and installed workflow guidance for deterministic hosted canaries.

‎.squad-templates/skills/gh-aw-enlistment/SKILL.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}"
389389
# Safe-update report shows ONLY the two documented secrets + squad-init → approve once
390390
gh aw compile --strict --approve
391391
node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime
392-
gh aw compile --strict # final, no --approve; only the bot-trigger warning remains
392+
gh aw compile --strict # final, no --approve; exactly the two documented warnings remain
393393
node .github/workflows/shared/squad-install-verifier.mjs \
394394
--verify-install --source-revision "${SQUAD_SHA}" --strict-compile
395395

@@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER.
446446
else is a STOP.
447447
- ❌ **Treating `--approve` as the final compile.** Always finish with a plain
448448
`gh aw compile --strict` (no `--approve`).
449-
- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is
450-
allowed; every other warning or error halts the run.
449+
- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md`
450+
`pull_request_target` advisory and `squad.md` bot-trigger warning are allowed;
451+
every other warning or error halts the run.
451452
- ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the
452453
log `.gitignore` if missing.
453454
- ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated

‎packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}"
389389
# Safe-update report shows ONLY the two documented secrets + squad-init → approve once
390390
gh aw compile --strict --approve
391391
node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime
392-
gh aw compile --strict # final, no --approve; only the bot-trigger warning remains
392+
gh aw compile --strict # final, no --approve; exactly the two documented warnings remain
393393
node .github/workflows/shared/squad-install-verifier.mjs \
394394
--verify-install --source-revision "${SQUAD_SHA}" --strict-compile
395395

@@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER.
446446
else is a STOP.
447447
- ❌ **Treating `--approve` as the final compile.** Always finish with a plain
448448
`gh aw compile --strict` (no `--approve`).
449-
- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is
450-
allowed; every other warning or error halts the run.
449+
- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md`
450+
`pull_request_target` advisory and `squad.md` bot-trigger warning are allowed;
451+
every other warning or error halts the run.
451452
- ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the
452453
log `.gitignore` if missing.
453454
- ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated

‎packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}"
389389
# Safe-update report shows ONLY the two documented secrets + squad-init → approve once
390390
gh aw compile --strict --approve
391391
node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime
392-
gh aw compile --strict # final, no --approve; only the bot-trigger warning remains
392+
gh aw compile --strict # final, no --approve; exactly the two documented warnings remain
393393
node .github/workflows/shared/squad-install-verifier.mjs \
394394
--verify-install --source-revision "${SQUAD_SHA}" --strict-compile
395395

@@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER.
446446
else is a STOP.
447447
- ❌ **Treating `--approve` as the final compile.** Always finish with a plain
448448
`gh aw compile --strict` (no `--approve`).
449-
- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is
450-
allowed; every other warning or error halts the run.
449+
- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md`
450+
`pull_request_target` advisory and `squad.md` bot-trigger warning are allowed;
451+
every other warning or error halts the run.
451452
- ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the
452453
log `.gitignore` if missing.
453454
- ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated

‎scripts/gh-aw-hosted-e2e.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ export function waitForBaseControlledReviewCanary(
611611
]);
612612
if (run.event !== 'pull_request_target'
613613
|| run.path !== '.github/workflows/squad-review.lock.yml'
614-
|| run.head_sha !== castPr.headSha
614+
|| run.head_sha !== castPr.baseSha
615615
|| run.repository?.full_name !== target
616616
|| run.display_title !== `Squad review — PR #${castPr.number}`
617617
|| run.run_attempt < verdict.run_attempt) {

‎templates/skills/gh-aw-enlistment/SKILL.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}"
389389
# Safe-update report shows ONLY the two documented secrets + squad-init → approve once
390390
gh aw compile --strict --approve
391391
node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime
392-
gh aw compile --strict # final, no --approve; only the bot-trigger warning remains
392+
gh aw compile --strict # final, no --approve; exactly the two documented warnings remain
393393
node .github/workflows/shared/squad-install-verifier.mjs \
394394
--verify-install --source-revision "${SQUAD_SHA}" --strict-compile
395395

@@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER.
446446
else is a STOP.
447447
- ❌ **Treating `--approve` as the final compile.** Always finish with a plain
448448
`gh aw compile --strict` (no `--approve`).
449-
- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is
450-
allowed; every other warning or error halts the run.
449+
- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md`
450+
`pull_request_target` advisory and `squad.md` bot-trigger warning are allowed;
451+
every other warning or error halts the run.
451452
- ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the
452453
log `.gitignore` if missing.
453454
- ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated

‎test/gh-aw-bootstrap-workflow.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -848,6 +848,10 @@ describe('automatic Squad bootstrap workflow', () => {
848848
expect(lock).toContain('"payload_sha256"');
849849
expect(lock).not.toMatch(/"materialize-bootstrap":\{"inputs":\{"payload":/);
850850
expect(lock).toContain('reconstructBootstrapPayload(items[0])');
851+
expect(lock).toContain("mkdtempSync(join(tmpdir(), 'squad-bootstrap-candidate-'))");
852+
expect(lock).toContain('Bootstrap payload path crosses a symbolic link');
853+
expect(lock.indexOf("validate(payload, 'placeholder')"))
854+
.toBeLessThan(lock.indexOf('safeTarget(checkout, file.path)'));
851855
expect(lock).toContain('mcp-cli');
852856
expect(lock).toContain('--submit-envelope');
853857
expect(parse(lock).jobs.agent.env.DEFAULT_BRANCH).toBe('${{ github.event.repository.default_branch }}');
@@ -865,6 +869,28 @@ describe('automatic Squad bootstrap workflow', () => {
865869
expect(lock).not.toMatch(/\$\{\{[^}]*\\u00(?:26|3[cCeE])/);
866870
}, 180000);
867871

872+
it('rejects a realistic source mutation that writes to the checkout before validation', () => {
873+
const assertValidationBeforeCheckoutWrite = (source: string) => {
874+
const validation = source.indexOf("validate(payload, 'placeholder')");
875+
const candidateWrite = source.indexOf(
876+
"const target = safeTarget(candidate, file.path)",
877+
);
878+
const checkoutWrite = source.indexOf(
879+
"const target = safeTarget(checkout, file.path)",
880+
);
881+
expect(candidateWrite).toBeGreaterThan(-1);
882+
expect(candidateWrite).toBeLessThan(validation);
883+
expect(validation).toBeGreaterThan(-1);
884+
expect(checkoutWrite).toBeGreaterThan(validation);
885+
};
886+
assertValidationBeforeCheckoutWrite(WORKFLOW);
887+
const mutated = WORKFLOW.replace(
888+
"const target = safeTarget(candidate, file.path);",
889+
"const target = safeTarget(checkout, file.path);",
890+
);
891+
expect(() => assertValidationBeforeCheckoutWrite(mutated)).toThrow();
892+
});
893+
868894
it('detects a realistic compiled-lock mutation that removes the default-branch gate', () => {
869895
const mutated = WORKFLOW.replace(
870896
"if: github.ref_name == github.event.repository.default_branch\n",

‎test/gh-aw-command-parse.test.ts‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import { pathToFileURL } from 'node:url';
44
import { join } from 'node:path';
55
import {
66
classifySquadCommand,
7+
commandRequiresAuthorization,
78
enforceSquadCommandContract,
9+
isAuthorizedPermission,
810
rejectionComment,
911
} from '../workflows/shared/squad-command-contract.mjs';
1012

@@ -36,9 +38,10 @@ describe('gh-aw: shared /squad command contract (#1824)', () => {
3638
])('%s call site', (_name, payload) => {
3739
it.each([
3840
['/squad cast', 'cast', null],
41+
[' /squad cast ', 'cast', null],
3942
[' /squad cast ', 'cast', null],
43+
[' /squad cast ', 'cast', null],
4044
['Context first.\n\n/squad status ', 'status', null],
41-
['Please run /squad plan implementation', 'plan implementation', null],
4245
['/squad research Focus only on proposal P1', 'research', null],
4346
['/squad activate phase 2', 'activate', 2],
4447
['/squad plan accept implementation phase 12', 'plan accept implementation', 12],
@@ -82,6 +85,27 @@ describe('gh-aw: shared /squad command contract (#1824)', () => {
8285
});
8386
},
8487
);
88+
89+
it.each([
90+
['Please run /squad plan implementation', 'embedded prose'],
91+
['Use `/squad cast` after review.', 'inline code'],
92+
['```text\n/squad cast\n```', 'fenced code'],
93+
['~~~text\n/squad cast\n~~~', 'tilde-fenced code'],
94+
['````text\n```\n/squad cast\n````', 'code after a shorter backtick fence'],
95+
['~~~~text\n~~~\n/squad cast\n~~~~', 'code after a shorter tilde fence'],
96+
['````text\n~~~~\n/squad cast\n````', 'code after a different fence delimiter'],
97+
['````text\n```` trailing-info\n/squad cast\n````', 'code after a fence with trailing info'],
98+
[' /squad cast', 'four-space-indented code'],
99+
['\t/squad cast', 'tab-indented code'],
100+
[' \t/squad cast', 'one-space-plus-tab-indented code'],
101+
[' \t/squad cast', 'two-spaces-plus-tab-indented code'],
102+
[' \t/squad cast', 'three-spaces-plus-tab-indented code'],
103+
])('ignores %s instead of activating control-plane work', (body) => {
104+
expect(classifySquadCommand(payload(body), 'issues')).toEqual({
105+
status: 'none',
106+
source: _name === 'issue body' ? 'issue' : 'comment',
107+
});
108+
});
85109
});
86110

87111
describe('workflow_dispatch call site', () => {
@@ -186,6 +210,10 @@ describe('gh-aw: shared /squad command contract (#1824)', () => {
186210
expect(DISCOVERY_WORKFLOW).toContain("startsWith(github.event.comment.body, '/squad ')");
187211
expect(DISCOVERY_WORKFLOW).toContain('squad-command-contract.mjs');
188212
expect(DISCOVERY_WORKFLOW).toContain('classifySquadCommand');
213+
expect(DISCOVERY_WORKFLOW).toContain('commandRequiresAuthorization');
214+
expect(DISCOVERY_WORKFLOW).toContain('getCollaboratorPermissionLevel');
215+
expect(DISCOVERY_WORKFLOW).toContain('isAuthorizedPermission');
216+
expect(DISCOVERY_WORKFLOW).toContain('No standalone Squad command was found');
189217
expect(DISCOVERY_WORKFLOW).toContain('rejectionComment');
190218
expect(DISCOVERY_WORKFLOW).toContain('github.rest.issues.createComment');
191219
expect(DISCOVERY_WORKFLOW).toContain('core.setFailed(`Squad rejected command:');
@@ -194,6 +222,21 @@ describe('gh-aw: shared /squad command contract (#1824)', () => {
194222
expect(DISCOVERY_WORKFLOW).toContain("command: result.argumentText || 'cast'");
195223
});
196224

225+
it('keeps open modes public and fails mutating modes closed on collaborator permission', () => {
226+
for (const mode of ['status', 'review', 'research', 'plan', 'revoke-improvement']) {
227+
expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(false);
228+
}
229+
for (const mode of ['cast', 'triage', 'plan implementation', 'implement']) {
230+
expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(true);
231+
}
232+
expect(isAuthorizedPermission('admin')).toBe(true);
233+
expect(isAuthorizedPermission('maintain')).toBe(true);
234+
expect(isAuthorizedPermission('write')).toBe(true);
235+
expect(isAuthorizedPermission('triage')).toBe(false);
236+
expect(isAuthorizedPermission('read')).toBe(false);
237+
expect(isAuthorizedPermission('unresolved')).toBe(false);
238+
});
239+
197240
it('realistic source mutation is caught by the accepted-command fixtures', async () => {
198241
mkdirSync(TEST_ROOT, { recursive: true });
199242
const root = mkdtempSync(join(TEST_ROOT, 'command-contract-mutant-'));
@@ -218,4 +261,63 @@ describe('gh-aw: shared /squad command contract (#1824)', () => {
218261
rmSync(root, { recursive: true, force: true });
219262
}
220263
});
264+
265+
it('realistic fence-length mutation exposes a hidden command and is caught', async () => {
266+
mkdirSync(TEST_ROOT, { recursive: true });
267+
const root = mkdtempSync(join(TEST_ROOT, 'command-fence-mutant-'));
268+
try {
269+
const source = join(process.cwd(), 'workflows', 'shared', 'squad-command-contract.mjs');
270+
const mutant = join(root, 'squad-command-contract-mutant.mjs');
271+
cpSync(source, mutant);
272+
const original = readFileSync(mutant, 'utf8');
273+
const changed = original.replace(
274+
' && closingFence[1].length >= fence.length) {',
275+
' ) {',
276+
);
277+
expect(changed).not.toBe(original);
278+
writeFileSync(mutant, changed);
279+
const module = await import(`${pathToFileURL(mutant).href}?mutation=${Date.now()}`);
280+
const hidden = comment('````text\n```\n/squad cast\n````');
281+
expect(module.classifySquadCommand(hidden, 'issue_comment')).toMatchObject({
282+
status: 'accepted',
283+
mode: 'cast',
284+
});
285+
expect(classifySquadCommand(hidden, 'issue_comment')).toEqual({
286+
status: 'none',
287+
source: 'comment',
288+
});
289+
} finally {
290+
rmSync(root, { recursive: true, force: true });
291+
}
292+
});
293+
294+
it('realistic CommonMark tab-indentation mutation exposes hidden commands and is caught', async () => {
295+
mkdirSync(TEST_ROOT, { recursive: true });
296+
const root = mkdtempSync(join(TEST_ROOT, 'command-indent-mutant-'));
297+
try {
298+
const source = join(process.cwd(), 'workflows', 'shared', 'squad-command-contract.mjs');
299+
const mutant = join(root, 'squad-command-contract-mutant.mjs');
300+
cpSync(source, mutant);
301+
const original = readFileSync(mutant, 'utf8');
302+
const changed = original.replace(
303+
'/^(?: {4}| {0,3}\\t)/',
304+
'/^(?: {4}|\\t)/',
305+
);
306+
expect(changed).not.toBe(original);
307+
writeFileSync(mutant, changed);
308+
const module = await import(`${pathToFileURL(mutant).href}?mutation=${Date.now()}`);
309+
for (const body of [' \t/squad cast', ' \t/squad cast', ' \t/squad cast']) {
310+
expect(module.classifySquadCommand(comment(body), 'issue_comment')).toMatchObject({
311+
status: 'accepted',
312+
mode: 'cast',
313+
});
314+
expect(classifySquadCommand(comment(body), 'issue_comment')).toEqual({
315+
status: 'none',
316+
source: 'comment',
317+
});
318+
}
319+
} finally {
320+
rmSync(root, { recursive: true, force: true });
321+
}
322+
});
221323
});

‎test/gh-aw-hosted-e2e.test.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ describe('Squad gh-aw hosted E2E controller', () => {
245245

246246
it.each([
247247
'native-job-name', 'ui-context-name', 'wrong-check-id', 'wrong-head',
248-
'failed-check', 'incomplete-check', 'foreign-app', 'foreign-app-id',
248+
'wrong-run-head', 'failed-check', 'incomplete-check', 'foreign-app', 'foreign-app-id',
249249
])('exercises the hosted canary API contract: %s', mutation => {
250250
const target = 'owner/consumer';
251251
const verdict = {
@@ -257,7 +257,8 @@ describe('Squad gh-aw hosted E2E controller', () => {
257257
workflow_path: '.github/workflows/squad-review.lock.yml', run_id: 31, run_attempt: 1,
258258
};
259259
const run = {
260-
id: 31, event: verdict.event, path: verdict.workflow_path, head_sha: CAST_SHA,
260+
id: 31, event: verdict.event, path: verdict.workflow_path,
261+
head_sha: currentCastPr.baseSha,
261262
repository: { full_name: target }, run_attempt: 1,
262263
display_title: `Squad review — PR #${currentCastPr.number}`,
263264
};
@@ -273,6 +274,7 @@ describe('Squad gh-aw hosted E2E controller', () => {
273274
case 'ui-context-name': check.name = 'Squad Review / review'; break;
274275
case 'wrong-check-id': check.id = 124; break;
275276
case 'wrong-head': check.head_sha = TARGET_SHA; break;
277+
case 'wrong-run-head': run.head_sha = CAST_SHA; break;
276278
case 'failed-check': check.conclusion = 'failure'; break;
277279
case 'incomplete-check': check.status = 'in_progress'; break;
278280
case 'foreign-app': check.app.slug = 'other'; break;
@@ -304,10 +306,14 @@ describe('Squad gh-aw hosted E2E controller', () => {
304306
expect(JSON.parse(readFileSync(
305307
resolve(evidence, 'base-controlled-review-canary.json'), 'utf8',
306308
)).check.name).toBe('review');
309+
} else if (mutation === 'wrong-run-head') {
310+
expect(execute).toThrow('not bound to the base-controlled workflow run');
307311
} else {
308312
expect(execute).toThrow('not bound to the trusted review job');
309313
}
310-
expect(routes.at(-1)).toBe(`repos/${target}/check-runs/123`);
314+
expect(routes.at(-1)).toBe(mutation === 'wrong-run-head'
315+
? `repos/${target}/actions/runs/31`
316+
: `repos/${target}/check-runs/123`);
311317
} finally {
312318
rmSync(evidence, { recursive: true, force: true });
313319
}

‎test/gh-aw-quality.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3961,8 +3961,8 @@ describe('gh-aw: canonical package integrity contract', () => {
39613961
expect(mutable).not.toContain(actionReference);
39623962
expect(createHash('sha256').update(normalizeCompiledLock(mutable, revisionA)).digest('hex'))
39633963
.toBe(sourceBinding === 'workflow'
3964-
? '1aa070ca330d8f8f9f874855c75db410851b30a38efc7543090b697352eda1d2'
3965-
: 'bf54175720e6e9ab3fe5d5e3054c28f3a69595c95cdf50e56f9b30988158270f');
3964+
? '8456d8129241269c107cdf4b16ae079e89a98ef944152df36f53ec0decf3f30d'
3965+
: 'bac4d8b90269d827fc9e626dd02c1b536bccde7105e9037971740054cda4c19b');
39663966
expect(() => validateCompilerActionPins(mutable)).toThrow(/invalid immutable action pin/);
39673967
writeFileSync(lockPath, mutable);
39683968
expect(verifyInstall(root).failures.join('\n'))
@@ -4016,7 +4016,7 @@ describe('gh-aw: canonical package integrity contract', () => {
40164016
compile();
40174017
const unpinned = readText(lockPath);
40184018
expect(createHash('sha256').update(normalizeCompiledLock(unpinned, revisionA)).digest('hex'))
4019-
.toBe('bf54175720e6e9ab3fe5d5e3054c28f3a69595c95cdf50e56f9b30988158270f');
4019+
.toBe('bac4d8b90269d827fc9e626dd02c1b536bccde7105e9037971740054cda4c19b');
40204020
expect(verifyInstall(root).failures.join('\n')).toContain('Installed digest mismatch');
40214021

40224022
const seedPins = () => spawnSync(process.execPath, ['--input-type=module', '-e', seed!], {

0 commit comments

Comments
 (0)