diff --git a/pkg/workflow/frontmatter_parsing.go b/pkg/workflow/frontmatter_parsing.go index 253d32e8d91..7ca1fbc8d98 100644 --- a/pkg/workflow/frontmatter_parsing.go +++ b/pkg/workflow/frontmatter_parsing.go @@ -67,6 +67,7 @@ func ParseFrontmatterConfig(frontmatter map[string]any) (*FrontmatterConfig, err if config.Checkout != nil { if checkoutValue, ok := config.Checkout.(bool); ok && !checkoutValue { config.CheckoutDisabled = true + config.CheckoutExplicitlyDisabled = true frontmatterTypesLog.Print("Checkout disabled via checkout: false") } else { checkoutConfigs, err := ParseCheckoutConfigs(config.Checkout) diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index f4e09bb204d..04e7f13340a 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -410,9 +410,10 @@ type FrontmatterConfig struct { // Controls how actions/checkout is invoked. // Can be a single CheckoutConfig object or an array of CheckoutConfig objects. // Set to false to disable the default checkout step entirely. - Checkout any `json:"checkout,omitempty"` // Raw value (object, array, or false) - CheckoutConfigs []*CheckoutConfig `json:"-"` // Parsed checkout configs (not in JSON) - CheckoutDisabled bool `json:"-"` // true when checkout: false is set in frontmatter + Checkout any `json:"checkout,omitempty"` // Raw value (object, array, or false) + CheckoutConfigs []*CheckoutConfig `json:"-"` // Parsed checkout configs (not in JSON) + CheckoutDisabled bool `json:"-"` // true when checkout: false is set in frontmatter + CheckoutExplicitlyDisabled bool `json:"-"` // true only when checkout: false is explicitly written by the user in frontmatter // Model is the top-level LLM model override. When set, it takes precedence over // engine.model. Use this field instead of engine.model. diff --git a/pkg/workflow/pull_request_target_validation.go b/pkg/workflow/pull_request_target_validation.go index b087c96ed96..d31a4209e37 100644 --- a/pkg/workflow/pull_request_target_validation.go +++ b/pkg/workflow/pull_request_target_validation.go @@ -59,7 +59,9 @@ var pullRequestTargetGitHubExpressionPattern = regexp.MustCompile(`^\$\{\{\s*([^ // // In strict mode, a warning is always emitted that pull_request_target is inherently dangerous // even with checkout disabled, since the workflow still runs with elevated permissions. -// This strict behavior is disabled when the workflow frontmatter explicitly sets strict: false. +// 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 +// and emits a non-strict warning when checkout is not explicitly disabled. func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, markdownPath string) error { // Fast path: skip expensive YAML parsing when the On field cannot possibly contain // a pull_request_target trigger. This avoids yaml.Unmarshal on every @@ -120,9 +122,12 @@ func (c *Compiler) validatePullRequestTargetTrigger(workflowData *WorkflowData, c.IncrementWarningCount() } - // If checkout is disabled, the workflow will not execute PR code — no further action needed. - if workflowData.CheckoutDisabled { - pullRequestTargetLog.Print("checkout: false is set, skipping insecure-checkout error") + // If checkout was explicitly disabled by the user (checkout: false in frontmatter), + // the workflow will not execute PR code — no further action needed. + // Auto-disabled checkout (when no checkout key is present) does not count as explicit + // acknowledgement of the security risk, so the warning/error is still emitted in that case. + if workflowData.CheckoutExplicitlyDisabled { + pullRequestTargetLog.Print("checkout: false is explicitly set by user, skipping insecure-checkout error") return nil } diff --git a/pkg/workflow/pull_request_target_validation_test.go b/pkg/workflow/pull_request_target_validation_test.go index ef25b0a85b1..cf9260f314b 100644 --- a/pkg/workflow/pull_request_target_validation_test.go +++ b/pkg/workflow/pull_request_target_validation_test.go @@ -52,7 +52,7 @@ Test workflow content.`, warningCount: 1, // sandbox.agent: false }, { - name: "pull_request_target with no checkout key - non-strict - checkout auto-disabled", + name: "pull_request_target with no checkout key - non-strict - insecure checkout warning", frontmatter: `--- strict: false on: @@ -72,7 +72,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: true, - warningCount: 1, // checkout is auto-disabled for pull_request_target; only sandbox.agent: false warning + warningCount: 2, // sandbox.agent: false warning + insecure-checkout warning (non-strict mode) }, { name: "pull_request_target with trusted checkout - non-strict - no warnings no error", @@ -170,7 +170,7 @@ Test workflow content.`, warningCount: 1, // dangerous-trigger warning }, { - name: "pull_request_target with no checkout key - strict - checkout auto-disabled, dangerous-trigger warning only", + name: "pull_request_target with no checkout key - strict - insecure checkout error", frontmatter: `--- on: pull_request_target: @@ -186,9 +186,10 @@ permissions: Test workflow content.`, filename: "prt-checkout-enabled-strict.md", strictMode: true, - expectError: false, - expectWarning: true, // dangerous-trigger warning only; checkout auto-disabled so no insecure-checkout error - warningCount: 1, // dangerous-trigger warning + expectError: true, + expectWarning: true, + errorContains: "pull_request_target trigger with checkout enabled is extremely insecure", + warningCount: 1, // dangerous-trigger warning; insecure-checkout is returned as an error }, { name: "pull_request_target with explicit checkout pinned to base sha - strict - warning only", @@ -340,7 +341,7 @@ Test workflow content.`, warningCount: 1, // dangerous-trigger warning }, { - name: "pull_request_target with no checkout key - strict CLI + frontmatter strict false - no warnings", + name: "pull_request_target with no checkout key - strict CLI + frontmatter strict false - insecure checkout warning", frontmatter: `--- strict: false on: @@ -358,8 +359,8 @@ Test workflow content.`, filename: "prt-checkout-enabled-strict-frontmatter-opt-out.md", strictMode: true, expectError: false, - expectWarning: false, - warningCount: 0, // checkout auto-disabled; strict:false suppresses dangerous-trigger warning; no insecure-checkout warning + expectWarning: true, + warningCount: 1, // strict: false lowers to non-strict mode (skips dangerous-trigger warning), but insecure-checkout warning still emits }, { name: "pull_request trigger (not target) - strict - no diagnostic", diff --git a/pkg/workflow/workflow_builder.go b/pkg/workflow/workflow_builder.go index 53615ad5dc3..280efeb03f4 100644 --- a/pkg/workflow/workflow_builder.go +++ b/pkg/workflow/workflow_builder.go @@ -97,9 +97,11 @@ func (c *Compiler) buildInitialWorkflowData( if toolsResult.parsedFrontmatter != nil { workflowData.CheckoutConfigs = toolsResult.parsedFrontmatter.CheckoutConfigs workflowData.CheckoutDisabled = toolsResult.parsedFrontmatter.CheckoutDisabled + workflowData.CheckoutExplicitlyDisabled = toolsResult.parsedFrontmatter.CheckoutExplicitlyDisabled } else if rawCheckout, ok := result.Frontmatter["checkout"]; ok { if checkoutValue, ok := rawCheckout.(bool); ok && !checkoutValue { workflowData.CheckoutDisabled = true + workflowData.CheckoutExplicitlyDisabled = true } else if configs, err := ParseCheckoutConfigs(rawCheckout); err == nil { workflowData.CheckoutConfigs = configs } diff --git a/pkg/workflow/workflow_data.go b/pkg/workflow/workflow_data.go index 939b59e7ab1..2198c926e76 100644 --- a/pkg/workflow/workflow_data.go +++ b/pkg/workflow/workflow_data.go @@ -163,6 +163,7 @@ type WorkflowData struct { InlinedImports bool // if true, inline all imports at compile time (from inlined-imports frontmatter field) CheckoutConfigs []*CheckoutConfig // user-configured checkout settings from frontmatter CheckoutDisabled bool // true when checkout: false is set in frontmatter, or auto-disabled for pull_request_target + CheckoutExplicitlyDisabled bool // true only when checkout: false is explicitly set in frontmatter (not auto-disabled) IsPullRequestTarget bool // true when the workflow's on: triggers contain pull_request_target (but NOT pull_request) HasDispatchItemNumber bool // true when workflow_dispatch has item_number input (generated by label trigger shorthand) ConcurrencyJobDiscriminator string // optional discriminator expression appended to job-level concurrency groups (from concurrency.job-discriminator)