Repository navigation
Add explicit strict-mode policy for pull_request_target checkouts - #66649
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Moved both options under |
There was a problem hiding this comment.
🟡 Changes recommended
Checkout fetch refs bypass the new trust validation and can expose a privileged job to PR-controlled shell injection.
2 open findings
What changed in this PR
Adds an explicit strict-mode policy for trusted pull_request_target checkouts while retaining checkout validation.
Changes:
- Adds checkout allowlisting and warning acknowledgment.
- Preserves checkout refs as YAML strings.
- Adds regression tests and reference documentation.
| File | Description |
|---|---|
pkg/workflow/pull_request_target_validation.go |
Implements the new trust policy. |
pkg/workflow/pull_request_target_validation_test.go |
Tests policy boundaries and ref preservation. |
pkg/workflow/frontmatter_on_section_cleanup.go |
Removes compiler-only policy fields from output. |
pkg/workflow/checkout_step_generator.go |
Uses safe ref rendering. |
pkg/workflow/checkout_input_render.go |
Quotes YAML-sensitive refs. |
pkg/parser/schemas/main_workflow_schema.json |
Defines policy schema. |
docs/src/content/docs/reference/frontmatter.md |
Links strict-mode policy guidance. |
docs/src/content/docs/reference/checkout.md |
Documents trusted checkout configuration. |
🧠 Review effort: Balanced
| } | ||
| for _, cfg := range configs { | ||
| if !isTrustedPullRequestTargetCheckout(cfg) { | ||
| if !isTrustedPullRequestTargetCheckout(cfg) && !isAllowedPullRequestTargetCheckout(cfg, allowedCheckouts) { |
There was a problem hiding this comment.
Trusted-base and allowlisted checkouts now fail closed when fetch is non-empty. Regression tests cover PR refs, PR-head expressions, and wildcard fetches. Commit: 79c1b9d7.
| "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": { |
There was a problem hiding this comment.
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.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One simplification opportunity found.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #66649 · codex · gpt56 · 12.8 AIC · ⌖ 6.34 AIC · ⊞ 13.4K
Comment /ponytail to run again
| return token != "" | ||
| } | ||
|
|
||
| func writeCheckoutRef(sb *strings.Builder, ref string) { |
There was a problem hiding this comment.
L50: shrink: YAML unmarshal-and-compare solely to decide quoting. Always use strconv.Quote(ref); checkout refs are strings and that preserves every literal while removing the parser branch.
There was a problem hiding this comment.
writeCheckoutRef now always uses strconv.Quote(ref); the YAML unmarshal-and-compare branch is removed. Commit: 79c1b9d7.
🏗️ Design Decision Gate — ADR Required (draft added)Gate status: ⛔ Blocking until the ADR is reviewed and accepted. Why this PR was gated
What I didI generated a draft ADR from the PR evidence and committed it to this branch:
Decision captured: opt-in Alternatives recorded: Evidence used: Your next actionReview the draft ADR, correct anything I inferred wrongly (notably the Deciders line marked Note The draft is my reading of the diff, not an approval of the security policy it describes. The negative consequences section intentionally states that this introduces a documented bypass of
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting changes on a security-relevant gap in the new checkout allowlist.
📋 Key Themes & Highlights
Key Themes
- Security gap:
isAllowedPullRequestTargetCheckout/isTrustedPullRequestTargetCheckoutvalidaterepositoryandrefbut never inspectcfg.Fetch. A checkout that passes the new allowlist (or the existing trusted-base check) can still declare an untrustedfetchref (e.g. the PR head), andgenerateFetchStepLineswill fetch it with the checkout's resolved credentials — this undermines the core security guarantee the PR is adding. See inline comment onpull_request_target_validation.go. - Doc/schema mismatch: the PR description's YAML example uses a top-level
pull-request-target:key, but the schema and the new"top-level policy is rejected"test both require nesting underon.pull_request_target. The example should be corrected to avoid confusing adopters. See inline comment onmain_workflow_schema.json. - Both of these align with and sharpen pre-existing review comments from Copilot's bot review — the fetch-bypass issue in particular should block merge since it's a silent security hole in a PR whose entire purpose is tightening
pull_request_targetcheckout security.
Positive Highlights
- ✅ Literal-pattern validation for
repository/ref(rejecting expressions, wildcards, andrefs/pull/...) is careful and well fail-closed, withstrict.HasPrefixchecks layered on top of regex for defense in depth. - ✅ Extensive table-driven regression tests cover most adversarial policy shapes (wildcard repo/ref, composite expressions, PR merge refs, empty ref, unknown fields, non-boolean acknowledgment).
- ✅
writeCheckoutReffix for literal-ref YAML scalar ambiguity (e.g.0x10becoming16) is a nice, narrowly-scoped correctness fix with its own test coverage across multiple tricky literals. - ✅ Doc updates in
checkout.mdclearly explain the new policy's scope and limits, including an explicit caution that this records maintainer trust rather than proof of safety.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 57.6 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
| } | ||
| for _, cfg := range configs { | ||
| if !isTrustedPullRequestTargetCheckout(cfg) { | ||
| if !isTrustedPullRequestTargetCheckout(cfg) && !isAllowedPullRequestTargetCheckout(cfg, allowedCheckouts) { |
There was a problem hiding this comment.
[/codebase-design] isAllowedPullRequestTargetCheckout validates cfg.Repository and cfg.Ref but never checks cfg.Fetch. An entry that is otherwise allowlisted (or trusted-base) can still set fetch: ["${{ github.event.pull_request.head.ref }}"], and generateFetchStepLines will emit an authenticated git fetch of that untrusted ref with the checkout's resolved credentials — defeating the allowlist's safety guarantee entirely.
💡 Suggested fix
Treat a non-empty cfg.Fetch the same as an untrusted repository/ref: either reject any allowlisted/trusted checkout that sets fetch to a non-literal value, or require each fetch entry to pass the same literal-pattern checks (pullRequestTargetLiteralRefPattern, no refs/pull/ prefix) as cfg.Ref. Add a regression test with checkout.fetch set to a PR-head expression paired with an otherwise-allowlisted checkout to lock in the fix — this is exactly the kind of case /diagnosing-bugs regression-test discipline would catch before merge.
@copilot please address this.
There was a problem hiding this comment.
Trusted-base and allowlisted checkouts now fail closed when fetch is non-empty. Regression tests cover PR-head expressions and PR refs. Commit: 79c1b9d7.
| "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": { |
There was a problem hiding this comment.
[/grill-with-docs] The PR description's copyable example configures the policy under a top-level pull-request-target: key, but the schema (and the new test "top-level policy is rejected") place these options under on.pull_request_target. A maintainer copy-pasting the PR description's YAML will get a hard schema-rejection instead of the intended behavior.
💡 Suggested fix
Align the PR description/example with the actual schema location (on.pull_request_target.acknowledge-risk / on.pull_request_target.allowed-checkouts), matching the docs in checkout.md which already use the correct nesting. This is a documentation-vocabulary mismatch the /grill-with-docs lens is meant to catch — the same domain concept (pull_request_target policy) is named inconsistently between the PR body and the schema/tests.
@copilot please address this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Review summary
Impeccable mode applied: harden + audit (change type: refactor_cleanup/security-policy addition touching compiler validation, schema, and docs).
Two existing review comments from Copilot already flag the two actionable issues I found, so I'm not duplicating them inline:
pkg/workflow/pull_request_target_validation.go:186(blocking) —isAllowedPullRequestTargetCheckout/isTrustedPullRequestTargetCheckoutnever validatecfg.Fetch. A checkout matching the new allowlist (or even the trusted-base case) can still carryfetch: ["refs/pull/*/head"]or an expression-based ref, andgenerateFetchStepLineswill emit agit fetchstep pulling untrusted refs into the "trusted" checkout — reopening the pwn-request vector this PR is meant to close, while strict validation reports it as safe. I confirmedFetch []stringis a realCheckoutConfigfield (pkg/workflow/checkout_manager.go:97-110) consumed unguarded bygenerateFetchStepLines(pkg/workflow/checkout_step_generator.go:477+).pkg/parser/schemas/main_workflow_schema.json:1739(non-blocking but should fix before merge) — the PR description's copy-pasteable YAML example nests the policy under a top-levelpull-request-target:block, but the actual schema/compiler implementation (and the PR's own new test"top-level policy is rejected") requireon.pull_request_target.acknowledge-risk/on.pull_request_target.allowed-checkouts. The example in the PR body will fail to compile as written.
No new inline comments added to avoid duplicating the bot's existing findings. Recommend addressing #1 before merge since it undermines the security guarantee the PR adds.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 61.8 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
The new pull_request_target allowlist still leaves a privileged code-loading path open: validation only blesses the initial checkout pair, but the generated fetch: step can still import untrusted PR refs into the same workspace. That means strict mode can now approve workflows that are still one git checkout away from a classic pwn-request shape.
Blocking theme
allowed-checkoutscurrently ignorescheckout.fetch, even thoughgenerateFetchStepLinesruns in the same job and can fetchrefs/pull/*/heador wildcard branches after the “trusted” checkout is complete.- The new docs/tests currently describe the policy as if every effective checkout surface is covered, but there is no regression proving that
fetch:remains forbidden underpull_request_target.
I would be comfortable with this once the trust decision treats fetch: as part of the checkout surface and the new policy tests include explicit rejection cases for PR-head and wildcard fetch patterns.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 67 AIC · ⌖ 5.57 AIC · ⊞ 19.6K
Comment /review to run again
| {name: "acknowledgment alone does not allow external checkout", policy: acknowledged, checkout: externalCheckout, errorContains: insecureCheckout}, | ||
| {name: "allowlist alone retains warning", policy: allowlist, checkout: externalCheckout, warnings: 1}, | ||
| {name: "false acknowledgment retains warning", policy: " acknowledge-risk: false\n" + allowlist, checkout: externalCheckout, warnings: 1}, | ||
| {name: "acknowledged literal external checkout", policy: acknowledged + allowlist, checkout: externalCheckout}, |
There was a problem hiding this comment.
This test matrix still misses the core pull_request_target bypass: an allowlisted checkout can add fetch: (for example refs/pulls/open/*) and the generated follow-up fetch step will pull untrusted PR refs into the same privileged workspace.
💡 Why this blocks the change
hasOnlyTrustedPullRequestTargetCheckouts now treats the checkout as safe as soon as the initial repository/ref pair matches, but generateFetchStepLines can immediately widen that workspace again by fetching PR heads or arbitrary branches. That means strict mode will approve a workflow that still makes untrusted code available to later git checkout or script steps under pull_request_target credentials.
Please add a regression case that keeps allowed-checkouts enabled but sets checkout.fetch to a PR ref or wildcard pattern, and make validation reject it. Until fetch: is part of the trust decision, the new allowlist is not actually enforcing “trusted checkouts only”.
There was a problem hiding this comment.
Added strict-mode regression cases for allowlisted PR-ref/expression fetches and a trusted-base wildcard fetch; these no longer pass trust validation. Commit: 79c1b9d7.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher. Commit |
|
@copilot update compiler threat spec and TLA+ model |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated CTR-008 and added a bounded TLA+ model for acknowledgment scope, checkout trust, and |
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 53d8277
|
…t-mode-policy Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Merged latest |
|
🎉 This pull request is included in a new release. Release: |
Strict mode rejects fixed external checkouts under
pull_request_targetand warns unconditionally, even when checkout is disabled or restricted to trusted base code. This adds a scoped opt-in for direct merged-PR workflows without disabling strict validation.pull-request-target.allowed-checkoutspermits exact, case-sensitive literal repository/ref pairs. Every checkout must satisfy the allowlist or existing base-checkout rules; expressions, wildcards, omitted external refs, and PR refs remain rejected.acknowledge-risk: truesuppresses only the dangerous-trigger warning. Checkout validation and unrelated diagnostics remain intact.0x10from becoming16.The policy records maintainer trust; authentication and safe-output publication remain separately configured. Defaults are unchanged when the policy is omitted.
pull_request_targetworkflows with fixed external checkouts #66564