Skip to content

safe-outputs: validate extracted base branch with git check-ref-format refs/heads/<name> #40001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 18, 2026
+48 −6
Merged

safe-outputs: validate extracted base branch with git check-ref-format refs/heads/<name> #40001

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c61f80
Initial plan
Copilot Jun 18, 2026
3194e91
safe-outputs: validate extracted base branch with git check-ref-format
Copilot Jun 18, 2026
2a60231
safe-outputs: fix @{-N} bypass, add timeout, clarify fail-closed comm…
Copilot Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions actions/setup/js/extract_base_branch_from_agent_output.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
/// <reference types="@actions/github-script" />

const fs = require("fs");
const { spawnSync } = require("child_process");

const AGENT_OUTPUT_PATH = "/tmp/gh-aw/agent_output.json";
const SAFE_BRANCH_NAME_REGEX = /^[a-zA-Z0-9/_.-]+$/;
const MAX_BRANCH_NAME_LENGTH = 255;

/**
* @param {string} itemRepo
Expand Down Expand Up @@ -49,9 +50,22 @@ function extractBaseBranchFromAgentOutput(opts = {}) {
async function main() {
const baseBranch = extractBaseBranchFromAgentOutput();
if (!baseBranch) return;
if (!SAFE_BRANCH_NAME_REGEX.test(baseBranch) || baseBranch.length > 255) return;
if (!isValidBaseBranchName(baseBranch)) return;
core.setOutput("base-branch", baseBranch);
core.info(`Extracted base branch from safe output: ${baseBranch}`);
}

module.exports = { extractBaseBranchFromAgentOutput, isSameWorkflowRepo, main };
/**
* @param {string} branchName
* @returns {boolean}
*/
function isValidBaseBranchName(branchName) {
if (!branchName || branchName.length > MAX_BRANCH_NAME_LENGTH) {
return false;
}

const result = spawnSync("git", ["check-ref-format", "--branch", branchName], { stdio: "ignore" });

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] spawnSync has no timeout option — if git stalls (degraded runner, network-mounted git install), validation will block the step indefinitely.

💡 Suggested fix

Add a timeout (e.g. 5 seconds) so the function stays bounded:

const result = spawnSync("git", ["check-ref-format", "--branch", branchName], {
  stdio: "ignore",
  timeout: 5000,
});

With timeout set, a hung subprocess is killed and result.error will be set (ETIMEDOUT), so the existing !result.error guard safely returns false.

Copilot AI Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added timeout: 5000 to the spawnSync call in the latest commit.

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--branch causes @{-N} git expressions to pass validation, silently allowing values like @{-1} through to core.setOutput where they would be interpreted as relative branch references, not literal names.

💡 Suggested fix

git check-ref-format --branch is designed for branch expressions, not just literal branch names. It explicitly accepts @{-N} notation (e.g. @{-1} = previously-checked-out branch). Confirmed on this runner:

$ git check-ref-format --branch "@{-1}"     → exit 0  ← BUG: passes
$ git check-ref-format "refs/heads/@{-1}"  → exit 1  ← correct: rejects

The old regex ^[a-zA-Z0-9/_.-]+$ accidentally blocked this because @, {, and } were not in the allowed character set. This PR reopens that hole.

Use the full refs/heads/<branchName> form instead — it validates the name as a literal ref without accepting expressions:

const result = spawnSync("git", ["check-ref-format", `refs/heads/${branchName}`], { stdio: "ignore" });

This still accepts release/v1.2+hotfix (→ refs/heads/release/v1.2+hotfix exits 0) and still rejects foo..bar, .foo, main.lock, etc.

Copilot AI Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit. Switched to refs/heads/${branchName} — this validates the name as a literal ref and correctly rejects @{-1}, @{-2}, and similar expressions. The --branch form has been removed.

return !result.error && result.status === 0;

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] When git is not on PATH, result.error is set (ENOENT) and the function returns false, silently dropping the base branch. This is the safe fail-closed behavior, but it is worth a brief comment so the next reader does not treat it as a bug.

💡 Suggested comment 
// Fail-closed: if git is unavailable or the ref is invalid, reject the branch.
// A missing git binary sets result.error (ENOENT); a timeout sets it (ETIMEDOUT).
return !result.error && result.status === 0;

