Flag issue_comment as a dangerous trigger#1614
Flag issue_comment as a dangerous trigger#1614dguido wants to merge 4 commits intozizmorcore:mainfrom
Conversation
Add issue_comment to the dangerous-triggers audit. Like pull_request_target and workflow_run, issue_comment workflows run in the base repository context with access to secrets. Uses Medium severity / Medium confidence (lower than the existing High/Medium for pull_request_target and workflow_run) to acknowledge that issue_comment has more legitimate uses. Closes zizmorcore#1606 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the `on: [push, issue_comment]` (BareEvents) variant to ensure all three Trigger enum arms are exercised. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
woodruffw
left a comment
There was a problem hiding this comment.
I'm vaguely worried this is going to be too noisy/disruptive, given how common issue_comment is. I'm not inherently opposed to it, but I think I'd like to add more "crater" tests to understand the impact before merging this.
(An alternative would be to push ahead with an approach like #935.)
Instead of flagging every issue_comment workflow unconditionally, analyze each job for an author_association guard in its `if:` condition. Jobs that check `github.event.comment.author_association` are suppressed since this server-controlled field restricts execution to trusted actors. Findings now point at the unguarded job rather than the workflow's `on:` key, giving more actionable output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ract Inline issue_comment job-level logic into audit_workflow instead of overriding both audit_workflow and audit_normal_job, which violated the trait contract. The guard now also validates that author_association is compared against a trusted role (OWNER, MEMBER, COLLABORATOR) rather than suppressing findings for any reference to the context. Remove duplicate test fixture (issue-comment-no-guard.yml was identical to issue-comment-bare.yml) and add a weak-guard test for CONTRIBUTOR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Pushed a new iteration that addresses code quality issues and should also help with the noise concern:
Agreed that crater testing before merge makes sense—happy to wait on that. The trusted-role validation in this iteration should make the audit more precise, which may help with the noise concern. |
Pre-submission checks
Summary
issue_commentto thedangerous-triggersaudit at Medium severity / Medium confidence (Regular persona), acknowledging it has more legitimate uses thanpull_request_targetorworkflow_runwhile still flagging the security riskhas_issue_comment()method to the Workflow model following existinghas_pull_request_target()/has_workflow_run()patterndangerous_triggersintegration test module with tests for all three trigger typesCloses #1606
Test Plan
issue_commentflagged as bare event (on: issue_comment)issue_commentflagged with event body (on: issue_comment: types: [created])pull_request_targetstill flagged at High severityworkflow_runstill flagged at High severitypush) produce no findingstemplate_injection/issue-418-reprosnapshot updated (usesissue_comment)