-
Notifications
You must be signed in to change notification settings - Fork 347
feat: prevent GitHub App safe-outputs from self-cancelling issue_comment workflows #22825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -205,6 +205,48 @@ func entityConcurrencyKey(primaryParts []string, tailParts []string, hasItemNumb | |||||||||||||||||||||||
| return "${{ " + strings.Join(parts, " || ") + " }}" | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // botIsolatedConcurrencyKey builds a ${{ ... }} concurrency-group expression that | ||||||||||||||||||||||||
| // routes GitHub App bot-triggered runs to a unique github.run_id key, preventing | ||||||||||||||||||||||||
| // passive bot-authored comment events from colliding with the primary run's group. | ||||||||||||||||||||||||
| // When contains(github.actor, '[bot]') is true, the expression short-circuits to | ||||||||||||||||||||||||
| // github.run_id so that bot-triggered runs never share a group with human runs. | ||||||||||||||||||||||||
| func botIsolatedConcurrencyKey(primaryParts []string, tailParts []string, hasItemNumber bool) string { | ||||||||||||||||||||||||
| parts := make([]string, 0, len(primaryParts)+len(tailParts)+2) | ||||||||||||||||||||||||
| // Prepend the bot-actor isolation check: bot runs always get a unique key | ||||||||||||||||||||||||
| parts = append(parts, "contains(github.actor, '[bot]') && github.run_id") | ||||||||||||||||||||||||
| parts = append(parts, primaryParts...) | ||||||||||||||||||||||||
| if hasItemNumber { | ||||||||||||||||||||||||
| parts = append(parts, "inputs.item_number") | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| parts = append(parts, tailParts...) | ||||||||||||||||||||||||
| return "${{ " + strings.Join(parts, " || ") + " }}" | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // hasIssueCommentTrigger returns true when the workflow has any trigger that uses | ||||||||||||||||||||||||
| // issue_comment events: explicit issue_comment, slash_command, or command triggers. | ||||||||||||||||||||||||
| // These are the only triggers where a GitHub App-authored comment on an issue can | ||||||||||||||||||||||||
| // re-enter the same workflow and match the primary run's workflow-level concurrency | ||||||||||||||||||||||||
| // group, creating the self-cancellation risk described in the issue. | ||||||||||||||||||||||||
| func hasIssueCommentTrigger(workflowData *WorkflowData) bool { | ||||||||||||||||||||||||
| on := workflowData.On | ||||||||||||||||||||||||
| return strings.Contains(on, "issue_comment") || | ||||||||||||||||||||||||
| strings.Contains(on, "slash_command") || | ||||||||||||||||||||||||
| len(workflowData.Command) > 0 | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // hasBotSelfCancelRisk returns true when the workflow's auto-generated concurrency | ||||||||||||||||||||||||
| // configuration can be collided with by a passive GitHub App bot-authored event. | ||||||||||||||||||||||||
| // The dangerous combination is: issue_comment triggers + GitHub App safe-outputs. | ||||||||||||||||||||||||
| // When this combination is present, App-authored comments posted by safe-outputs can | ||||||||||||||||||||||||
| // re-trigger the workflow and resolve to the same workflow-level concurrency group | ||||||||||||||||||||||||
| // as the originating run, causing cancel-in-progress to cancel the original run. | ||||||||||||||||||||||||
| func hasBotSelfCancelRisk(workflowData *WorkflowData) bool { | ||||||||||||||||||||||||
| if workflowData.SafeOutputs == nil || workflowData.SafeOutputs.GitHubApp == nil { | ||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return hasIssueCommentTrigger(workflowData) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // buildConcurrencyGroupKeys builds an array of keys for the concurrency group | ||||||||||||||||||||||||
| func buildConcurrencyGroupKeys(workflowData *WorkflowData, isCommandTrigger bool) []string { | ||||||||||||||||||||||||
| keys := []string{"gh-aw", "${{ github.workflow }}"} | ||||||||||||||||||||||||
|
|
@@ -214,16 +256,40 @@ func buildConcurrencyGroupKeys(workflowData *WorkflowData, isCommandTrigger bool | |||||||||||||||||||||||
| // use distinct groups and don't cancel each other. | ||||||||||||||||||||||||
| hasItemNumber := workflowData.HasDispatchItemNumber | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Detect whether the workflow is at risk of bot-self-cancellation. | ||||||||||||||||||||||||
| // When safe-outputs uses a GitHub App token AND the workflow has issue_comment triggers, | ||||||||||||||||||||||||
| // App-authored comments can re-trigger the workflow and collide with the primary run's | ||||||||||||||||||||||||
| // concurrency group. When this risk is present we use botIsolatedConcurrencyKey so that | ||||||||||||||||||||||||
| // bot-triggered runs always resolve to a unique github.run_id key instead. | ||||||||||||||||||||||||
| botRisk := hasBotSelfCancelRisk(workflowData) | ||||||||||||||||||||||||
| if botRisk { | ||||||||||||||||||||||||
| concurrencyLog.Print("Bot self-cancel risk detected: applying bot-actor isolation to concurrency group key") | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // entityKey selects the correct concurrency key builder based on bot risk. | ||||||||||||||||||||||||
| // It captures both botRisk and hasItemNumber from the outer scope, so call | ||||||||||||||||||||||||
| // sites only need to supply the entity-specific parts. | ||||||||||||||||||||||||
| entityKey := func(primaryParts []string, tailParts []string) string { | ||||||||||||||||||||||||
| if botRisk { | ||||||||||||||||||||||||
| return botIsolatedConcurrencyKey(primaryParts, tailParts, hasItemNumber) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return entityConcurrencyKey(primaryParts, tailParts, hasItemNumber) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if isCommandTrigger || isSlashCommandWorkflow(workflowData.On) { | ||||||||||||||||||||||||
| // For command/slash_command workflows: use issue/PR number; fall back to run_id when | ||||||||||||||||||||||||
| // neither is available (e.g. manual workflow_dispatch of the outer workflow). | ||||||||||||||||||||||||
| keys = append(keys, "${{ github.event.issue.number || github.event.pull_request.number || github.run_id }}") | ||||||||||||||||||||||||
| // When bot risk is detected, prepend the bot-actor isolation check. | ||||||||||||||||||||||||
| if botRisk { | ||||||||||||||||||||||||
| keys = append(keys, "${{ contains(github.actor, '[bot]') && github.run_id || github.event.issue.number || github.event.pull_request.number || github.run_id }}") | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| keys = append(keys, "${{ github.event.issue.number || github.event.pull_request.number || github.run_id }}") | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+282
to
+287
|
||||||||||||||||||||||||
| // When bot risk is detected, prepend the bot-actor isolation check. | |
| if botRisk { | |
| keys = append(keys, "${{ contains(github.actor, '[bot]') && github.run_id || github.event.issue.number || github.event.pull_request.number || github.run_id }}") | |
| } else { | |
| keys = append(keys, "${{ github.event.issue.number || github.event.pull_request.number || github.run_id }}") | |
| } | |
| // Bot-actor isolation is applied via entityKey when botRisk is detected. | |
| keys = append(keys, entityKey( | |
| []string{"github.event.issue.number", "github.event.pull_request.number"}, | |
| []string{"github.run_id"}, | |
| )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new warning path isn’t covered by tests. There are existing warning-count tests in this package; adding a unit test that asserts (1) warning is emitted/incremented for a risky workflow with custom concurrency +
cancel-in-progress: true, and (2) no warning when bot isolation is already present in the custom group, would help prevent regressions.This issue also appears on line 232 of the same file.