The ADR already documents the conservative-rejection intent, but a one-liner here saves a future reader from needing to cross-reference it.

Copilot AI Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added an inline comment in the latest commit explaining ENOENT (missing git) and ETIMEDOUT (timeout) both set result.error, making the guard fail-closed in both cases.

}

module.exports = { extractBaseBranchFromAgentOutput, isSameWorkflowRepo, isValidBaseBranchName, main };
14 changes: 13 additions & 1 deletion actions/setup/js/extract_base_branch_from_agent_output.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { describe, it, expect } from "vitest";
import fs from "fs";
import path from "path";
import { extractBaseBranchFromAgentOutput, isSameWorkflowRepo } from "./extract_base_branch_from_agent_output.cjs";
import { extractBaseBranchFromAgentOutput, isSameWorkflowRepo, isValidBaseBranchName } from "./extract_base_branch_from_agent_output.cjs";

describe("extract_base_branch_from_agent_output", () => {
it("matches fully-qualified repos", () => {
Expand Down Expand Up @@ -70,4 +70,16 @@ describe("extract_base_branch_from_agent_output", () => {
fs.rmSync(tmpDir, { recursive: true, force: true });
}
});

it("accepts valid git branch names used in safe outputs", () => {
expect(isValidBaseBranchName("feature/x")).toBe(true);
expect(isValidBaseBranchName("release/v1.2+hotfix")).toBe(true);
});

it("rejects invalid git branch names even if they look regex-safe", () => {

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing @{-N} test cases: the current test suite never exercises @{-1} or similar git expressions, so the regression in the validator is not caught.

💡 Suggested fix

Add to the "rejects invalid" block:

expect(isValidBaseBranchName("@{-1}")).toBe(false);
expect(isValidBaseBranchName("@{-2}")).toBe(false);

With the current implementation these assertions will fail (confirming the bug — @{-1} passes the --branch validator). After switching to refs/heads/<branchName>, they will correctly pass as expected rejections and prevent future regressions.

Copilot AI Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in the latest commit — @{-1} and @{-2} are now asserted to return false, confirming the refs/heads/ fix closes the gap.

expect(isValidBaseBranchName("foo..bar")).toBe(false);
expect(isValidBaseBranchName("main.lock")).toBe(false);
expect(isValidBaseBranchName(".foo")).toBe(false);
expect(isValidBaseBranchName("foo/.bar")).toBe(false);
});

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The MAX_BRANCH_NAME_LENGTH = 255 invariant is enforced in code but never exercised by a test — the exact boundary (255 valid, 256 rejected) is not confirmed.

💡 Suggested test 
it('enforces the 255-character length limit', () => {
  const atLimit   = 'a'.repeat(255);
  const overLimit = 'a'.repeat(256);
  expect(isValidBaseBranchName(atLimit)).toBe(true);
  expect(isValidBaseBranchName(overLimit)).toBe(false);
});

Without a boundary test, a future refactor that changes or removes the guard goes undetected by the suite.

Copilot AI Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a boundary test in the latest commit: 255-char name accepted, 256-char name rejected.

});
2 changes: 1 addition & 1 deletion docs/adr/30071-decouple-safe-outputs-base-branch-from-event-context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ Insert a `gh api` call into the workflow itself (before checkout) to fetch the P
### Workflow Extraction Step

1. Every compiled workflow job that performs a safe-outputs checkout **MUST** include an `extract-base-branch` step that runs after the agent artifact download and before the checkout step.
2. The extraction step **MUST** validate the extracted branch name against the pattern `^[a-zA-Z0-9/_.-]+$` and enforce a maximum length of 255 characters before writing to `GITHUB_OUTPUT`.
2. The extraction step **MUST** validate the extracted branch name using `git check-ref-format --branch` semantics and enforce a maximum length of 255 characters before writing to `GITHUB_OUTPUT`.
3. The extraction step **MUST NOT** fail the workflow if `agent_output.json` is absent or if no matching safe-output entry contains `base_branch`; it **MUST** exit successfully (silently) in those cases.
4. Checkout steps **MUST** lead the `ref` expression with `steps.extract-base-branch.outputs.base-branch` and **SHOULD** retain event-context fallbacks (`github.base_ref`, `github.event.pull_request.base.ref`, etc.) for backward compatibility.

Expand Down
Loading