diff --git a/pkg/workflow/pull_request_target_validation.go b/pkg/workflow/pull_request_target_validation.go index 6d8436f86aa..22d72a515fe 100644 --- a/pkg/workflow/pull_request_target_validation.go +++ b/pkg/workflow/pull_request_target_validation.go @@ -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*\}\}$`) // validatePullRequestTargetTrigger validates security requirements for pull_request_target triggers. // @@ -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") @@ -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 { @@ -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 +} diff --git a/pkg/workflow/pull_request_target_validation_test.go b/pkg/workflow/pull_request_target_validation_test.go index a3c37d90315..bcd5bd051b1 100644 --- a/pkg/workflow/pull_request_target_validation_test.go +++ b/pkg/workflow/pull_request_target_validation_test.go @@ -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: `--- @@ -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", + 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 + }, + { + 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: `--- @@ -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, + }, + } + + 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) + } + }) + } +}