[WIP] Fix failing GitHub Actions job for CLI completion#46859
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…sable When a pull_request_target workflow has no explicit 'checkout:' key in its frontmatter, the compiler previously auto-disabled checkout (setting CheckoutDisabled=true for runtime reasons), which inadvertently caused the security validation to skip the 'extremely insecure' warning/error. The fix introduces a new CheckoutExplicitlyDisabled field that is only set when the user explicitly writes 'checkout: false' in their frontmatter. The security validation now checks this explicit flag instead of the general CheckoutDisabled flag, ensuring that auto-disabled workflows still receive the appropriate security diagnostic. Additionally, when 'strict: false' is present in frontmatter, ALL pull_request_target security warnings are now suppressed (including non-strict mode warnings), since the author has acknowledged the risks. Also updates the unit test for the 'no checkout key, strict mode' case to reflect the new correct behavior: a strict-mode compile of a workflow without explicit checkout:false now returns a hard error. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46859 does not have the 'implementation' label and has only 26 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Fixes pull-request-target checkout validation so auto-disabled checkout remains distinguishable from explicit checkout: false.
Changes:
- Tracks explicit checkout disabling separately.
- Enforces strict-mode diagnostics when checkout is only auto-disabled.
- Updates the related validation test.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Adds explicit-disable state. |
pkg/workflow/workflow_builder.go |
Populates explicit-disable state. |
pkg/workflow/pull_request_target_validation.go |
Revises security diagnostics. |
pkg/workflow/pull_request_target_validation_test.go |
Updates strict-mode expectations. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| if frontmatterOptOut { | ||
| pullRequestTargetLog.Print("Frontmatter strict: false opt-out detected, skipping all pull_request_target security warnings") | ||
| return nil |
There was a problem hiding this comment.
Fixed in 5e88c0c. Removed the frontmatterOptOut early return block. strict: false now only lowers effectiveStrictMode to false (skipping the strict-only dangerous-trigger warning); the insecure-checkout check runs in all cases and emits a non-strict warning when checkout is not explicitly disabled. Updated two tests to reflect the new expected warning counts.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security: strict: false silently suppresses all pull_request_target security diagnostics
The frontmatterOptOut early return at lines 112–115 exits the entire validation function when the workflow frontmatter contains strict: false. This means a pull_request_target workflow with checkout enabled and strict: false emits zero security warnings — the user gets no indication they are using a dangerous trigger with an insecure checkout.
strict: false is meant to downgrade strict-mode errors to warnings (or skip strict-only checks), not to bypass all security validation. The fix is to remove the frontmatterOptOut early return block and let execution fall through to the dangerous-trigger warning and the insecure-checkout error/warning, both of which should fire regardless of strict mode.
The test update (flipping the no-checkout-key case expectation to expectError: true) and the CheckoutExplicitlyDisabled field plumbing are both correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 23.6 AIC · ⌖ 5.47 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (1 test case modified)
Modified Test Case
Verdict✅ Passed. 0% implementation tests (threshold: 30%). Test directly validates the security behavior contract of pull_request_target strict mode handling. The modification is minimal, high-value, and properly tests the new error condition with appropriate assertions (expectError, errorContains, warningCount). Confidence: Very High. This is a surgical test update aligned with the production code security fix.
|
There was a problem hiding this comment.
REQUEST_CHANGES — critical security regression
The strict: false opt-out logic is over-scoped and introduces a security regression.
Blocking issues (2 critical/high)
1. strict: false silences the insecure-checkout/pwn-request error (line 112–114 in pull_request_target_validation.go)
The new early return nil exits before the checkout safety checks at lines 137–147. A workflow with strict: false and no checkout: false will compile with zero warnings about running untrusted fork code with full write permissions — precisely the pwn-request scenario this validator exists to prevent. The fix is to remove the frontmatterOptOut early-return block; setting effectiveStrictMode = false is sufficient to suppress only the strict-mode warning.
2. effectiveStrictMode is dead code when frontmatterOptOut is true (line 104)
This is a symptom of issue #1 — the variable was clearly meant to be the mechanism for the opt-out, not an early return.
Medium issue (1)
3. CheckoutExplicitlyDisabled inferred from parsedFrontmatter.CheckoutDisabled (workflow_builder.go line 80)
If the frontmatter parser ever sets CheckoutDisabled for a non-explicit reason (e.g. an imported tool default), this will silently bypass the insecure-checkout diagnostic. The invariant needs structural enforcement, not just call-site discipline.
🔎 Code quality review by PR Code Quality Reviewer · 39.1 AIC · ⌖ 4.83 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/pull_request_target_validation.go:114
Security regression: strict: false now suppresses the insecure-checkout / pwn-request error, not just the strict-mode warning.
<details>
<summary>💡 Explanation and suggested fix</summary>
Before this PR, strict: false in frontmatter only disabled effectiveStrictMode, which gated the dangerous-trigger warning at line 120. The insecure-checkout error at ~line 152 fires unconditionally — it is not gated on effectiveStrictMode. By adding an early return nil at line 114, `strict…
pkg/workflow/pull_request_target_validation.go:104
effectiveStrictMode is set to false but then never used — it becomes dead code when frontmatterOptOut is true.
<details>
<summary>💡 Details</summary>
When frontmatterOptOut is true, the function returns at line 114 before effectiveStrictMode is read at line 120. This dead assignment is a signal that the scoping of the opt-out is wrong: the original design set effectiveStrictMode = false precisely to skip only the strict-mode warning at line 120, not the entire function. Removin…
pkg/workflow/workflow_builder.go:80
CheckoutExplicitlyDisabled is inferred from parsedFrontmatter.CheckoutDisabled without verifying that flag only reflects explicit user intent.
<details>
<summary>💡 Details</summary>
parsedFrontmatter.CheckoutDisabled is populated by the frontmatter parser. If FrontmatterConfig.CheckoutDisabled is ever set for a reason other than an explicit checkout: false key (e.g., a default applied during tool resolution, or an imported tool definition that disables checkout), this block will fa…
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs — requesting changes on two issues.
Issues:
- strict: false now early-returns before checkout safety checks, suppressing ALL diagnostics including the non-strict insecure-checkout warning. If intentional, add a test for strict:false + untrusted checkout-enabled. See inline comment.
- CheckoutExplicitlyDisabled is inferred from parsedFrontmatter.CheckoutDisabled in workflow_builder.go rather than set at parse time — fragile if auto-disable paths are ever added to frontmatter_parsing.go. See inline comment.
Positives:
- Correct semantic distinction between auto-disabled and explicitly-disabled checkout.
- Test case correctly updated to expect error on missing checkout key.
- Clear log messages at each branch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 37.2 AIC · ⌖ 4.62 AIC · ⊞ 6.7K
Comment /matt to run again
| // pull_request_target security warnings. The author has acknowledged the risks and chosen to | ||
| // suppress these diagnostics for their specific use case. | ||
| if frontmatterOptOut { | ||
| pullRequestTargetLog.Print("Frontmatter strict: false opt-out detected, skipping all pull_request_target security warnings") |
There was a problem hiding this comment.
[/diagnosing-bugs] The early return on frontmatterOptOut silently suppresses ALL security diagnostics — including the insecure-checkout warning — when strict: false is set, even if checkout is explicitly enabled with untrusted code. This is a broader suppression than the old effectiveStrictMode = false path, which still emitted non-strict warnings.
💡 Risk & suggested fix
Before this change, strict: false only disabled strict-mode errors; the non-strict-mode insecure-checkout warning was still emitted. After this change, an author who writes:
strict: false
on:
pull_request_target:
checkout:
repository: ... # some untrusted refgets zero diagnostics. If this is intentional (opt-out means opt-out of everything), add a test that explicitly covers strict: false + untrusted checkout-enabled, with a comment confirming the intended behaviour. If it is not intentional, move the early return to only skip the strict-mode warning block, not the checkout safety check.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 5e88c0c. Removed the early return; strict: false now only sets effectiveStrictMode = false, which skips the strict-mode dangerous-trigger warning while leaving the insecure-checkout check intact. A pull_request_target workflow with strict: false and an untrusted checkout now correctly receives the non-strict insecure-checkout warning.
| workflowData.CheckoutConfigs = toolsResult.parsedFrontmatter.CheckoutConfigs | ||
| workflowData.CheckoutDisabled = toolsResult.parsedFrontmatter.CheckoutDisabled | ||
| if toolsResult.parsedFrontmatter.CheckoutDisabled { | ||
| workflowData.CheckoutExplicitlyDisabled = true |
There was a problem hiding this comment.
[/diagnosing-bugs] When parsedFrontmatter.CheckoutDisabled is true, CheckoutExplicitlyDisabled is always set to true — but parsedFrontmatter.CheckoutDisabled is also set by the auto-disable path in frontmatter_parsing.go (line 69). If the frontmatter parser ever auto-disables checkout (e.g., for pull_request_target triggers), this branch will incorrectly mark it as explicitly disabled by the user.
💡 Details
frontmatter_parsing.go:69 sets config.CheckoutDisabled = true only when the user writes checkout: false, so in practice today this is safe. However, the assumption is fragile — a future change to frontmatter parsing that auto-disables checkout would silently promote it to an explicit opt-out, suppressing security warnings.
Consider adding a separate CheckoutExplicitlyDisabled field to the parsed frontmatter struct and setting it at the source (frontmatter_parsing.go) rather than inferring it here.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 5e88c0c. Added a dedicated CheckoutExplicitlyDisabled field to FrontmatterConfig in frontmatter_types.go and set it at parse time in frontmatter_parsing.go (only when checkout: false is explicitly written by the user). workflow_builder.go now reads parsedFrontmatter.CheckoutExplicitlyDisabled directly, removing the fragile inference from CheckoutDisabled.
|
@copilot run pr-finisher skill |
|
@copilot please run the Unresolved review threads:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…abled to FrontmatterConfig - Remove the `frontmatterOptOut` early return block from validatePullRequestTargetTrigger. strict: false now lowers to non-strict mode (effectiveStrictMode=false) without suppressing the insecure-checkout warning. A pull_request_target workflow with strict: false and no explicit checkout: false still emits the non-strict insecure-checkout warning. - Add CheckoutExplicitlyDisabled field to FrontmatterConfig (frontmatter_types.go) and set it at parse time in frontmatter_parsing.go, removing the fragile inference from parsedFrontmatter.CheckoutDisabled in workflow_builder.go. - Update two tests to match the new behavior: - 'no checkout key - non-strict': warningCount 1→2 (sandbox + insecure-checkout warning) - 'strict CLI + frontmatter strict false': warningCount 0→1, expectWarning false→true Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed pr-finisher pass. Summary:
Actions taken: No file changes needed in this pass — all fixes were already committed in prior commits ( Hand-off: A maintainer needs to (1) manually resolve the three outdated review threads, and (2) re-trigger CI (close/reopen PR or workflow_dispatch) to confirm the integration test fix on CI. |
|
🎉 This pull request is included in a new release. Release: |
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.