Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions .github/workflows/ai-moderator.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion pkg/workflow/pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import (
var prLog = logger.New("workflow:pr")

// ShouldGeneratePRCheckoutStep returns true if the checkout-pr step should be generated
// based on the workflow permissions. The step requires contents read access.
// based on the workflow permissions and checkout configuration.
// The step requires contents read access and checkout must not be disabled.
func ShouldGeneratePRCheckoutStep(data *WorkflowData) bool {
if data.CheckoutDisabled {
return false
}
if data.CachedPermissions != nil {
return data.CachedPermissions.HasContentsReadAccess()
}
Expand Down
118 changes: 118 additions & 0 deletions pkg/workflow/pr_ready_for_review_checkout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,121 @@ Test workflow with pull_request triggers.
}
}
}

// TestPullRequestTargetCheckoutDisabledByDefault verifies that pull_request_target workflows
// do not generate a "Checkout PR branch" step when checkout is not explicitly configured.
// This prevents the step from hard-failing when head branches are deleted (merged PRs) or
// inaccessible (fork PRs).
func TestPullRequestTargetCheckoutDisabledByDefault(t *testing.T) {
tests := []struct {
name string
workflowContent string
expectPRCheckout bool
description string
}{
{
name: "pull_request_target with no checkout key - no PR checkout step",
workflowContent: `---
on:
pull_request_target:
types: [closed]
permissions:
contents: read
pull-requests: read
engine: claude
strict: false
---

# Thank you note workflow
Workflow triggered when a PR is closed.
`,
expectPRCheckout: false,
description: "pull_request_target without explicit checkout key should not generate 'Checkout PR branch' step",
},
{
name: "pull_request_target with checkout: false - no PR checkout step",
workflowContent: `---
on:
pull_request_target:
types: [closed]
permissions:
contents: read
pull-requests: read
engine: claude
strict: false
checkout: false
---

# Thank you note workflow
Workflow triggered when a PR is closed.
`,
expectPRCheckout: false,
description: "pull_request_target with explicit checkout: false should not generate 'Checkout PR branch' step",
},
{
name: "pull_request_target with trusted checkout mapping - PR checkout step present",
workflowContent: `---
on:
pull_request_target:
types: [opened]
permissions:
contents: read
pull-requests: read
engine: claude
checkout:
repository: ${{ github.repository }}
ref: ${{ github.event.pull_request.base.sha }}
---

# PR review workflow
Workflow triggered when a PR is opened with a trusted base checkout.
`,
expectPRCheckout: true,
description: "pull_request_target with explicit checkout mapping should generate 'Checkout PR branch' step",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f534297 and 37bc639. The original code only checked CheckoutDisabled, so an explicit checkout: mapping on a pull_request_target workflow still emitted checkout_pr_branch.cjs. The fix adds an IsPullRequestTarget field (set in workflow_builder.go for all pull_request_target-only workflows) and checks it first in ShouldGeneratePRCheckoutStep — so the PR-head checkout step is always suppressed for pull_request_target, regardless of whether an explicit actions/checkout config was provided. The test expectation was corrected to expectPRCheckout: false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The test asserts expectPRCheckout: true for the trusted base-SHA opt-in path but only checks for the presence of "Checkout PR branch" — it doesn't verify that an insecure head ref (e.g. refs/pull/.../head) is absent. A future regression in checkout_pr_branch.cjs that resolves to the head ref would still pass this test while silently breaking the security guarantee.

💡 Suggested assertion

After the existing hasPRCheckout check, add:

if hasPRCheckout {
    for _, insecure := range []string{"refs/pull", "head.ref", "head_ref"} {
        if strings.Contains(lockStr, insecure) {
            t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern %q", tt.description, insecure)
        }
    }
}

This makes the integration test a genuine security regression guard, not just a presence check.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f534297 and 37bc639. The root cause was that ShouldGeneratePRCheckoutStep did not suppress checkout_pr_branch.cjs for pull_request_target workflows — so adding a security assertion on an expectPRCheckout: true case would always fail. The fix: ShouldGeneratePRCheckoutStep now returns false for pull_request_target-only workflows (via the new IsPullRequestTarget field), and the test case is corrected to expectPRCheckout: false. A refs/pull absence assertion is still present as a regression guard for any future test case where a PR checkout step IS expected.

},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create temporary directory for test
tempDir, err := os.MkdirTemp("", "prt-checkout-test")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)

// Create workflows directory
workflowsDir := filepath.Join(tempDir, ".github", "workflows")
if err := os.MkdirAll(workflowsDir, 0755); err != nil {
t.Fatalf("Failed to create workflows directory: %v", err)
}

// Write test workflow file
workflowPath := filepath.Join(workflowsDir, "test-prt-workflow.md")
if err := os.WriteFile(workflowPath, []byte(tt.workflowContent), 0644); err != nil {
t.Fatalf("Failed to write workflow file: %v", err)
}

// Compile workflow
compiler := NewCompiler()
compiler.SetActionMode(ActionModeDev)
if err := compiler.CompileWorkflow(workflowPath); err != nil {
t.Fatalf("Failed to compile workflow: %v", err)
}

// Read generated lock file
lockPath := filepath.Join(workflowsDir, "test-prt-workflow.lock.yml")
lockContent, err := os.ReadFile(lockPath)
if err != nil {
t.Fatalf("Failed to read lock file: %v", err)
}
lockStr := string(lockContent)

// Check for PR checkout step
hasPRCheckout := strings.Contains(lockStr, "Checkout PR branch")
if hasPRCheckout != tt.expectPRCheckout {
t.Errorf("%s: expected PR checkout step: %v, got: %v", tt.description, tt.expectPRCheckout, hasPRCheckout)
}
})
}
}
20 changes: 20 additions & 0 deletions pkg/workflow/pr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,26 @@ pull-requests: read`,
}
}

func TestShouldGeneratePRCheckoutStep_CheckoutDisabled(t *testing.T) {
t.Run("returns false when CheckoutDisabled is true even with contents read", func(t *testing.T) {
data := &WorkflowData{
Permissions: "contents: read",
CheckoutDisabled: true,
}
result := ShouldGeneratePRCheckoutStep(data)
assert.False(t, result, "ShouldGeneratePRCheckoutStep() should return false when CheckoutDisabled is true")
})

t.Run("returns true when CheckoutDisabled is false with contents read", func(t *testing.T) {
data := &WorkflowData{
Permissions: "contents: read",
CheckoutDisabled: false,
}
result := ShouldGeneratePRCheckoutStep(data)
assert.True(t, result, "ShouldGeneratePRCheckoutStep() should return true when CheckoutDisabled is false and permissions allow")
})
}

func TestGeneratePRReadyForReviewCheckout_IncludesWorkflowDispatchIssueCommentContext(t *testing.T) {
compiler := NewCompiler()
var yaml strings.Builder
Expand Down
19 changes: 9 additions & 10 deletions pkg/workflow/pull_request_target_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Test workflow content.`,
warningCount: 1, // sandbox.agent: false
},
{
name: "pull_request_target with checkout enabled - non-strict - should warn",
name: "pull_request_target with no checkout key - non-strict - checkout auto-disabled",
frontmatter: `---
strict: false
on:
Expand All @@ -72,7 +72,7 @@ Test workflow content.`,
strictMode: false,
expectError: false,
expectWarning: true,
warningCount: 2, // 1 for insecure checkout + 1 for sandbox.agent: false
warningCount: 1, // checkout is auto-disabled for pull_request_target; only sandbox.agent: false warning
},
{
name: "pull_request_target with trusted checkout - non-strict - no warnings no error",
Expand Down Expand Up @@ -170,7 +170,7 @@ Test workflow content.`,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with checkout enabled - strict - error (extremely insecure)",
name: "pull_request_target with no checkout key - strict - checkout auto-disabled, dangerous-trigger warning only",
frontmatter: `---
on:
pull_request_target:
Expand All @@ -186,10 +186,9 @@ permissions:
Test workflow content.`,
filename: "prt-checkout-enabled-strict.md",
strictMode: true,
expectError: true,
expectWarning: true, // dangerous-trigger warning is still emitted before the error
errorContains: "pull_request_target trigger with checkout enabled is extremely insecure",
warningCount: 1, // dangerous-trigger warning
expectError: false,
expectWarning: true, // dangerous-trigger warning only; checkout auto-disabled so no insecure-checkout error
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with explicit checkout pinned to base sha - strict - warning only",
Expand Down Expand Up @@ -341,7 +340,7 @@ Test workflow content.`,
warningCount: 1, // dangerous-trigger warning
},
{
name: "pull_request_target with checkout enabled - strict CLI + frontmatter strict false - warning only",
name: "pull_request_target with no checkout key - strict CLI + frontmatter strict false - no warnings",
frontmatter: `---
strict: false
on:
Expand All @@ -359,8 +358,8 @@ Test workflow content.`,
filename: "prt-checkout-enabled-strict-frontmatter-opt-out.md",
strictMode: true,
expectError: false,
expectWarning: true,
warningCount: 1, // insecure-checkout warning only
expectWarning: false,
warningCount: 0, // checkout auto-disabled; strict:false suppresses dangerous-trigger warning; no insecure-checkout warning
},
{
name: "pull_request trigger (not target) - strict - no diagnostic",
Expand Down
17 changes: 17 additions & 0 deletions pkg/workflow/workflow_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,23 @@ func (c *Compiler) buildInitialWorkflowData(
}
}

