Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/minor-preserve-branch-name-in-safe-outputs.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 12 additions & 8 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ async function main(config = {}) {
const ifNoChanges = config.if_no_changes || "warn";
const allowEmpty = parseBoolTemplatable(config.allow_empty, false);
const autoMerge = parseBoolTemplatable(config.auto_merge, false);
const preserveBranchName = config.preserve_branch_name === true;
const expiresHours = config.expires ? parseInt(String(config.expires), 10) : 0;
const maxCount = config.max || 1; // PRs are typically limited to 1
const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024;
Expand Down Expand Up @@ -527,18 +528,26 @@ async function main(config = {}) {

let bodyLines = processedBody.split("\n");
let branchName = pullRequestItem.branch ? pullRequestItem.branch.trim() : null;
const randomHex = crypto.randomBytes(8).toString("hex");

// SECURITY: Sanitize branch name to prevent shell injection (CWE-78)
// Branch names from user input must be normalized before use in git commands
// Branch names from user input must be normalized before use in git commands.
// When preserve-branch-name is disabled (default), a random salt suffix is
// appended to avoid collisions.
if (branchName) {
const originalBranchName = branchName;
branchName = normalizeBranchName(branchName);
branchName = normalizeBranchName(branchName, preserveBranchName ? null : randomHex);

// Validate it's not empty after normalization
if (!branchName) {
throw new Error(`Invalid branch name: sanitization resulted in empty string (original: "${originalBranchName}")`);
}

if (preserveBranchName) {
core.info(`Using branch name from JSONL without salt suffix (preserve-branch-name enabled): ${branchName}`);
} else {
core.info(`Using branch name from JSONL with added salt: ${branchName}`);
}
if (originalBranchName !== branchName) {
core.info(`Branch name sanitized: "${originalBranchName}" -> "${branchName}"`);
}
Expand Down Expand Up @@ -623,15 +632,10 @@ async function main(config = {}) {
core.info(`Draft: ${draft}`);
core.info(`Body length: ${body.length}`);

const randomHex = crypto.randomBytes(8).toString("hex");
// Use branch name from JSONL if provided, otherwise generate unique branch name
// When no branch name was provided by the agent, generate a unique one.
if (!branchName) {
core.info("No branch name provided in JSONL, generating unique branch name");
// Generate unique branch name using cryptographic random hex
branchName = `${workflowId}-${randomHex}`;
} else {
branchName = `${branchName}-${randomHex}`;
core.info(`Using branch name from JSONL with added salt: ${branchName}`);
}

core.info(`Generated branch name: ${branchName}`);
Expand Down
53 changes: 50 additions & 3 deletions actions/setup/js/create_pull_request.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,58 @@ describe("create_pull_request - security: branch name sanitization", () => {
expect(normalizeBranchName("---")).toBe("");
});

it("should convert to lowercase", () => {
it("should preserve original casing (no lowercase conversion)", () => {
const { normalizeBranchName } = require("./normalize_branch_name.cjs");

expect(normalizeBranchName("Feature/MyBranch")).toBe("feature/mybranch");
expect(normalizeBranchName("UPPERCASE")).toBe("uppercase");
expect(normalizeBranchName("Feature/MyBranch")).toBe("Feature/MyBranch");
expect(normalizeBranchName("UPPERCASE")).toBe("UPPERCASE");
// Motivating use-case: Jira keys stay uppercase
expect(normalizeBranchName("bugfix/BR-329-red")).toBe("bugfix/BR-329-red");
});
});

// ──────────────────────────────────────────────────────
// normalizeBranchName: salt argument
// ──────────────────────────────────────────────────────

describe("create_pull_request - normalizeBranchName: salt argument", () => {
it("should append salt suffix when salt argument is provided", () => {
const { normalizeBranchName } = require("./normalize_branch_name.cjs");

expect(normalizeBranchName("feature/my-branch", "abc123")).toBe("feature/my-branch-abc123");
expect(normalizeBranchName("bugfix/BR-329-red", "cde2a954af3b8fa8")).toBe("bugfix/BR-329-red-cde2a954af3b8fa8");
});

it("should preserve original casing and add salt (default behaviour)", () => {
const { normalizeBranchName } = require("./normalize_branch_name.cjs");

// Default: preserve case + salt
expect(normalizeBranchName("bugfix/BR-329-red", "cde2a954")).toBe("bugfix/BR-329-red-cde2a954");

// preserve-branch-name=true: no salt
expect(normalizeBranchName("bugfix/BR-329-red")).toBe("bugfix/BR-329-red");
});

it("should still replace shell metacharacters for security even when preserving case (CWE-78)", () => {
const { normalizeBranchName } = require("./normalize_branch_name.cjs");

const dangerousNames = [
{ input: "Feature; rm -rf /", expected: "Feature-rm-rf-/" },
{ input: "Branch$(malicious)", expected: "Branch-malicious" },
{ input: "BRANCH`backdoor`", expected: "BRANCH-backdoor" },
{ input: "Branch| curl EVIL.COM", expected: "Branch-curl-EVIL.COM" },
{ input: "Branch && echo HACKED", expected: "Branch-echo-HACKED" },
];

for (const { input, expected } of dangerousNames) {
const result = normalizeBranchName(input);
expect(result).toBe(expected);
expect(result).not.toContain(";");
expect(result).not.toContain("$");
expect(result).not.toContain("`");
expect(result).not.toContain("|");
expect(result).not.toContain("&");
}
});
});

Expand Down
13 changes: 8 additions & 5 deletions actions/setup/js/normalize_branch_name.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,21 @@
* IMPORTANT: Keep this function in sync with the normalizeBranchName function in upload_assets.cjs
*
* Valid characters: alphanumeric (a-z, A-Z, 0-9), dash (-), underscore (_), forward slash (/), dot (.)
* Max length: 128 characters
* Max length: 128 characters (before salt is appended)
*
* The normalization process:
* 1. Replaces invalid characters with a single dash
* 2. Collapses multiple consecutive dashes to a single dash
* 3. Removes leading and trailing dashes
* 4. Truncates to 128 characters
* 5. Removes trailing dashes after truncation
* 6. Converts to lowercase
* 6. Appends `-<salt>` suffix when a salt value is provided
*
* @param {string} branchName - The branch name to normalize
* @param {string | null} [salt=null] - When set, appends `-<salt>` to the branch name
* @returns {string} The normalized branch name
*/
function normalizeBranchName(branchName) {
function normalizeBranchName(branchName, salt = null) {
if (!branchName || typeof branchName !== "string" || branchName.trim() === "") {
return branchName;
}
Expand All @@ -43,8 +44,10 @@ function normalizeBranchName(branchName) {
// Ensure it doesn't end with a dash after truncation
normalized = normalized.replace(/-+$/, "");

// Convert to lowercase
normalized = normalized.toLowerCase();
// Append a salt suffix when provided
if (salt) {
normalized = `${normalized}-${salt}`;
}

return normalized;
}
Expand Down
34 changes: 30 additions & 4 deletions actions/setup/js/normalize_branch_name.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,13 @@ describe("normalizeBranchName", () => {
expect(result).toBe("a".repeat(128));
});

it("should convert to lowercase", async () => {
it("should preserve original casing (no lowercase conversion)", async () => {
const { normalizeBranchName } = await import("./normalize_branch_name.cjs");

expect(normalizeBranchName("Feature/Add-Login")).toBe("feature/add-login");
expect(normalizeBranchName("MY-BRANCH")).toBe("my-branch");
expect(normalizeBranchName("Feature/Add-Login")).toBe("Feature/Add-Login");
expect(normalizeBranchName("MY-BRANCH")).toBe("MY-BRANCH");
expect(normalizeBranchName("bugfix/BR-329-red")).toBe("bugfix/BR-329-red");
expect(normalizeBranchName("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature");
});

it("should handle empty and invalid inputs", async () => {
Expand All @@ -66,7 +68,7 @@ describe("normalizeBranchName", () => {
it("should handle complex combinations", async () => {
const { normalizeBranchName } = await import("./normalize_branch_name.cjs");

expect(normalizeBranchName("Feature@Test/Branch#123")).toBe("feature-test/branch-123");
expect(normalizeBranchName("Feature@Test/Branch#123")).toBe("Feature-Test/Branch-123");
expect(normalizeBranchName("__test__branch__")).toBe("__test__branch__");
});

Expand All @@ -79,4 +81,28 @@ describe("normalizeBranchName", () => {
expect(result.length).toBeLessThanOrEqual(128);
expect(result).not.toMatch(/-$/);
});

it("should append salt suffix when salt argument is provided", async () => {
const { normalizeBranchName } = await import("./normalize_branch_name.cjs");

expect(normalizeBranchName("feature/my-branch", "abc123")).toBe("feature/my-branch-abc123");
expect(normalizeBranchName("bugfix/BR-329-red", "cde2a954af3b8fa8")).toBe("bugfix/BR-329-red-cde2a954af3b8fa8");
});

it("should not append salt when salt is null or undefined", async () => {
const { normalizeBranchName } = await import("./normalize_branch_name.cjs");

expect(normalizeBranchName("feature/my-branch", null)).toBe("feature/my-branch");
expect(normalizeBranchName("feature/my-branch", undefined)).toBe("feature/my-branch");
expect(normalizeBranchName("feature/my-branch")).toBe("feature/my-branch");
});

it("should truncate to 128 chars before appending salt", async () => {
const { normalizeBranchName } = await import("./normalize_branch_name.cjs");

const longName = "a".repeat(150);
const result = normalizeBranchName(longName, "abc123");
// Salt is appended after truncation so total length may exceed 128
expect(result).toBe("a".repeat(128) + "-abc123");
});
});
6 changes: 1 addition & 5 deletions actions/setup/js/upload_assets.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const { ERR_API, ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_cod
/**
* Normalizes a branch name to be a valid git branch name.
*
* IMPORTANT: Keep this function in sync with the normalizeBranchName function in safe_outputs_mcp_server.cjs
* IMPORTANT: Keep this function in sync with the normalizeBranchName function in normalize_branch_name.cjs
*
* Valid characters: alphanumeric (a-z, A-Z, 0-9), dash (-), underscore (_), forward slash (/), dot (.)
* Max length: 128 characters
Expand All @@ -22,7 +22,6 @@ const { ERR_API, ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_cod
* 3. Removes leading and trailing dashes
* 4. Truncates to 128 characters
* 5. Removes trailing dashes after truncation
* 6. Converts to lowercase
*
* @param {string} branchName - The branch name to normalize
* @returns {string} The normalized branch name
Expand Down Expand Up @@ -50,9 +49,6 @@ function normalizeBranchName(branchName) {
// Ensure it doesn't end with a dash after truncation
normalized = normalized.replace(/-+$/, "");

// Convert to lowercase
normalized = normalized.toLowerCase();

return normalized;
}

Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/upload_assets.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const mockCore = { debug: vi.fn(), info: vi.fn(), notice: vi.fn(), warning: vi.f
it("should normalize branch names correctly", async () => {
((process.env.GH_AW_ASSETS_BRANCH = "assets/My Branch!@#$%"), (process.env.GH_AW_SAFE_OUTPUTS_STAGED = "false"), setAgentOutput({ items: [] }), await executeScript());
const branchNameCall = mockCore.setOutput.mock.calls.find(call => "branch_name" === call[0]);
(expect(branchNameCall).toBeDefined(), expect(branchNameCall[1]).toBe("assets/my-branch"));
(expect(branchNameCall).toBeDefined(), expect(branchNameCall[1]).toBe("assets/My-Branch"));
});
}),
describe("branch prefix validation", () => {
Expand Down
7 changes: 6 additions & 1 deletion pkg/parser/schemas/main_workflow_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -5341,7 +5341,12 @@
"items": {
"type": "string"
},
"description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern \u2014 files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)."
"description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern — files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)."
},
"preserve-branch-name": {
"type": "boolean",
"description": "When true, the random salt suffix is not appended to the agent-specified branch name. Invalid characters are still replaced for security, and casing is always preserved regardless of this setting. Useful when the target repository enforces branch naming conventions (e.g. Jira keys in uppercase such as 'bugfix/BR-329-red'). Defaults to false.",
"default": false
}
},
"additionalProperties": false,
Expand Down
Loading
Loading