From 82df40a35226a7569d28b6d8c97acaf2e219d71c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:38:54 +0000 Subject: [PATCH 1/5] Initial plan From 36427c98ec56eaa8087c8f7eed2e74fbf0563778 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:14:35 +0000 Subject: [PATCH 2/5] Disable checkout by default for pull_request_target workflows - ShouldGeneratePRCheckoutStep now respects CheckoutDisabled flag - Auto-disable checkout for pull_request_target when not explicitly configured - Update tests to reflect new default behavior - Recompile ai-moderator workflow (checkout: false now correctly removes PR checkout step)" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/ai-moderator.lock.yml | 22 ---- pkg/workflow/pr.go | 6 +- .../pr_ready_for_review_checkout_test.go | 118 ++++++++++++++++++ pkg/workflow/pr_test.go | 20 +++ .../pull_request_target_validation_test.go | 19 ++- pkg/workflow/workflow_builder.go | 17 +++ 6 files changed, 169 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ai-moderator.lock.yml b/.github/workflows/ai-moderator.lock.yml index f5994b3114e..7bad2fb9514 100644 --- a/.github/workflows/ai-moderator.lock.yml +++ b/.github/workflows/ai-moderator.lock.yml @@ -494,7 +494,6 @@ jobs: ambient_context: ${{ steps.parse-mcp-gateway.outputs.ambient_context }} cache_memory_restore_0_cache_hit: ${{ steps.restore_cache_memory_0.outputs.cache-hit || 'false' }} cache_memory_restore_0_matched_key: ${{ steps.restore_cache_memory_0.outputs.cache-matched-key || '' }} - checkout_pr_success: ${{ steps.checkout-pr.outputs.checkout_pr_success || 'true' }} effective_tokens: ${{ steps.parse-mcp-gateway.outputs.effective_tokens }} has_patch: ${{ steps.collect_output.outputs.has_patch }} http_400_response_error: ${{ steps.detect-agent-errors.outputs.http_400_response_error || 'false' }} @@ -570,20 +569,6 @@ jobs: GH_AW_MIN_INTEGRITY: none GH_AW_ALLOWED_EXTENSIONS: '.json' run: bash "${RUNNER_TEMP}/gh-aw/actions/setup_cache_memory_git.sh" - - name: Checkout PR branch - id: checkout-pr - if: | - github.event.pull_request || github.event.issue.pull_request || github.event_name == 'workflow_dispatch' && fromJSON(github.event.inputs.aw_context || '{}').item_type == 'pull_request' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - GH_TOKEN: ${{ secrets.GH_AW_GITHUB_MCP_SERVER_TOKEN || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} - with: - github-token: ${{ secrets.GH_AW_GITHUB_MCP_SERVER_TOKEN || secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} - script: | - const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs'); - setupGlobals(core, github, context, exec, io, getOctokit); - const { main } = require('${{ runner.temp }}/gh-aw/actions/checkout_pr_branch.cjs'); - await main(); - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -611,12 +596,6 @@ jobs: GH_AW_TRUSTED_USERS_VAR: ${{ vars.GH_AW_GITHUB_TRUSTED_USERS || '' }} GH_AW_APPROVAL_LABELS_VAR: ${{ vars.GH_AW_GITHUB_APPROVAL_LABELS || '' }} run: bash "${RUNNER_TEMP}/gh-aw/actions/parse_guard_list.sh" - - name: Restore agent config folders from base branch - if: steps.checkout-pr.outcome == 'success' - env: - GH_AW_AGENT_FOLDERS: ".agents .antigravity .claude .codex .gemini .github .opencode .pi" - GH_AW_AGENT_FILES: "AGENTS.md ANTIGRAVITY.md CLAUDE.md GEMINI.md PI.md opencode.jsonc" - run: bash "${RUNNER_TEMP}/gh-aw/actions/restore_base_github_folders.sh" - name: Restore inline sub-agents from activation artifact env: GH_AW_SUB_AGENT_DIR: ".codex/agents" @@ -1385,7 +1364,6 @@ jobs: GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "12" GH_AW_ENGINE_ID: "codex" GH_AW_SECRET_VERIFICATION_RESULT: ${{ needs.activation.outputs.secret_verification_result }} - GH_AW_CHECKOUT_PR_SUCCESS: ${{ needs.agent.outputs.checkout_pr_success }} GH_AW_EFFECTIVE_TOKENS: ${{ needs.agent.outputs.effective_tokens || '' }} GH_AW_AI_CREDITS_RATE_LIMIT_ERROR: ${{ needs.agent.outputs.ai_credits_rate_limit_error || 'false' }} GH_AW_UNKNOWN_MODEL_AI_CREDITS: ${{ needs.agent.outputs.unknown_model_ai_credits || 'false' }} diff --git a/pkg/workflow/pr.go b/pkg/workflow/pr.go index b97f7ed5fea..2d13b683fb3 100644 --- a/pkg/workflow/pr.go +++ b/pkg/workflow/pr.go @@ -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() } diff --git a/pkg/workflow/pr_ready_for_review_checkout_test.go b/pkg/workflow/pr_ready_for_review_checkout_test.go index 0894ddbf9a0..3294c6ce916 100644 --- a/pkg/workflow/pr_ready_for_review_checkout_test.go +++ b/pkg/workflow/pr_ready_for_review_checkout_test.go @@ -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", + }, + } + + 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) + } + }) + } +} diff --git a/pkg/workflow/pr_test.go b/pkg/workflow/pr_test.go index a3387cae621..3f03e9d530f 100644 --- a/pkg/workflow/pr_test.go +++ b/pkg/workflow/pr_test.go @@ -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 diff --git a/pkg/workflow/pull_request_target_validation_test.go b/pkg/workflow/pull_request_target_validation_test.go index c5de575852e..ef25b0a85b1 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 checkout enabled - non-strict - should warn", + name: "pull_request_target with no checkout key - non-strict - checkout auto-disabled", frontmatter: `--- strict: false on: @@ -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", @@ -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: @@ -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", @@ -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: @@ -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", diff --git a/pkg/workflow/workflow_builder.go b/pkg/workflow/workflow_builder.go index 09872dacb3c..038973f1497 100644 --- a/pkg/workflow/workflow_builder.go +++ b/pkg/workflow/workflow_builder.go @@ -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. + if len(workflowData.CheckoutConfigs) == 0 { + if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { + if onVal, ok := result.Frontmatter["on"]; ok { + if onMap, ok := onVal.(map[string]any); ok { + if _, hasPRT := onMap["pull_request_target"]; hasPRT { + 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). From ceaaf87babb2a3708ba2f8500b5b3d532d21dfa0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:32:45 +0000 Subject: [PATCH 3/5] docs(adr): add ADR-46771 for disabling PR checkout by default on pull_request_target Co-Authored-By: Claude Sonnet 4.6 --- ...kout-by-default-for-pull-request-target.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/46771-disable-checkout-by-default-for-pull-request-target.md diff --git a/docs/adr/46771-disable-checkout-by-default-for-pull-request-target.md b/docs/adr/46771-disable-checkout-by-default-for-pull-request-target.md new file mode 100644 index 00000000000..522c0493292 --- /dev/null +++ b/docs/adr/46771-disable-checkout-by-default-for-pull-request-target.md @@ -0,0 +1,50 @@ +# ADR-46771: Disable PR Branch Checkout by Default for pull_request_target Workflows + +**Date**: 2026-07-20 +**Status**: Draft +**Deciders**: pelikhan (via Copilot SWE agent, PR #46771) + +--- + +### Context + +The `pull_request_target` GitHub Actions trigger runs in the context of the base branch, which is a security boundary that enables accessing repository secrets from fork PRs. The workflow compiler previously generated a "Checkout PR branch" step by default whenever the workflow had `contents: read` permission, regardless of the trigger type. + +For `pull_request_target` workflows, the "Checkout PR branch" step hard-fails in two common scenarios: (1) when the event type is `closed`, because the head branch is deleted on merge; and (2) on any fork PR, because the fork's head branch is inaccessible from the base repository. A pre-existing bug compounded this: the `checkout: false` frontmatter key was not suppressing the PR checkout step — it only skipped the default `actions/checkout` — so users who set `checkout: false` still saw the step generated and failing. + +### Decision + +We will auto-disable the "Checkout PR branch" step for `pull_request_target` workflows at compile time when no explicit `checkout:` key is present in the frontmatter and no checkout configurations are set. Users who require PR branch checkout for `pull_request_target` can opt in by providing an explicit checkout mapping (e.g., pinning to `base.sha`). We will also fix the `ShouldGeneratePRCheckoutStep` function to respect the `CheckoutDisabled` flag, so `checkout: false` in frontmatter correctly suppresses the step. + +### Alternatives Considered + +#### Alternative 1: Make the Checkout Step Resilient with `continue-on-error` + +Add `continue-on-error: true` to the generated "Checkout PR branch" step, or add explicit error handling in the step script, so that failures on merged/fork PRs do not fail the whole workflow. + +This was not chosen because it silently hides failures — the step would report as successful even when checkout did not happen, making downstream steps that depend on the checked-out state behave unpredictably. It also does not address the security concern of attempting to access fork branch content in a `pull_request_target` context. + +#### Alternative 2: Runtime Detection and Dynamic Skip + +Detect at runtime (inside the step) whether the head branch is accessible — checking if the PR is merged or from a fork — and emit a skip-with-warning rather than executing checkout. + +This was not chosen because it adds runtime complexity and latency (the check requires an API call), and the failure condition can be determined statically from the trigger type. Compile-time disabling is simpler, more predictable, and avoids unnecessary step execution in the generated workflow YAML entirely. + +### Consequences + +#### Positive +- `pull_request_target` workflows no longer hard-fail on `closed` events or fork PRs due to the checkout step attempting to access a deleted or inaccessible branch. +- The pre-existing bug where `checkout: false` did not suppress the "Checkout PR branch" step is fixed; `CheckoutDisabled` is now respected by `ShouldGeneratePRCheckoutStep`. +- Reduces the attack surface from insecure checkouts in `pull_request_target` contexts, since checking out fork code in this trigger is a known security anti-pattern. + +#### Negative +- Existing `pull_request_target` workflows that previously relied on the implicit checkout step (without an explicit `checkout:` key) will have the step removed after recompile. In practice these workflows were already failing on closed/fork events, so the blast radius is low. +- Users who intentionally need checkout in a `pull_request_target` context must now explicitly configure it, adding a small onboarding friction for new workflows. + +#### Neutral +- The `ai-moderator.lock.yml` compiled workflow is updated to reflect the removal of the checkout step and its associated `checkout_pr_success` output variable and downstream references. +- Validation test expectations for `pull_request_target` are updated: cases that previously emitted an "insecure checkout" error/warning now only emit the "dangerous-trigger" warning, since checkout is no longer auto-generated. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From f534297aa567d759ba33d7d862c5aab715171a27 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 10:33:36 +0000 Subject: [PATCH 4/5] fix: address review feedback on pull_request_target checkout safety - Add frontmatterHasTrigger() helper supporting string/[]any/map forms of the YAML `on:` field (fixes silent bypass when on: is scalar/list) - Add IsPullRequestTarget field to WorkflowData; set it when pull_request_target trigger is present without pull_request co-trigger - ShouldGeneratePRCheckoutStep now returns false for IsPullRequestTarget to prevent checkout_pr_branch.cjs from fetching refs/pull//head in a privileged context (the trusted base-SHA checkout still happens) - Move auto-disable block after import-merge so imported checkout configs prevent auto-disable - Guard auto-disable with !CheckoutDisabled to avoid redundant writes - Fix mixed-trigger regression: only auto-disable when pull_request trigger is absent - Update integration test: trusted base-SHA checkout case no longer expects the PR checkout step (expectPRCheckout: false) - Add security assertion: PR checkout step must not contain refs/pull - Add unit tests for frontmatterHasTrigger and IsPullRequestTarget - Update checkout.md docs to describe pull_request_target defaults and that checkout: false suppresses both steps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- docs/src/content/docs/reference/checkout.md | 21 ++++++- pkg/workflow/pr.go | 5 +- .../pr_ready_for_review_checkout_test.go | 17 ++++- pkg/workflow/pr_test.go | 39 ++++++++++++ pkg/workflow/workflow_builder.go | 62 ++++++++++++++----- pkg/workflow/workflow_data.go | 3 +- 6 files changed, 123 insertions(+), 24 deletions(-) diff --git a/docs/src/content/docs/reference/checkout.md b/docs/src/content/docs/reference/checkout.md index 90dd9070977..710fa97cbbe 100644 --- a/docs/src/content/docs/reference/checkout.md +++ b/docs/src/content/docs/reference/checkout.md @@ -7,7 +7,7 @@ sidebar: The `checkout:` frontmatter field controls how `actions/checkout` is invoked in the agent job. Configure custom checkout settings, check out multiple repositories, or disable checkout entirely. -By default, the agent checks out the repository where the workflow is running with a shallow fetch (`fetch-depth: 1`). If triggered by a pull request event, it also checks out the PR head ref. For most workflows, this default checkout is sufficient and no `checkout:` configuration is necessary. +By default, the agent checks out the repository where the workflow is running with a shallow fetch (`fetch-depth: 1`). If triggered by a `pull_request` event, it also checks out the PR head ref. For `pull_request_target` events, checkout of the PR head branch is **disabled by default** — the head branch may be deleted (merged/closed PRs) or inaccessible (fork PRs), causing the step to hard-fail. For most workflows, this default checkout is sufficient and no `checkout:` configuration is necessary. Use `checkout:` when you need to check out additional branches, check out multiple repositories, or to disable checkout entirely for workflows that don't need to access code or can access code dynamically through the GitHub Tools. @@ -112,7 +112,7 @@ Fetch everything the workflow needs at checkout time using `fetch-depth` and [`f ## Disabling Checkout (`checkout: false`) -Set `checkout: false` to suppress the default `actions/checkout` step entirely. Use this for workflows that access repositories through MCP servers or other mechanisms that do not require a local clone: +Set `checkout: false` to suppress both the default `actions/checkout` step and the PR-specific "Checkout PR branch" step entirely. Use this for workflows that access repositories through MCP servers or other mechanisms that do not require a local clone: ```yaml wrap checkout: false @@ -120,6 +120,23 @@ checkout: false This is equivalent to omitting the checkout step from the agent job. Custom dev-mode steps (such as "Checkout actions folder") are unaffected. +## `pull_request_target` Checkout + +For `pull_request_target` workflows, the "Checkout PR branch" step is auto-disabled when no `checkout:` key is present in frontmatter. This prevents hard failures when the PR head branch is deleted (merged/closed events) or inaccessible (fork PRs). + +To check out a trusted ref such as the base SHA, opt in explicitly with a `checkout:` mapping: + +```yaml wrap +on: + pull_request_target: + types: [opened, synchronize] +checkout: + repository: ${{ github.repository }} + ref: ${{ github.event.pull_request.base.sha }} +``` + +This produces the "Checkout PR branch" step pointing at the safe base ref. Do **not** use `github.event.pull_request.head.sha` or `refs/pull/.../head` in a privileged `pull_request_target` job — that would check out untrusted code in a context with write permissions. + ## Marking a Primary Repository (`current: true`) When a workflow running from a central repository targets a different repository, use `current: true` to tell the agent which repository to treat as its primary working target. The agent uses this as the default for all GitHub operations (creating issues, opening PRs, reading content) unless the prompt instructs otherwise. When omitted, the agent defaults to the repository where the workflow is running. diff --git a/pkg/workflow/pr.go b/pkg/workflow/pr.go index 2d13b683fb3..e4b2d35a19d 100644 --- a/pkg/workflow/pr.go +++ b/pkg/workflow/pr.go @@ -12,8 +12,11 @@ var prLog = logger.New("workflow:pr") // ShouldGeneratePRCheckoutStep returns true if the checkout-pr step should be generated // based on the workflow permissions and checkout configuration. // The step requires contents read access and checkout must not be disabled. +// For pull_request_target workflows, the step is always suppressed regardless of checkout +// configuration: checkout_pr_branch.cjs fetches refs/pull//head (the untrusted PR head), +// which replaces any safe base-SHA checkout and is inaccessible for fork PRs. func ShouldGeneratePRCheckoutStep(data *WorkflowData) bool { - if data.CheckoutDisabled { + if data.CheckoutDisabled || data.IsPullRequestTarget { return false } if data.CachedPermissions != nil { diff --git a/pkg/workflow/pr_ready_for_review_checkout_test.go b/pkg/workflow/pr_ready_for_review_checkout_test.go index 3294c6ce916..7dd0bbb9843 100644 --- a/pkg/workflow/pr_ready_for_review_checkout_test.go +++ b/pkg/workflow/pr_ready_for_review_checkout_test.go @@ -276,7 +276,7 @@ Workflow triggered when a PR is closed. 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", + name: "pull_request_target with trusted checkout mapping - no PR checkout step", workflowContent: `--- on: pull_request_target: @@ -293,8 +293,8 @@ checkout: # 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", + expectPRCheckout: false, + description: "pull_request_target with explicit checkout mapping should NOT generate 'Checkout PR branch' step (checkout_pr_branch.cjs fetches refs/pull//head, not the trusted base SHA)", }, } @@ -339,6 +339,17 @@ Workflow triggered when a PR is opened with a trusted base checkout. if hasPRCheckout != tt.expectPRCheckout { t.Errorf("%s: expected PR checkout step: %v, got: %v", tt.description, tt.expectPRCheckout, hasPRCheckout) } + + // When a PR checkout step IS present, verify it does not reference insecure + // PR-head refs (e.g. refs/pull//head). The trusted-base opt-in must check + // out a safe ref such as the base SHA, not the untrusted PR head. + if hasPRCheckout { + for _, insecurePattern := range []string{"refs/pull", "head.ref", "head_ref"} { + if strings.Contains(lockStr, insecurePattern) { + t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern %q", tt.description, insecurePattern) + } + } + } }) } } diff --git a/pkg/workflow/pr_test.go b/pkg/workflow/pr_test.go index 3f03e9d530f..b94382974a7 100644 --- a/pkg/workflow/pr_test.go +++ b/pkg/workflow/pr_test.go @@ -94,6 +94,16 @@ func TestShouldGeneratePRCheckoutStep_CheckoutDisabled(t *testing.T) { result := ShouldGeneratePRCheckoutStep(data) assert.True(t, result, "ShouldGeneratePRCheckoutStep() should return true when CheckoutDisabled is false and permissions allow") }) + + t.Run("returns false when IsPullRequestTarget is true even with contents read and explicit checkout", func(t *testing.T) { + data := &WorkflowData{ + Permissions: "contents: read", + CheckoutDisabled: false, + IsPullRequestTarget: true, + } + result := ShouldGeneratePRCheckoutStep(data) + assert.False(t, result, "ShouldGeneratePRCheckoutStep() should return false for pull_request_target workflows to prevent refs/pull//head checkout") + }) } func TestGeneratePRReadyForReviewCheckout_IncludesWorkflowDispatchIssueCommentContext(t *testing.T) { @@ -112,3 +122,32 @@ func TestGeneratePRReadyForReviewCheckout_IncludesWorkflowDispatchIssueCommentCo assert.Contains(t, rendered, "fromJSON(github.event.inputs.aw_context || '{}').item_type == 'pull_request'") assert.NotContains(t, rendered, "fromJSON(github.event.inputs.aw_context || '{}').event_type") } + +func TestFrontmatterHasTrigger(t *testing.T) { + tests := []struct { + name string + onVal any + trigger string + expected bool + }{ + // scalar string form: on: pull_request_target + {name: "scalar matches", onVal: "pull_request_target", trigger: "pull_request_target", expected: true}, + {name: "scalar no match", onVal: "push", trigger: "pull_request_target", expected: false}, + // sequence form: on: [pull_request_target, push] + {name: "slice matches first", onVal: []any{"pull_request_target", "push"}, trigger: "pull_request_target", expected: true}, + {name: "slice matches second", onVal: []any{"push", "pull_request_target"}, trigger: "pull_request_target", expected: true}, + {name: "slice no match", onVal: []any{"push", "schedule"}, trigger: "pull_request_target", expected: false}, + // mapping form: on:\n pull_request_target:\n types: [closed] + {name: "map matches", onVal: map[string]any{"pull_request_target": map[string]any{"types": []any{"closed"}}}, trigger: "pull_request_target", expected: true}, + {name: "map no match", onVal: map[string]any{"push": nil}, trigger: "pull_request_target", expected: false}, + // nil / unknown + {name: "nil returns false", onVal: nil, trigger: "pull_request_target", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := frontmatterHasTrigger(tt.onVal, tt.trigger) + assert.Equal(t, tt.expected, got) + }) + } +} diff --git a/pkg/workflow/workflow_builder.go b/pkg/workflow/workflow_builder.go index 038973f1497..53615ad5dc3 100644 --- a/pkg/workflow/workflow_builder.go +++ b/pkg/workflow/workflow_builder.go @@ -105,23 +105,6 @@ 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. - if len(workflowData.CheckoutConfigs) == 0 { - if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { - if onVal, ok := result.Frontmatter["on"]; ok { - if onMap, ok := onVal.(map[string]any); ok { - if _, hasPRT := onMap["pull_request_target"]; hasPRT { - 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). @@ -145,6 +128,29 @@ func (c *Compiler) buildInitialWorkflowData( } } + // Auto-disable checkout for pull_request_target-only 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. + // This block runs after import merging so that imported checkout configs prevent auto-disable. + // Auto-disable is skipped when pull_request (or other checkout-compatible) triggers co-exist, + // because those events do have accessible head branches. + onVal := result.Frontmatter["on"] + hasPRT := frontmatterHasTrigger(onVal, "pull_request_target") + hasPR := frontmatterHasTrigger(onVal, "pull_request") + if hasPRT && !hasPR { + // Mark the workflow as pull_request_target-only so ShouldGeneratePRCheckoutStep + // suppresses the checkout_pr_branch.cjs step regardless of checkout configuration. + workflowData.IsPullRequestTarget = true + + if !workflowData.CheckoutDisabled && len(workflowData.CheckoutConfigs) == 0 { + if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { + workflowBuilderLog.Print("Auto-disabling checkout for pull_request_target workflow") + workflowData.CheckoutDisabled = true + } + } + } + // Populate check-for-updates flag: disabled when check-for-updates: false is set in frontmatter. if toolsResult.parsedFrontmatter != nil && toolsResult.parsedFrontmatter.UpdateCheck != nil { workflowData.UpdateCheckDisabled = !*toolsResult.parsedFrontmatter.UpdateCheck @@ -920,3 +926,25 @@ func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflow } return nil } + +// frontmatterHasTrigger reports whether the given "on:" frontmatter value contains +// the specified trigger name. It handles all three YAML "on:" forms: +// - string scalar: on: pull_request_target +// - sequence: on: [pull_request_target, push] +// - mapping: on:\n pull_request_target:\n types: [closed] +func frontmatterHasTrigger(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 +} diff --git a/pkg/workflow/workflow_data.go b/pkg/workflow/workflow_data.go index 3390e1969f1..939b59e7ab1 100644 --- a/pkg/workflow/workflow_data.go +++ b/pkg/workflow/workflow_data.go @@ -162,7 +162,8 @@ type WorkflowData struct { HasExplicitGitHubTool bool // true if tools.github was explicitly configured in frontmatter 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 + CheckoutDisabled bool // true when checkout: false is set in frontmatter, or auto-disabled for pull_request_target + 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) IsDetectionRun bool // true when this WorkflowData is used for inline threat detection (not the main agent run) From 37bc639b05a862932794d91c03c0e8af17359663 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Jul 2026 10:35:17 +0000 Subject: [PATCH 5/5] fix: tighten insecure pattern check and clarify comment in ShouldGeneratePRCheckoutStep - Remove head.ref/head_ref from security assertion (only check refs/pull which is the specific pattern emitted by checkout_pr_branch.cjs) - Clarify pr.go comment: suppression is for pull_request_target-only workflows (not all pull_request_target workflows) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/pr.go | 5 +++-- pkg/workflow/pr_ready_for_review_checkout_test.go | 15 ++++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/workflow/pr.go b/pkg/workflow/pr.go index e4b2d35a19d..3ee5a1e76e9 100644 --- a/pkg/workflow/pr.go +++ b/pkg/workflow/pr.go @@ -12,8 +12,9 @@ var prLog = logger.New("workflow:pr") // ShouldGeneratePRCheckoutStep returns true if the checkout-pr step should be generated // based on the workflow permissions and checkout configuration. // The step requires contents read access and checkout must not be disabled. -// For pull_request_target workflows, the step is always suppressed regardless of checkout -// configuration: checkout_pr_branch.cjs fetches refs/pull//head (the untrusted PR head), +// For pull_request_target-only workflows (where pull_request is not also a trigger), +// the step is always suppressed regardless of checkout configuration: +// checkout_pr_branch.cjs fetches refs/pull//head (the untrusted PR head), // which replaces any safe base-SHA checkout and is inaccessible for fork PRs. func ShouldGeneratePRCheckoutStep(data *WorkflowData) bool { if data.CheckoutDisabled || data.IsPullRequestTarget { diff --git a/pkg/workflow/pr_ready_for_review_checkout_test.go b/pkg/workflow/pr_ready_for_review_checkout_test.go index 7dd0bbb9843..6b0390e20e3 100644 --- a/pkg/workflow/pr_ready_for_review_checkout_test.go +++ b/pkg/workflow/pr_ready_for_review_checkout_test.go @@ -340,14 +340,15 @@ Workflow triggered when a PR is opened with a trusted base checkout. t.Errorf("%s: expected PR checkout step: %v, got: %v", tt.description, tt.expectPRCheckout, hasPRCheckout) } - // When a PR checkout step IS present, verify it does not reference insecure - // PR-head refs (e.g. refs/pull//head). The trusted-base opt-in must check - // out a safe ref such as the base SHA, not the untrusted PR head. + // When a PR checkout step IS present, verify it does not fetch the insecure + // PR-head ref (refs/pull//head). The trusted-base opt-in must use a safe ref + // such as the base SHA — checkout_pr_branch.cjs always fetches refs/pull/N/head. + // We check only for "refs/pull" because that is the specific pattern emitted by + // checkout_pr_branch.cjs; broader substrings (e.g. "head_ref") could appear in + // unrelated step names or comments and would produce false positives. if hasPRCheckout { - for _, insecurePattern := range []string{"refs/pull", "head.ref", "head_ref"} { - if strings.Contains(lockStr, insecurePattern) { - t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern %q", tt.description, insecurePattern) - } + if strings.Contains(lockStr, "refs/pull") { + t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern \"refs/pull\"", tt.description) } } })