Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions .github/workflows/q.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .github/workflows/q.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ on:
roles: [admin, maintainer, write]
slash_command:
name: q
events: [issues, issue_comment, pull_request_comment, discussion, discussion_comment]
reaction: rocket
status-comment: true
permissions:
Expand Down
23 changes: 22 additions & 1 deletion pkg/workflow/compiler_pre_activation_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ func (c *Compiler) applyPreActivationIfConditionGuards(data *WorkflowData, needs
// 2. The compiled on: section itself contains a GitHub Actions expression (contains ${{):
// event detection cannot be performed reliably at compile time.
if needsPermissionCheck && hasCommentEventInOn(data.On) && !botsContainExpression(data.Bots) && !strings.Contains(data.On, "${{") {
jobIfCondition = combinePreActivationIfCondition(RenderCondition(buildCommentAuthorAssociationCondition(data.Bots)), jobIfCondition)
jobIfCondition = combinePreActivationIfCondition(RenderCondition(buildCommentAuthorAssociationCondition(data.Bots, activeCommentEventsInOn(data.On))), jobIfCondition)
}
// Add optional skip-author-associations event guards as a job-level if condition.
// This compiles to a static expression so skipped runs exit early without pre-activation
Expand Down Expand Up @@ -577,6 +577,27 @@ func hasCommentEventInOn(on string) bool {
return strings.Contains(on, "issue_comment:") || strings.Contains(on, "pull_request_review_comment:")
}

// activeCommentEventsInOn returns the subset of guarded comment event names
// ("issue_comment", "pull_request_review_comment") that are present as trigger
// keys in the rendered on: section. The result is used to emit a precise
// author_association guard that only references events actually in the workflow.
//
// The on: string is compiler-generated YAML whose trigger keys always appear as
// top-level YAML keys followed immediately by a colon (e.g. "issue_comment:\n").
// Matching "<event>:" therefore reliably identifies a trigger key without
// false-positives from embedded user strings or comments — the same pattern used
// by the pre-existing hasCommentEventInOn helper.
func activeCommentEventsInOn(on string) []string {
var events []string
if strings.Contains(on, "issue_comment:") {
events = append(events, "issue_comment")
}
if strings.Contains(on, "pull_request_review_comment:") {
events = append(events, "pull_request_review_comment")
}
return events
}

// botsContainExpression reports whether any entry in bots is a GitHub Actions expression
// (i.e. contains "${{"). When true, the static author_association guard must be disabled so
// that check_membership always runs and evaluates the bot list at runtime.
Expand Down
54 changes: 43 additions & 11 deletions pkg/workflow/expression_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package workflow
import (
"encoding/json"
"fmt"
"slices"
"sort"
"strings"

Expand Down Expand Up @@ -164,28 +165,59 @@ func buildDispatchSourceEventCondition(includeIssues bool, includePullRequests b
// Actors listed in bots (from on.bots) are also exempted so that bot/app-triggered workflows
// continue to work even though bots rarely carry an OWNER/MEMBER/COLLABORATOR association.
//
// The generated expression (without bots) is:
// activeCommentEvents lists which comment events are actually in the compiled on: section
// (e.g. ["issue_comment", "pull_request_review_comment"]). Only events present in this list
// get a "not equal" guard clause; omitting an event that is not a trigger avoids dead-code
// conditions and keeps the generated expression consistent with the workflow's actual triggers.
//
// The generated expression for activeCommentEvents=["issue_comment","pull_request_review_comment"]
// (without bots) is:
//
// (github.event_name != 'issue_comment' && github.event_name != 'pull_request_review_comment')
// || contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
//
// For activeCommentEvents=["issue_comment"] the expression simplifies to:
//
// github.event_name != 'issue_comment'
// || contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
//
// With one or more bots an additional OR clause is appended for each bot:
//
// || github.actor == 'dependabot[bot]'
//
// This satisfies the RGS-004 rule (explicit author_association check for comment-triggered
// workflows) while remaining transparent to non-comment events such as push or schedule,
// and preserves existing on.bots allow-list behaviour.
func buildCommentAuthorAssociationCondition(bots []string) ConditionNode {
notIssueComment := BuildNotEquals(
BuildPropertyAccess("github.event_name"),
BuildStringLiteral("issue_comment"),
)
notPRReviewComment := BuildNotEquals(
BuildPropertyAccess("github.event_name"),
BuildStringLiteral("pull_request_review_comment"),
)
notCommentEvent := BuildAnd(notIssueComment, notPRReviewComment)
func buildCommentAuthorAssociationCondition(bots []string, activeCommentEvents []string) ConditionNode {
// Build "not equal" guards only for comment events that are active in this workflow.
// The canonical guarded events are issue_comment and pull_request_review_comment.
guardedEvents := []string{"issue_comment", "pull_request_review_comment"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guardedEvents is a hidden dependency on a list maintained in two places — adding a third guarded event type requires updating both this list and activeCommentEventsInOn(), with no compile-time enforcement linking them.

💡 Detail and suggested fix

guardedEvents in buildCommentAuthorAssociationCondition (line 194) and the two strings.Contains checks in activeCommentEventsInOn (lines 592–597) independently hardcode the same event name set. If a developer adds e.g. workflow_run_comment to guardedEvents to guard a future event type, they must also add a corresponding strings.Contains in activeCommentEventsInOn or the new event will never appear in the active list and its guard will silently be omitted.

Suggested: define a single package-level var guardedCommentEvents = []string{"issue_comment", "pull_request_review_comment"} and drive both functions from it, so the two sites cannot diverge.

var notCommentTerms []ConditionNode
for _, ev := range guardedEvents {
if slices.Contains(activeCommentEvents, ev) {
notCommentTerms = append(notCommentTerms, BuildNotEquals(
BuildPropertyAccess("github.event_name"),
BuildStringLiteral(ev),
))
}
}

var notCommentEvent ConditionNode
switch len(notCommentTerms) {
case 0:
// No guarded comment events are active; the condition is always true.
notCommentEvent = BuildBooleanLiteral(true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The case 0 branch (BuildBooleanLiteral(true)) is unreachable in practice — this function is only called when hasCommentEventInOn() returns true, guaranteeing at least one active comment event. If that guard is ever relaxed, case 0 would silently emit a permissive always-true expression, bypassing the author-association check entirely.

💡 Suggestion

Make the invariant explicit with a panic to fail loudly rather than silently permitting all actors:

case 0:
    // Unreachable: callers only invoke this function when hasCommentEventInOn() is
    // true, so activeCommentEvents always has at least one entry.
    panic("buildCommentAuthorAssociationCondition: no active comment events")

Alternatively, add a doc comment on the function signature declaring the precondition.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 0 silently drops the security guard when activeCommentEvents is empty — any future caller passing an empty slice will compile away the author-association check with no error or warning.

💡 Detail and suggested fix

The outer gate in compiler_pre_activation_job.go (line 521) currently prevents this path, but that's an undocumented precondition enforced at a distance. If any future call site passes an empty slice (e.g. from a different compiler path, a refactor, or a test), the function silently returns BuildBooleanLiteral(true). When ORed with authorizedAssoc, the full expression short-circuits to true — the guard is entirely removed.

The old code was fail-closed: it always emitted the guard. The new code is fail-open for the empty-input case. Security controls should default to the more restrictive posture.

Suggested fix — preserve the full guard as the fallback:

case 0:
    // No active events supplied; stay fail-closed by emitting the full unconditional guard.
    notCommentEvent = BuildAnd(
        BuildNotEquals(BuildPropertyAccess("github.event_name"), BuildStringLiteral("issue_comment")),
        BuildNotEquals(BuildPropertyAccess("github.event_name"), BuildStringLiteral("pull_request_review_comment")),
    )

Or add a panic/error so misuse is caught at test time rather than deployed silently.

case 1:
notCommentEvent = notCommentTerms[0]
default:
// Combine all terms with AND regardless of how many there are, so that
// adding a third guarded event type in the future doesn't silently drop it.
combined := notCommentTerms[0]
for _, term := range notCommentTerms[1:] {
combined = BuildAnd(combined, term)
}
notCommentEvent = combined
}

authorizedAssoc := BuildFunctionCall(
"contains",
Expand Down
Loading