Add pull_request_target security validation (pwn request detection)#29433
Add pull_request_target security validation (pwn request detection)#29433
Conversation
- Error in strict mode when checkout is not false (extremely insecure pwn request vector) - Warning in non-strict mode when checkout is not false - Warning always in strict mode that pull_request_target is a very dangerous trigger even with checkout: false - 7 test cases covering all scenarios Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a3b45cce-5f26-4c14-944f-be9230cce0f1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot make sure it works with imported shared agentic workflows, add integration workflows in pkg/cli/workflows |
…rget validation - Add test-pull-request-target-checkout-false.md fixture (PRT + imports + checkout:false) - Add test-pull-request-target-with-imports.md fixture (PRT + imports, no checkout:false) - Add compile_pull_request_target_integration_test.go with 4 integration sub-tests covering non-strict (warning) and strict (error) modes with imported shared workflows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/14bec267-67b9-4244-9a72-e297378d1a51 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in cffaacf. Added two workflow fixtures that include shared imports:
And Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new security validator to detect risky pull_request_target workflows (the “pwn request” footgun), emitting warnings in non-strict mode and escalating to hard errors in strict mode when PR code checkout is not explicitly disabled.
Changes:
- Added
validatePullRequestTargetTriggerto detectpull_request_targettriggers and enforce checkout safety rules. - Hooked the new validator into the permissions validation pipeline.
- Added unit + integration tests and workflow fixtures (including cases with shared-workflow imports).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/pull_request_target_validation.go | Implements pull_request_target trigger security validation (warn/error based on strictness + checkout state). |
| pkg/workflow/permissions_compiler_validator.go | Invokes the new validator during permission validation. |
| pkg/workflow/pull_request_target_validation_test.go | Adds unit tests for strict/non-strict behavior and checkout disabled/enabled variants. |
| pkg/cli/workflows/test-pull-request-target-checkout-false.md | Adds an integration fixture for pull_request_target with checkout: false and imports. |
| pkg/cli/workflows/test-pull-request-target-with-imports.md | Adds an integration fixture for pull_request_target with imports and checkout enabled. |
| pkg/cli/compile_pull_request_target_integration_test.go | Adds end-to-end integration coverage for warnings/errors with imported workflows. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
| onData, hasOn := parsedData["on"] | ||
| if !hasOn { | ||
| return nil | ||
| } | ||
|
|
||
| onMap, isMap := onData.(map[string]any) | ||
| if !isMap { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
validatePullRequestTargetTrigger only handles the case where the parsed on value is a map (i.e., on: { pull_request_target: ... }). Workflows can also legally declare events as a scalar (on: pull_request_target) or sequence (on: [pull_request_target]), which would make onData a string/slice and this validator would silently skip, missing the intended security diagnostic. Please extend the parsing to detect pull_request_target when on is a string or list (or consider inspecting workflowData.RawFrontmatter["on"] directly), and add a unit test covering the scalar/sequence forms.
| contents: read | ||
| pull-requests: read | ||
| engine: copilot | ||
| checkout: false |
There was a problem hiding this comment.
This fixture is used by the integration test's "non-strict mode" subtest (invoked without --strict). In this codebase, strict mode defaults to enabled unless frontmatter sets strict: false, so this fixture will actually run in strict mode by default and emit the dangerous-trigger warning, making the non-strict subtest not exercise the intended behavior. Add strict: false to the frontmatter so the test genuinely covers non-strict mode, while the strict subtest can continue to override it with --strict.
| checkout: false | |
| checkout: false | |
| strict: false |
| // The dangerous-trigger warning should also have been emitted before the error. | ||
| assert.True(t, | ||
| strings.Contains(combined, "very dangerous trigger") || | ||
| strings.Contains(combined, "extremely insecure"), | ||
| "output should contain security diagnostics") |
There was a problem hiding this comment.
This assertion is intended to verify the strict-mode output includes the dangerous-trigger warning in addition to the hard error, but as written it will always pass whenever the error message contains "extremely insecure" (which is already asserted above). Tighten this to explicitly require the dangerous-trigger warning text so the test actually validates that both diagnostics are emitted in strict mode.
| tests := []struct { | ||
| name string | ||
| frontmatter string | ||
| filename string | ||
| strictMode bool | ||
| expectError bool | ||
| expectWarning bool | ||
| errorContains string | ||
| warningCount int | ||
| }{ |
There was a problem hiding this comment.
The test case struct includes expectWarning but the test never asserts on it (only warningCount). Either remove expectWarning to reduce confusion, or add an assertion that matches the intended semantic (e.g., warningCount > 0 when expectWarning is true).
…ation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (522 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 📋 ADR Summary — what was capturedDecision: Add a dedicated
Key alternatives documented:
Normative requirements cover: trigger detection (fast-path + YAML parse), diagnostic rules per mode, error message content, and pipeline integration ordering. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25201985048
|
pull_request_targetruns with full write permissions and secret access on the base branch, making it a critical attack surface ("pwn request") when combined with checkout of untrusted PR code.Behavior
checkout: falsecheckout: falseIn strict mode, a warning is always emitted regardless of checkout state — even a
checkout: falseworkflow still runs with elevated privileges.Changes
pull_request_target_validation.go— newvalidatePullRequestTargetTriggermethod on*Compiler:validateWorkflowRunBranches), then YAML parse to confirm the trigger keycheckout: false)permissions_compiler_validator.go— calls the new validator as step 6 invalidatePermissions, aftervalidateWorkflowRunBranchespull_request_target_validation_test.go— 7 unit test cases covering both modes,checkout: falseon/off, and non-pull_request_targettriggers that must not regresspkg/cli/workflows/test-pull-request-target-checkout-false.md— integration workflow fixture:pull_request_targetwith shared-workflow imports andcheckout: falsepkg/cli/workflows/test-pull-request-target-with-imports.md— integration workflow fixture:pull_request_targetwith shared-workflow imports, nocheckout: falsecompile_pull_request_target_integration_test.go— 4 integration sub-tests confirming the validation works end-to-end when shared workflow imports are present, covering non-strict (warning) and strict (error) modes