diff --git a/actions/setup/js/pr_helpers.cjs b/actions/setup/js/pr_helpers.cjs index 53a772352f0..414c0bf92cf 100644 --- a/actions/setup/js/pr_helpers.cjs +++ b/actions/setup/js/pr_helpers.cjs @@ -1,5 +1,7 @@ // @ts-check +const { getErrorMessage } = require("./error_helpers.cjs"); + /** * Detect if a pull request is from a fork repository. * @@ -110,4 +112,70 @@ function buildBranchInstruction(effectiveBaseBranch, resolvedDefaultBranch) { return `IMPORTANT: Create your branch from the '${effectiveBaseBranch}' branch${notClause}.`; } -module.exports = { detectForkPR, getPullRequestNumber, resolvePullRequestRepo, buildBranchInstruction }; +/** + * Check whether a branch is safe to push to. + * + * Performs two security checks: + * 1. Ensures the branch is not the repository's default branch. + * 2. When `checkBranchProtection` is true, queries the branch protection API to + * verify the branch has no protection rules. + * + * Returns null when the push is safe to proceed, or a string error message that + * should be surfaced as a hard failure when the push must be blocked. + * + * @param {any} githubClient - Octokit REST client + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {string} branchName - Target branch to validate + * @param {boolean} checkBranchProtection - Whether to call the branch protection API + * @returns {Promise} Error message if push is blocked, null if safe + */ +async function checkBranchPushable(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) { + return `Cannot push to branch "${branchName}": this is the repository's default branch. Agents must not push directly to the default branch.`; + } + + // Check whether the branch has protection rules + if (checkBranchProtection) { + 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. + return `Cannot verify branch protection rules for "${branchName}": ${getErrorMessage(protectionError)}. Push blocked to prevent accidental writes to protected branches.`; + } + } + + if (isBranchProtected) { + return `Cannot push to branch "${branchName}": this branch has protection rules. Agents must not push directly to protected branches.`; + } + } else { + core.info(`Branch protection check skipped (check-branch-protection: false)`); + } + + return null; +} + +module.exports = { detectForkPR, getPullRequestNumber, resolvePullRequestRepo, buildBranchInstruction, checkBranchPushable }; diff --git a/actions/setup/js/pr_helpers.test.cjs b/actions/setup/js/pr_helpers.test.cjs index 352958c4a72..74c080184dc 100644 --- a/actions/setup/js/pr_helpers.test.cjs +++ b/actions/setup/js/pr_helpers.test.cjs @@ -351,3 +351,60 @@ describe("buildBranchInstruction", () => { expect(instruction).not.toContain("NOT from"); }); }); + +describe("checkBranchPushable", () => { + const { checkBranchPushable } = require("./pr_helpers.cjs"); + + const mockCore = { info: vi.fn(), warning: vi.fn(), error: vi.fn() }; + beforeEach(() => { + global.core = mockCore; + vi.clearAllMocks(); + }); + + const makeClient = (defaultBranch, protectionStatus) => ({ + rest: { + repos: { + get: vi.fn().mockResolvedValue({ data: { default_branch: defaultBranch } }), + getBranchProtection: protectionStatus === null ? vi.fn().mockResolvedValue({}) : vi.fn().mockRejectedValue(Object.assign(new Error("error"), { status: protectionStatus })), + }, + }, + }); + + it("returns null when branch is not default and has no protection rules (404)", async () => { + const client = makeClient("main", 404); + const result = await checkBranchPushable(client, "owner", "repo", "feature", true); + expect(result).toBeNull(); + }); + + it("blocks push when branch is the default branch", async () => { + const client = makeClient("main", 404); + const result = await checkBranchPushable(client, "owner", "repo", "main", true); + expect(result).toContain("default branch"); + }); + + it("blocks push when branch has protection rules", async () => { + const client = makeClient("main", null); // null status = successful getBranchProtection response + const result = await checkBranchPushable(client, "owner", "repo", "feature", true); + expect(result).toContain("protection rules"); + }); + + it("returns null and skips protection check when checkBranchProtection is false", async () => { + const client = makeClient("main", null); + const result = await checkBranchPushable(client, "owner", "repo", "feature", false); + expect(result).toBeNull(); + expect(client.rest.repos.getBranchProtection).not.toHaveBeenCalled(); + }); + + it("returns error on unexpected protection check failure (5xx)", async () => { + const client = makeClient("main", 500); + const result = await checkBranchPushable(client, "owner", "repo", "feature", true); + expect(result).toContain("Cannot verify branch protection rules"); + }); + + it("returns null and warns on 403 (insufficient permissions)", async () => { + const client = makeClient("main", 403); + const result = await checkBranchPushable(client, "owner", "repo", "feature", true); + expect(result).toBeNull(); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("insufficient permissions")); + }); +}); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 643cb90b95a..41f19229181 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -10,7 +10,7 @@ const { updateActivationCommentWithCommit, updateActivationComment } = require(" const { getErrorMessage } = require("./error_helpers.cjs"); const { normalizeBranchName } = require("./normalize_branch_name.cjs"); const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); -const { detectForkPR } = require("./pr_helpers.cjs"); +const { detectForkPR, checkBranchPushable } = require("./pr_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { checkFileProtection } = require("./manifest_file_helpers.cjs"); @@ -57,6 +57,7 @@ async function main(config = {}) { const ifNoChanges = config.if_no_changes || "warn"; const ignoreMissingBranchFailure = config.ignore_missing_branch_failure === true; const fallbackAsPullRequest = config.fallback_as_pull_request !== false; + const checkBranchProtection = config.check_branch_protection !== false; const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; const maxCount = config.max || 0; // 0 means no limit @@ -93,6 +94,7 @@ async function main(config = {}) { core.info(`If no changes: ${ifNoChanges}`); core.info(`Ignore missing branch failure: ${ignoreMissingBranchFailure}`); core.info(`Fallback as pull request: ${fallbackAsPullRequest}`); + core.info(`Check branch protection: ${checkBranchProtection}`); if (commitTitleSuffix) { core.info(`Commit title suffix: ${commitTitleSuffix}`); } @@ -407,57 +409,10 @@ async function main(config = {}) { // 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 }; + const blockReason = await checkBranchPushable(githubClient, repoParts.owner, repoParts.repo, branchName, checkBranchProtection); + if (blockReason) { + core.error(blockReason); + return { success: false, error: blockReason }; } } diff --git a/docs/adr/28365-optional-branch-protection-check-for-push-to-pull-request-branch.md b/docs/adr/28365-optional-branch-protection-check-for-push-to-pull-request-branch.md new file mode 100644 index 00000000000..392b3b55c1a --- /dev/null +++ b/docs/adr/28365-optional-branch-protection-check-for-push-to-pull-request-branch.md @@ -0,0 +1,89 @@ +# ADR-28365: Optional Branch Protection Pre-flight Check for `push-to-pull-request-branch` + +**Date**: 2026-04-25 +**Status**: Draft +**Deciders**: Unknown + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `push-to-pull-request-branch` safe output handler performs a pre-flight check against the GitHub branch protection API (`GET /repos/{owner}/{repo}/branches/{branch}/protection`) before pushing. This call requires `administration: read` permission, which is a GitHub App-only scope not available on standard `GITHUB_TOKEN`. Without this scope, the API returns HTTP 403, which the handler silently swallowed—logging a warning and continuing—making the check effectively useless noise. At the same time, automatically granting `administration: read` to all workflows that use this handler would increase every deployment's permission surface area, even for teams that don't need or want the protection check. + +### Decision + +We will make the branch protection pre-flight check opt-out via a new `check-branch-protection` boolean frontmatter key (default `true`). When the check is enabled (the default), the permission compiler automatically adds `administration: read` to the GitHub App token so the API call succeeds. When `check-branch-protection: false` is set, neither the API call nor the `administration: read` permission is added. The default branch guard (blocking pushes to the repository's default branch) runs unconditionally regardless of this setting, because it uses the repos API rather than the branch protection API. + +### Alternatives Considered + +#### Alternative 1: Always require `administration: read` with no opt-out + +All workflows using `push-to-pull-request-branch` would automatically receive `administration: read`. This eliminates the silent-failure problem and simplifies the implementation. It was rejected because it forces every team to grant a broad GitHub App scope even when they have no interest in the branch protection check and prefer minimal permission footprints. + +#### Alternative 2: Remove the branch protection pre-flight check entirely + +Eliminating the check avoids the permission problem without any new configuration surface. It was rejected because the check provides genuine defense-in-depth: it prevents agentic pushes to branches under review policies, catching misconfigurations before GitHub's push-time enforcement triggers a harder failure. The value of the check outweighs the complexity of making it configurable. + +#### Alternative 3: Silently ignore 403 and keep the existing behavior + +The current code already falls through on 403 with a warning, meaning the check is already a no-op for most GitHub App tokens. This was rejected because it perpetuates misleading log output and provides no security value while still requiring `administration: read` to appear in the schema. An explicit opt-out is more honest about what the check does and does not guarantee. + +### Consequences + +#### Positive +- The 403-swallow silent failure is eliminated: when the check is enabled, the token has the right scope and the check is meaningful. +- Users with strict permission policies can set `check-branch-protection: false` to avoid requesting `administration: read` entirely. +- The default branch guard (using the repos API) continues to run unconditionally, preserving the most critical safety check. +- Extracting the combined guard logic into a `checkBranchPushable` helper improves testability and isolation. + +#### Negative +- Enabling the default increases the permission footprint of all existing `push-to-pull-request-branch` deployments that use GitHub App tokens (they gain `administration: read` automatically on upgrade). +- The configuration surface for the handler grows by one field, adding a new concept callers must understand. +- The `administration: read` permission is a GitHub App-only scope with no effect on `GITHUB_TOKEN`; this asymmetry may confuse users who rely on `GITHUB_TOKEN`. + +#### Neutral +- The JSON schema for `push-to-pull-request-branch` gains a `check-branch-protection` boolean property, which may require documentation updates in tooling that consumes the schema. +- The permission computation in `ComputePermissionsForSafeOutputs` now branches on the effective `CheckBranchProtection` value, so permission calculations are no longer purely additive for this handler. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Branch Protection Check Configuration + +1. The `push-to-pull-request-branch` handler **MUST** treat `check-branch-protection` as an optional boolean key in workflow frontmatter, defaulting to `true` when absent. +2. When `check-branch-protection` is `true` (the default), the handler **MUST** call `GET /repos/{owner}/{repo}/branches/{branch}/protection` before executing the push. +3. When `check-branch-protection` is `false`, the handler **MUST NOT** call the branch protection API and **MUST NOT** request `administration: read` permission. +4. The handler **MUST** block the push and return an error when the branch protection API responds with a successful (2xx) status, indicating active protection rules exist. +5. The handler **MUST** permit the push when the branch protection API responds with HTTP 404 (no rules configured). +6. The handler **SHOULD** log a warning and permit the push when the branch protection API responds with HTTP 403 (insufficient permissions), because the GitHub platform still enforces protection at push time. +7. The handler **MUST** block the push when the branch protection API responds with any unexpected error (e.g., 5xx, network failure) to fail closed and prevent accidental writes to potentially protected branches. + +### Default Branch Guard + +1. The handler **MUST** check whether the target branch is the repository's default branch, regardless of the `check-branch-protection` setting. +2. The handler **MUST** block the push and return an error when the target branch equals the repository's default branch. +3. The handler **SHOULD** log a warning and continue when the repositories API call needed to resolve the default branch fails, rather than blocking the push due to an unrelated API error. + +### Permission Computation + +1. The permission compiler **MUST** automatically add `administration: read` to the computed GitHub App token permissions when `check-branch-protection` is `true` (or absent/defaulted). +2. The permission compiler **MUST NOT** add `administration: read` when `check-branch-protection` is explicitly `false`. +3. Implementations **SHOULD** document that `administration: read` is a GitHub App-only scope with no effect on standard `GITHUB_TOKEN`. + +### Schema + +1. The JSON schema for `push-to-pull-request-branch` **MUST** declare `check-branch-protection` as an optional boolean property with `default: true`. +2. The schema **MUST NOT** require `check-branch-protection` to be present. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24917291168) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index e3405f2e3f2..a2696ec89cd 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -4843,6 +4843,13 @@ safe-outputs: # (optional) allow-workflows: true + # When false, skips the branch protection API pre-flight check before pushing. Set + # to false to avoid requiring administration: read permission. The GitHub platform + # will still enforce branch protection at push time. Default is true (check + # enabled). + # (optional) + check-branch-protection: true + # Enable AI agents to minimize (hide) comments on issues or pull requests based on # relevance, spam detection, or moderation rules. # (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 9006f76e546..4348afccde5 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 # set to false to skip the branch protection pre-flight 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 branch protection API pre-flight check is skipped. By default (`true`), the handler calls `GET /repos/{owner}/{repo}/branches/{branch}/protection` before pushing to detect whether the target branch is protected. This API call requires `administration: read`. If the token lacks that permission, the check logs a warning and continues (the GitHub platform still enforces protection at push time). Set `check-branch-protection: false` to suppress the warning and avoid the API call entirely. + ## 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 f67edfabfd4..d62a38fba97 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -7213,6 +7213,11 @@ "description": "When true, adds workflows: write to the GitHub App token permissions. Required when allowed-files targets .github/workflows/ paths. Requires safe-outputs.github-app to be configured because the workflows permission is a GitHub App-only permission and cannot be granted via GITHUB_TOKEN.", "default": false, "examples": [true, false] + }, + "check-branch-protection": { + "type": "boolean", + "description": "When false, skips the branch protection API pre-flight check before pushing. Set to false to avoid requiring administration: read permission. The GitHub platform will still enforce branch protection at push time. Default is true (check enabled).", + "default": true } }, "additionalProperties": false diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index d8b45725950..0991a98efa4 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -444,6 +444,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringSlice("excluded_files", c.ExcludedFiles). AddIfNotEmpty("patch_format", c.PatchFormat). AddBoolPtr("fallback_as_pull_request", c.FallbackAsPullRequest). + AddBoolPtr("check_branch_protection", c.CheckBranchProtection). Build() }, "update_pull_request": func(cfg *SafeOutputsConfig) map[string]any { diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index 4530a332a20..5003973f1ea 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -28,6 +28,7 @@ type PushToPullRequestBranchConfig struct { PatchFormat string `yaml:"patch-format,omitempty"` // Transport format for packaging changes: "am" (default, uses git format-patch) or "bundle" (uses git bundle, preserves merge topology and per-commit metadata). FallbackAsPullRequest *bool `yaml:"fallback-as-pull-request,omitempty"` // When true (default), creates a fallback pull request if direct push fails due to diverged/non-fast-forward branch. When false, fallback is disabled and pull-requests: write is not requested. AllowWorkflows bool `yaml:"allow-workflows,omitempty"` // When true, adds workflows: write to the GitHub App token. Requires safe-outputs.github-app to be configured. + CheckBranchProtection *bool `yaml:"check-branch-protection,omitempty"` // When false, skips the branch protection API pre-flight check. Default is true (check enabled). Set to false to avoid needing administration: read permission. } // buildCheckoutRepository generates a checkout step with optional target repository and custom token @@ -188,6 +189,13 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) } } + // Parse check-branch-protection: when false, skips the branch protection API pre-flight check + if checkBranchProtection, exists := configMap["check-branch-protection"]; exists { + if checkBranchProtectionBool, ok := checkBranchProtection.(bool); ok { + pushToBranchConfig.CheckBranchProtection = &checkBranchProtectionBool + } + } + // Parse common base fields with default max of 0 (no limit) c.parseBaseSafeOutputConfig(configMap, &pushToBranchConfig.BaseSafeOutputConfig, 0) } diff --git a/pkg/workflow/push_to_pull_request_branch_test.go b/pkg/workflow/push_to_pull_request_branch_test.go index 998c006ac11..3b0a8e6c2d3 100644 --- a/pkg/workflow/push_to_pull_request_branch_test.go +++ b/pkg/workflow/push_to_pull_request_branch_test.go @@ -1094,3 +1094,90 @@ This test verifies that the aw-*.patch artifact is downloaded in the safe_output t.Errorf("Expected condition to check for 'push_to_pull_request_branch' in output_types") } } + +func TestPushToPullRequestBranchCheckBranchProtectionFalse(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 Branch with check-branch-protection: false + +This workflow skips the branch protection pre-flight check. +` + + mdFile := filepath.Join(tmpDir, "test-push-to-pull-request-branch-no-protection-check.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) + } + + // Verify check_branch_protection: false is passed to the handler config + pushCfg := extractPushToPullRequestBranchHandlerConfig(t, lockContent) + + checkBranchProtection, exists := pushCfg["check_branch_protection"] + if !exists { + t.Fatalf("Handler config should contain check_branch_protection key when set to false") + } + if checkBranchProtection != false { + t.Errorf("check_branch_protection should be false, got %v", checkBranchProtection) + } +} + +func TestPushToPullRequestBranchCheckBranchProtectionDefaultOmitted(t *testing.T) { + tmpDir := testutil.TempDir(t, "test-*") + + testMarkdown := `--- +on: + pull_request: + types: [opened, synchronize] +safe-outputs: + push-to-pull-request-branch: +--- + +# Test Push to Branch default check-branch-protection + +By default, check-branch-protection is not set (JS handler defaults to true). +` + + mdFile := filepath.Join(tmpDir, "test-push-to-pull-request-branch-default-protection.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) + } + + // When not specified, check_branch_protection should not be in the handler config + // (the JS handler defaults to true via !== false) + pushCfg := extractPushToPullRequestBranchHandlerConfig(t, lockContent) + if _, exists := pushCfg["check_branch_protection"]; exists { + t.Errorf("Handler config should NOT contain check_branch_protection when not explicitly set") + } +} diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index 89ea00a0447..6e75492f492 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -59,6 +59,14 @@ func getPushFallbackAsPullRequest(config *PushToPullRequestBranchConfig) bool { return *config.FallbackAsPullRequest } +// getCheckBranchProtection returns the effective check-branch-protection setting (defaults to true). +func getCheckBranchProtection(config *PushToPullRequestBranchConfig) bool { + if config == nil || config.CheckBranchProtection == nil { + return true // Default: check is enabled + } + return *config.CheckBranchProtection +} + // ComputePermissionsForSafeOutputs computes the minimal required permissions // based on the configured safe-outputs. This function is used by both the // consolidated safe outputs job and the conclusion job to ensure they only @@ -161,6 +169,13 @@ func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio safeOutputsPermissionsLog.Print("Adding workflows: write for push-to-pull-request-branch (allow-workflows: true)") permissions.Set(PermissionWorkflows, PermissionWrite) } + // Add administration: read when check-branch-protection is enabled (GitHub App-only permission) + // The branch protection API requires administration: read for GitHub App tokens. + // Standard GITHUB_TOKEN lacks this scope; set it so the minted app token includes it. + if getCheckBranchProtection(safeOutputs.PushToPullRequestBranch) { + safeOutputsPermissionsLog.Print("Adding administration: read for push-to-pull-request-branch (check-branch-protection enabled)") + permissions.Set(PermissionAdministration, PermissionRead) + } } if safeOutputs.UpdatePullRequests != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdatePullRequests.Staged) { safeOutputsPermissionsLog.Print("Adding permissions for update-pull-request") diff --git a/pkg/workflow/safe_outputs_permissions_test.go b/pkg/workflow/safe_outputs_permissions_test.go index 0ee03e7c2f0..c07fa97274e 100644 --- a/pkg/workflow/safe_outputs_permissions_test.go +++ b/pkg/workflow/safe_outputs_permissions_test.go @@ -274,27 +274,56 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "push-to-pull-request-branch default fallback - requires pull-requests write", + name: "push-to-pull-request-branch default fallback - requires pull-requests write and administration read", safeOutputs: &SafeOutputsConfig{ PushToPullRequestBranch: &PushToPullRequestBranchConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{}, }, }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionWrite, + PermissionPullRequests: PermissionWrite, + PermissionAdministration: PermissionRead, + }, + }, + { + name: "push-to-pull-request-branch with fallback-as-pull-request false - no pull-requests permission but administration read", + safeOutputs: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + FallbackAsPullRequest: boolPtr(false), + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionWrite, + PermissionAdministration: PermissionRead, + }, + }, + { + name: "push-to-pull-request-branch with check-branch-protection false - no administration permission", + safeOutputs: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + CheckBranchProtection: boolPtr(false), + }, + }, expected: map[PermissionScope]PermissionLevel{ PermissionContents: PermissionWrite, PermissionPullRequests: PermissionWrite, }, }, { - name: "push-to-pull-request-branch with fallback-as-pull-request false - no pull-requests permission", + name: "push-to-pull-request-branch with check-branch-protection explicit true - includes administration read", safeOutputs: &SafeOutputsConfig{ PushToPullRequestBranch: &PushToPullRequestBranchConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{}, - FallbackAsPullRequest: boolPtr(false), + CheckBranchProtection: boolPtr(true), }, }, expected: map[PermissionScope]PermissionLevel{ - PermissionContents: PermissionWrite, + PermissionContents: PermissionWrite, + PermissionPullRequests: PermissionWrite, + PermissionAdministration: PermissionRead, }, }, {