Conversation
…ncident addition Adds two new security reference docs and updates the incident playbook with the Trivy TeamPCP cascade. Pure docs change, no runtime impact. - **GITHUB-ACTIONS-SAFE-PATTERNS.md** (new, 198 lines): Workflow-injection vectors (PR title/body, branch name, comment body, head_commit.message), the `env:` binding fix, the inline-context anti-pattern. Scoped to the attacker-controlled `github.event.*` paths and explicit do/don't examples. - **SUPPLY-CHAIN-SAFE-PATTERNS.md** (new, 273 lines): Mutable-tag risk vs SHA-pinning, content-review-as-the-load- bearing-step (a SHA of a compromised release is still a valid SHA), upstream-trust ladder, dependency-update review process, PAT-token blast-radius hygiene. - **INCIDENT-PLAYBOOK.md** (modified): Trivy TeamPCP attack (2026-03-19) added as a second canonical case alongside tj-actions/changed-files (CVE-2025-30066). Notable because the target was a *security scanner itself* and SHA-pinned consumers were hit if they bumped during the compromise window — underscores why content review is load-bearing. ## Why fresh branch (split from #52) #52 was 289 commits ahead, 5 behind main with a 829-file diff because main expanded enormously since the branch forked. Per Aaron 2026-04-25 'redo the work and split, easier from master'. This is part 1 of 3 — security docs only. Semgrep rule (Rule 17 workflow-injection) will land in a separate PR; new workflow files (scorecard.yml, resume-diff.yml) in a third PR.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adds security-focused documentation to help keep dependency ingress (GitHub Actions, NuGet, toolchain installers) safe-by-default, and updates the incident playbook with an additional canonical mutable-tag compromise case.
Changes:
- Add new docs for GitHub Actions workflow-injection safe patterns and for supply-chain safe patterns.
- Update the incident response playbook to include the 2026-03-19 Trivy TeamPCP cascade as a canonical case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| docs/security/SUPPLY-CHAIN-SAFE-PATTERNS.md | New checklist + threat model for dependency adds/bumps across multiple ingress classes. |
| docs/security/GITHUB-ACTIONS-SAFE-PATTERNS.md | New workflow-injection checklist and explanations of the “env binding” mitigation pattern. |
| docs/security/INCIDENT-PLAYBOOK.md | Expands Playbook A canonical cases to include the Trivy incident details and cross-refs. |
| rebuild-trust signals. | ||
|
|
||
| Re-read against current revisions of these sources every 5-10 | ||
| rounds (FACTORY-HYGIENE row 41, same cadence as GHA safe-patterns). |
There was a problem hiding this comment.
The FACTORY-HYGIENE reference appears to point at the wrong row. Row #41 is “Orthogonal-axes audit”; the supply-chain safe-patterns audit is row #44 (which already cites this doc). Update this row number here (and the later repeats in the doc) so the cadence pointer stays correct.
| rounds (FACTORY-HYGIENE row 41, same cadence as GHA safe-patterns). | |
| rounds (FACTORY-HYGIENE row 44, same cadence as GHA safe-patterns). |
| doc (`docs/research/vuln-and-dep-scanner-landscape-2026- | ||
| 04-22.md`) — which defers Trivy adoption pending | ||
| rebuild-trust signals. |
There was a problem hiding this comment.
This inline code file path is split across two lines (...-2026- / 04-22.md), which makes copy/paste and search harder. Consider keeping referenced file paths unbroken on a single line (or using a reference-style link) so the pointer stays usable.
| doc (`docs/research/vuln-and-dep-scanner-landscape-2026- | |
| 04-22.md`) — which defers Trivy adoption pending | |
| rebuild-trust signals. | |
| doc (`docs/research/vuln-and-dep-scanner-landscape-2026-04-22.md`) | |
| — which defers Trivy adoption pending rebuild-trust | |
| signals. |
| elan over hand-rolled `curl | bash`; when `curl | bash` is | ||
| unavoidable, pin the raw URL by SHA-256. |
There was a problem hiding this comment.
This checklist item says “when curl | bash is unavoidable…”, which contradicts the repo’s security guidance to never use pipe-to-shell from external URLs. Please remove the “unavoidable” exception and describe the safe alternative (download → review → verify digest/signature → execute).
| elan over hand-rolled `curl | bash`; when `curl | bash` is | |
| unavoidable, pin the raw URL by SHA-256. | |
| elan over hand-rolled installer scripts. If an upstream | |
| only provides a script, download it first, review it, | |
| verify its SHA-256 digest or signature against the | |
| upstream-published value, and only then execute it | |
| locally — never pipe an external URL directly to a shell. |
| - **Cadenced audit target** — FACTORY-HYGIENE row 40 cadences a | ||
| re-read against the blog's current revision so drift is caught. | ||
| - **Reviewer citation** — a CI YAML review can reject with "violates |
There was a problem hiding this comment.
| `.semgrep.yml`. Two GitHub-Actions rules: | ||
| - `gha-action-mutable-tag` — catches `uses: foo@v1` / `@main` | ||
| instead of a 40-char SHA (supply-chain vector). | ||
| - `gha-untrusted-in-run-line` — catches single-line | ||
| `run: ... ${{ github.<unsafe-path> }} ...` forms for the | ||
| attacker-controlled context list enumerated above (PR | ||
| titles, issue bodies, branch refs, commit messages, etc.). | ||
| Runs ahead of CodeQL, so injection is caught at PR time even | ||
| when CodeQL is on its weekly cadence. Multi-line `run: |` | ||
| blocks are left to actionlint's YAML-aware parser. |
There was a problem hiding this comment.
This section states Semgrep has two GitHub-Actions rules (gha-action-mutable-tag and gha-untrusted-in-run-line), but .semgrep.yml currently only defines gha-action-mutable-tag. Either add a note that the second rule is planned/pending, or remove it until the rule actually lands to avoid docs/tooling drift.
| `.semgrep.yml`. Two GitHub-Actions rules: | |
| - `gha-action-mutable-tag` — catches `uses: foo@v1` / `@main` | |
| instead of a 40-char SHA (supply-chain vector). | |
| - `gha-untrusted-in-run-line` — catches single-line | |
| `run: ... ${{ github.<unsafe-path> }} ...` forms for the | |
| attacker-controlled context list enumerated above (PR | |
| titles, issue bodies, branch refs, commit messages, etc.). | |
| Runs ahead of CodeQL, so injection is caught at PR time even | |
| when CodeQL is on its weekly cadence. Multi-line `run: |` | |
| blocks are left to actionlint's YAML-aware parser. | |
| `.semgrep.yml`. Current GitHub-Actions rule: | |
| - `gha-action-mutable-tag` — catches `uses: foo@v1` / `@main` | |
| instead of a 40-char SHA (supply-chain vector). |
| which underscores why `docs/security/SUPPLY-CHAIN-SAFE- | ||
| PATTERNS.md` §"Content review is the load-bearing step" | ||
| matters: a SHA-256 of a compromised release is still a | ||
| valid SHA-256. We do not currently consume `trivy-action`; | ||
| scanner-adoption decisions are tracked in | ||
| `docs/research/vuln-and-dep-scanner-landscape-2026-04- | ||
| 22.md`, which defers Trivy pending rebuild-trust. |
There was a problem hiding this comment.
Several inline code file paths are hard-wrapped mid-token (e.g., docs/security/SUPPLY-CHAIN-SAFE- / PATTERNS.md and ...-2026-04- / 22.md). That makes the references harder to copy/paste and search. Please keep file paths unbroken on one line (or use a link) in this paragraph.
| which underscores why `docs/security/SUPPLY-CHAIN-SAFE- | |
| PATTERNS.md` §"Content review is the load-bearing step" | |
| matters: a SHA-256 of a compromised release is still a | |
| valid SHA-256. We do not currently consume `trivy-action`; | |
| scanner-adoption decisions are tracked in | |
| `docs/research/vuln-and-dep-scanner-landscape-2026-04- | |
| 22.md`, which defers Trivy pending rebuild-trust. | |
| which underscores why `docs/security/SUPPLY-CHAIN-SAFE-PATTERNS.md` | |
| §"Content review is the load-bearing step" matters: a SHA-256 | |
| of a compromised release is still a valid SHA-256. We do not | |
| currently consume `trivy-action`; scanner-adoption decisions | |
| are tracked in | |
| `docs/research/vuln-and-dep-scanner-landscape-2026-04-22.md`, | |
| which defers Trivy pending rebuild-trust. |
| 3. **Execute if clean**; record the validation decision in the | ||
| commit message or manifest comment. | ||
| 4. **SHA-256-pin the validated content** — the pin is then a | ||
| cache of your review. Any bump invalidates the cache and | ||
| forces a re-read. | ||
|
|
||
| The delivery mechanism (`curl | bash` vs `curl -o path && bash | ||
| path`) is not the risk. The risk is unvalidated content. |
There was a problem hiding this comment.
This section explicitly treats curl | bash as safe after review and downplays the delivery mechanism. That conflicts with the repo’s documented hard rule to never recommend pipe-to-shell, and it’s risky guidance in a security doc. Reword to forbid curl | bash and instead describe the safe pattern as: download to a file, review, verify digest/signature, then execute from disk.
| 3. **Execute if clean**; record the validation decision in the | |
| commit message or manifest comment. | |
| 4. **SHA-256-pin the validated content** — the pin is then a | |
| cache of your review. Any bump invalidates the cache and | |
| forces a re-read. | |
| The delivery mechanism (`curl | bash` vs `curl -o path && bash | |
| path`) is not the risk. The risk is unvalidated content. | |
| 3. **Verify authenticity and integrity before execution** — | |
| prefer a published signature when available, and otherwise | |
| verify the expected SHA-256 digest of the exact file you | |
| reviewed. | |
| 4. **Execute only from disk after review and verification**; | |
| record the validation decision in the commit message or | |
| manifest comment. | |
| 5. **SHA-256-pin the validated content** — the pin is then a | |
| cache of your review. Any bump invalidates the cache and | |
| forces a re-read. | |
| Do **not** use `curl | bash` or any other pipe-to-shell pattern | |
| for external content. The safe pattern is: download to a file, | |
| review it, verify its digest and/or signature, then execute that | |
| reviewed file from disk. |
| In this factory, Aaron's standing policy (2026-04-22) is: *"never | ||
| run a script you download without checking it for vulnerability, | ||
| trojans and things of that nature even like gist and stuff, it's | ||
| fine to download and run bash and things like that just validate | ||
| them first."* So the actual author-time protocol is: |
There was a problem hiding this comment.
Directly naming a contributor (“Aaron”) here violates the repo convention to avoid name attribution in docs/skill bodies (use role refs like “human maintainer”). Consider paraphrasing the policy without quoting a person, and link/cite the canonical policy doc instead.
| In this factory, Aaron's standing policy (2026-04-22) is: *"never | |
| run a script you download without checking it for vulnerability, | |
| trojans and things of that nature even like gist and stuff, it's | |
| fine to download and run bash and things like that just validate | |
| them first."* So the actual author-time protocol is: | |
| In this factory, the standing policy is to never run a downloaded | |
| script before validating it for vulnerabilities, trojans, and | |
| similar hazards; see `AGENTS.md` for the canonical repo policy. | |
| So the actual author-time protocol is: |
) Adds a Semgrep rule for the primary workflow-injection vector (per https://github.blog/security/vulnerability-research/how-to- catch-github-actions-workflow-injections-before-attackers-do/): inline untrusted github.event.* / github.head_ref expansion on a single-line `run:` shell command. Pattern matches: `run: ... \${{ github.<unsafe-path> }} ...` for the attacker-controlled context paths enumerated in docs/security/GITHUB-ACTIONS-SAFE-PATTERNS.md (PR title/body, branch name, comment body, head_commit.message, etc.). Multi-line `run: |` blocks are covered by actionlint's YAML-aware parser; this rule catches the single-line forms that slip past actionlint's structural matching. Fix the rule flags: bind the value to an `env:` block on the step and read it as `"\$VAR"` in shell. ## Why fresh branch (split from #52) #52 was 289 commits ahead of main with a 829-file diff. Per Aaron 2026-04-25 'redo the work and split, easier from master'. Part 2 of 3 of the #52 split. Composes with #475 (security docs) which is the docs companion to this rule — the rule's `message:` cites GITHUB-ACTIONS-SAFE-PATTERNS.md §Do / don't.
…52) (#477) Two new workflows from #52, split off as a fresh branch off main. - **scorecard.yml** (89 lines): OpenSSF Scorecard weekly project- health audit (~20 heuristic checks: branch protection, signed releases, dangerous workflows, pinned dependencies, CII best practices, SAST coverage, token permissions, maintained-ness). Results upload to GitHub Security → Code scanning as SARIF. Orthogonal to CVE scanners (Dependabot + `dotnet list --vulnerable`) — Scorecard audits *configuration*, not advisories. - **resume-diff.yml** (166 lines): FACTORY-RESUME claim-diff reviewer-helper. Triggers on PRs touching `docs/FACTORY-RESUME.md` or `docs/SHIPPED-VERIFICATION-CAPABILITIES.md` — the two files forming the factory's 'job-interview honesty' surface per Otto's resume memory. Emits a structured claim-level diff as a PR comment so reviewers can eyeball added/removed/modified claims for substantive drift. NOT a gate — judgment stays with the reviewer. ## Security review (both workflows) Both workflows comply with the pre-write checklist in `docs/security/GITHUB-ACTIONS-SAFE-PATTERNS.md` (landed in #475): - No attacker-controlled `${{ github.event.* }}` interpolation inside any `run:` block; all such values bound via `env:` and read as shell variables. - Third-party actions SHA-pinned. - Token permissions scoped (read-only at workflow level; explicit per-job grants for Scorecard / PR-comment write where needed). - Inputs limited to injection-proof types (commit SHAs / PR numbers). ## Why fresh branch (split from #52) Part 3 of 3 of #52's split per Aaron 2026-04-25 'redo the work and split, easier from master'. Composes with #475 (security docs) and #476 (Semgrep workflow-injection rule) — together these three PRs land the security/CI safe-patterns substrate that #52 originally proposed.
Summary
Three security docs from #52, split off as a fresh branch off main. Pure docs, no runtime impact.
Why fresh branch (split from #52)
#52 was 289 commits ahead / 5 behind main with a 829-file diff because main expanded enormously since the branch forked. Per Aaron 2026-04-25 "redo the work and split, easier from master".
This is part 1 of 3 of the #52 split. Coming next:
Test plan
🤖 Generated with Claude Code