Repository navigation
Add explicit strict-mode policy for pull_request_target checkouts #66649
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1730,6 +1730,34 @@ | |
| } | ||
| ], | ||
| "description": "When true, allows workflow to run on pull requests from forked repositories with write permissions. Security consideration: use cautiously as fork PRs run with base repository permissions." | ||
| }, | ||
| "acknowledge-risk": { | ||
| "type": "boolean", | ||
| "default": false, | ||
| "description": "Acknowledge elevated permissions and secret access to suppress only the strict-mode pull_request_target trigger warning. Checkout validation remains enforced." | ||
| }, | ||
| "allowed-checkouts": { | ||
|
Contributor
There was a problem hiding this comment. [/grill-with-docs] The PR description's copyable example configures the policy under a top-level 💡 Suggested fixAlign the PR description/example with the actual schema location ( @copilot please address this.
Contributor
Author
There was a problem hiding this comment. The repository docs and copyable workflow example use |
||
| "type": "array", | ||
| "description": "Exact literal repository/ref pairs trusted by the maintainer in addition to the supported base-repository checkouts. Expressions, wildcards, omitted refs, and PR refs are not allowed.", | ||
| "items": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["repository", "ref"], | ||
| "properties": { | ||
| "repository": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$", | ||
| "description": "Literal owner/repository slug. Matches exactly, including case." | ||
| }, | ||
| "ref": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "pattern": "^[^\\s${}*?\\[\\\\:~^]+$", | ||
| "not": { "pattern": "^refs/pull/" }, | ||
| "description": "Literal trusted branch, tag, or commit SHA; not a pull request ref. Prefer an immutable commit SHA." | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "additionalProperties": false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,11 @@ | |
| // | ||
| // # Validation Rules | ||
| // | ||
| // 1. In strict mode: emit a warning about elevated permissions and secret access, | ||
| // unless on.pull_request_target.acknowledge-risk is explicitly true. | ||
| // | ||
| // 2. When checkout is neither explicitly disabled nor restricted to trusted | ||
| // base-repository refs or literal on.pull_request_target.allowed-checkouts: | ||
| // - In strict mode: return a hard error (extremely insecure). | ||
| // - In non-strict mode: emit a warning. | ||
| // | ||
|
|
@@ -50,14 +49,17 @@ var pullRequestTargetLog = logger.New("workflow:pull_request_target_validation") | |
| // are never treated as a trusted literal allowlist match. | ||
| var pullRequestTargetGitHubExpressionPattern = regexp.MustCompile(`^\$\{\{\s*([^{}]+?)\s*\}\}$`) | ||
|
|
||
| var pullRequestTargetLiteralRepositoryPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`) | ||
| var pullRequestTargetLiteralRefPattern = regexp.MustCompile(`^[^\s${}*?\[\\:~^]+$`) | ||
|
|
||
| // validatePullRequestTargetTrigger validates security requirements for pull_request_target triggers. | ||
| // | ||
| // The pull_request_target trigger runs with full write permissions and repository secret access | ||
| // on the base branch. When checkout is not explicitly disabled (checkout: false), the workflow | ||
| // may execute untrusted PR code with elevated privileges — a critical security vulnerability | ||
| // commonly known as a "pwn request" attack. | ||
| // | ||
| // In strict mode, a warning is emitted unless the trigger-specific risk is acknowledged, | ||
| // even with checkout disabled, since the workflow still runs with elevated permissions. | ||
| // When the workflow frontmatter sets strict: false, effectiveStrictMode is lowered so the | ||
| // dangerous-trigger strict-only warning is skipped; the insecure-checkout check still runs | ||
|
|
@@ -99,18 +101,29 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, | |
| } | ||
|
|
||
| effectiveStrictMode := c.effectiveStrictMode(workflowData.RawFrontmatter) | ||
| var acknowledged bool | ||
| var allowedCheckouts []any | ||
| if on, ok := workflowData.RawFrontmatter["on"].(map[string]any); ok { | ||
| if trigger, ok := on["pull_request_target"].(map[string]any); ok { | ||
| acknowledged = trigger["acknowledge-risk"] == true | ||
| if entries, ok := trigger["allowed-checkouts"].([]any); ok { | ||
| allowedCheckouts = entries | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // In strict mode, emit a warning unless the trigger-specific risk is acknowledged, | ||
| // regardless of whether checkout is disabled. The workflow still runs with full write | ||
| // permissions and has access to all repository secrets. | ||
| if effectiveStrictMode && !acknowledged { | ||
| pullRequestTargetLog.Print("Emitting strict mode warning: pull_request_target is a very dangerous trigger") | ||
| warningMsg := "pull_request_target is a very dangerous trigger.\n" + | ||
| "This event runs with full write permissions and access to all repository secrets.\n" + | ||
| "Unlike pull_request, it runs in the context of the target (base) branch, giving\n" + | ||
| "the workflow elevated access even for PRs from untrusted fork contributors.\n" + | ||
| "Even with checkout: false, consider whether pull_request_target is truly necessary.\n" + | ||
| "If you only need to react to PR events without write access, use pull_request instead.\n" + | ||
| "To acknowledge this risk only, set on.pull_request_target.acknowledge-risk: true.\n" + | ||
| "See: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/" | ||
| fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", warningMsg)) | ||
| c.IncrementWarningCount() | ||
|
|
@@ -125,10 +138,9 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, | |
| return nil | ||
| } | ||
|
|
||
| // Every checkout must target a supported base ref or a declared literal repository/ref pair. | ||
| if hasOnlyTrustedPullRequestTargetCheckouts(workflowData.CheckoutConfigs, allowedCheckouts) { | ||
| pullRequestTargetLog.Print("checkout config is restricted to trusted repository/ref pairs, skipping insecure-checkout error") | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -151,6 +163,8 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, | |
| "checkout:\n" + | ||
| " repository: ${{ github.repository }}\n\n" + | ||
| "You can also use 'ref: ${{ github.event.pull_request.base.ref }}'.\n" + | ||
| "For a fixed external checkout, declare its literal repository/ref pair in\n" + | ||
| "on.pull_request_target.allowed-checkouts and configure checkout to match exactly.\n" + | ||
| "See: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/" | ||
|
|
||
| if effectiveStrictMode { | ||
|
|
@@ -164,18 +178,43 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, | |
| return nil | ||
| } | ||
|
|
||
| func hasOnlyTrustedPullRequestTargetCheckouts(configs []*CheckoutConfig, allowedCheckouts []any) bool { | ||
| if len(configs) == 0 { | ||
| return false | ||
| } | ||
| for _, cfg := range configs { | ||
| if !isTrustedPullRequestTargetCheckout(cfg) && !isAllowedPullRequestTargetCheckout(cfg, allowedCheckouts) { | ||
|
Contributor
Author
There was a problem hiding this comment. Trusted-base and allowlisted checkouts now fail closed when
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Suggested fixTreat a non-empty @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Trusted-base and allowlisted checkouts now fail closed when |
||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func isAllowedPullRequestTargetCheckout(cfg *CheckoutConfig, allowedCheckouts []any) bool { | ||
| if cfg == nil || cfg.Wiki { | ||
| return false | ||
| } | ||
| // Fail closed even when called without schema validation. Never allow expressions | ||
| // or PR refs merely because the same string appears in the policy. | ||
| if !pullRequestTargetLiteralRepositoryPattern.MatchString(cfg.Repository) || | ||
| !pullRequestTargetLiteralRefPattern.MatchString(cfg.Ref) || | ||
| strings.HasPrefix(cfg.Ref, "refs/pull/") { | ||
| return false | ||
| } | ||
| for _, entry := range allowedCheckouts { | ||
| pair, ok := entry.(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } | ||
| repository, repositoryOK := pair["repository"].(string) | ||
| ref, refOK := pair["ref"].(string) | ||
| if repositoryOK && refOK && repository == cfg.Repository && ref == cfg.Ref { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func isTrustedPullRequestTargetCheckout(cfg *CheckoutConfig) bool { | ||
| if cfg == nil { | ||
| return false | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository docs and copyable workflow example use
on.pull_request_target. The PR body still has the earlier top-level example, and the available tools in this session do not expose PR-description editing; a maintainer will need to update that body.