// Auto-disable checkout for pull_request_target workflows when not explicitly configured.
// For pull_request_target events, the head branch is often deleted (closed/merged PRs)
// or inaccessible (fork PRs), causing the "Checkout PR branch" step to fail.
// Users who need checkout can explicitly set a checkout configuration in frontmatter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 36427c9 and expanded further in f534297. docs/src/content/docs/reference/checkout.md now has an explicit ## pull_request_target Checkout section describing: (1) the PR head checkout is auto-disabled by default for pull_request_target workflows, (2) opt-in with an explicit checkout: mapping for a trusted ref (e.g. base SHA), and a warning against using head.sha or refs/pull/.../head in a privileged job. The checkout: false description was also updated to say it suppresses both actions/checkout and the PR-specific step.

if len(workflowData.CheckoutConfigs) == 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-disable guard fires before imported checkout configs are merged, so a workflow that imports a shared workflow providing checkout configs will have CheckoutConfigs empty at this check, trigger auto-disable, and lose the shared checkout — which is the opposite of the intended behavior.

💡 Suggested fix

Move the auto-disable block to after the import-merge block (lines 125–144), or alternatively re-evaluate after merging:

// ... merge imported checkout configs first ...

// Auto-disable for pull_request_target only when nothing was configured at all
if len(workflowData.CheckoutConfigs) == 0 && !workflowData.CheckoutDisabled {
    if hasPullRequestTarget(result.Frontmatter) {
        workflowData.CheckoutDisabled = true
    }
}

