docs: add roles: as security-critical pattern in gh-aw skill#724
docs: add roles: as security-critical pattern in gh-aw skill#724
Conversation
Feedback from jonathanpeppers on the blog post: roles: is security-critical and was missing from the quick-reference patterns section. Without it, any user (including the PR author) can trigger /review on a malicious PR. Also updated allowed-events guidance to present both COMMENT-only and REQUEST_CHANGES as valid options with tradeoffs, rather than only recommending COMMENT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📋 Design-Level Finding (Outside Diff)🟡 MODERATE — Anti-patterns table has no entry for The anti-patterns table (~line 60) has an entry for
|
![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 #724
Methodology: 3 independent reviewers with adversarial consensus. Disputed findings (flagged by only 1 reviewer) were escalated to the other 2 for verification before inclusion or discard.
Findings (ranked by severity)
| # | Severity | Finding | Consensus |
|---|---|---|---|
| 1 | 🟡 MODERATE | Pattern #1 description overstates risk of omitting roles: — the default is already [admin, maintainer, write] per architecture.md, so omission is not a security gap |
2/3 reviewers |
| 2 | 🟡 MODERATE | Pattern #2's commented REQUEST_CHANGES alternative contradicts the anti-patterns table in the same document |
2/3 reviewers |
| 3 | 🟡 MODERATE | Anti-patterns table has no entry for roles: allowlist to complement new Pattern #1 (outside diff — posted as design-level comment) |
2/3 reviewers (after follow-up) |
Discarded Findings (single reviewer only)
roles:YAML indentation wrong — Both follow-up reviewers confirmedon.roles:is a sibling ofslash_command:underon:, matchingarchitecture.md's "Authorization Model (on.roles:)" notation. The PR's indentation is correct.- Pattern #2 omits "no dismiss safe output" detail — Both follow-up reviewers agreed that "manually dismissed" sufficiently communicates the risk for a quick-reference pattern, with deeper details available in
architecture.md.
Context
- CI Status:
pre_activation✅activation✅agent🔄 in progress (this review) - Prior Reviews: None
- Scope: Documentation-only change (1 file, 19 additions, 6 deletions)
- Test Coverage: N/A — no code changes; renumbering is internally consistent with no broken cross-references
Overall Assessment
This is a valuable security documentation improvement — adding roles: as a first-class security-critical pattern addresses a real gap in the quick-reference. The two inline findings are about documentation accuracy relative to architecture.md and internal consistency within the same file. Neither represents a security regression in the advice given.
Generated by Expert Code Review (auto) for issue #724 · ● 5.8M
|
|
||
| **1. Prevent accidental PR approvals** — always restrict review workflows; otherwise the agent can approve PRs and bypass branch protection rules (gh-aw#25439). Use `[COMMENT]` to avoid stale blocking reviews that can't be dismissed: | ||
| **1. Role-based access control** — the `roles:` field controls who can trigger the workflow. Without it, any user (including the PR author) can trigger `/review` on a malicious PR designed to prompt-inject the reviewer: |
There was a problem hiding this comment.
🟡 MODERATE — Description overstates the risk of omitting roles: (Flagged by: 2/3 reviewers)
The text says "Without it, any user (including the PR author) can trigger /review on a malicious PR" — but architecture.md documents that on.roles: defaults to [admin, maintainer, write] (§ Authorization Model). Omitting roles: does not leave the workflow open; the default is already the restrictive set.
Failing scenario: A maintainer reads this, audits existing workflows missing explicit roles:, files false-positive security bugs — or worse, hastily adds roles: and accidentally sets a wrong value during the edit.
Suggested fix: Rephrase to reflect the actual default behavior, e.g.:
1. Role-based access control —
roles:controls who can trigger the workflow. The default ([admin, maintainer, write]) is secure, but always set it explicitly so it's visible during code review. Never useroles: allon workflows that process PR content — it gives any read-only user access to everything the workflow grants.
| # Or allow REQUEST_CHANGES for stronger merge-gating (but stale reviews persist until manually dismissed): | ||
| # allowed-events: [COMMENT, REQUEST_CHANGES] |
There was a problem hiding this comment.
🟡 MODERATE — Contradicts the anti-patterns table in the same document (Flagged by: 2/3 reviewers)
Presenting REQUEST_CHANGES as a valid alternative here conflicts with the anti-patterns table (~line 68, unchanged by this PR), which says:
allowed-events: [COMMENT]onsubmit-pull-request-review(prefer over[COMMENT, REQUEST_CHANGES]to avoid stale blocking reviews)
architecture.md also lists "Stale REQUEST_CHANGES reviews" as an explicit anti-pattern. A developer reading Pattern #2 first sees REQUEST_CHANGES as a viable option; a developer reading the anti-patterns table first sees it as something to avoid. The document is now internally inconsistent.
Suggested fix: Either (a) remove the commented-out REQUEST_CHANGES alternative to stay consistent with the anti-patterns table, or (b) update the anti-patterns table to reflect the nuanced tradeoff framing (e.g., "prefer [COMMENT]; use REQUEST_CHANGES only if merge-gating badge is needed — stale reviews require manual dismissal").
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
Superseded by #754 which includes this change plus integrity filtering, trigger guide, idempotency, and more. |
1 participant
Feedback from jonathanpeppers:
roles:is security-critical and was missing from the SKILL.md quick-reference. Also presents REQUEST_CHANGES as a valid option with tradeoffs.