Allow trusted base-branch checkout in pull_request_target validation#37099
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…lidation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pull_request_target validation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Updates pull_request_target security validation to recognize explicitly trusted base-branch checkouts as safe (avoiding false-positive “insecure checkout” errors), while expanding the diagnostic guidance and adding targeted unit tests to harden expression matching.
Changes:
- Treat explicit
checkout:as trusted inpull_request_targetwhenrepositoryis omitted or exactly${{ github.repository }}, andrefis omitted or exactly${{ github.event.pull_request.base.sha }}/${{ github.event.pull_request.base.ref }}. - Strengthen expression parsing by requiring a strict
${{ ... }}shape and exact inner-expression matches (preventing “trusted” status from malformed literals). - Extend unit tests to cover safe vs unsafe explicit checkout cases and expression-shape edge cases.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/pull_request_target_validation.go | Adds trusted base-checkout recognition and strict ${{ ... }} expression matching; improves insecure-checkout guidance text. |
| pkg/workflow/pull_request_target_validation_test.go | Adds unit tests covering trusted base repo/ref forms, compact syntax, and malformed expression edge cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (239 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 — especially valuable for security-sensitive trust decisions like which checkout forms are considered safe under 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — commenting with test coverage gaps and one clarification suggestion. No blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: All 5 new trusted-checkout integration tests are strict-mode only; the non-strict path and the mixed-checkout path are untested.
TestMatchesGitHubExpressionscope: Unit tests only exercisegithub.repository; the actual trusted ref expressions (base.sha,base.ref) have no dedicated unit coverage.base.refvsbase.shadistinction: Both are accepted as trusted, but they have different mutability semantics that are worth documenting.- Regex intent: The
[^{}]+?brace exclusion is a deliberate bypass-hardening choice but reads as incidental without a comment.
Positive Highlights
- ✅ Strict regex + exact-match comparison prevents any expression-injection bypass — this is the right approach for a security allowlist.
- ✅
hasOnlyTrustedPullRequestTargetCheckoutscorrectly requires all configs to be trusted, not just any one. - ✅ The dangerous-trigger strict warning is still always emitted before the trusted-checkout early return — the security posture is preserved.
- ✅ The "unwrapped literal ref" test explicitly guards against the most obvious bypass attempt.
- ✅ Whitelist is deliberately narrow and static; no dynamic lookup that could be manipulated.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.2M · 322.5 AIC
| expectWarning: true, | ||
| errorContains: "pull_request_target trigger with checkout enabled is extremely insecure", | ||
| warningCount: 1, // dangerous-trigger warning | ||
| }, |
There was a problem hiding this comment.
[/tdd] All five new trusted-checkout test cases use strictMode: true, leaving the non-strict path completely untested.
In non-strict mode with a trusted checkout the function should return nil with zero warnings, but this path is never exercised. A missed regression in effectiveStrictMode evaluation or in hasOnlyTrustedPullRequestTargetCheckouts would go undetected.
💡 Suggested addition
{
name: "pull_request_target with trusted checkout - non-strict - no warnings no error",
frontmatter: `---
on:
pull_request_target:
types: [opened]
permissions:
pull-requests: read
checkout:
repository: ${{ github.repository }}
ref: ${{ github.event.pull_request.base.sha }}
---
# PR Target Non-Strict Trusted Checkout
Test workflow content.`,
filename: "prt-checkout-trusted-non-strict.md",
strictMode: false, // key: non-strict path
expectError: false,
expectWarning: false,
warningCount: 0,
},With effectiveStrictMode = false the dangerous-trigger warning is not emitted, and the trusted checkout returns nil early, so warningCount should be 0.
| warningCount: 1, // dangerous-trigger warning | ||
| }, | ||
| { | ||
| name: "pull_request_target with explicit checkout pinned to base sha - strict - warning only", |
There was a problem hiding this comment.
[/tdd] The new test suite has no case for a mixed checkout list — one trusted config and one untrusted config in the same checkout: array.
hasOnlyTrustedPullRequestTargetCheckouts iterates all entries and returns false if any is untrusted. Without a test for this, a refactor that accidentally short-circuits the loop (e.g. returns true on first trusted match) would not be caught.
💡 Suggested addition
{
name: "pull_request_target with mixed trusted and untrusted checkouts - strict - errors",
frontmatter: `---
on:
pull_request_target:
types: [opened]
permissions:
pull-requests: read
checkout:
- repository: ${{ github.repository }}
ref: ${{ github.event.pull_request.base.sha }}
- repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.event.pull_request.head.sha }}
---
# PR Target Mixed Checkouts
Test workflow content.`,
filename: "prt-checkout-mixed-strict.md",
strictMode: true,
expectError: true,
expectWarning: true,
errorContains: "pull_request_target trigger with checkout enabled is extremely insecure",
warningCount: 1,
},| expected: "github.repository", | ||
| match: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] TestMatchesGitHubExpression only exercises github.repository as the expected argument. The actual whitelist trusted expressions — github.event.pull_request.base.sha and github.event.pull_request.base.ref — are never tested at the unit level.
Adding cases for those longer expressions would catch any future typo or off-by-one in the expression strings passed to matchesAnyGitHubExpression.
💡 Suggested additions
{
name: "base sha expression matches",
value: "${{ github.event.pull_request.base.sha }}",
expected: "github.event.pull_request.base.sha",
match: true,
},
{
name: "base ref expression matches",
value: "${{ github.event.pull_request.base.ref }}",
expected: "github.event.pull_request.base.ref",
match: true,
},
{
name: "head sha expression does not match base sha",
value: "${{ github.event.pull_request.head.sha }}",
expected: "github.event.pull_request.base.sha",
match: false,
},The third case also documents the intentional rejection of fork-head expressions.
|
|
||
| ref := strings.TrimSpace(cfg.Ref) | ||
| return ref == "" || matchesAnyGitHubExpression(ref, | ||
| "github.event.pull_request.base.sha", |
There was a problem hiding this comment.
[/diagnose] github.event.pull_request.base.ref resolves to the base branch name (e.g. main), not a pinned commit SHA. Unlike base.sha, it is mutable — a new commit pushed to the base branch between the PR event and the workflow execution can cause a different commit to be checked out.
This is still safe (it only ever checks out trusted base code, never fork head), but the behavioural difference is worth a brief comment so future maintainers don't equate the two forms.
💡 Suggested inline comment
return ref == "" || matchesAnyGitHubExpression(ref,
"github.event.pull_request.base.sha", // immutable commit SHA - preferred for reproducibility
"github.event.pull_request.base.ref", // branch name - mutable tip, still trusted base code
)| ) | ||
|
|
||
| var pullRequestTargetLog = newValidationLogger("pull_request_target") | ||
| var pullRequestTargetGitHubExpressionPattern = regexp.MustCompile(`^\$\{\{\s*([^{}]+?)\s*\}\}$`) |
There was a problem hiding this comment.
[/diagnose] The [^{}]+? character class in the regex intentionally prevents nested brace expressions from being accepted as trusted. This is a deliberate bypass-hardening choice, but it is implicit.
A short comment would help future maintainers understand why braces are excluded from the inner capture — otherwise it looks like an arbitrary restriction rather than a security invariant.
💡 Suggested comment
// [^{}]+? deliberately excludes brace characters to prevent nested expression constructs
// (e.g. "${{ foo || bar }}" or "${{ env.VAR }}") from being accepted as a trusted literal.
var pullRequestTargetGitHubExpressionPattern = regexp.MustCompile(`^\$\{\{\s*([^{}]+?)\s*\}\}$`)
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in ccdcffc. I reviewed the in-scope feedback and applied the requested smoke-review follow-up items: added missing non-strict trusted-checkout coverage, mixed trusted+untrusted checkout coverage, expression matcher cases for base.sha/base.ref (plus head.sha negative), and clarifying security comments in validation code. Targeted workflow validation tests pass. |
pull_request_targetworkflows with explicitcheckout:were being treated as insecure by default, even when checkout was intentionally pinned to trusted base-branch context. This update removes that false-negative path and makes the security guidance explicit about safe base-checkout patterns.Trusted checkout recognition in strict validation
pull_request_targetsecurity validation to treat explicit checkout as safe when it targets the base repository and trusted base ref expressions.repository: ${{ github.repository }}(or omitted)ref: ${{ github.event.pull_request.base.sha }},ref: ${{ github.event.pull_request.base.ref }}, or omitted ref (base-branch default forpull_request_target).Actionable security diagnostic text
checkout: falsegithub.repository+ base ref)Expression parsing hardening
${{ ... }}and explicit inner-expression validation.Focused coverage for safe vs unsafe explicit checkout
${{github.repository}})