Placing it after the merge ensures that an explicitly imported checkout config prevents auto-disable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. The auto-disable block was moved to after the import-merge block. Imports are merged at lines 112–129; auto-disable runs at lines 131–152 (after). The guard checks len(workflowData.CheckoutConfigs) == 0 at that point, so an imported checkout config (merged in at 127) prevents auto-disable. The comment in the code also explicitly notes this ordering: "This block runs after import merging so that imported checkout configs prevent auto-disable."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The outer guard len(workflowData.CheckoutConfigs) == 0 is checked before checkoutExplicitlySet, so a workflow that sets checkout: false first populates CheckoutDisabled = true (line 102) but also happens to have empty CheckoutConfigs, making it pass this outer condition — and then re-sets CheckoutDisabled = true unnecessarily for any pull_request_target workflow. This is harmless today, but it is logically redundant and fragile: if CheckoutDisabled is used as a sentinel for something in the future the double-write may mask a real state.

💡 Suggestion

Add an early exit for the already-explicitly-disabled case:

if !workflowData.CheckoutDisabled && len(workflowData.CheckoutConfigs) == 0 {
    if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet {
        ...
    }
}

This makes the intent explicit: only auto-disable when nothing has been configured.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. The outer guard now reads if !workflowData.CheckoutDisabled && len(workflowData.CheckoutConfigs) == 0 { so a workflow with checkout: false (which already sets CheckoutDisabled = true) bypasses the block entirely and avoids the redundant write.

if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet {
if onVal, ok := result.Frontmatter["on"]; ok {
if onMap, ok := onVal.(map[string]any); ok {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on: as scalar or list silently bypasses the auto-disable logic — if a user writes on: pull_request_target (scalar) or on: [push, pull_request_target] (sequence), the inner onMap type assertion fails and CheckoutDisabled is never set, defeating the safety mechanism.

💡 Suggested fix

Extract a helper that handles all three YAML on: forms:

func hasTrigger(onVal any, trigger string) bool {
    switch v := onVal.(type) {
    case string:
        return v == trigger
    case []any:
        for _, item := range v {
            if s, ok := item.(string); ok && s == trigger {
                return true
            }
        }
    case map[string]any:
        _, ok := v[trigger]
        return ok
    }
    return false
}

Then replace the nested map assertion with hasTrigger(onVal, "pull_request_target"). The same blind spot exists at lines 159 and 615 in this file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. Added frontmatterHasTrigger(onVal, trigger string) bool in workflow_builder.go that handles all three YAML on: forms: string scalar (on: pull_request_target), sequence (on: [push, pull_request_target]), and mapping (on:\n pull_request_target:\n types: [closed]). Both hasPRT and hasPR are detected using this helper, so on: pull_request_target (scalar) and on: [pull_request_target] (sequence) correctly trigger auto-disable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The map[string]any assertion silently skips auto-disable when on: is a string or list — e.g. on: pull_request_target or on: [pull_request_target, push] — leaving those workflows exposed to the original failure.

💡 Suggested fix

After the existing map-path check, also handle the string form:

if onStr, ok := onVal.(string); ok && onStr == "pull_request_target" {
    workflowBuilderLog.Print("Auto-disabling checkout for pull_request_target workflow")
    workflowData.CheckoutDisabled = true
}

And for the list form ([]any), iterate and check each element. Without this, a workflow authored with on: pull_request_target (the shorthand YAML syntax) will not receive the auto-disable.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. Added frontmatterHasTrigger(onVal, trigger) that handles all three YAML on: forms: string scalar (on: pull_request_target), sequence (on: [pull_request_target, push]), and mapping. Both the auto-disable check and the IsPullRequestTarget flag now use this helper.

if _, hasPRT := onMap["pull_request_target"]; hasPRT {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixed-trigger regression: if a workflow uses both pull_request and pull_request_target triggers, checkout is auto-disabled for all events — including pull_request events where head branches are accessible. The guard should skip auto-disabling when pull_request (or other normal checkout-compatible triggers) co-exist.

_, hasPR := onMap["pull_request"]
if hasPRT && !hasPR {
    workflowData.CheckoutDisabled = true
}

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. The auto-disable block now guards with !hasPR (using the new frontmatterHasTrigger helper) so workflows combining pull_request and pull_request_target triggers are not affected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-trigger workflows have checkout globally disabled — if a workflow uses both push and pull_request_target in its on: map, this block sets CheckoutDisabled = true for all triggers, breaking the push path which has no such restriction.

💡 Suggested fix

This is inherently tricky because CheckoutDisabled is a single boolean. The auto-disable should only apply when pull_request_target is the only (or at least the primary) trigger. Either restrict auto-disable to single-trigger pull_request_target workflows, or introduce per-trigger checkout configuration rather than a global flag.

As a minimal safe approach, skip auto-disable when other triggers coexist:

if onMap, ok := onVal.(map[string]any); ok {
    _, hasPRT := onMap["pull_request_target"]
    onlyPRT := hasPRT && len(onMap) == 1
    if onlyPRT {
        workflowData.CheckoutDisabled = true
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f534297. Auto-disable only fires when hasPRT && !hasPR — i.e., when pull_request_target is present but pull_request is not. Workflows combining pull_request_target with other triggers like push or workflow_dispatch do still get IsPullRequestTarget = true (to suppress checkout_pr_branch.cjs), but CheckoutDisabled is only auto-set when no pull_request trigger co-exists and no explicit checkout config is present.

workflowBuilderLog.Print("Auto-disabling checkout for pull_request_target workflow")
workflowData.CheckoutDisabled = true
}
}
}
}
}

// Merge checkout configs from imported shared workflows.
// Imported configs are appended after the main workflow's configs so that the main
// workflow's entries take precedence when CheckoutManager deduplicates by (repository, path).
Expand Down
Loading