test(gh-aw): enforce shell input security contract over compiled output (#1834) - #1866
Conversation
The contract in workflows/squad.md was declared but unenforced. This adds a fail-closed gate that compiles the real workflow with `gh aw compile --strict` and scans the compiled `run:` blocks for attacker-controlled event expressions (UNTRUSTED_TEMPLATE_IN_RUN), plus scans the runtime-imported /squad parser source for the printf/eval/bash-c/awk hops that gh-aw never inlines into the lock. A committed positive-control fixture proves the gate can turn red; failures name token + file + line. Missing gh aw, absent lock, or zero inspected surfaces all fail rather than skip. Closes #1834 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🏗️ Architectural Review
Automated architectural review — informational only. |
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 1 active Copilot thread(s) resolved (2 outdated skipped) |
| ✅ | CI passing | All checks passing |
Files Changed (5 files, +821 −51)
| File | +/− |
|---|---|
.github/workflows/squad-ci.yml |
+6 −4 |
test/fixtures/gh-aw-shell-contract/violating.lock.yml |
+42 −0 |
test/gh-aw-quality.test.ts |
+404 −43 |
test/gh-aw-shell-contract.ts |
+349 −0 |
workflows/squad.md |
+20 −4 |
Total: +821 −51
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🟡 Impact Analysis — PR #1866Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
root (1 file)
tests (3 files)
This report is generated automatically for every PR. See #733 for details. |
There was a problem hiding this comment.
🟡 Changes recommended
The new scanner has confirmed bypass/accuracy issues (tainted-variable matching and printf -- handling, plus run-block extraction behavior) that could let violations slip or produce incorrect scan results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a fail-closed security gate to enforce the “Shell input security contract [MANDATORY]” by scanning (1) compiled run: blocks in squad.lock.yml and (2) runtime-imported parser shell in workflows/squad.md, with a positive-control fixture proving the gate can turn red.
Changes:
- Add a new scanner module to extract
run:blocks / parser shell lines and detect the four contract anti-pattern tokens. - Strengthen
gh-aw-qualitytests to compile viagh aw, scan both enforcement surfaces, and fail closed (no more silent skips). - Update contract documentation and CI commentary to reflect the new enforcement and its compiled/runtime-import split.
File summaries
| File | Description |
|---|---|
workflows/squad.md |
Updates contract text to accurately describe the now-implemented, two-surface gate. |
test/gh-aw-shell-contract.ts |
New scanner that extracts/inspects compiled run: blocks and runtime-imported parser shell to detect contract violations. |
test/gh-aw-quality.test.ts |
Adds the fail-closed gate tests, including real compilation, parser-source scan, and positive/negative controls. |
test/fixtures/gh-aw-shell-contract/violating.lock.yml |
Positive-control compiled-shaped fixture proving the gate can fail and report token/file/line. |
.github/workflows/squad-ci.yml |
Updates comment to reflect the gate now fails closed and depends on installing gh-aw. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review found three cases where the #1834 gate reported green because the detector never looked - the same "unmeasured is indistinguishable from clean" failure the gate exists to eliminate. - BODY_VAR allowed only one segment before the _BODY/_TITLE suffix, so multi-segment carriers ($SQUAD_EVENT_BODY, $GITHUB_EVENT_ISSUE_BODY) bypassed every detector while the doc comment claimed "any *_BODY". Since event carriers are conventionally multi-segment SCREAMING_SNAKE, the unenforced shape was the idiomatic one. - firstPrintfArg() read `--` as the format operand, so `printf -- "$body"` (the idiom used when a body may start with `-`) never had its real format slot inspected. Option stripping now loops and honors `--`. - extractRunBlocks() never advanced the cursor past a consumed block scalar, so a `run:` line inside a heredoc re-entered the header branch, opening a phantom block that double-reported violations and inflated the block count the fail-closed assertion depends on. Adds 28 unit tests over the exported scanner (no `gh aw`, never skippable). Mutation-verified: reverting each fix kills its tests (17 / 4 / 2), and over-advancing the cursor kills the sibling-block guard. Sanctioned forms including `printf -- '%s\n' "$body"` stay green, so no permanent red. Closes #1834 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
What
Enforces
workflows/squad.md§"Shell input security contract [MANDATORY]" with a fail-closed gate over compiled gh-aw output. The contract was declared by #1832 but had no observer — this is the seventh sibling of the "reads as enforcement, catches nothing" defect (#1824/#1812/#1801/#1822/#1827/#1833). Closes #1834.How it works
The contract spans two artifacts, so the gate verifies two surfaces:
squad.lock.yml).UNTRUSTED_TEMPLATE_IN_RUNis a property of compiled output — GitHub Actions expands template expressions before the shell starts, so an attacker-controlledgithub.event.*.body/titleexpression left in a compiledrun:block is the observable failure. The gate compiles the real workflow withgh aw compile … --strict(reusing the existing harness) and scans everyrun:block.workflows/squad.md). Theprintf/eval/bash -c/awkhops live in the/squadparser one-liners, which gh-aw pulls in verbatim at runtime via{{#runtime-import}}and never inlines into the lock. That imported source is therefore the only surface on which those hops are observable, and the gate scans it directly. This is not a parallel compiler or a source proxy — those snippets are genuinely never compiled.Detectors key on body references (
github.event.(issue|comment|pull_request|discussion).(body|title)expressions, or body-shaped vars likeSQUAD_TRIGGER_BODY/*_BODY/*_TITLE/$body), not arbitrary$, so gh-aw's own machinery (bash -c 'export PATH="$PATH"',source "${RUNNER_TEMP}/…") does not false-positive.Failures name token + file + line + evidence and print a remediation
grepthat actually observes the violation.Fail-closed (every mode fails, never skips)
gh awmissing → hard fail with the exact install command (gh extension install github/gh-aw).squad.lock.yml→ fail.run:blocks inspected → fail (a scanner with nothing to scan is a permanently-green gate).The old
it.skipIf(!ghAwAvailable)— the exact "silently skipped while green" defect — is removed.Positive control (required by the issue)
test/fixtures/gh-aw-shell-contract/violating.lock.ymlhas 4run:blocks, one per token, including RETRO's mandated exact linerun: printf '%s\n' "${{ github.event.issue.body }}". The gate's own test exercises it, proving the gate can turn red rather than assuming it.Files
test/gh-aw-shell-contract.ts(new)extractRunBlocks,scanRunBlocks,scanShellLines,extractBodyHandlingShell,formatViolations.test/fixtures/gh-aw-shell-contract/violating.lock.yml(new)test/fixtures/so actionlint doesn't lint it.test/gh-aw-quality.test.tsworkflows/squad.md.github/workflows/squad-ci.ymlskipIf) and references #1834.Validation
vitest run test/gh-aw-quality.test.ts→ 115 passed (5 new contract tests green on real compiled output + real parser source).run: printf "${{ github.event.issue.body }}"into the compiled lock turned the gate RED, namingUNTRUSTED_TEMPLATE_IN_RUNandUNTRUSTED_PRINTF_FORMATatsquad.lock.yml:2418with evidence; removed → GREEN.tsc(bundler resolution + node types) andeslintclean on both changed test files.check-workflow-input-interpolation.mjspasses on the editedsquad.md.Honest limitation
The
printf/eval/awkhops cannot be observed in the compiled lock because gh-aw runtime-imports them; they are verified on the imported source instead. This is disclosed in the contract text, not hidden. No changeset needed — nopackages/*/srctouched.