From 5acf54c7846a4a4a6c44054b6d702a373c370fff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Mar 2026 12:53:47 +0000 Subject: [PATCH 1/5] Initial plan From 56b2eb2a85036829ecd87dbf6d2bbada708816ed Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Mar 2026 13:18:15 +0000 Subject: [PATCH 2/5] feat: add preserve-branch-name option to create-pull-request safe output When preserve-branch-name: true is set in the create-pull-request safe output configuration, the agent-specified branch name is used as-is: - No random salt suffix is appended - No lowercase conversion - Invalid characters are still replaced for security (CWE-78 prevention) Useful when CI enforces branch naming conventions such as Jira keys in uppercase (e.g. bugfix/BR-329-red). Closes #" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...or-preserve-branch-name-in-safe-outputs.md | 5 + actions/setup/js/create_pull_request.cjs | 15 +- actions/setup/js/create_pull_request.test.cjs | 37 +++++ actions/setup/js/normalize_branch_name.cjs | 45 ++++++ .../setup/js/normalize_branch_name.test.cjs | 58 +++++++ pkg/parser/schemas/main_workflow_schema.json | 5 + pkg/workflow/compile_outputs_pr_test.go | 144 ++++++++++++++++++ pkg/workflow/compiler_safe_outputs_config.go | 3 +- pkg/workflow/create_pull_request.go | 1 + pkg/workflow/safe_outputs_config_helpers.go | 4 + 10 files changed, 312 insertions(+), 5 deletions(-) create mode 100644 .changeset/minor-preserve-branch-name-in-safe-outputs.md diff --git a/.changeset/minor-preserve-branch-name-in-safe-outputs.md b/.changeset/minor-preserve-branch-name-in-safe-outputs.md new file mode 100644 index 00000000000..9b4b2698f7a --- /dev/null +++ b/.changeset/minor-preserve-branch-name-in-safe-outputs.md @@ -0,0 +1,5 @@ +--- +"gh-aw": minor +--- + +Add `preserve-branch-name: true` option to `create-pull-request` safe outputs. When enabled, the agent-specified branch name is used as-is — no random salt suffix is appended and the name is not lowercased. Invalid characters are still replaced for security. Useful when the target repository enforces branch naming conventions such as Jira keys in uppercase (e.g. `bugfix/BR-329-red`). diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 5a45d2ff81f..ac24f8c0a81 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -17,7 +17,7 @@ const { generateWorkflowIdMarker } = require("./generate_footer.cjs"); const { parseBoolTemplatable } = require("./templatable.cjs"); const { generateFooterWithMessages } = require("./messages_footer.cjs"); const { generateHistoryUrl } = require("./generate_history_link.cjs"); -const { normalizeBranchName } = require("./normalize_branch_name.cjs"); +const { normalizeBranchName, sanitizeBranchNamePreserveCase } = require("./normalize_branch_name.cjs"); const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); const { createCheckoutManager } = require("./dynamic_checkout.cjs"); const { getBaseBranch } = require("./get_base_branch.cjs"); @@ -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; @@ -529,10 +530,12 @@ async function main(config = {}) { 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 + // Branch names from user input must be normalized before use in git commands. + // When preserve-branch-name is enabled, original casing is kept but invalid + // characters are still replaced to prevent injection attacks. if (branchName) { const originalBranchName = branchName; - branchName = normalizeBranchName(branchName); + branchName = preserveBranchName ? sanitizeBranchNamePreserveCase(branchName) : normalizeBranchName(branchName); // Validate it's not empty after normalization if (!branchName) { @@ -624,11 +627,15 @@ async function main(config = {}) { 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 + // Use branch name from JSONL if provided, otherwise generate unique branch name. + // When preserve-branch-name is enabled, the agent-specified branch name is used as-is + // (no salt suffix appended) so CI branch naming conventions are respected. 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 if (preserveBranchName) { + core.info(`Using branch name from JSONL as-is (preserve-branch-name enabled): ${branchName}`); } else { branchName = `${branchName}-${randomHex}`; core.info(`Using branch name from JSONL with added salt: ${branchName}`); diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index e6c08f517d5..71ecb21450d 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -364,6 +364,43 @@ describe("create_pull_request - security: branch name sanitization", () => { }); }); +// ────────────────────────────────────────────────────── +// preserve-branch-name: sanitizeBranchNamePreserveCase +// ────────────────────────────────────────────────────── + +describe("create_pull_request - preserve-branch-name: sanitizeBranchNamePreserveCase", () => { + it("should preserve original casing while still sanitizing invalid characters", () => { + const { sanitizeBranchNamePreserveCase } = require("./normalize_branch_name.cjs"); + + // The motivating use-case: Jira keys in uppercase branch names + expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); + expect(sanitizeBranchNamePreserveCase("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature"); + expect(sanitizeBranchNamePreserveCase("hotfix/UPPERCASE")).toBe("hotfix/UPPERCASE"); + }); + + it("should still replace shell metacharacters for security (CWE-78)", () => { + const { sanitizeBranchNamePreserveCase } = 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 = sanitizeBranchNamePreserveCase(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("&"); + } + }); +}); + // ────────────────────────────────────────────────────── // allowed-files strict allowlist // ────────────────────────────────────────────────────── diff --git a/actions/setup/js/normalize_branch_name.cjs b/actions/setup/js/normalize_branch_name.cjs index ce8d4473a3e..4f14e382fe6 100644 --- a/actions/setup/js/normalize_branch_name.cjs +++ b/actions/setup/js/normalize_branch_name.cjs @@ -49,6 +49,51 @@ function normalizeBranchName(branchName) { return normalized; } +/** + * Sanitizes a branch name by replacing invalid characters while preserving the original case. + * + * This is a security-safe alternative to normalizeBranchName for use when + * preserve-branch-name is enabled. It still prevents shell injection by replacing + * invalid characters, but skips the lowercase conversion. + * + * The sanitization 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 + * (No lowercase conversion) + * + * @param {string} branchName - The branch name to sanitize + * @returns {string} The sanitized branch name with original casing preserved + */ +function sanitizeBranchNamePreserveCase(branchName) { + if (!branchName || typeof branchName !== "string" || branchName.trim() === "") { + return branchName; + } + + // Replace any sequence of invalid characters with a single dash + // Valid characters are: a-z, A-Z, 0-9, -, _, /, . + let sanitized = branchName.replace(/[^a-zA-Z0-9\-_/.]+/g, "-"); + + // Collapse multiple consecutive dashes to a single dash + sanitized = sanitized.replace(/-+/g, "-"); + + // Remove leading and trailing dashes + sanitized = sanitized.replace(/^-+|-+$/g, ""); + + // Truncate to max 128 characters + if (sanitized.length > 128) { + sanitized = sanitized.substring(0, 128); + } + + // Ensure it doesn't end with a dash after truncation + sanitized = sanitized.replace(/-+$/, ""); + + return sanitized; +} + module.exports = { normalizeBranchName, + sanitizeBranchNamePreserveCase, }; diff --git a/actions/setup/js/normalize_branch_name.test.cjs b/actions/setup/js/normalize_branch_name.test.cjs index b45c693eed4..f9c0e3eff6b 100644 --- a/actions/setup/js/normalize_branch_name.test.cjs +++ b/actions/setup/js/normalize_branch_name.test.cjs @@ -80,3 +80,61 @@ describe("normalizeBranchName", () => { expect(result).not.toMatch(/-$/); }); }); + +describe("sanitizeBranchNamePreserveCase", () => { + it("should preserve valid branch names with original casing", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); + expect(sanitizeBranchNamePreserveCase("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature"); + expect(sanitizeBranchNamePreserveCase("hotfix/UPPERCASE")).toBe("hotfix/UPPERCASE"); + }); + + it("should replace invalid characters with dashes", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("Feature@Test")).toBe("Feature-Test"); + expect(sanitizeBranchNamePreserveCase("branch; rm -rf /")).toBe("branch-rm-rf-/"); + expect(sanitizeBranchNamePreserveCase("branch$(malicious)")).toBe("branch-malicious"); + }); + + it("should NOT convert to lowercase", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("Feature/Add-Login")).toBe("Feature/Add-Login"); + expect(sanitizeBranchNamePreserveCase("MY-BRANCH")).toBe("MY-BRANCH"); + expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); + }); + + it("should collapse multiple dashes", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("Test---Branch")).toBe("Test-Branch"); + expect(sanitizeBranchNamePreserveCase("A--B--C")).toBe("A-B-C"); + }); + + it("should remove leading and trailing dashes", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("-Test-Branch-")).toBe("Test-Branch"); + expect(sanitizeBranchNamePreserveCase("---Test---")).toBe("Test"); + }); + + it("should truncate to 128 characters", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + const longName = "A".repeat(150); + const result = sanitizeBranchNamePreserveCase(longName); + expect(result.length).toBe(128); + expect(result).toBe("A".repeat(128)); + }); + + it("should handle empty and invalid inputs", async () => { + const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + + expect(sanitizeBranchNamePreserveCase("")).toBe(""); + expect(sanitizeBranchNamePreserveCase(" ")).toBe(" "); + expect(sanitizeBranchNamePreserveCase(null)).toBe(null); + expect(sanitizeBranchNamePreserveCase(undefined)).toBe(undefined); + }); +}); diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index e605ffd59a4..320ef5f83bd 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5342,6 +5342,11 @@ "type": "string" }, "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 agent-specified branch name is used as-is: the random salt suffix is not appended and the name is not lowercased. Invalid characters are still replaced for security. 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, diff --git a/pkg/workflow/compile_outputs_pr_test.go b/pkg/workflow/compile_outputs_pr_test.go index 5a14dc1771b..8d280b033d9 100644 --- a/pkg/workflow/compile_outputs_pr_test.go +++ b/pkg/workflow/compile_outputs_pr_test.go @@ -862,3 +862,147 @@ This workflow tests the create-pull-request with default fallback-as-issue behav t.Error("Did not expect fallback_as_issue:false in handler config JSON when using default") } } + +func TestOutputPullRequestPreserveBranchName(t *testing.T) { + // Create temporary directory for test files + tmpDir := testutil.TempDir(t, "output-pr-preserve-branch-test") + + // Test case with create-pull-request configuration with preserve-branch-name: true + testContent := `--- +on: push +permissions: + contents: read + pull-requests: read +engine: claude +strict: false +safe-outputs: + create-pull-request: + title-prefix: "[test] " + preserve-branch-name: true +--- + +# Test Output Pull Request Preserve Branch Name + +This workflow tests the create-pull-request with preserve-branch-name enabled. +` + + testFile := filepath.Join(tmpDir, "test-output-pr-preserve-branch.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + + // Parse the workflow data + workflowData, err := compiler.ParseWorkflowFile(testFile) + if err != nil { + t.Fatalf("Unexpected error parsing workflow with preserve-branch-name: true: %v", err) + } + + // Verify that preserve-branch-name is parsed correctly + if workflowData.SafeOutputs == nil { + t.Fatal("Expected output configuration to be parsed") + } + + if workflowData.SafeOutputs.CreatePullRequests == nil { + t.Fatal("Expected pull-request configuration to be parsed") + } + + if !workflowData.SafeOutputs.CreatePullRequests.PreserveBranchName { + t.Error("Expected preserve-branch-name to be true") + } + + // Compile the workflow + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("Unexpected error compiling workflow with preserve-branch-name: true: %v", err) + } + + // Read the generated lock file + lockFile := stringutil.MarkdownToLockFile(testFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read generated lock file: %v", err) + } + + // Convert to string for easier testing + lockContentStr := string(lockContent) + + // Verify GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG is present + if !strings.Contains(lockContentStr, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + t.Error("Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG environment variable") + } + + // Verify preserve_branch_name is present in the handler config JSON + if !strings.Contains(lockContentStr, `preserve_branch_name\":true`) { + t.Error("Expected preserve_branch_name:true in handler config JSON") + } +} + +func TestOutputPullRequestPreserveBranchNameDefault(t *testing.T) { + // Create temporary directory for test files + tmpDir := testutil.TempDir(t, "output-pr-preserve-branch-default-test") + + // Test case with create-pull-request configuration without preserve-branch-name (default) + testContent := `--- +on: push +permissions: + contents: read + pull-requests: read +engine: claude +strict: false +safe-outputs: + create-pull-request: + title-prefix: "[test] " +--- + +# Test Output Pull Request Preserve Branch Name Default + +This workflow tests the create-pull-request without preserve-branch-name (default behavior). +` + + testFile := filepath.Join(tmpDir, "test-output-pr-preserve-branch-default.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + + // Parse the workflow data + workflowData, err := compiler.ParseWorkflowFile(testFile) + if err != nil { + t.Fatalf("Unexpected error parsing workflow without preserve-branch-name: %v", err) + } + + // Verify that preserve-branch-name defaults to false + if workflowData.SafeOutputs == nil { + t.Fatal("Expected output configuration to be parsed") + } + + if workflowData.SafeOutputs.CreatePullRequests == nil { + t.Fatal("Expected pull-request configuration to be parsed") + } + + if workflowData.SafeOutputs.CreatePullRequests.PreserveBranchName { + t.Error("Expected preserve-branch-name to default to false") + } + + // Compile the workflow + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("Unexpected error compiling workflow without preserve-branch-name: %v", err) + } + + // Read the generated lock file + lockFile := stringutil.MarkdownToLockFile(testFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read generated lock file: %v", err) + } + + // Convert to string for easier testing + lockContentStr := string(lockContent) + + // When not explicitly set, preserve_branch_name should NOT be in the config + if strings.Contains(lockContentStr, `preserve_branch_name`) { + t.Error("Did not expect preserve_branch_name in handler config JSON when using default") + } +} diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index 99e887ec40c..46b6e76d6dd 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -486,7 +486,8 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringPtr("protected_files_policy", c.ManifestFilesPolicy). AddStringSlice("protected_files", getAllManifestFiles()). AddStringSlice("protected_path_prefixes", getProtectedPathPrefixes()). - AddStringSlice("allowed_files", c.AllowedFiles) + AddStringSlice("allowed_files", c.AllowedFiles). + AddIfTrue("preserve_branch_name", c.PreserveBranchName) return builder.Build() }, "push_to_pull_request_branch": func(cfg *SafeOutputsConfig) map[string]any { diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index 6ee0a375c4c..1c7275aa4e9 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -34,6 +34,7 @@ type CreatePullRequestsConfig struct { GithubTokenForExtraEmptyCommit string `yaml:"github-token-for-extra-empty-commit,omitempty"` // Token used to push an empty commit to trigger CI events. Use a PAT or "app" for GitHub App auth. ManifestFilesPolicy *string `yaml:"protected-files,omitempty"` // Controls protected-file protection: "blocked" (default) hard-blocks, "allowed" permits all changes, "fallback-to-issue" pushes the branch but creates a review issue. AllowedFiles []string `yaml:"allowed-files,omitempty"` // Strict allowlist of glob patterns for files eligible for create. Checked independently of protected-files; both checks must pass. + PreserveBranchName bool `yaml:"preserve-branch-name,omitempty"` // When true, skips lowercase conversion and random salt suffix on agent-specified branch names. Invalid characters are still replaced for security. Useful when CI enforces branch naming conventions (e.g. Jira keys in uppercase). } // parsePullRequestsConfig handles only create-pull-request (singular) configuration diff --git a/pkg/workflow/safe_outputs_config_helpers.go b/pkg/workflow/safe_outputs_config_helpers.go index 3d12845ceca..955231dd8db 100644 --- a/pkg/workflow/safe_outputs_config_helpers.go +++ b/pkg/workflow/safe_outputs_config_helpers.go @@ -171,6 +171,10 @@ func generatePullRequestConfig(prConfig *CreatePullRequestsConfig, defaultMax in if prConfig.FallbackAsIssue != nil { additionalFields["fallback_as_issue"] = *prConfig.FallbackAsIssue } + // Pass preserve_branch_name to skip lowercase and salt suffix + if prConfig.PreserveBranchName { + additionalFields["preserve_branch_name"] = true + } // Use generateTargetConfigWithRepos to include target-repo and allowed_repos targetConfig := SafeOutputTargetConfig{ From 9ca33ca7a4a5e119026aec909fc2bd916b194152 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Mar 2026 13:33:52 +0000 Subject: [PATCH 3/5] refactor: merge salt and lowercase into normalizeBranchName options - normalizeBranchName now accepts { lowercase, salt } options - lowercase defaults to false (no forced case conversion) - salt defaults to null (no suffix) - Removed sanitizeBranchNamePreserveCase (no longer needed) - create_pull_request.cjs uses options instead of two separate functions - Updated all tests to cover the new options API Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/create_pull_request.cjs | 32 ++++---- actions/setup/js/create_pull_request.test.cjs | 46 +++++++---- actions/setup/js/normalize_branch_name.cjs | 62 ++++---------- .../setup/js/normalize_branch_name.test.cjs | 82 ++++++++----------- 4 files changed, 94 insertions(+), 128 deletions(-) diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index ac24f8c0a81..94b0daa5747 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -17,7 +17,7 @@ const { generateWorkflowIdMarker } = require("./generate_footer.cjs"); const { parseBoolTemplatable } = require("./templatable.cjs"); const { generateFooterWithMessages } = require("./messages_footer.cjs"); const { generateHistoryUrl } = require("./generate_history_link.cjs"); -const { normalizeBranchName, sanitizeBranchNamePreserveCase } = require("./normalize_branch_name.cjs"); +const { normalizeBranchName } = require("./normalize_branch_name.cjs"); const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); const { createCheckoutManager } = require("./dynamic_checkout.cjs"); const { getBaseBranch } = require("./get_base_branch.cjs"); @@ -134,7 +134,7 @@ async function main(config = {}) { // SECURITY: If base branch is explicitly configured, validate it at factory level if (configBaseBranch) { - const normalizedConfigBase = normalizeBranchName(configBaseBranch); + const normalizedConfigBase = normalizeBranchName(configBaseBranch, { lowercase: true }); if (!normalizedConfigBase) { throw new Error(`Invalid baseBranch: sanitization resulted in empty string (original: "${configBaseBranch}")`); } @@ -283,7 +283,7 @@ async function main(config = {}) { // SECURITY: Sanitize dynamically resolved base branch to prevent shell injection const originalBaseBranch = baseBranch; - baseBranch = normalizeBranchName(baseBranch); + baseBranch = normalizeBranchName(baseBranch, { lowercase: true }); if (!baseBranch) { return { success: false, @@ -528,20 +528,29 @@ 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. - // When preserve-branch-name is enabled, original casing is kept but invalid - // characters are still replaced to prevent injection attacks. + // When preserve-branch-name is disabled (default), also lowercase the name and + // append a random salt suffix to avoid collisions. if (branchName) { const originalBranchName = branchName; - branchName = preserveBranchName ? sanitizeBranchNamePreserveCase(branchName) : normalizeBranchName(branchName); + branchName = normalizeBranchName(branchName, { + lowercase: !preserveBranchName, + salt: preserveBranchName ? undefined : 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 as-is (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}"`); } @@ -626,19 +635,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 preserve-branch-name is enabled, the agent-specified branch name is used as-is - // (no salt suffix appended) so CI branch naming conventions are respected. + // 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 if (preserveBranchName) { - core.info(`Using branch name from JSONL as-is (preserve-branch-name enabled): ${branchName}`); - } else { - branchName = `${branchName}-${randomHex}`; - core.info(`Using branch name from JSONL with added salt: ${branchName}`); } core.info(`Generated branch name: ${branchName}`); diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 71ecb21450d..7b1ea606962 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -356,30 +356,48 @@ describe("create_pull_request - security: branch name sanitization", () => { expect(normalizeBranchName("---")).toBe(""); }); - it("should convert to lowercase", () => { + it("should preserve original casing by default (no forced lowercase)", () => { 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"); + }); + + it("should convert to lowercase when lowercase option is true", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("Feature/MyBranch", { lowercase: true })).toBe("feature/mybranch"); + expect(normalizeBranchName("UPPERCASE", { lowercase: true })).toBe("uppercase"); + expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true })).toBe("bugfix/br-329-red"); }); }); // ────────────────────────────────────────────────────── -// preserve-branch-name: sanitizeBranchNamePreserveCase +// normalizeBranchName: salt option // ────────────────────────────────────────────────────── -describe("create_pull_request - preserve-branch-name: sanitizeBranchNamePreserveCase", () => { - it("should preserve original casing while still sanitizing invalid characters", () => { - const { sanitizeBranchNamePreserveCase } = require("./normalize_branch_name.cjs"); +describe("create_pull_request - normalizeBranchName: salt option", () => { + it("should append salt suffix when salt option is provided", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("feature/my-branch", { salt: "abc123" })).toBe("feature/my-branch-abc123"); + expect(normalizeBranchName("bugfix/BR-329-red", { salt: "cde2a954af3b8fa8" })).toBe("bugfix/BR-329-red-cde2a954af3b8fa8"); + }); + + it("should preserve original casing and add salt (preserve-branch-name use-case)", () => { + const { normalizeBranchName } = require("./normalize_branch_name.cjs"); - // The motivating use-case: Jira keys in uppercase branch names - expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); - expect(sanitizeBranchNamePreserveCase("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature"); - expect(sanitizeBranchNamePreserveCase("hotfix/UPPERCASE")).toBe("hotfix/UPPERCASE"); + // preserve-branch-name=false: lowercase + salt + expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true, salt: "cde2a954" })).toBe("bugfix/br-329-red-cde2a954"); + + // preserve-branch-name=true: no lowercase, no salt + expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: false })).toBe("bugfix/BR-329-red"); }); - it("should still replace shell metacharacters for security (CWE-78)", () => { - const { sanitizeBranchNamePreserveCase } = require("./normalize_branch_name.cjs"); + 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-/" }, @@ -390,7 +408,7 @@ describe("create_pull_request - preserve-branch-name: sanitizeBranchNamePreserve ]; for (const { input, expected } of dangerousNames) { - const result = sanitizeBranchNamePreserveCase(input); + const result = normalizeBranchName(input); expect(result).toBe(expected); expect(result).not.toContain(";"); expect(result).not.toContain("$"); diff --git a/actions/setup/js/normalize_branch_name.cjs b/actions/setup/js/normalize_branch_name.cjs index 4f14e382fe6..4159660f92e 100644 --- a/actions/setup/js/normalize_branch_name.cjs +++ b/actions/setup/js/normalize_branch_name.cjs @@ -7,7 +7,7 @@ * 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 @@ -15,12 +15,16 @@ * 3. Removes leading and trailing dashes * 4. Truncates to 128 characters * 5. Removes trailing dashes after truncation - * 6. Converts to lowercase + * 6. Optionally converts to lowercase (opt-in via `options.lowercase`) + * 7. Optionally appends a salt suffix (opt-in via `options.salt`) * * @param {string} branchName - The branch name to normalize + * @param {{ lowercase?: boolean, salt?: string | null }} [options] - Normalization options + * @param {boolean} [options.lowercase=false] - When true, converts the branch name to lowercase + * @param {string | null} [options.salt=null] - When set, appends `-` to the branch name * @returns {string} The normalized branch name */ -function normalizeBranchName(branchName) { +function normalizeBranchName(branchName, { lowercase = false, salt = null } = {}) { if (!branchName || typeof branchName !== "string" || branchName.trim() === "") { return branchName; } @@ -43,57 +47,19 @@ function normalizeBranchName(branchName) { // Ensure it doesn't end with a dash after truncation normalized = normalized.replace(/-+$/, ""); - // Convert to lowercase - normalized = normalized.toLowerCase(); - - return normalized; -} - -/** - * Sanitizes a branch name by replacing invalid characters while preserving the original case. - * - * This is a security-safe alternative to normalizeBranchName for use when - * preserve-branch-name is enabled. It still prevents shell injection by replacing - * invalid characters, but skips the lowercase conversion. - * - * The sanitization 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 - * (No lowercase conversion) - * - * @param {string} branchName - The branch name to sanitize - * @returns {string} The sanitized branch name with original casing preserved - */ -function sanitizeBranchNamePreserveCase(branchName) { - if (!branchName || typeof branchName !== "string" || branchName.trim() === "") { - return branchName; + // Optionally convert to lowercase + if (lowercase) { + normalized = normalized.toLowerCase(); } - // Replace any sequence of invalid characters with a single dash - // Valid characters are: a-z, A-Z, 0-9, -, _, /, . - let sanitized = branchName.replace(/[^a-zA-Z0-9\-_/.]+/g, "-"); - - // Collapse multiple consecutive dashes to a single dash - sanitized = sanitized.replace(/-+/g, "-"); - - // Remove leading and trailing dashes - sanitized = sanitized.replace(/^-+|-+$/g, ""); - - // Truncate to max 128 characters - if (sanitized.length > 128) { - sanitized = sanitized.substring(0, 128); + // Optionally append a salt suffix + if (salt) { + normalized = `${normalized}-${salt}`; } - // Ensure it doesn't end with a dash after truncation - sanitized = sanitized.replace(/-+$/, ""); - - return sanitized; + return normalized; } module.exports = { normalizeBranchName, - sanitizeBranchNamePreserveCase, }; diff --git a/actions/setup/js/normalize_branch_name.test.cjs b/actions/setup/js/normalize_branch_name.test.cjs index f9c0e3eff6b..191f8ad9626 100644 --- a/actions/setup/js/normalize_branch_name.test.cjs +++ b/actions/setup/js/normalize_branch_name.test.cjs @@ -40,11 +40,20 @@ describe("normalizeBranchName", () => { expect(result).toBe("a".repeat(128)); }); - it("should convert to lowercase", async () => { + it("should NOT convert to lowercase by default", 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"); + }); + + it("should convert to lowercase when lowercase option is true", async () => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); + + expect(normalizeBranchName("Feature/Add-Login", { lowercase: true })).toBe("feature/add-login"); + expect(normalizeBranchName("MY-BRANCH", { lowercase: true })).toBe("my-branch"); + expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true })).toBe("bugfix/br-329-red"); }); it("should handle empty and invalid inputs", async () => { @@ -66,7 +75,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__"); }); @@ -79,62 +88,35 @@ describe("normalizeBranchName", () => { expect(result.length).toBeLessThanOrEqual(128); expect(result).not.toMatch(/-$/); }); -}); - -describe("sanitizeBranchNamePreserveCase", () => { - it("should preserve valid branch names with original casing", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); - - expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); - expect(sanitizeBranchNamePreserveCase("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature"); - expect(sanitizeBranchNamePreserveCase("hotfix/UPPERCASE")).toBe("hotfix/UPPERCASE"); - }); - - it("should replace invalid characters with dashes", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); - - expect(sanitizeBranchNamePreserveCase("Feature@Test")).toBe("Feature-Test"); - expect(sanitizeBranchNamePreserveCase("branch; rm -rf /")).toBe("branch-rm-rf-/"); - expect(sanitizeBranchNamePreserveCase("branch$(malicious)")).toBe("branch-malicious"); - }); - - it("should NOT convert to lowercase", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); - expect(sanitizeBranchNamePreserveCase("Feature/Add-Login")).toBe("Feature/Add-Login"); - expect(sanitizeBranchNamePreserveCase("MY-BRANCH")).toBe("MY-BRANCH"); - expect(sanitizeBranchNamePreserveCase("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); - }); - - it("should collapse multiple dashes", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + it("should append salt suffix when salt option is provided", async () => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - expect(sanitizeBranchNamePreserveCase("Test---Branch")).toBe("Test-Branch"); - expect(sanitizeBranchNamePreserveCase("A--B--C")).toBe("A-B-C"); + expect(normalizeBranchName("feature/my-branch", { salt: "abc123" })).toBe("feature/my-branch-abc123"); + expect(normalizeBranchName("bugfix/BR-329-red", { salt: "cde2a954af3b8fa8" })).toBe("bugfix/BR-329-red-cde2a954af3b8fa8"); }); - it("should remove leading and trailing dashes", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + it("should not append salt when salt option is null or undefined", async () => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - expect(sanitizeBranchNamePreserveCase("-Test-Branch-")).toBe("Test-Branch"); - expect(sanitizeBranchNamePreserveCase("---Test---")).toBe("Test"); + expect(normalizeBranchName("feature/my-branch", { salt: null })).toBe("feature/my-branch"); + expect(normalizeBranchName("feature/my-branch", { salt: undefined })).toBe("feature/my-branch"); + expect(normalizeBranchName("feature/my-branch", {})).toBe("feature/my-branch"); + expect(normalizeBranchName("feature/my-branch")).toBe("feature/my-branch"); }); - it("should truncate to 128 characters", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + it("should support lowercase and salt together", async () => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - const longName = "A".repeat(150); - const result = sanitizeBranchNamePreserveCase(longName); - expect(result.length).toBe(128); - expect(result).toBe("A".repeat(128)); + expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true, salt: "cde2a954" })).toBe("bugfix/br-329-red-cde2a954"); }); - it("should handle empty and invalid inputs", async () => { - const { sanitizeBranchNamePreserveCase } = await import("./normalize_branch_name.cjs"); + it("should truncate to 128 chars before appending salt", async () => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - expect(sanitizeBranchNamePreserveCase("")).toBe(""); - expect(sanitizeBranchNamePreserveCase(" ")).toBe(" "); - expect(sanitizeBranchNamePreserveCase(null)).toBe(null); - expect(sanitizeBranchNamePreserveCase(undefined)).toBe(undefined); + const longName = "a".repeat(150); + const result = normalizeBranchName(longName, { salt: "abc123" }); + // Salt is appended after truncation so total length may exceed 128 + expect(result).toBe("a".repeat(128) + "-abc123"); }); }); From c6d5fb416e3428155505b37da0e0a818070d94f0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Mar 2026 13:45:59 +0000 Subject: [PATCH 4/5] refactor: remove lowercase option, make salt a positional parameter - normalizeBranchName(branchName, salt = null) - salt is the 2nd argument - No lowercase option - casing is always preserved - upload_assets.cjs local copy updated (remove lowercase) - create_pull_request.cjs uses normalizeBranchName(name, salt) directly - All tests updated to reflect new API Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/create_pull_request.cjs | 13 +++----- actions/setup/js/create_pull_request.test.cjs | 30 +++++++---------- actions/setup/js/normalize_branch_name.cjs | 16 +++------- .../setup/js/normalize_branch_name.test.cjs | 32 ++++++------------- actions/setup/js/upload_assets.cjs | 6 +--- 5 files changed, 30 insertions(+), 67 deletions(-) diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 94b0daa5747..2f36b4eb61b 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -134,7 +134,7 @@ async function main(config = {}) { // SECURITY: If base branch is explicitly configured, validate it at factory level if (configBaseBranch) { - const normalizedConfigBase = normalizeBranchName(configBaseBranch, { lowercase: true }); + const normalizedConfigBase = normalizeBranchName(configBaseBranch); if (!normalizedConfigBase) { throw new Error(`Invalid baseBranch: sanitization resulted in empty string (original: "${configBaseBranch}")`); } @@ -283,7 +283,7 @@ async function main(config = {}) { // SECURITY: Sanitize dynamically resolved base branch to prevent shell injection const originalBaseBranch = baseBranch; - baseBranch = normalizeBranchName(baseBranch, { lowercase: true }); + baseBranch = normalizeBranchName(baseBranch); if (!baseBranch) { return { success: false, @@ -532,14 +532,11 @@ async function main(config = {}) { // SECURITY: Sanitize branch name to prevent shell injection (CWE-78) // Branch names from user input must be normalized before use in git commands. - // When preserve-branch-name is disabled (default), also lowercase the name and - // append a random salt suffix to avoid collisions. + // When preserve-branch-name is disabled (default), a random salt suffix is + // appended to avoid collisions. if (branchName) { const originalBranchName = branchName; - branchName = normalizeBranchName(branchName, { - lowercase: !preserveBranchName, - salt: preserveBranchName ? undefined : randomHex, - }); + branchName = normalizeBranchName(branchName, preserveBranchName ? null : randomHex); // Validate it's not empty after normalization if (!branchName) { diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 7b1ea606962..1c9fe47e0d5 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -356,7 +356,7 @@ describe("create_pull_request - security: branch name sanitization", () => { expect(normalizeBranchName("---")).toBe(""); }); - it("should preserve original casing by default (no forced lowercase)", () => { + it("should preserve original casing (no lowercase conversion)", () => { const { normalizeBranchName } = require("./normalize_branch_name.cjs"); expect(normalizeBranchName("Feature/MyBranch")).toBe("Feature/MyBranch"); @@ -364,36 +364,28 @@ describe("create_pull_request - security: branch name sanitization", () => { // Motivating use-case: Jira keys stay uppercase expect(normalizeBranchName("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); }); - - it("should convert to lowercase when lowercase option is true", () => { - const { normalizeBranchName } = require("./normalize_branch_name.cjs"); - - expect(normalizeBranchName("Feature/MyBranch", { lowercase: true })).toBe("feature/mybranch"); - expect(normalizeBranchName("UPPERCASE", { lowercase: true })).toBe("uppercase"); - expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true })).toBe("bugfix/br-329-red"); - }); }); // ────────────────────────────────────────────────────── -// normalizeBranchName: salt option +// normalizeBranchName: salt argument // ────────────────────────────────────────────────────── -describe("create_pull_request - normalizeBranchName: salt option", () => { - it("should append salt suffix when salt option is provided", () => { +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", { salt: "abc123" })).toBe("feature/my-branch-abc123"); - expect(normalizeBranchName("bugfix/BR-329-red", { salt: "cde2a954af3b8fa8" })).toBe("bugfix/BR-329-red-cde2a954af3b8fa8"); + 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 (preserve-branch-name use-case)", () => { + it("should preserve original casing and add salt (default behaviour)", () => { const { normalizeBranchName } = require("./normalize_branch_name.cjs"); - // preserve-branch-name=false: lowercase + salt - expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true, salt: "cde2a954" })).toBe("bugfix/br-329-red-cde2a954"); + // Default: preserve case + salt + expect(normalizeBranchName("bugfix/BR-329-red", "cde2a954")).toBe("bugfix/BR-329-red-cde2a954"); - // preserve-branch-name=true: no lowercase, no salt - expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: false })).toBe("bugfix/BR-329-red"); + // 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)", () => { diff --git a/actions/setup/js/normalize_branch_name.cjs b/actions/setup/js/normalize_branch_name.cjs index 4159660f92e..96aa5b16720 100644 --- a/actions/setup/js/normalize_branch_name.cjs +++ b/actions/setup/js/normalize_branch_name.cjs @@ -15,16 +15,13 @@ * 3. Removes leading and trailing dashes * 4. Truncates to 128 characters * 5. Removes trailing dashes after truncation - * 6. Optionally converts to lowercase (opt-in via `options.lowercase`) - * 7. Optionally appends a salt suffix (opt-in via `options.salt`) + * 6. Appends `-` suffix when a salt value is provided * * @param {string} branchName - The branch name to normalize - * @param {{ lowercase?: boolean, salt?: string | null }} [options] - Normalization options - * @param {boolean} [options.lowercase=false] - When true, converts the branch name to lowercase - * @param {string | null} [options.salt=null] - When set, appends `-` to the branch name + * @param {string | null} [salt=null] - When set, appends `-` to the branch name * @returns {string} The normalized branch name */ -function normalizeBranchName(branchName, { lowercase = false, salt = null } = {}) { +function normalizeBranchName(branchName, salt = null) { if (!branchName || typeof branchName !== "string" || branchName.trim() === "") { return branchName; } @@ -47,12 +44,7 @@ function normalizeBranchName(branchName, { lowercase = false, salt = null } = {} // Ensure it doesn't end with a dash after truncation normalized = normalized.replace(/-+$/, ""); - // Optionally convert to lowercase - if (lowercase) { - normalized = normalized.toLowerCase(); - } - - // Optionally append a salt suffix + // Append a salt suffix when provided if (salt) { normalized = `${normalized}-${salt}`; } diff --git a/actions/setup/js/normalize_branch_name.test.cjs b/actions/setup/js/normalize_branch_name.test.cjs index 191f8ad9626..41e9fb7857e 100644 --- a/actions/setup/js/normalize_branch_name.test.cjs +++ b/actions/setup/js/normalize_branch_name.test.cjs @@ -40,20 +40,13 @@ describe("normalizeBranchName", () => { expect(result).toBe("a".repeat(128)); }); - it("should NOT convert to lowercase by default", 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("bugfix/BR-329-red")).toBe("bugfix/BR-329-red"); - }); - - it("should convert to lowercase when lowercase option is true", async () => { - const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - - expect(normalizeBranchName("Feature/Add-Login", { lowercase: true })).toBe("feature/add-login"); - expect(normalizeBranchName("MY-BRANCH", { lowercase: true })).toBe("my-branch"); - expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true })).toBe("bugfix/br-329-red"); + expect(normalizeBranchName("feature/JIRA-123-MyFeature")).toBe("feature/JIRA-123-MyFeature"); }); it("should handle empty and invalid inputs", async () => { @@ -89,33 +82,26 @@ describe("normalizeBranchName", () => { expect(result).not.toMatch(/-$/); }); - it("should append salt suffix when salt option is provided", async () => { + it("should append salt suffix when salt argument is provided", async () => { const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - expect(normalizeBranchName("feature/my-branch", { salt: "abc123" })).toBe("feature/my-branch-abc123"); - expect(normalizeBranchName("bugfix/BR-329-red", { salt: "cde2a954af3b8fa8" })).toBe("bugfix/BR-329-red-cde2a954af3b8fa8"); + 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 option is null or undefined", async () => { + 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", { salt: null })).toBe("feature/my-branch"); - expect(normalizeBranchName("feature/my-branch", { salt: undefined })).toBe("feature/my-branch"); - expect(normalizeBranchName("feature/my-branch", {})).toBe("feature/my-branch"); + 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 support lowercase and salt together", async () => { - const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); - - expect(normalizeBranchName("bugfix/BR-329-red", { lowercase: true, salt: "cde2a954" })).toBe("bugfix/br-329-red-cde2a954"); - }); - 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, { salt: "abc123" }); + const result = normalizeBranchName(longName, "abc123"); // Salt is appended after truncation so total length may exceed 128 expect(result).toBe("a".repeat(128) + "-abc123"); }); diff --git a/actions/setup/js/upload_assets.cjs b/actions/setup/js/upload_assets.cjs index 7003cfac39f..08da377e973 100644 --- a/actions/setup/js/upload_assets.cjs +++ b/actions/setup/js/upload_assets.cjs @@ -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 @@ -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 @@ -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; } From 6f64d55a2e371639a97de5bd46885d9a5b497293 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Mar 2026 14:26:58 +0000 Subject: [PATCH 5/5] fix: update stale comments and fix upload_assets test after removing lowercase - upload_assets.test.cjs: expect preserved-case output (assets/My-Branch) - create_pull_request.cjs: log says "without salt suffix" not "as-is" - create_pull_request.go: field comment says only salt is skipped - safe_outputs_config_helpers.go: comment says only salt is skipped - Schema: description removes reference to lowercasing - Changeset: description updated accordingly Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .changeset/minor-preserve-branch-name-in-safe-outputs.md | 2 +- actions/setup/js/create_pull_request.cjs | 2 +- actions/setup/js/upload_assets.test.cjs | 2 +- pkg/parser/schemas/main_workflow_schema.json | 2 +- pkg/workflow/create_pull_request.go | 2 +- pkg/workflow/safe_outputs_config_helpers.go | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.changeset/minor-preserve-branch-name-in-safe-outputs.md b/.changeset/minor-preserve-branch-name-in-safe-outputs.md index 9b4b2698f7a..f44258f504b 100644 --- a/.changeset/minor-preserve-branch-name-in-safe-outputs.md +++ b/.changeset/minor-preserve-branch-name-in-safe-outputs.md @@ -2,4 +2,4 @@ "gh-aw": minor --- -Add `preserve-branch-name: true` option to `create-pull-request` safe outputs. When enabled, the agent-specified branch name is used as-is — no random salt suffix is appended and the name is not lowercased. Invalid characters are still replaced for security. Useful when the target repository enforces branch naming conventions such as Jira keys in uppercase (e.g. `bugfix/BR-329-red`). +Add `preserve-branch-name: true` option to `create-pull-request` safe outputs. When enabled, no random salt suffix is 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 such as Jira keys in uppercase (e.g. `bugfix/BR-329-red`). diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 2f36b4eb61b..a6f52827628 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -544,7 +544,7 @@ async function main(config = {}) { } if (preserveBranchName) { - core.info(`Using branch name from JSONL as-is (preserve-branch-name enabled): ${branchName}`); + 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}`); } diff --git a/actions/setup/js/upload_assets.test.cjs b/actions/setup/js/upload_assets.test.cjs index 5d37cd60c85..9f755d29cdf 100644 --- a/actions/setup/js/upload_assets.test.cjs +++ b/actions/setup/js/upload_assets.test.cjs @@ -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", () => { diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 5403196bc2c..cad4debbfd5 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -5345,7 +5345,7 @@ }, "preserve-branch-name": { "type": "boolean", - "description": "When true, the agent-specified branch name is used as-is: the random salt suffix is not appended and the name is not lowercased. Invalid characters are still replaced for security. Useful when the target repository enforces branch naming conventions (e.g. Jira keys in uppercase such as 'bugfix/BR-329-red'). Defaults to false.", + "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 } }, diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index 1c7275aa4e9..977788db946 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -34,7 +34,7 @@ type CreatePullRequestsConfig struct { GithubTokenForExtraEmptyCommit string `yaml:"github-token-for-extra-empty-commit,omitempty"` // Token used to push an empty commit to trigger CI events. Use a PAT or "app" for GitHub App auth. ManifestFilesPolicy *string `yaml:"protected-files,omitempty"` // Controls protected-file protection: "blocked" (default) hard-blocks, "allowed" permits all changes, "fallback-to-issue" pushes the branch but creates a review issue. AllowedFiles []string `yaml:"allowed-files,omitempty"` // Strict allowlist of glob patterns for files eligible for create. Checked independently of protected-files; both checks must pass. - PreserveBranchName bool `yaml:"preserve-branch-name,omitempty"` // When true, skips lowercase conversion and random salt suffix on agent-specified branch names. Invalid characters are still replaced for security. Useful when CI enforces branch naming conventions (e.g. Jira keys in uppercase). + PreserveBranchName bool `yaml:"preserve-branch-name,omitempty"` // When true, skips the random salt suffix on agent-specified branch names. Invalid characters are still replaced for security; casing is always preserved. Useful when CI enforces branch naming conventions (e.g. Jira keys in uppercase). } // parsePullRequestsConfig handles only create-pull-request (singular) configuration diff --git a/pkg/workflow/safe_outputs_config_helpers.go b/pkg/workflow/safe_outputs_config_helpers.go index 955231dd8db..c3a7958b383 100644 --- a/pkg/workflow/safe_outputs_config_helpers.go +++ b/pkg/workflow/safe_outputs_config_helpers.go @@ -171,7 +171,7 @@ func generatePullRequestConfig(prConfig *CreatePullRequestsConfig, defaultMax in if prConfig.FallbackAsIssue != nil { additionalFields["fallback_as_issue"] = *prConfig.FallbackAsIssue } - // Pass preserve_branch_name to skip lowercase and salt suffix + // Pass preserve_branch_name to skip the random salt suffix if prConfig.PreserveBranchName { additionalFields["preserve_branch_name"] = true }