Flag pull_request → pull_request_target trigger escalation in compiler safe-update analysis#44925
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds safe-update detection for escalation from pull_request to pull_request_target.
Changes:
- Records pull-request trigger metadata in lock-file manifests.
- Enforces escalation checks and adds tests.
- Changes the generated auto-upgrade schedule.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_update_manifest.go |
Adds trigger metadata and detection. |
pkg/workflow/safe_update_manifest_test.go |
Tests trigger capture. |
pkg/workflow/safe_update_enforcement.go |
Adds escalation enforcement. |
pkg/workflow/safe_update_enforcement_test.go |
Tests escalation behavior and messages. |
pkg/workflow/compiler.go |
Passes current triggers into enforcement. |
pkg/workflow/compiler_yaml.go |
Records triggers in generated manifests. |
pkg/workflow/compiler_threat_detection_formal_test.go |
Updates enforcement call sites. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Medium
| HasPullRequest bool `json:"has_pull_request,omitempty"` // whether on: includes pull_request | ||
| HasPullRequestTarget bool `json:"has_pull_request_target,omitempty"` // whether on: includes pull_request_target |
|
|
||
| return buildSafeUpdateError(secretViolations, addedActions, removedActions, addedRedirect, removedRedirect) | ||
| func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool { | ||
| return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTarget |
| on: | ||
| schedule: | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning The 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (137 new lines in 📄 Draft ADR committed:
📋 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 MatterADRs 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (33 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review: hasPullRequestTargetEscalation has incomplete coverage
The PR correctly adds detection for pull_request → pull_request_target replacement, but the guard condition on line 97 is narrower than the actual security boundary.
Current condition fires only when the old workflow had pull_request (not pull_request_target) AND the new workflow has pull_request_target but no longer has pull_request. Two scenarios bypass it:
- Fresh PRT addition — old workflow had no PR triggers; new adds
pull_request_target. Guard never fires becausemanifest.HasPullRequest == false. - Additive PRT — old kept
pull_request; new keeps it and also addspull_request_target. Guard never fires becausecurrentHasPullRequest == true. The existing test marks thiswantErr: false, but any new introduction ofpull_request_targetis a privilege escalation (base-branch context, full secrets for fork PRs).
Broadened fix: return !manifest.HasPullRequestTarget && currentHasPullRequestTarget
Everything else — detectPullRequestEvents, manifest fields, error message wording, and test structure — is well done.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 36.4 AIC · ⌖ 4.52 AIC · ⊞ 4.8K
|
|
||
| return buildSafeUpdateError(secretViolations, addedActions, removedActions, addedRedirect, removedRedirect) | ||
| func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool { | ||
| return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTarget |
There was a problem hiding this comment.
The escalation condition checks only the pull_request → pull_request_target replacement pattern (old had PR, old lacked PRT, new lacks PR, new has PRT). This leaves two privilege-escalation scenarios undetected:
- Fresh addition: old workflow had no PR triggers at all (
HasPullRequest == false), new workflow addspull_request_target. Becausemanifest.HasPullRequestisfalse, the guard never fires. - Additive escalation: old workflow had
pull_request, new keeps it and addspull_request_target. The existing test explicitly marks thiswantErr: false, butpull_request_targetruns with base-branch context and full secret access for fork PRs — adding it is a privilege upgrade regardless of whetherpull_requestis retained.
Suggested broadened check:
// Flag if PRT is newly introduced (regardless of whether PR was kept or dropped)
func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool {
return !manifest.HasPullRequestTarget && currentHasPullRequestTarget
}This fires whenever pull_request_target goes from absent → present, which is the actual security boundary. The PR author should evaluate whether cases 1 & 2 are intentionally out of scope (and document that decision) or are gaps.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking security gap
The PR correctly detects pull_request to pull_request_target conversions, but the escalation predicate is too narrow and will miss the equally dangerous case where a workflow adds pull_request_target fresh (with no prior pull_request trigger).
Blocking issues:
- hasPullRequestTargetEscalation only fires when manifest.HasPullRequest == true (safe_update_enforcement.go:97). A push/schedule-only workflow silently passes enforcement even when pull_request_target is newly introduced. Fix: drop the manifest.HasPullRequest guard and key only on !manifest.HasPullRequestTarget && currentHasPullRequestTarget.
- No test covers the fresh-PRT addition case (safe_update_enforcement_test.go). Without a test for HasPullRequest=false, currentHasPRTarget=true, the gap above will never be caught by CI.
Non-blocking: omitempty on the two new bool fields (safe_update_manifest.go:60-61) makes JSON ambiguous; removing it so false is always explicit would be more robust.
🔎 Code quality review by PR Code Quality Reviewer · 54.1 AIC · ⌖ 4.85 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| return buildSafeUpdateError(secretViolations, addedActions, removedActions, addedRedirect, removedRedirect) | ||
| func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool { | ||
| return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTarget |
There was a problem hiding this comment.
Security gap: escalation detection is blind to fresh pull_request_target additions — a workflow that was previously push/schedule-only can silently add pull_request_target without triggering enforcement.
💡 Analysis and suggested fix
The current condition is:
return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTargetThis only fires when the old manifest explicitly recorded has_pull_request: true. A workflow with only push or schedule in the old manifest will have manifest.HasPullRequest == false, so the entire expression short-circuits to false. An attacker-authored PR can therefore introduce pull_request_target (which grants write access to repo secrets/code in a fork-triggered context) without any enforcement warning.
Suggested fix — widen the detection to any new appearance of pull_request_target:
func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequestTarget bool) bool {
// Flag any new introduction of pull_request_target, not just PR→PRT conversion.
return !manifest.HasPullRequestTarget && currentHasPullRequestTarget
}The existing test case "pull_request converted to pull_request_target is flagged" would still pass; the additional case "push-only workflow adds pull_request_target" would now also be caught.
| ResolutionFailures []GHAWManifestResolutionFailure `json:"resolution_failures,omitempty"` // unresolved action-ref pinning failures | ||
| Containers []GHAWManifestContainer `json:"containers,omitempty"` // container images used, with digest when available | ||
| Redirect string `json:"redirect,omitempty"` // frontmatter redirect target for moved workflows | ||
| HasPullRequest bool `json:"has_pull_request,omitempty"` // whether on: includes pull_request |
There was a problem hiding this comment.
omitempty on bool fields makes the manifest JSON ambiguous between false and absent — deserialized old manifests are indistinguishable from manifests that explicitly recorded false.
💡 Details
HasPullRequest bool `json:"has_pull_request,omitempty"`
HasPullRequestTarget bool `json:"has_pull_request_target,omitempty"`With omitempty, a false value is silently dropped from the JSON. When the lock file is read back by ExtractGHAWManifestFromLockFile, the missing fields unmarshal to false — identical to an explicitly-set false. This matters for the escalation check: if HasPullRequest is absent (old manifest, pre-feature), it reads as false, and that is already handled by the nil-manifest guard. However, if a manifest was written between the introduction of HasPullRequestTarget and HasPullRequest (or vice versa), the absent field silently reads as false rather than "unknown", potentially suppressing a legitimate escalation warning.
Remove omitempty from both fields so their values are always serialized and the absence/presence distinction is unambiguous:
HasPullRequest bool `json:"has_pull_request"`
HasPullRequestTarget bool `json:"has_pull_request_target"`| @@ -335,11 +337,35 @@ func TestEnforceSafeUpdate(t *testing.T) { | |||
| redirect: " owner/repo/workflows/new.md@main ", | |||
There was a problem hiding this comment.
Missing test for the security gap: no test covers adding pull_request_target to a workflow that never had pull_request (e.g., push/schedule-only manifest).
💡 Suggested test case
{
name: "push-only workflow adds pull_request_target is flagged",
manifest: &GHAWManifest{
Version: 1,
HasPullRequest: false, // was push/schedule only
},
secretNames: []string{},
actionRefs: []string{},
hasPR: false,
hasPRTarget: true,
wantErr: true,
wantErrMsgs: []string{"Event trigger security escalation"},
},Without this test (and the corresponding fix in hasPullRequestTargetEscalation), the security gap documented in the companion comment goes undetected in CI.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /codebase-design — requesting changes on two actionable issues.
📋 Key Themes & Highlights
Key Issues
[]anylist branch untested (safe_update_manifest_test.go) — The most common real-worldon:format has no coverage; see inline comment.- Escalation predicate scope (
safe_update_enforcement.go:97) — Addingpull_request_targetto a non-PR workflow bypasses the check; design intent should be stated and tested. - Missing doc comment on security predicate (
safe_update_enforcement.go:96) — The six-field boolean needs an explaining comment.
Positive Highlights
- ✅
detectPullRequestEventscleanly handles all three YAMLon:shapes (string / list / map) - ✅ Warning-not-error approach is consistent with existing safe-update philosophy
- ✅ Good extension of existing
EnforceSafeUpdatesignature pattern - ✅ Existing formal tests updated cleanly with the new parameters
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 65.8 AIC · ⌖ 4.75 AIC · ⊞ 6.6K
Comment /matt to run again
| wantVersion: 1, | ||
| wantSecrets: []string{}, | ||
| wantHasPRTarget: true, | ||
| }, |
There was a problem hiding this comment.
[/tdd] The []any (YAML list) branch of detectPullRequestEvents has no test coverage — the only two new test cases use string and map[string]any formats.
💡 Suggested test cases
Add to the TestNewGHAWManifest table:
{
name: "detect pull_request_target from on list",
onField: []any{"push", "pull_request_target"},
wantVersion: 1,
wantSecrets: []string{},
wantHasPR: false,
wantHasPRTarget: true,
},
{
name: "detect both from on list",
onField: []any{"pull_request", "pull_request_target"},
wantVersion: 1,
wantSecrets: []string{},
wantHasPR: true,
wantHasPRTarget: true,
},The list format is the most common real-world on: shape (e.g. on: [push, pull_request_target]), so an uncovered branch is a high-probability regression risk.
@copilot please address this.
|
|
||
| return buildSafeUpdateError(secretViolations, addedActions, removedActions, addedRedirect, removedRedirect) | ||
| func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool { | ||
| return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTarget |
There was a problem hiding this comment.
[/grill-with-docs] The escalation predicate silently allows adding pull_request_target to a workflow that previously had neither PR trigger — only conversions from pull_request-only are blocked. Adding pull_request_target to a push-only workflow is equally dangerous but falls through.
💡 What the logic currently accepts
// This returns false (no violation) when:
// manifest.HasPullRequest = false (was a push-only workflow)
// manifest.HasPullRequestTarget = false
// currentHasPullRequest = false
// currentHasPullRequestTarget = true ← NEW pull_request_target added
return manifest.HasPullRequest && !manifest.HasPullRequestTarget && !currentHasPullRequest && currentHasPullRequestTargetConsider whether the intent is "block any fresh introduction of pull_request_target" or strictly "detect a swap". If the former, the condition should be:
return !manifest.HasPullRequestTarget && currentHasPullRequestTargetDocument the design decision (e.g. in a comment) so future editors know the intended scope.
@copilot please address this.
| } | ||
|
|
||
| return buildSafeUpdateError(secretViolations, addedActions, removedActions, addedRedirect, removedRedirect) | ||
| func hasPullRequestTargetEscalation(manifest *GHAWManifest, currentHasPullRequest bool, currentHasPullRequestTarget bool) bool { |
There was a problem hiding this comment.
[/codebase-design] hasPullRequestTargetEscalation lacks a doc comment. It's a security-critical predicate with subtle semantics — the parameter order and boolean logic aren't self-evident, and the lack of comment will slow down future reviewers.
💡 Suggested comment
// hasPullRequestTargetEscalation reports whether the workflow has converted from a
// pull_request-only trigger to pull_request_target without the old manifest
// recording a pull_request_target. This pattern grants the workflow write
// permissions from untrusted fork code and requires explicit approval.
func hasPullRequestTargetEscalation(...) bool {@copilot please address this.
This change extends compiler security analysis to detect event-trigger privilege escalation during workflow updates. Specifically, it flags when a workflow removes
pull_requestand replaces it withpull_request_target, which requires explicit security review.Safe-update enforcement: detect trigger escalation
EnforceSafeUpdate(...)to compare previous vs current trigger posture for PR events.pull_request(withoutpull_request_target) and the updated workflow shifts topull_request_targetonly.Compiler wiring: baseline/current trigger extraction
on:data.Diagnostics and test coverage
pull_request→pull_request_targetescalation detection