diff --git a/actions/setup/js/check_rate_limit.cjs b/actions/setup/js/check_rate_limit.cjs index 00003165f12..44d1d62e640 100644 --- a/actions/setup/js/check_rate_limit.cjs +++ b/actions/setup/js/check_rate_limit.cjs @@ -9,7 +9,8 @@ const { fetchAndLogRateLimit } = require("./github_rate_limit_logger.cjs"); * Prevents users from triggering workflows too frequently */ -const PROGRAMMATIC_EVENTS = ["workflow_dispatch", "repository_dispatch", "issue_comment", "pull_request_review", "pull_request_review_comment", "discussion_comment"]; +// Keep in sync with pkg/workflow/role_checks.go and the user-rate-limit.events schema enum. +const PROGRAMMATIC_EVENTS = ["discussion", "discussion_comment", "issue_comment", "issues", "pull_request", "pull_request_review", "pull_request_review_comment", "repository_dispatch", "workflow_dispatch"]; async function main() { const { diff --git a/actions/setup/js/check_rate_limit.test.cjs b/actions/setup/js/check_rate_limit.test.cjs index 6f909788b94..4658004421d 100644 --- a/actions/setup/js/check_rate_limit.test.cjs +++ b/actions/setup/js/check_rate_limit.test.cjs @@ -751,14 +751,18 @@ describe("check_rate_limit", () => { expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); }); - it("should skip non-programmatic events like pull_request by default", async () => { + it("should apply rate limiting to pull_request events by default", async () => { mockContext.eventName = "pull_request"; + mockGithub.rest.actions.listWorkflowRuns.mockResolvedValue({ + data: { workflow_runs: [] }, + }); + await checkRateLimit.main(); expect(mockCore.setOutput).toHaveBeenCalledWith("rate_limit_ok", "true"); - expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Event 'pull_request' is not a programmatic trigger")); - expect(mockGithub.rest.actions.listWorkflowRuns).not.toHaveBeenCalled(); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Rate limiting applies to programmatic events")); + expect(mockGithub.rest.actions.listWorkflowRuns).toHaveBeenCalled(); }); it("should log stack trace for errors that have one", async () => { diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 1ac0470c2d3..9a35a9f1b14 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -22002,8 +22002,8 @@ user-rate-limit: window: 1 # Optional list of event types to apply rate limiting to. If not specified, rate - # limiting applies to all programmatically triggered events (e.g., - # workflow_dispatch, issue_comment, pull_request_review). + # limiting is inferred from the workflow triggers; if no supported programmatic + # triggers are found, it falls back to all supported programmatic events. # (optional) events: [] # Array of strings diff --git a/docs/src/content/docs/reference/frontmatter.md b/docs/src/content/docs/reference/frontmatter.md index e69d3c9b50c..112ab4f3a93 100644 --- a/docs/src/content/docs/reference/frontmatter.md +++ b/docs/src/content/docs/reference/frontmatter.md @@ -499,13 +499,13 @@ max-daily-ai-credits: -1 ### Per-User Rate Limiting (`user-rate-limit:`) -Limits how frequently a single user can trigger the workflow. When the limit is exceeded, the pre-activation job cancels the run before the agent executes. Rate limiting applies to programmatically triggered events (such as `workflow_dispatch`, `issue_comment`, and `pull_request_review`); when `events` is omitted, the applicable events are inferred from the `on:` section. +Limits how frequently a single user can trigger the workflow. When the limit is exceeded, the pre-activation job cancels the run before the agent executes. Rate limiting applies to programmatically triggered events (such as `workflow_dispatch`, `issue_comment`, and `pull_request_review`); when `events` is omitted, the applicable events are inferred from the `on:` section, falling back to all supported programmatic events if no supported triggers are found. ```yaml wrap user-rate-limit: max-runs-per-window: 5 # Required: maximum runs per user per window (1-10) window: 60 # Optional: window in minutes (default: 60, max: 180) - events: [workflow_dispatch, issue_comment] # Optional: events to rate limit (inferred from `on:` when omitted) + events: [workflow_dispatch, issue_comment] # Optional: events to rate limit (inferred from `on:` when omitted; fallback to all supported programmatic events) ignored-roles: [admin, maintain] # Optional: exempt roles (default: [admin, maintain, write]) ``` @@ -513,7 +513,7 @@ user-rate-limit: Users with any of the `ignored-roles` are not rate limited. The default exemptions are `admin`, `maintain`, and `write`; set `ignored-roles: []` to rate limit every user, including administrators. -Legacy frontmatter that used a top-level `rate-limit:` section, or `max:`/`max-runs:` instead of `max-runs-per-window:`, is migrated automatically by `gh aw fix`. +Legacy frontmatter that used a top-level `rate-limit:` section, or `max:`/`max-runs:` instead of `max-runs-per-window:`, must be migrated with `gh aw fix`. See [Rate Limiting and Controls](/gh-aw/reference/rate-limiting-controls/) for more details. diff --git a/docs/src/content/docs/reference/rate-limiting-controls.md b/docs/src/content/docs/reference/rate-limiting-controls.md index 28bad3eb3a9..1319fd99cfa 100644 --- a/docs/src/content/docs/reference/rate-limiting-controls.md +++ b/docs/src/content/docs/reference/rate-limiting-controls.md @@ -117,7 +117,7 @@ The `user-rate-limit` frontmatter field prevents users from triggering workflows user-rate-limit: max-runs-per-window: 5 # Required: Maximum runs per window (1-10) window: 60 # Optional: Time window in minutes (default: 60, max: 180) - events: [workflow_dispatch, issue_comment] # Optional: Specific events (auto-inferred if omitted) + events: [workflow_dispatch, issue_comment] # Optional: Specific events (inferred from `on:` when omitted; fallback to all supported programmatic events) ignored-roles: [admin, maintain] # Optional: Roles exempt from rate limiting (default: [admin, maintain, write]) ``` diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index 40748f9b058..9f5043344b4 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -112,6 +112,68 @@ func TestValidateMainWorkflowFrontmatter_Plugins(t *testing.T) { } } +func TestValidateMainWorkflowFrontmatter_UserRateLimitMaxField(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + rateLimit map[string]any + wantErr bool + errContains string + }{ + { + name: "canonical max-runs-per-window", + rateLimit: map[string]any{ + "max-runs-per-window": 5, + }, + }, + { + name: "legacy max-runs alias", + rateLimit: map[string]any{"max-runs": 5}, + wantErr: true, + errContains: "Unknown property: max-runs", + }, + { + name: "legacy max alias", + rateLimit: map[string]any{"max": 5}, + wantErr: true, + errContains: "Unknown property: max", + }, + { + name: "legacy max-runs expression alias", + rateLimit: map[string]any{"max-runs": "${{ inputs.max_runs }}"}, + wantErr: true, + errContains: "Unknown property: max-runs", + }, + { + name: "missing max field", + rateLimit: map[string]any{"window": 60}, + wantErr: true, + }, + { + name: "unknown nested field", + rateLimit: map[string]any{"max-runs-per-window": 5, "limit": 5}, + wantErr: true, + errContains: "Unknown property: limit", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(map[string]any{ + "on": "workflow_dispatch", + "user-rate-limit": tt.rateLimit, + }, "workflow.md") + if (err != nil) != tt.wantErr { + t.Fatalf("validation error = %v, wantErr %t", err, tt.wantErr) + } + if tt.errContains != "" && !strings.Contains(err.Error(), tt.errContains) { + t.Fatalf("validation error = %v, want substring %q", err, tt.errContains) + } + }) + } +} + func TestValidateMainWorkflowFrontmatterEnclaves(t *testing.T) { valid := map[string]any{ "on": "workflow_dispatch", diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 6f879808cd1..5d3e9fba123 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -12371,7 +12371,7 @@ }, "events": { "type": "array", - "description": "Optional list of event types to apply rate limiting to. If not specified, rate limiting applies to all programmatically triggered events (e.g., workflow_dispatch, issue_comment, pull_request_review).", + "description": "Optional list of event types to apply rate limiting to. If not specified, rate limiting is inferred from the workflow triggers; if no supported programmatic triggers are found, it falls back to all supported programmatic events.", "items": { "type": "string", "enum": ["workflow_dispatch", "issue_comment", "pull_request_review", "pull_request_review_comment", "issues", "pull_request", "discussion_comment", "discussion", "repository_dispatch"] diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index 3571aca403d..cd804cf4496 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -15,6 +15,19 @@ import ( var roleLog = logger.New("workflow:role_checks") +// Keep in sync with actions/setup/js/check_rate_limit.cjs and the user-rate-limit.events schema enum. +var rateLimitProgrammaticEvents = []string{ + "discussion", + "discussion_comment", + "issue_comment", + "issues", + "pull_request", + "pull_request_review", + "pull_request_review_comment", + "repository_dispatch", + "workflow_dispatch", +} + // generateMembershipCheck generates steps for the check_membership job that only sets outputs func (c *Compiler) generateMembershipCheck(data *WorkflowData, steps []string) []string { if len(data.Command) > 0 { @@ -221,86 +234,76 @@ func parseOptionalStringSliceField(value any, fieldName string) []string { // extractRateLimitConfig extracts the user-rate-limit config from frontmatter. func (c *Compiler) extractRateLimitConfig(frontmatter map[string]any) *RateLimitConfig { rateLimitValue, exists := frontmatter["user-rate-limit"] + if !exists || rateLimitValue == nil { + roleLog.Print("No user-rate-limit configuration specified") + return nil + } - if exists && rateLimitValue != nil { - switch v := rateLimitValue.(type) { - case map[string]any: - config := &RateLimitConfig{} + rateLimitMap, ok := rateLimitValue.(map[string]any) + if !ok { + roleLog.Printf("user-rate-limit value is not an object, ignoring configuration: %T", rateLimitValue) + return nil + } - // Extract max-runs-per-window (default: 5) - maxValue, ok := v["max-runs-per-window"] - if !ok { - maxValue, ok = v["max-runs"] - } - if !ok { - maxValue, ok = v["max"] // legacy compatibility - } - if ok { - switch max := maxValue.(type) { - case int: - config.Max = max - case int64: - config.Max = int(max) - case uint64: - config.Max = int(max) - case float64: - config.Max = int(max) - } - } + config := &RateLimitConfig{ + Max: extractRateLimitInt(rateLimitMap, "max-runs-per-window"), + Window: extractRateLimitInt(rateLimitMap, "window"), + Events: c.extractRateLimitEvents(rateLimitMap, frontmatter), + IgnoredRoles: extractRateLimitIgnoredRoles(rateLimitMap), + } - // Extract window (default: 60 minutes) - if windowValue, ok := v["window"]; ok { - switch window := windowValue.(type) { - case int: - config.Window = window - case int64: - config.Window = int(window) - case uint64: - config.Window = int(window) - case float64: - config.Window = int(window) - } - } + roleLog.Printf("Extracted user-rate-limit config: max=%d, window=%d, events=%v, ignored-roles=%v", config.Max, config.Window, config.Events, config.IgnoredRoles) + return config +} - // Extract events - if eventsValue, ok := v["events"]; ok { - switch events := eventsValue.(type) { - case []any: - config.Events = parseStringSliceAny(events, nil) - case []string: - config.Events = events - case string: - config.Events = []string{events} - } - } else { - // If events not specified, infer from the 'on:' section of frontmatter - config.Events = c.inferEventsFromTriggers(frontmatter) - if len(config.Events) > 0 { - roleLog.Printf("Inferred events from workflow triggers: %v", config.Events) - } +func extractRateLimitInt(config map[string]any, keys ...string) int { + for _, key := range keys { + if value, ok := config[key]; ok { + switch typedValue := value.(type) { + case int: + return typedValue + case int64: + return int(typedValue) + case uint64: + return int(typedValue) + case float64: + return int(typedValue) } + } + } + return 0 +} - // Extract ignored-roles - if ignoredRolesValue, ok := v["ignored-roles"]; ok { - switch ignoredRoles := ignoredRolesValue.(type) { - case []any: - config.IgnoredRoles = parseStringSliceAny(ignoredRoles, nil) - case []string: - config.IgnoredRoles = ignoredRoles - case string: - config.IgnoredRoles = []string{ignoredRoles} - } - } else { - // Default: admin, maintain, and write roles are exempt from rate limiting - config.IgnoredRoles = []string{"admin", "maintain", "write"} - roleLog.Print("No ignored-roles specified, using defaults: admin, maintain, write") - } +func (c *Compiler) extractRateLimitEvents(config map[string]any, frontmatter map[string]any) []string { + if eventsValue, ok := config["events"]; ok { + return extractRateLimitStringSlice(eventsValue) + } - roleLog.Printf("Extracted user-rate-limit config: max=%d, window=%d, events=%v, ignored-roles=%v", config.Max, config.Window, config.Events, config.IgnoredRoles) - return config - } + events := c.inferEventsFromTriggers(frontmatter) + if len(events) > 0 { + roleLog.Printf("Inferred events from workflow triggers: %v", events) + } + return events +} + +func extractRateLimitIgnoredRoles(config map[string]any) []string { + if ignoredRolesValue, ok := config["ignored-roles"]; ok { + return extractRateLimitStringSlice(ignoredRolesValue) + } + + roleLog.Print("No ignored-roles specified, using defaults: admin, maintain, write") + return []string{"admin", "maintain", "write"} +} + +func extractRateLimitStringSlice(value any) []string { + switch typedValue := value.(type) { + case []any: + return parseStringSliceAny(typedValue, nil) + case []string: + return typedValue + case string: + return []string{typedValue} } - roleLog.Print("No user-rate-limit configuration specified") return nil } @@ -308,20 +311,13 @@ func (c *Compiler) extractRateLimitConfig(frontmatter map[string]any) *RateLimit func (c *Compiler) inferEventsFromTriggers(frontmatter map[string]any) []string { onValue, exists := frontmatter["on"] if !exists || onValue == nil { - return nil + return append([]string(nil), rateLimitProgrammaticEvents...) } var events []string - programmaticTriggers := map[string]string{ - "discussion": "discussion", - "discussion_comment": "discussion_comment", - "issue_comment": "issue_comment", - "issues": "issues", - "pull_request": "pull_request", - "pull_request_review": "pull_request_review", - "pull_request_review_comment": "pull_request_review_comment", - "repository_dispatch": "repository_dispatch", - "workflow_dispatch": "workflow_dispatch", + programmaticTriggers := make(map[string]string, len(rateLimitProgrammaticEvents)) + for _, event := range rateLimitProgrammaticEvents { + programmaticTriggers[event] = event } switch on := onValue.(type) { @@ -347,6 +343,10 @@ func (c *Compiler) inferEventsFromTriggers(frontmatter map[string]any) []string // Sort events alphabetically for consistent output sort.Strings(events) + if len(events) == 0 { + // If "on" exists but has no supported rate-limit triggers, keep the omitted-events fallback broad. + return append([]string(nil), rateLimitProgrammaticEvents...) + } return events } @@ -385,78 +385,56 @@ func (c *Compiler) hasSafeEventsOnly(data *WorkflowData, frontmatter map[string] // Parse the "on" section to determine events if onValue, exists := frontmatter["on"]; exists { if onMap, ok := onValue.(map[string]any); ok { - // Check if only safe events are present - hasUnsafeEvents := false - hasWorkflowDispatch := false - - for eventName := range onMap { - // Skip command events as they are handled separately - // Skip stop-after and reaction as they are not event types - // Skip roles, bots, labels, and other configuration keys as they are not event types - if eventName == "command" || eventName == "stop-after" || eventName == "reaction" || eventName == "roles" || eventName == "bots" || eventName == "labels" || eventName == "allow-bot-authored-trigger-comment" { - continue - } + return hasOnlySafeOnMapEvents(onMap, data.Roles) + } + } - // Track if workflow_dispatch is present - if eventName == "workflow_dispatch" { - hasWorkflowDispatch = true - } + // If no "on" section or it's a string, check for default command trigger + // For command workflows, they are not considered "safe only" + return false +} - // Check if this event is in the safe list - isSafe := slices.Contains(constants.SafeWorkflowEvents, eventName) - if !isSafe { - hasUnsafeEvents = true - break - } - } +func hasOnlySafeOnMapEvents(onMap map[string]any, roles []string) bool { + hasUnsafeEvents := false + hasWorkflowDispatch := false - // If there are events and none are unsafe, then it's safe - eventCount := len(onMap) - // Subtract non-event entries - if _, hasSlashCommand := onMap["slash_command"]; hasSlashCommand { - eventCount-- - } - if _, hasCommand := onMap["command"]; hasCommand { - eventCount-- - } - if _, hasStopAfter := onMap["stop-after"]; hasStopAfter { - eventCount-- - } - if _, hasReaction := onMap["reaction"]; hasReaction { - eventCount-- - } - if _, hasRoles := onMap["roles"]; hasRoles { - eventCount-- - } - if _, hasBots := onMap["bots"]; hasBots { - eventCount-- - } - if _, hasLabelNames := onMap["labels"]; hasLabelNames { - eventCount-- - } - if _, hasAllowBotAuthored := onMap["allow-bot-authored-trigger-comment"]; hasAllowBotAuthored { - eventCount-- - } + for eventName := range onMap { + if isRoleCheckConfigKey(eventName) { + continue + } + if eventName == "workflow_dispatch" { + hasWorkflowDispatch = true + } + if !slices.Contains(constants.SafeWorkflowEvents, eventName) { + hasUnsafeEvents = true + break + } + } - // Special handling for workflow_dispatch: - // workflow_dispatch can be triggered by users with "write" access, - // so it's only considered "safe" if "write" is in the allowed roles - if hasWorkflowDispatch && !hasUnsafeEvents { - // Check if "write" is in the allowed roles - hasWriteRole := slices.Contains(data.Roles, "write") - // If write is not in the allowed roles, workflow_dispatch needs permission checks - if !hasWriteRole { - return false - } - } + if hasWorkflowDispatch && !hasUnsafeEvents && !slices.Contains(roles, "write") { + return false + } + return countWorkflowEvents(onMap) > 0 && !hasUnsafeEvents +} - return eventCount > 0 && !hasUnsafeEvents - } +func isRoleCheckConfigKey(key string) bool { + switch key { + case "allow-bot-authored-trigger-comment", "bots", "command", "labels", "reaction", "roles", "stop-after": + return true + default: + return false } +} - // If no "on" section or it's a string, check for default command trigger - // For command workflows, they are not considered "safe only" - return false +func countWorkflowEvents(onMap map[string]any) int { + eventCount := 0 + for eventName := range onMap { + // slash_command is not a standalone GitHub event, but the safety scan still treats it as unsafe. + if !isRoleCheckConfigKey(eventName) && eventName != "slash_command" { + eventCount++ + } + } + return eventCount } // hasWorkflowRunTrigger checks if the agentic workflow's frontmatter declares a workflow_run trigger diff --git a/pkg/workflow/role_checks_test.go b/pkg/workflow/role_checks_test.go index 13cf2db2950..f044f1fc286 100644 --- a/pkg/workflow/role_checks_test.go +++ b/pkg/workflow/role_checks_test.go @@ -3,8 +3,10 @@ package workflow import ( + "encoding/json" "os" "path/filepath" + "regexp" "strings" "testing" @@ -12,6 +14,24 @@ import ( "github.com/stretchr/testify/assert" ) +func TestRateLimitProgrammaticEventsMatchJavaScript(t *testing.T) { + source, err := os.ReadFile(filepath.Join("..", "..", "actions", "setup", "js", "check_rate_limit.cjs")) + if err != nil { + t.Fatal(err) + } + + match := regexp.MustCompile(`const PROGRAMMATIC_EVENTS = (\[[^;]+\]);`).FindSubmatch(source) + if len(match) != 2 { + t.Fatal("PROGRAMMATIC_EVENTS not found in check_rate_limit.cjs") + } + + var javascriptEvents []string + if err := json.Unmarshal(match[1], &javascriptEvents); err != nil { + t.Fatal(err) + } + assert.Equal(t, rateLimitProgrammaticEvents, javascriptEvents) +} + // TestRoleMembershipUsesGitHubToken tests that the role membership check // explicitly uses the GitHub Actions token (GITHUB_TOKEN) and not any other secret func TestRoleMembershipUsesGitHubToken(t *testing.T) { @@ -221,16 +241,39 @@ func TestInferEventsFromTriggers(t *testing.T) { expected: []string{"issues"}, }, { - name: "no triggers", + name: "no recognized triggers falls back to all programmatic triggers", frontmatter: map[string]any{ - "on": map[string]any{}, + "on": map[string]any{ + "push": map[string]any{}, + "schedule": "daily", + }, + }, + expected: []string{ + "discussion", + "discussion_comment", + "issue_comment", + "issues", + "pull_request", + "pull_request_review", + "pull_request_review_comment", + "repository_dispatch", + "workflow_dispatch", }, - expected: nil, }, { name: "missing on section", frontmatter: map[string]any{}, - expected: nil, + expected: []string{ + "discussion", + "discussion_comment", + "issue_comment", + "issues", + "pull_request", + "pull_request_review", + "pull_request_review_comment", + "repository_dispatch", + "workflow_dispatch", + }, }, { name: "all programmatic triggers", @@ -291,6 +334,30 @@ func TestExtractRateLimitConfig(t *testing.T) { } }) + t.Run("ignores legacy max-runs alias", func(t *testing.T) { + cfg := c.extractRateLimitConfig(map[string]any{ + "user-rate-limit": map[string]any{ + "max-runs": 4, + }, + }) + + if assert.NotNil(t, cfg) { + assert.Zero(t, cfg.Max) + } + }) + + t.Run("ignores legacy max alias", func(t *testing.T) { + cfg := c.extractRateLimitConfig(map[string]any{ + "user-rate-limit": map[string]any{ + "max": 2, + }, + }) + + if assert.NotNil(t, cfg) { + assert.Zero(t, cfg.Max) + } + }) + t.Run("legacy rate-limit key is ignored", func(t *testing.T) { cfg := c.extractRateLimitConfig(map[string]any{ "rate-limit": map[string]any{