Skip to content

Allow trusted base-branch checkout in pull_request_target validation #37099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 8 commits into from
Jun 5, 2026
+310 −2
Merged

Allow trusted base-branch checkout in pull_request_target validation #37099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
df8ec2f
Initial plan
Copilot Jun 5, 2026
e111e56
Allow trusted base checkout for pull_request_target validation
Copilot Jun 5, 2026
03cf85a
Accept compact trusted checkout expressions in pull_request_target va…
Copilot Jun 5, 2026
018b7c8
Harden trusted checkout expression matching for pull_request_target
Copilot Jun 5, 2026
503658d
Clarify trusted pull_request_target ref guidance in validation error
Copilot Jun 5, 2026
f5253af
Tighten trusted checkout parsing and cover expression edge cases
Copilot Jun 5, 2026
bfe9398
Clarify third safe base-checkout remediation pattern
Copilot Jun 5, 2026
ccdcffc
Address review gaps for trusted pull_request_target checkout validation
Copilot Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions pkg/workflow/pull_request_target_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@ package workflow
import (
"fmt"
"os"
"regexp"
"strings"

"github.com/goccy/go-yaml"
)

var pullRequestTargetLog = newValidationLogger("pull_request_target")
// [^{}]+? deliberately excludes brace characters so nested expression constructs
// are never treated as a trusted literal allowlist match.
var pullRequestTargetGitHubExpressionPattern = regexp.MustCompile(`^\$\{\{\s*([^{}]+?)\s*\}\}$`)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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*\}\}$`)


// validatePullRequestTargetTrigger validates security requirements for pull_request_target triggers.
//
Expand Down Expand Up @@ -119,6 +123,13 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData,
return nil
}

// Explicit checkout configurations that are pinned to the base repository/ref are considered
// safe for pull_request_target because they do not execute untrusted PR head code.
if hasOnlyTrustedPullRequestTargetCheckouts(workflowData.CheckoutConfigs) {
pullRequestTargetLog.Print("checkout config is pinned to trusted base repository/ref, skipping insecure-checkout error")
return nil
}

// Checkout is not disabled — the workflow may execute untrusted PR code with elevated privileges.
pullRequestTargetLog.Print("checkout is NOT disabled, emitting pull_request_target insecure-checkout diagnostic")

Expand All @@ -127,9 +138,17 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData,
"but the workflow will check out code from a potentially untrusted PR contributor.\n" +
"This is a well-known attack vector: a fork PR can inject malicious code that\n" +
"executes with access to your repository's secrets (\"pwn request\" attack).\n\n" +
"Suggested fix: Add 'checkout: false' to your workflow frontmatter to prevent\n" +
"checking out untrusted PR code:\n" +
"Suggested fix: Use one of these safe patterns:\n" +
"1) Disable checkout entirely:\n" +
"checkout: false\n\n" +
"2) Check out only the trusted base repo/ref:\n" +
"checkout:\n" +
" repository: ${{ github.repository }}\n" +
" ref: ${{ github.event.pull_request.base.sha }}\n\n" +
"3) Check out only the trusted base repository and omit ref:\n" +
"checkout:\n" +
" repository: ${{ github.repository }}\n\n" +
"You can also use 'ref: ${{ github.event.pull_request.base.ref }}'.\n" +
"See: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/"

if effectiveStrictMode {
Expand All @@ -142,3 +161,50 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData,

return nil
}

func hasOnlyTrustedPullRequestTargetCheckouts(configs []*CheckoutConfig) bool {
if len(configs) == 0 {
return false
}
for _, cfg := range configs {
if !isTrustedPullRequestTargetCheckout(cfg) {
return false
}
}
return true
}

func isTrustedPullRequestTargetCheckout(cfg *CheckoutConfig) bool {
if cfg == nil {
return false
}

repository := strings.TrimSpace(cfg.Repository)
if repository != "" && !matchesGitHubExpression(repository, "github.repository") {
return false
}

ref := strings.TrimSpace(cfg.Ref)
return ref == "" || matchesAnyGitHubExpression(ref,
"github.event.pull_request.base.sha", // immutable commit SHA
"github.event.pull_request.base.ref", // mutable branch tip, still trusted base code
)
}

func matchesAnyGitHubExpression(value string, expectedExpressions ...string) bool {
for _, expected := range expectedExpressions {
if matchesGitHubExpression(value, expected) {
return true
}
}
return false
}

func matchesGitHubExpression(value string, expectedExpression string) bool {
trimmed := strings.TrimSpace(value)
matches := pullRequestTargetGitHubExpressionPattern.FindStringSubmatch(trimmed)
if len(matches) != 2 {
return false
}
return strings.TrimSpace(matches[1]) == expectedExpression
}
242 changes: 242 additions & 0 deletions pkg/workflow/pull_request_target_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,30 @@ Test workflow content.`,
expectWarning: true,
warningCount: 2, // 1 for insecure checkout + 1 for sandbox.agent: false
},
{
name: "pull_request_target with trusted checkout - non-strict - no warnings no error",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
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,
expectError: false,
expectWarning: false,
warningCount: 0,
},
{
name: "pull_request trigger (not target) - non-strict - no diagnostic",
frontmatter: `---
Expand Down Expand Up @@ -159,6 +183,155 @@ Test workflow content.`,
errorContains: "pull_request_target trigger with checkout enabled is extremely insecure",
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with explicit checkout pinned to base sha - strict - warning only",
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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,
},

frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
checkout:
repository: ${{ github.repository }}
ref: ${{ github.event.pull_request.base.sha }}
---

# PR Target Strict Trusted Checkout
Test workflow content.`,
filename: "prt-checkout-base-sha-strict.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with explicit checkout default ref in base repo - strict - warning only",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
checkout:
repository: ${{ github.repository }}
sparse-checkout: |
.github
---

# PR Target Strict Trusted Default Ref
Test workflow content.`,
filename: "prt-checkout-base-repo-default-ref-strict.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with trusted checkout expressions using compact syntax - strict - warning only",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
checkout:
repository: ${{github.repository}}
ref: ${{github.event.pull_request.base.sha}}
---

# PR Target Strict Trusted Compact Expressions
Test workflow content.`,
filename: "prt-checkout-trusted-compact-expr-strict.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with explicit checkout pinned to base ref expression - strict - warning only",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
checkout:
repository: ${{ github.repository }}
ref: ${{ github.event.pull_request.base.ref }}
---

# PR Target Strict Trusted Base Ref
Test workflow content.`,
filename: "prt-checkout-base-ref-strict.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with unwrapped trusted-looking ref string - strict - still errors",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
permissions:
pull-requests: read
checkout:
repository: ${{ github.repository }}
ref: github.event.pull_request.base.sha
---

# PR Target Strict Unwrapped Ref String
Test workflow content.`,
filename: "prt-checkout-unwrapped-ref-string-strict.md",
strictMode: true,
expectError: true,
expectWarning: true,
errorContains: "pull_request_target trigger with checkout enabled is extremely insecure",
warningCount: 1, // dangerous-trigger warning
},
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

{
name: "pull_request_target with mixed trusted and untrusted checkouts - strict - errors",
frontmatter: `---
on:
pull_request_target:
types: [opened]
tools:
github:
toolsets: [pull_requests]
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, // dangerous-trigger warning
},
{
name: "pull_request_target with checkout enabled - strict CLI + frontmatter strict false - warning only",
frontmatter: `---
Expand Down Expand Up @@ -235,3 +408,72 @@ Test workflow content.`,
})
}
}

func TestMatchesGitHubExpression(t *testing.T) {
t.Parallel()

assertions := []struct {
name string
value string
expected string
match bool
}{
{
name: "spaced expression",
value: "${{ github.repository }}",
expected: "github.repository",
match: true,
},
{
name: "compact expression",
value: "${{github.repository}}",
expected: "github.repository",
match: true,
},
{
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,
},
{
name: "missing closing braces",
value: "${{ github.repository",
expected: "github.repository",
match: false,
},
{
name: "empty expression",
value: "${{}}",
expected: "github.repository",
match: false,
},
{
name: "extra trailing tokens",
value: "${{ github.repository }}bar}}",
expected: "github.repository",
match: false,
},
}
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.


for _, tc := range assertions {
t.Run(tc.name, func(t *testing.T) {
actual := matchesGitHubExpression(tc.value, tc.expected)
if actual != tc.match {
t.Fatalf("matchesGitHubExpression(%q, %q) = %v, want %v", tc.value, tc.expected, actual, tc.match)
}
})
}
}