diff --git a/actions/setup/js/push_branch_safety.cjs b/actions/setup/js/push_branch_safety.cjs new file mode 100644 index 00000000000..32b90c976a3 --- /dev/null +++ b/actions/setup/js/push_branch_safety.cjs @@ -0,0 +1,77 @@ +// @ts-check +const { getErrorMessage } = require("./error_helpers.cjs"); + +/** + * Validate that a target branch is safe for agent pushes. + * Blocks pushes to default/protected branches and fails closed on unexpected + * branch protection lookup errors. + * + * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} githubClient + * @param {string} owner + * @param {string} repo + * @param {string} branchName + * @param {boolean} checkBranchProtection + * @returns {Promise<{ success: true } | { success: false, error: string }>} + */ +async function validatePushBranchSafety(githubClient, owner, repo, branchName, checkBranchProtection) { + // Check whether the branch is the repository default branch + let defaultBranch = null; + try { + const { data: repoData } = await githubClient.rest.repos.get({ + owner, + repo, + }); + defaultBranch = repoData.default_branch; + } catch (repoError) { + core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`); + } + + if (defaultBranch && branchName === defaultBranch) { + const msg = `Cannot push to branch "${branchName}": this is the repository's default branch. Agents must not push directly to the default branch.`; + core.error(msg); + return { success: false, error: msg }; + } + + if (checkBranchProtection) { + // Check whether the branch has protection rules + let isBranchProtected = false; + try { + await githubClient.rest.repos.getBranchProtection({ + owner, + repo, + branch: branchName, + }); + // Successful response means branch protection rules exist + isBranchProtected = true; + } catch (protectionError) { + const protectionStatus = protectionError && typeof protectionError === "object" && "status" in protectionError ? protectionError.status : undefined; + if (protectionStatus === 404) { + // 404 means no protection rules – safe to proceed + core.info(`Branch "${branchName}" has no protection rules`); + } else if (protectionStatus === 403) { + // 403 means the token lacks permission to read branch protection rules. + // The GitHub platform will still enforce branch protection at push time, + // so warn and allow the push to proceed. + core.warning(`Could not check branch protection rules for "${branchName}" (insufficient permissions): ${getErrorMessage(protectionError)}`); + } else { + // Unexpected errors (5xx, network failures, etc.) – fail closed to + // avoid bypassing branch protection due to transient API issues. + const msg = `Cannot verify branch protection rules for "${branchName}": ${getErrorMessage(protectionError)}. Push blocked to prevent accidental writes to protected branches.`; + core.error(msg); + return { success: false, error: msg }; + } + } + + if (isBranchProtected) { + const msg = `Cannot push to branch "${branchName}": this branch has protection rules. Agents must not push directly to protected branches.`; + core.error(msg); + return { success: false, error: msg }; + } + } else { + core.info(`Branch protection pre-flight check disabled for "${branchName}" by config (check-branch-protection: false)`); + } + + return { success: true }; +} + +module.exports = { validatePushBranchSafety }; diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 2337566f7c4..e6c303c7f24 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -17,6 +17,7 @@ const { checkFileProtection } = require("./manifest_file_helpers.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { renderTemplateFromFile, buildProtectedFileList } = require("./messages_core.cjs"); const { getGitAuthEnv } = require("./git_helpers.cjs"); +const { validatePushBranchSafety } = require("./push_branch_safety.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -56,6 +57,7 @@ async function main(config = {}) { const envLabels = config.labels ? (Array.isArray(config.labels) ? config.labels : config.labels.split(",")).map(label => String(label).trim()).filter(label => label) : []; const ifNoChanges = config.if_no_changes || "warn"; const ignoreMissingBranchFailure = config.ignore_missing_branch_failure === true; + const checkBranchProtection = config.check_branch_protection !== false; const fallbackAsPullRequest = config.fallback_as_pull_request !== false; const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; @@ -92,6 +94,7 @@ async function main(config = {}) { } core.info(`If no changes: ${ifNoChanges}`); core.info(`Ignore missing branch failure: ${ignoreMissingBranchFailure}`); + core.info(`Check branch protection: ${checkBranchProtection}`); core.info(`Fallback as pull request: ${fallbackAsPullRequest}`); if (commitTitleSuffix) { core.info(`Commit title suffix: ${commitTitleSuffix}`); @@ -350,63 +353,11 @@ async function main(config = {}) { core.info(`PR title: ${prTitle}`); core.info(`PR labels: ${prLabels.join(", ")}`); - // SECURITY: Block pushing to the repository's default branch or any branch with - // protection rules. PR head branches must never be default or protected branches. - // This prevents agents from pushing directly to branches that should only receive - // changes through reviewed pull requests. - { - // Check whether the branch is the repository default branch - let defaultBranch = null; - try { - const { data: repoData } = await githubClient.rest.repos.get({ - owner: repoParts.owner, - repo: repoParts.repo, - }); - defaultBranch = repoData.default_branch; - } catch (repoError) { - core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`); - } - - if (defaultBranch && branchName === defaultBranch) { - const msg = `Cannot push to branch "${branchName}": this is the repository's default branch. Agents must not push directly to the default branch.`; - core.error(msg); - return { success: false, error: msg }; - } - - // Check whether the branch has protection rules - let isBranchProtected = false; - try { - await githubClient.rest.repos.getBranchProtection({ - owner: repoParts.owner, - repo: repoParts.repo, - branch: branchName, - }); - // Successful response means branch protection rules exist - isBranchProtected = true; - } catch (protectionError) { - const protectionStatus = protectionError && typeof protectionError === "object" && "status" in protectionError ? protectionError.status : undefined; - if (protectionStatus === 404) { - // 404 means no protection rules – safe to proceed - core.info(`Branch "${branchName}" has no protection rules`); - } else if (protectionStatus === 403) { - // 403 means the token lacks permission to read branch protection rules. - // The GitHub platform will still enforce branch protection at push time, - // so warn and allow the push to proceed. - core.warning(`Could not check branch protection rules for "${branchName}" (insufficient permissions): ${getErrorMessage(protectionError)}`); - } else { - // Unexpected errors (5xx, network failures, etc.) – fail closed to - // avoid bypassing branch protection due to transient API issues. - const msg = `Cannot verify branch protection rules for "${branchName}": ${getErrorMessage(protectionError)}. Push blocked to prevent accidental writes to protected branches.`; - core.error(msg); - return { success: false, error: msg }; - } - } - - if (isBranchProtected) { - const msg = `Cannot push to branch "${branchName}": this branch has protection rules. Agents must not push directly to protected branches.`; - core.error(msg); - return { success: false, error: msg }; - } + // SECURITY: Block pushing to the repository's default branch or any branch + // with protection rules. PR head branches must never be default/protected. + const branchSafetyResult = await validatePushBranchSafety(githubClient, repoParts.owner, repoParts.repo, branchName, checkBranchProtection); + if (!branchSafetyResult.success) { + return branchSafetyResult; } // Validate title prefix if specified diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs index b7f94059c63..b1800640b73 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -617,6 +617,20 @@ index 0000000..abc1234 // Should not attempt any git operations expect(mockExec.exec).not.toHaveBeenCalled(); }); + + it("should skip branch protection API check when check_branch_protection is false", async () => { + mockGithub.rest.repos.getBranchProtection.mockRejectedValue(Object.assign(new Error("Internal Server Error"), { status: 500 })); + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ check_branch_protection: false }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.repos.getBranchProtection).not.toHaveBeenCalled(); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Branch protection pre-flight check disabled")); + }); }); // ────────────────────────────────────────────────────── diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 0c5b380c1d8..dd66f96c30c 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -4720,6 +4720,12 @@ safe-outputs: # (optional) ignore-missing-branch-failure: true + # When true (default), perform a pre-flight branch protection API check before + # pushing. Set to false to skip the API check (for example, when the token does + # not have administration: read); GitHub still enforces protection at push time. + # (optional) + check-branch-protection: true + # Optional suffix to append to generated commit titles (e.g., ' [skip ci]' to # prevent triggering CI on the commit) # (optional) diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index 68a28e72a58..7e35169a3da 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -262,6 +262,7 @@ safe-outputs: github-token-for-extra-empty-commit: ${{ secrets.CI_TOKEN }} # optional token to push empty commit triggering CI fallback-as-pull-request: true # on non-fast-forward failure, create fallback PR to original PR branch (default: true) ignore-missing-branch-failure: false # treat deleted/missing branch errors as skipped instead of failed (default: false) + check-branch-protection: true # pre-flight branch protection API check (default: true) protected-files: fallback-to-issue # create review issue if protected files modified ``` @@ -315,6 +316,8 @@ When `fallback-as-pull-request` is enabled (default), non-fast-forward push fail When `ignore-missing-branch-failure: true` is set, push failures caused by a deleted or missing PR branch return `skipped: true` instead of a hard failure. This is useful when the PR branch may have been deleted before the safe-output job runs (for example, on auto-merged PRs). Without this flag, a missing branch is a terminal error. +When `check-branch-protection: false` is set, the handler skips the branch-protection pre-flight API call (which otherwise requires `administration: read` on GitHub App tokens). GitHub still enforces branch protection at push time. + ## Protected Files Both `create-pull-request` and `push-to-pull-request-branch` enforce protected file protection by default. Patches that modify package manifests, agent instruction files, or repository security configuration are refused unless you explicitly configure a policy. diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 8a6e2e4505a..4678502ca8e 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -7106,6 +7106,11 @@ "description": "When true, treat deleted/missing pull request branch errors as a skipped push instead of a hard failure. Useful when the PR branch may be deleted before safe outputs run.", "default": false }, + "check-branch-protection": { + "type": "boolean", + "description": "When true (default), perform a pre-flight branch protection API check before pushing. Set to false to skip the API check (for example, when the token does not have administration: read); GitHub still enforces protection at push time.", + "default": true + }, "commit-title-suffix": { "type": "string", "description": "Optional suffix to append to generated commit titles (e.g., ' [skip ci]' to prevent triggering CI on the commit)" diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index 323799e839c..87709234d19 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -429,6 +429,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringSlice("labels", c.Labels). AddIfNotEmpty("if_no_changes", c.IfNoChanges). AddIfTrue("ignore_missing_branch_failure", c.IgnoreMissingBranchFailure). + AddBoolPtr("check_branch_protection", c.CheckBranchProtection). AddIfNotEmpty("commit_title_suffix", c.CommitTitleSuffix). AddDefault("max_patch_size", maxPatchSize). AddIfNotEmpty("target-repo", c.TargetRepoSlug). diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index 4530a332a20..bfdab4df2de 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -17,6 +17,7 @@ type PushToPullRequestBranchConfig struct { Labels []string `yaml:"labels,omitempty"` // Required labels for pull request validation IfNoChanges string `yaml:"if-no-changes,omitempty"` // Behavior when no changes to push: "warn", "error", or "ignore" (default: "warn") IgnoreMissingBranchFailure bool `yaml:"ignore-missing-branch-failure,omitempty"` // When true, missing/deleted target branches are treated as skipped instead of hard failures. + CheckBranchProtection *bool `yaml:"check-branch-protection,omitempty"` // When false, skip branch protection pre-flight API check (default: true). CommitTitleSuffix string `yaml:"commit-title-suffix,omitempty"` // Optional suffix to append to generated commit titles GithubTokenForExtraEmptyCommit string `yaml:"github-token-for-extra-empty-commit,omitempty"` // Token used to push an empty commit to trigger CI events. Use a PAT or "app" for GitHub App auth. TargetRepoSlug string `yaml:"target-repo,omitempty"` // Target repository in format "owner/repo" for cross-repository push to pull request branch @@ -122,6 +123,13 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) } } + // Parse check-branch-protection (optional, defaults to true in handler) + if checkBranchProtection, exists := configMap["check-branch-protection"]; exists { + if checkBranchProtectionBool, ok := checkBranchProtection.(bool); ok { + pushToBranchConfig.CheckBranchProtection = &checkBranchProtectionBool + } + } + // Parse title-prefix using shared helper pushToBranchConfig.TitlePrefix = parseTitlePrefixFromConfig(configMap) diff --git a/pkg/workflow/push_to_pull_request_branch_test.go b/pkg/workflow/push_to_pull_request_branch_test.go index 998c006ac11..d745e60c791 100644 --- a/pkg/workflow/push_to_pull_request_branch_test.go +++ b/pkg/workflow/push_to_pull_request_branch_test.go @@ -192,6 +192,51 @@ safe-outputs: } } +func TestPushToPullRequestBranchCheckBranchProtectionDisabled(t *testing.T) { + tmpDir := testutil.TempDir(t, "test-*") + + testMarkdown := `--- +on: + pull_request: + types: [opened, synchronize] +safe-outputs: + push-to-pull-request-branch: + check-branch-protection: false +--- + +# Test Push to PR Branch Branch-Protection Check Disabled +` + + mdFile := filepath.Join(tmpDir, "test-push-to-pull-request-branch-check-protection-disabled.md") + if err := os.WriteFile(mdFile, []byte(testMarkdown), 0644); err != nil { + t.Fatalf("Failed to write test markdown file: %v", err) + } + + compiler := NewCompiler() + if err := compiler.CompileWorkflow(mdFile); err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + lockFile := stringutil.MarkdownToLockFile(mdFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + pushConfig := extractPushToPullRequestBranchHandlerConfig(t, lockContent) + checkBranchProtection, exists := pushConfig["check_branch_protection"] + if !exists { + t.Fatalf("Expected check_branch_protection in handler config when check-branch-protection is explicitly set") + } + checkBranchProtectionBool, isBool := checkBranchProtection.(bool) + if !isBool { + t.Fatalf("Expected check_branch_protection to be a bool, got %#v", checkBranchProtection) + } + if checkBranchProtectionBool { + t.Fatalf("Expected check_branch_protection=false, got true") + } +} + func TestPushToPullRequestBranchFallbackAsPullRequestDisabled(t *testing.T) { tmpDir := testutil.TempDir(t, "test-*")