diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index dfa5f1dce0d..aad00abe1a0 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -15,6 +15,7 @@ const { replaceTemporaryIdReferences, isTemporaryId } = require("./temporary_id. const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { createExpirationLine, generateFooterWithExpiration } = require("./ephemerals.cjs"); const { generateWorkflowIdMarker } = require("./generate_footer.cjs"); +const { normalizeBranchName } = require("./normalize_branch_name.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -96,7 +97,7 @@ async function main(config = {}) { const autoMerge = config.auto_merge || false; const expiresHours = config.expires ? parseInt(String(config.expires), 10) : 0; const maxCount = config.max || 1; // PRs are typically limited to 1 - const baseBranch = config.base_branch || ""; + let baseBranch = config.base_branch || ""; const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const includeFooter = config.footer !== false; // Default to true (include footer) @@ -112,6 +113,18 @@ async function main(config = {}) { throw new Error("base_branch configuration is required"); } + // SECURITY: Sanitize base branch name to prevent shell injection (defense in depth) + // Even though base_branch comes from workflow config, normalize it for safety + const originalBaseBranch = baseBranch; + baseBranch = normalizeBranchName(baseBranch); + if (!baseBranch) { + throw new Error(`Invalid base_branch: sanitization resulted in empty string (original: "${originalBaseBranch}")`); + } + // Fail if base branch name changes during normalization (indicates invalid config) + if (originalBaseBranch !== baseBranch) { + throw new Error(`Invalid base_branch: contains invalid characters (original: "${originalBaseBranch}", normalized: "${baseBranch}")`); + } + // Extract triggering issue number from context (for auto-linking PRs to issues) const triggeringIssueNumber = context.payload?.issue?.number && !context.payload?.issue?.pull_request ? context.payload.issue.number : undefined; @@ -411,6 +424,22 @@ async function main(config = {}) { let bodyLines = processedBody.split("\n"); let branchName = pullRequestItem.branch ? pullRequestItem.branch.trim() : null; + // SECURITY: Sanitize branch name to prevent shell injection (CWE-78) + // Branch names from user input must be normalized before use in git commands + if (branchName) { + const originalBranchName = branchName; + branchName = normalizeBranchName(branchName); + + // Validate it's not empty after normalization + if (!branchName) { + throw new Error(`Invalid branch name: sanitization resulted in empty string (original: "${originalBranchName}")`); + } + + if (originalBranchName !== branchName) { + core.info(`Branch name sanitized: "${originalBranchName}" -> "${branchName}"`); + } + } + // If no title was found, use a default if (!title) { title = "Agent Output"; diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 6f0d680d356..f2bf54491da 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -104,3 +104,133 @@ describe("create_pull_request - max limit enforcement", () => { expect(() => enforcePullRequestLimits(patchContent)).not.toThrow(); }); }); + +describe("create_pull_request - security: branch name sanitization", () => { + it("should sanitize branch names with shell metacharacters", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + // Test shell injection attempts - forward slashes and dots are valid in git branch names + 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" }, + { input: "branch || evil", expected: "branch-evil" }, + { input: "branch > /etc/passwd", expected: "branch-/etc/passwd" }, + { input: "branch < input.txt", expected: "branch-input.txt" }, + { input: "branch\x00null", expected: "branch-null" }, // Actual null byte, not escaped string + { input: "branch\\x00null", expected: "branch-x00null" }, // Escaped string representation + ]; + + for (const { input, expected } of dangerousNames) { + const result = normalizeBranchName(input); + expect(result).toBe(expected); + // Verify dangerous shell metacharacters are removed + expect(result).not.toContain(";"); + expect(result).not.toContain("$"); + expect(result).not.toContain("`"); + expect(result).not.toContain("|"); + expect(result).not.toContain("&"); + expect(result).not.toContain(">"); + expect(result).not.toContain("<"); + expect(result).not.toContain("\x00"); // Actual null byte + expect(result).not.toContain("\\x00"); // Escaped string + } + }); + + it("should sanitize branch names with newlines and control characters", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + const controlCharNames = [ + { input: "branch\nwith\nnewlines", expected: "branch-with-newlines" }, + { input: "branch\rwith\rcarriage", expected: "branch-with-carriage" }, + { input: "branch\twith\ttabs", expected: "branch-with-tabs" }, + { input: "branch\x1b[31mwith\x1b[0mescapes", expected: "branch-31mwith-0mescapes" }, + ]; + + for (const { input, expected } of controlCharNames) { + const result = normalizeBranchName(input); + expect(result).toBe(expected); + expect(result).not.toContain("\n"); + expect(result).not.toContain("\r"); + expect(result).not.toContain("\t"); + expect(result).not.toMatch(/\x1b/); + } + }); + + it("should sanitize branch names with spaces and special characters", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + const specialCharNames = [ + { input: "branch with spaces", expected: "branch-with-spaces" }, + { input: "branch!@#$%^&*()", expected: "branch" }, + { input: "branch[brackets]", expected: "branch-brackets" }, + { input: "branch{braces}", expected: "branch-braces" }, + { input: "branch:colon", expected: "branch-colon" }, + { input: 'branch"quotes"', expected: "branch-quotes" }, + { input: "branch'single'quotes", expected: "branch-single-quotes" }, + ]; + + for (const { input, expected } of specialCharNames) { + const result = normalizeBranchName(input); + expect(result).toBe(expected); + } + }); + + it("should preserve valid branch name characters", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + const validNames = [ + { input: "feature/my-branch_v1.0", expected: "feature/my-branch_v1.0" }, + { input: "hotfix-123", expected: "hotfix-123" }, + { input: "release_v2.0.0", expected: "release_v2.0.0" }, + ]; + + for (const { input, expected } of validNames) { + const result = normalizeBranchName(input); + expect(result).toBe(expected); + } + }); + + it("should handle empty strings after sanitization", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + // Branch names that become empty after sanitization + const emptyAfterSanitization = ["!@#$%^&*()", ";;;", "|||", "---"]; + + for (const input of emptyAfterSanitization) { + const result = normalizeBranchName(input); + expect(result).toBe(""); + } + }); + + it("should truncate long branch names to 128 characters", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + const longBranchName = "a".repeat(200); + const result = normalizeBranchName(longBranchName); + expect(result.length).toBeLessThanOrEqual(128); + }); + + it("should collapse multiple dashes to single dash", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("branch---with---dashes")).toBe("branch-with-dashes"); + expect(normalizeBranchName("branch with spaces")).toBe("branch-with-spaces"); + }); + + it("should remove leading and trailing dashes", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("---branch---")).toBe("branch"); + expect(normalizeBranchName("---")).toBe(""); + }); + + it("should convert to lowercase", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("Feature/MyBranch")).toBe("feature/mybranch"); + expect(normalizeBranchName("UPPERCASE")).toBe("uppercase"); + }); +}); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index fb2734d8474..36191563599 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -7,6 +7,7 @@ const { generateStagedPreview } = require("./staged_preview.cjs"); const { updateActivationCommentWithCommit } = require("./update_activation_comment.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { replaceTemporaryIdReferences } = require("./temporary_id.cjs"); +const { normalizeBranchName } = require("./normalize_branch_name.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -208,6 +209,22 @@ async function main(config = {}) { return { success: false, error: `Failed to determine branch name for PR ${pullNumber}` }; } + // SECURITY: Sanitize branch name to prevent shell injection (CWE-78) + // Branch names from GitHub API must be normalized before use in git commands + if (branchName) { + const originalBranchName = branchName; + branchName = normalizeBranchName(branchName); + + // Validate it's not empty after normalization + if (!branchName) { + return { success: false, error: `Invalid branch name: sanitization resulted in empty string (original: "${originalBranchName}")` }; + } + + if (originalBranchName !== branchName) { + core.info(`Branch name sanitized: "${originalBranchName}" -> "${branchName}"`); + } + } + core.info(`Target branch: ${branchName}`); core.info(`PR title: ${prTitle}`); core.info(`PR labels: ${prLabels.join(", ")}`);