security: block agents from pushing to default/protected branches in safeoutputs#22776
security: block agents from pushing to default/protected branches in safeoutputs#22776
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3eafba71-8866-4a6f-aeae-4e8436199b9a
…safeoutputs Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3eafba71-8866-4a6f-aeae-4e8436199b9a
* Initial plan * Initial plan Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/339fab7a-9e6e-4ab8-bc11-159fb851e2e4 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
* Initial plan * fix: add type narrowing for protectionError.status in push_to_pull_request_branch.cjs Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cec972d3-973f-430c-a2a6-4e28c234890b --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
* Initial plan * fix: properly YAML-quote JSON env var values in FormatStepWithCommandAndEnv Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3385ade1-575c-470d-af03-d2f886c1e301 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds safeguards to prevent automated agents from pushing to unsafe branches, and updates Gemini workflow generation to safely embed JSON configs in YAML.
Changes:
- Block
push_to_pull_request_branchfrom pushing to default branches and branches with protection rules (plus tests). - Quote JSON-like env var values when rendering GitHub Actions YAML steps (plus Go tests).
- Update the generated smoke Gemini workflow to pass Gemini base config via an env var.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/push_to_pull_request_branch.cjs |
Adds default-branch + branch-protection checks before performing git operations. |
actions/setup/js/push_to_pull_request_branch.test.cjs |
Adds mocks and new tests for default/protected branch blocking behavior. |
pkg/workflow/engine_helpers.go |
Ensures env values that look like JSON are YAML-safe by single-quoting them. |
pkg/workflow/engine_helpers_test.go |
Adds unit tests for YAML-safe quoting and step formatting. |
pkg/workflow/gemini_engine_test.go |
Adds a test asserting Gemini base config env var is quoted for YAML validity. |
.github/workflows/smoke-gemini.lock.yml |
Updates generated workflow to read Gemini base config from an env var (YAML-safe JSON scalar). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If the value starts with a YAML flow indicator ('{' or '[') or other characters | ||
| // that would cause it to be misinterpreted by YAML parsers, it wraps the value | ||
| // in single quotes. Any embedded single quotes are escaped by doubling them (' becomes ”). |
There was a problem hiding this comment.
The doc comment for yamlStringValue is inaccurate and contains a broken quote escape example: it says values starting with "{" or "[" or other characters are quoted, but the implementation only checks '{' and '['. It also says ("' becomes ”"), but YAML single-quote escaping is done by doubling the quote ("'" -> "''"). Please update the comment to match the actual behavior and fix the escape example character.
| // If the value starts with a YAML flow indicator ('{' or '[') or other characters | |
| // that would cause it to be misinterpreted by YAML parsers, it wraps the value | |
| // in single quotes. Any embedded single quotes are escaped by doubling them (' becomes ”). | |
| // If the value starts with a YAML flow indicator ('{' or '['), it wraps the value | |
| // in single quotes so it is treated as a plain string rather than a flow collection. | |
| // Any embedded single quotes are escaped by doubling them (' becomes ''). |
| core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
The default-branch protection check is currently fail-open: if repos.get() fails, the code only warns and continues, which can still allow pushes to the repo's default branch when it is not protected (getBranchProtection would return 404). Given the stated security requirement (agents must not push to default branches), this should fail closed or use a non-network fallback source for default_branch (e.g., default_branch from the event payload / PR base repo data) before proceeding.
| core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`); | |
| } | |
| core.warning(`Could not check repository default branch via API: ${getErrorMessage(repoError)}`); | |
| // Fallback: try to derive the default branch from the GitHub event payload | |
| try { | |
| const eventPath = process.env.GITHUB_EVENT_PATH; | |
| if (eventPath && fs.existsSync(eventPath)) { | |
| const eventRaw = fs.readFileSync(eventPath, "utf8"); | |
| const eventPayload = JSON.parse(eventRaw); | |
| if ( | |
| eventPayload && | |
| eventPayload.pull_request && | |
| eventPayload.pull_request.base && | |
| eventPayload.pull_request.base.repo && | |
| eventPayload.pull_request.base.repo.default_branch | |
| ) { | |
| defaultBranch = eventPayload.pull_request.base.repo.default_branch; | |
| core.info(`Resolved default branch from pull_request.base.repo.default_branch: "${defaultBranch}"`); | |
| } else if ( | |
| eventPayload && | |
| eventPayload.repository && | |
| eventPayload.repository.default_branch | |
| ) { | |
| defaultBranch = eventPayload.repository.default_branch; | |
| core.info(`Resolved default branch from repository.default_branch: "${defaultBranch}"`); | |
| } else { | |
| core.warning("Default branch not present in event payload (pull_request.base.repo.default_branch or repository.default_branch)."); | |
| } | |
| } else { | |
| core.warning("GITHUB_EVENT_PATH is not set or does not point to an existing file; cannot derive default branch from event payload."); | |
| } | |
| } catch (eventError) { | |
| core.warning(`Failed to read event payload for default-branch fallback: ${getErrorMessage(eventError)}`); | |
| } | |
| } | |
| // Fail closed if we still cannot determine the repository's default branch | |
| if (!defaultBranch) { | |
| const msg = `Cannot determine repository default branch for "${repoParts.owner}/${repoParts.repo}". Push blocked to prevent accidental writes to the default branch.`; | |
| core.error(msg); | |
| return { success: false, error: msg }; | |
| } |
| t.Run("GH_AW_GEMINI_BASE_CONFIG env var is single-quoted for valid YAML", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| Tools: map[string]any{}, | ||
| } |
There was a problem hiding this comment.
This PR introduces YAML-quoting behavior changes for Gemini settings/env rendering (and updates the smoke-gemini workflow accordingly), but the PR title/description only discuss blocking pushes to default/protected branches. Please update the PR description to cover the Gemini/YAML changes (or split them into a separate PR) so reviewers can validate scope and rationale appropriately.
Summary
Addresses the security review requirement from https://github.com/github/security-reviews/issues/2067#issuecomment-4101658915:
Problem
push_to_pull_request_branchpushed to the PR's head branch without checking whether that branch is the repository's default branch or has GitHub branch protection rules. An agent could accidentally (or maliciously) push to the default branch if a PR was opened with the default branch as the head.Solution
Added a runtime check in
push_to_pull_request_branch.cjsthat runs after fork detection and branch sanitization, before any git operations:Default branch check: Calls
repos.get()to fetch the repository's default branch and blocks the push if the target branch matches. If the API call fails (network, permissions), warns and continues — the default branch check is best-effort since the protection API call below provides an additional layer.Branch protection rules check: Calls
repos.getBranchProtection()and blocks the push if the branch has any protection rules.Error handling strategy
getBranchProtection→ 200getBranchProtection→ 404getBranchProtection→ 403getBranchProtection→ 5xx / otherChanges
actions/setup/js/push_to_pull_request_branch.cjs: Added default branch + branch protection checksactions/setup/js/push_to_pull_request_branch.test.cjs: Added mocks forrepos.get/repos.getBranchProtectionand 6 new tests covering all branch protection scenarios