docs: add sanitization, GITHUB_TOKEN recursion, synchronize gotchas#761
docs: add sanitization, GITHUB_TOKEN recursion, synchronize gotchas#761
Conversation
…ronize gotchas New patterns added to gh-aw skill: - Payload sanitization: use steps.<id>.outputs.text (sanitized) in pre-agent steps, never raw github.event.comment.body - GITHUB_TOKEN recursion boundary: GITHUB_TOKEN actions don't fire events (prevents infinite loops); GitHub App tokens and PATs DO - Comment-based state tracking for scheduled pollers: visible markdown status (⏳ - push trigger: bare on:push cost blowout warning, rapid push stacking - pull_request.synchronize gotchas: draft→ready doesn't fire it, base-ref edits fire edited not synchronize, approval dismissal on any push including content-identical rebases All validated against official GitHub docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review — PR #761
PR: docs: add sanitization, GITHUB_TOKEN recursion, synchronize gotchas
Scope: Documentation-only change to .claude/skills/gh-aw-guide/SKILL.md (+19/−1)
Findings (ranked by severity)
| # | Severity | Finding | Location |
|---|---|---|---|
| 1 | 🟡 MODERATE | "(sanitized)" label on steps.<id>.outputs.text is gh-aw-specific but reads as universal |
Line 105 |
| 2 | 🟡 MODERATE | "unsanitized input is acceptable in sandbox" understates prompt injection risk | Line 105 |
| 3 | 🟡 MODERATE | GITHUB_TOKEN "does NOT fire new workflow events" is overly absolute — workflow_dispatch/repository_dispatch are exceptions |
Lines 107, 382 |
| 4 | 🟢 MINOR | "Approval dismissal" bullet is a side effect of synchronize, not a case where it doesn't fire — misplaced in list |
Line 365 |
Discarded findings (single reviewer only, beyond follow-up cap):
- GITHUB_TOKEN recursion info duplicated across two locations (style)
- "codeflow branches" is non-standard terminology (style)
Methodology
3 independent reviewers with adversarial consensus:
- Initial round: 3 parallel reviewers with different models analyzed the full diff and source file
- Consensus: Findings agreed by 2+/3 included immediately; 1/3 findings sent to the other 2 reviewers for agree/disagree follow-up (capped at 3 disputes)
- Result: All 4 included findings reached 3/3 agreement after follow-up
CI Status
pre_activation: ✅ successactivation: ✅ successagent: 🔄 in progress (this review workflow)- Combined commit status: pending
Prior Reviews
No prior reviews on this PR.
Generated by Expert Code Review (auto) for issue #761 · ● 5.2M
|
|
||
| ### Payload Sanitization | ||
|
|
||
| Comment bodies, issue titles, and PR descriptions are **user-controlled untrusted input**. In pre-agent `steps:`, always use `steps.<id>.outputs.text` (sanitized) instead of raw `${{ github.event.comment.body }}`. Within the agent job itself, unsanitized input is acceptable because the agent runs in a sandboxed container — but pair with tight `safe-outputs:`. |
There was a problem hiding this comment.
🟡 MODERATE — Two related issues on this line. Flagged by: 3/3 reviewers (2 initial + 1 follow-up)
1. "(sanitized)" label is misleadingly absolute. The sanitization is specific to gh-aw's framework step output handling, not a general property of all GitHub Actions steps.<id>.outputs.*. A reader who internalizes this pattern and applies it outside gh-aw (e.g., a custom action that sets an output from untrusted input) would have a false sense of security.
Suggested fix: Qualify as steps.<id>.outputs.text (sanitized by gh-aw) to scope the safety claim.
2. "unsanitized input is acceptable because the agent runs in a sandboxed container" understates prompt injection risk. The sandbox prevents host runner exploitation, but it provides no protection against prompt injection. A malicious PR description or comment body passed directly to the agent can hijack agent reasoning and cause unintended safe-output actions. The sandbox reduces blast radius but does not eliminate injection risk.
Suggested fix: Replace with something like: "Within the agent job, the sandbox limits blast radius, but still constrain with tight safe-outputs: — raw user text is the primary prompt-injection vector."
| - **Draft → ready-for-review**: Fires `ready_for_review`, not `synchronize`. Workflows using default `types: [opened, synchronize, reopened]` won't re-run CI when a draft is marked ready | ||
| - **Base-ref edits**: Changing the PR's base branch fires `edited` (with `changes.base`), not `synchronize` | ||
| - **Pushes to the base branch**: Someone merging to `main` while your PR targets `main` does NOT fire `synchronize` on your PR — it fires `push` on `main`. Your CI won't re-run against the new base unless you push to your branch | ||
| - **Approval dismissal**: Branch protection's "Dismiss stale approvals on new commits" fires on the same head-SHA-changed event. A force-push that doesn't change file contents (rebase onto current `main`) still invalidates all prior approvals |
There was a problem hiding this comment.
🟢 MINOR — This bullet is misplaced in the "things that do NOT fire synchronize" list. Flagged by: 3/3 reviewers (1 initial + 2 follow-up)
The other bullets describe scenarios where synchronize does not fire (draft→ready, base-ref edits, base branch pushes). This bullet describes a side effect that happens after synchronize fires (branch protection dismissing stale approvals). Placing it here conflates event triggering with downstream policy behavior.
Suggested fix: Move to a separate callout or sub-section, e.g., "Side effects of synchronize" or a standalone note below the list.
|
|
||
| Comment bodies, issue titles, and PR descriptions are **user-controlled untrusted input**. In pre-agent `steps:`, always use `steps.<id>.outputs.text` (sanitized) instead of raw `${{ github.event.comment.body }}`. Within the agent job itself, unsanitized input is acceptable because the agent runs in a sandboxed container — but pair with tight `safe-outputs:`. | ||
|
|
||
| > 🛑 **Recursive workflow triggering**: Actions performed via `GITHUB_TOKEN` do **NOT** fire new workflow events (prevents infinite loops). Actions via GitHub App installation tokens or PATs **DO** fire events. This is why `github-token-for-extra-empty-commit:` requires a PAT — `GITHUB_TOKEN` pushes won't trigger CI on agent-created PRs. |
There was a problem hiding this comment.
🟡 MODERATE — GITHUB_TOKEN recursion claim is overly absolute. Flagged by: 3/3 reviewers (1 initial + 2 follow-up)
The statement "Actions performed via GITHUB_TOKEN do NOT fire new workflow events" is inaccurate. GitHub's documentation carves out explicit exceptions: calling the workflow_dispatch or repository_dispatch REST API endpoints with GITHUB_TOKEN will trigger new workflow runs. The restriction only prevents implicit side-effect events (e.g., a push from a token-authenticated git push won't re-fire push workflows).
A reader trusting this absolute claim could inadvertently create a dispatch-based recursive loop — exactly the kind of bug this guide should help prevent.
Suggested fix: Qualify the statement: "do not trigger most events (push, PR, comment, etc.) — but explicit dispatch events (workflow_dispatch, repository_dispatch) are exceptions."
Note: The same claim appears at line 382 (best practice #6) and should be updated there as well.
…kflow_run pwn detail Three patterns adopted into the trigger selection guide: - workflow_dispatch: branch selection is user-controlled — a write user can dispatch against a stale branch with weaker permissions or a friendlier prompt. The main-branch version is NOT what runs. - label_command: set min-integrity to none because the label application IS the human gate, replacing integrity filtering. - workflow_run: expanded from one-liner to full artifact-smuggling pwn scenario — sandboxed PR uploads malicious artifacts consumed by privileged downstream workflow. Treat all downloaded artifacts as untrusted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tution to files
The steps: in gh-aw run in the agent job, but template substitution
(${{ steps.X.outputs.Y }}) happens in the activation job where
those step outputs don't exist yet. Result: the agent always saw
empty data, always called noop, and drift was never detected.
Fix: write script outputs to /tmp/drift-results/*.json files instead
of GITHUB_OUTPUT template substitution. The agent reads the files
at runtime since steps: and the agent share the same runner.
Also: stop suppressing stderr (was 2>/dev/null), redirect to
error log files for debugging. Print result previews in step output.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Adds sanitization, GITHUB_TOKEN recursion boundary, comment-based state tracking, push cost blowout, and synchronize gotchas to gh-aw skill.