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 0000000000..f44258f504 --- /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, 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 5a45d2ff81..a6f5282762 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.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; @@ -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}"`); } @@ -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}`); diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index e6c08f517d..1c9fe47e0d 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -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("&"); + } }); }); diff --git a/actions/setup/js/normalize_branch_name.cjs b/actions/setup/js/normalize_branch_name.cjs index ce8d4473a3..96aa5b1672 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,13 @@ * 3. Removes leading and trailing dashes * 4. Truncates to 128 characters * 5. Removes trailing dashes after truncation - * 6. Converts to lowercase + * 6. Appends `-` suffix when a salt value is provided * * @param {string} branchName - The branch name to normalize + * @param {string | null} [salt=null] - When set, appends `-` 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; } @@ -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; } diff --git a/actions/setup/js/normalize_branch_name.test.cjs b/actions/setup/js/normalize_branch_name.test.cjs index b45c693eed..41e9fb7857 100644 --- a/actions/setup/js/normalize_branch_name.test.cjs +++ b/actions/setup/js/normalize_branch_name.test.cjs @@ -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 () => { @@ -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__"); }); @@ -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"); + }); }); diff --git a/actions/setup/js/upload_assets.cjs b/actions/setup/js/upload_assets.cjs index 7003cfac39..08da377e97 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; } diff --git a/actions/setup/js/upload_assets.test.cjs b/actions/setup/js/upload_assets.test.cjs index 5d37cd60c8..9f755d29cd 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 be32a4cd42..cad4debbfd 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -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, diff --git a/pkg/workflow/compile_outputs_pr_test.go b/pkg/workflow/compile_outputs_pr_test.go index 5a14dc1771..8d280b033d 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 2ad4a5f660..5e8bd41372 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 6ee0a375c4..977788db94 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 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 3d12845cec..c3a7958b38 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 the random salt suffix + if prConfig.PreserveBranchName { + additionalFields["preserve_branch_name"] = true + } // Use generateTargetConfigWithRepos to include target-repo and allowed_repos targetConfig := SafeOutputTargetConfig{