feat(safe-outputs): add allowed-events filter to submit-pull-request-review#25484
feat(safe-outputs): add allowed-events filter to submit-pull-request-review#25484
allowed-events filter to submit-pull-request-review#25484Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/43196536-353b-438f-a615-21c245bcd453 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
allowed-events filter to submit-pull-request-review
There was a problem hiding this comment.
Pull request overview
Adds an allowed-events allowlist to the submit-pull-request-review safe-output to prevent agent-submitted review events (notably APPROVE) from bypassing human review requirements.
Changes:
- Extend Go safe-output config parsing to accept
allowed-eventsand propagate it into handler config. - Enforce
allowed_eventsat runtime in the JSsubmit_pr_reviewhandler (blocked events don’t consumemax). - Update schema, type definitions, tests, and docs to describe and validate the new option.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/submit_pr_review.go | Adds AllowedEvents to config and parses allowed-events from workflow frontmatter. |
| pkg/workflow/submit_pr_review_footer_test.go | Adds unit tests for parsing/normalization/invalid filtering of allowed-events. |
| pkg/workflow/compiler_safe_outputs_config.go | Emits allowed_events into JS handler configuration. |
| pkg/parser/schemas/main_workflow_schema.json | Adds allowed-events to schema with enum validation. |
| docs/src/content/docs/reference/safe-outputs.md | Documents allowed-events usage and intent. |
| docs/src/content/docs/reference/frontmatter-full.md | Documents the new frontmatter field in the full reference. |
| actions/setup/js/types/safe-outputs-config.d.ts | Adds TS type for allowed-events. |
| actions/setup/js/submit_pr_review.test.cjs | Adds handler tests for allow/deny behavior and max-slot handling. |
| actions/setup/js/submit_pr_review.cjs | Enforces allowed review event types via allowed_events. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/9 changed files
- Comments generated: 2
pkg/workflow/submit_pr_review.go
Outdated
| if eventsSlice, ok := allowedEvents.([]any); ok { | ||
| validEvents := map[string]bool{"APPROVE": true, "COMMENT": true, "REQUEST_CHANGES": true} | ||
| for _, e := range eventsSlice { | ||
| if eventStr, ok := e.(string); ok { | ||
| upper := strings.ToUpper(eventStr) | ||
| if validEvents[upper] { | ||
| config.AllowedEvents = append(config.AllowedEvents, upper) | ||
| } else { | ||
| submitPRReviewLog.Printf("Ignoring invalid allowed-events value: %s", eventStr) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
allowed-events currently fails open: if the key is present but all provided values are invalid/filtered out, AllowedEvents stays empty and the handler config omits allowed_events, which results in all review events being allowed. For a security control, this should fail closed (e.g., treat “allowed-events present but no valid values” as a configuration error, or explicitly encode an empty allowlist and have the JS handler interpret it as “deny all”).
| if eventsSlice, ok := allowedEvents.([]any); ok { | |
| validEvents := map[string]bool{"APPROVE": true, "COMMENT": true, "REQUEST_CHANGES": true} | |
| for _, e := range eventsSlice { | |
| if eventStr, ok := e.(string); ok { | |
| upper := strings.ToUpper(eventStr) | |
| if validEvents[upper] { | |
| config.AllowedEvents = append(config.AllowedEvents, upper) | |
| } else { | |
| submitPRReviewLog.Printf("Ignoring invalid allowed-events value: %s", eventStr) | |
| } | |
| } | |
| } | |
| } | |
| eventsSlice, ok := allowedEvents.([]any) | |
| if !ok { | |
| submitPRReviewLog.Printf("Invalid allowed-events configuration: must be a list of review event types") | |
| return nil | |
| } | |
| validEvents := map[string]bool{"APPROVE": true, "COMMENT": true, "REQUEST_CHANGES": true} | |
| for _, e := range eventsSlice { | |
| if eventStr, ok := e.(string); ok { | |
| upper := strings.ToUpper(eventStr) | |
| if validEvents[upper] { | |
| config.AllowedEvents = append(config.AllowedEvents, upper) | |
| } else { | |
| submitPRReviewLog.Printf("Ignoring invalid allowed-events value: %s", eventStr) | |
| } | |
| } | |
| } | |
| if len(config.AllowedEvents) == 0 { | |
| submitPRReviewLog.Printf("Invalid allowed-events configuration: at least one valid event type is required when allowed-events is specified") | |
| return nil | |
| } |
| AddIfNotEmpty("target", c.Target). | ||
| AddIfNotEmpty("target-repo", c.TargetRepoSlug). | ||
| AddStringSlice("allowed_repos", c.AllowedRepos). | ||
| AddStringSlice("allowed_events", c.AllowedEvents). | ||
| AddIfNotEmpty("github-token", c.GitHubToken). | ||
| AddStringPtr("footer", getEffectiveFooterString(c.Footer, cfg.Footer)). |
There was a problem hiding this comment.
There are JS + Go parse unit tests for allowed-events, but there’s no test asserting the compiler actually emits allowed_events into GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG for submit_pull_request_review. Add a focused test that sets SubmitPullRequestReview.AllowedEvents and verifies the generated handler config JSON contains the expected allowed_events array, to prevent regressions in the Go→JS config wiring.
|
@copilot review comments |
| "description": "List of additional repositories in format 'owner/repo' that PR reviews can be submitted in. When specified, the agent can use a 'repo' field in the output to specify which repository to submit the review in. The target repository (current or target-repo) is always implicitly allowed." | ||
| }, | ||
| "allowed-events": { | ||
| "type": "array", |
There was a problem hiding this comment.
Added minItems: 1 to the allowed-events schema in commit 5b475a7. Also addressed the security reviewer's fail-closed feedback in the same commit: the parser now returns nil if allowed-events is present but not a list, or if all provided values are invalid (rather than silently falling back to allowing all events).
…a constraint, compiler test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f51da69c-737b-4d35-b3dc-c9ce3c172474 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
AI agents running as
github-actions[bot]can submit APPROVE reviews that count toward branch protection requirements, bypassing human review gates. Prompt-level restrictions are a soft control vulnerable to injection or agent error.Changes
submit_pr_review.go): AddedAllowedEvents []stringfield; parsesallowed-eventsfrom frontmatter, normalizing to uppercase and dropping invalid values. Fails closed: ifallowed-eventsis present but not a list, or if all provided values are invalid, the config is rejected rather than falling back to allowing all events.compiler_safe_outputs_config.go): Passesallowed_eventsslice into handler configsubmit_pr_review.cjs): Enforcesallowed_eventsset — rejects disallowed events without consuming themaxcount slotmain_workflow_schema.json): Addedallowed-eventsproperty with enum[APPROVE, COMMENT, REQUEST_CHANGES]andminItems: 1constraintsafe-outputs-config.d.ts): Added"allowed-events"?: Array<"APPROVE" | "COMMENT" | "REQUEST_CHANGES">safe-outputs.mdandfrontmatter-full.mdUsage
When
allowed-eventsis omitted, all three event types remain allowed (backward compatible).