From 609c388361a522912370f658074908547a5d36c6 Mon Sep 17 00:00:00 2001 From: bogini Date: Fri, 12 Dec 2025 11:30:28 -0800 Subject: [PATCH] Fix command injection vulnerability in branch setup (#736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Prevent command injection in branch operations Replace Bun shell template literals with Node.js execFileSync to prevent command injection attacks via malicious branch names. Branch names from PR data (headRefName) are now validated against a strict whitelist pattern before use in git commands. Changes: - Add validateBranchName() function with strict character whitelist - Replace $`git ...` shell templates with execGit() using execFileSync - Validate all branch names before use in git operations * fix: Address review comments for branch validation security - Enhanced execGit JSDoc to explain security benefits of execFileSync - Added comprehensive branch name validation: - Leading dash check (prevents option injection) - Control characters and special git characters (~^:?*[\]) - Leading/trailing period checks - Trailing slash and consecutive slash checks - Added -- separator to git checkout commands - Added 30 unit tests for validateBranchName covering: - Valid branch names - Command injection attempts - Option injection attempts - Path traversal attempts - Git-specific invalid patterns - Control characters and edge cases 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --------- Co-authored-by: Claude --- src/github/operations/branch.ts | 123 ++++++++++++++++-- test/validate-branch-name.test.ts | 201 ++++++++++++++++++++++++++++++ 2 files changed, 316 insertions(+), 8 deletions(-) create mode 100644 test/validate-branch-name.test.ts diff --git a/src/github/operations/branch.ts b/src/github/operations/branch.ts index 42e7829..f14e93c 100644 --- a/src/github/operations/branch.ts +++ b/src/github/operations/branch.ts @@ -6,13 +6,112 @@ * - For Issues: Create a new branch */ -import { $ } from "bun"; +import { execFileSync } from "child_process"; import * as core from "@actions/core"; import type { ParsedGitHubContext } from "../context"; import type { GitHubPullRequest } from "../types"; import type { Octokits } from "../api/client"; import type { FetchDataResult } from "../data/fetcher"; +/** + * Validates a git branch name against a strict whitelist pattern. + * This prevents command injection by ensuring only safe characters are used. + * + * Valid branch names: + * - Start with alphanumeric character (not dash, to prevent option injection) + * - Contain only alphanumeric, forward slash, hyphen, underscore, or period + * - Do not start or end with a period + * - Do not end with a slash + * - Do not contain '..' (path traversal) + * - Do not contain '//' (consecutive slashes) + * - Do not end with '.lock' + * - Do not contain '@{' + * - Do not contain control characters or special git characters (~^:?*[\]) + */ +export function validateBranchName(branchName: string): void { + // Check for empty or whitespace-only names + if (!branchName || branchName.trim().length === 0) { + throw new Error("Branch name cannot be empty"); + } + + // Check for leading dash (prevents option injection like --help, -x) + if (branchName.startsWith("-")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot start with a dash.`, + ); + } + + // Check for control characters and special git characters (~^:?*[\]) + // eslint-disable-next-line no-control-regex + if (/[\x00-\x1F\x7F ~^:?*[\]\\]/.test(branchName)) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot contain control characters, spaces, or special git characters (~^:?*[\\]).`, + ); + } + + // Strict whitelist pattern: alphanumeric start, then alphanumeric/slash/hyphen/underscore/period + const validPattern = /^[a-zA-Z0-9][a-zA-Z0-9/_.-]*$/; + + if (!validPattern.test(branchName)) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names must start with an alphanumeric character and contain only alphanumeric characters, forward slashes, hyphens, underscores, or periods.`, + ); + } + + // Check for leading/trailing periods + if (branchName.startsWith(".") || branchName.endsWith(".")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot start or end with a period.`, + ); + } + + // Check for trailing slash + if (branchName.endsWith("/")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot end with a slash.`, + ); + } + + // Check for consecutive slashes + if (branchName.includes("//")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot contain consecutive slashes.`, + ); + } + + // Additional git-specific validations + if (branchName.includes("..")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot contain '..'`, + ); + } + + if (branchName.endsWith(".lock")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot end with '.lock'`, + ); + } + + if (branchName.includes("@{")) { + throw new Error( + `Invalid branch name: "${branchName}". Branch names cannot contain '@{'`, + ); + } +} + +/** + * Executes a git command safely using execFileSync to avoid shell interpolation. + * + * Security: execFileSync passes arguments directly to the git binary without + * invoking a shell, preventing command injection attacks where malicious input + * could be interpreted as shell commands (e.g., branch names containing `;`, `|`, `&&`). + * + * @param args - Git command arguments (e.g., ["checkout", "branch-name"]) + */ +function execGit(args: string[]): void { + execFileSync("git", args, { stdio: "inherit" }); +} + export type BranchInfo = { baseBranch: string; claudeBranch?: string; @@ -53,14 +152,19 @@ export async function setupBranch( `PR #${entityNumber}: ${commitCount} commits, using fetch depth ${fetchDepth}`, ); + // Validate branch names before use to prevent command injection + validateBranchName(branchName); + // Execute git commands to checkout PR branch (dynamic depth based on PR size) - await $`git fetch origin --depth=${fetchDepth} ${branchName}`; - await $`git checkout ${branchName} --`; + // Using execFileSync instead of shell template literals for security + execGit(["fetch", "origin", `--depth=${fetchDepth}`, branchName]); + execGit(["checkout", branchName, "--"]); console.log(`Successfully checked out PR branch for PR #${entityNumber}`); // For open PRs, we need to get the base branch of the PR const baseBranch = prData.baseRefName; + validateBranchName(baseBranch); return { baseBranch, @@ -118,8 +222,9 @@ export async function setupBranch( // Ensure we're on the source branch console.log(`Fetching and checking out source branch: ${sourceBranch}`); - await $`git fetch origin ${sourceBranch} --depth=1`; - await $`git checkout ${sourceBranch}`; + validateBranchName(sourceBranch); + execGit(["fetch", "origin", sourceBranch, "--depth=1"]); + execGit(["checkout", sourceBranch, "--"]); // Set outputs for GitHub Actions core.setOutput("CLAUDE_BRANCH", newBranch); @@ -138,11 +243,13 @@ export async function setupBranch( // Fetch and checkout the source branch first to ensure we branch from the correct base console.log(`Fetching and checking out source branch: ${sourceBranch}`); - await $`git fetch origin ${sourceBranch} --depth=1`; - await $`git checkout ${sourceBranch}`; + validateBranchName(sourceBranch); + validateBranchName(newBranch); + execGit(["fetch", "origin", sourceBranch, "--depth=1"]); + execGit(["checkout", sourceBranch, "--"]); // Create and checkout the new branch from the source branch - await $`git checkout -b ${newBranch}`; + execGit(["checkout", "-b", newBranch]); console.log( `Successfully created and checked out local branch: ${newBranch}`, diff --git a/test/validate-branch-name.test.ts b/test/validate-branch-name.test.ts new file mode 100644 index 0000000..539932d --- /dev/null +++ b/test/validate-branch-name.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, it } from "bun:test"; +import { validateBranchName } from "../src/github/operations/branch"; + +describe("validateBranchName", () => { + describe("valid branch names", () => { + it("should accept simple alphanumeric names", () => { + expect(() => validateBranchName("main")).not.toThrow(); + expect(() => validateBranchName("feature123")).not.toThrow(); + expect(() => validateBranchName("Branch1")).not.toThrow(); + }); + + it("should accept names with hyphens", () => { + expect(() => validateBranchName("feature-branch")).not.toThrow(); + expect(() => validateBranchName("fix-bug-123")).not.toThrow(); + }); + + it("should accept names with underscores", () => { + expect(() => validateBranchName("feature_branch")).not.toThrow(); + expect(() => validateBranchName("fix_bug_123")).not.toThrow(); + }); + + it("should accept names with forward slashes", () => { + expect(() => validateBranchName("feature/new-thing")).not.toThrow(); + expect(() => validateBranchName("user/feature/branch")).not.toThrow(); + }); + + it("should accept names with periods", () => { + expect(() => validateBranchName("v1.0.0")).not.toThrow(); + expect(() => validateBranchName("release.1.2.3")).not.toThrow(); + }); + + it("should accept typical branch name formats", () => { + expect(() => + validateBranchName("claude/issue-123-20250101-1234"), + ).not.toThrow(); + expect(() => validateBranchName("refs/heads/main")).not.toThrow(); + expect(() => validateBranchName("bugfix/JIRA-1234")).not.toThrow(); + }); + }); + + describe("command injection attempts", () => { + it("should reject shell command substitution with $()", () => { + expect(() => validateBranchName("$(whoami)")).toThrow(); + expect(() => validateBranchName("branch-$(rm -rf /)")).toThrow(); + expect(() => validateBranchName("test$(cat /etc/passwd)")).toThrow(); + }); + + it("should reject shell command substitution with backticks", () => { + expect(() => validateBranchName("`whoami`")).toThrow(); + expect(() => validateBranchName("branch-`rm -rf /`")).toThrow(); + }); + + it("should reject command chaining with semicolons", () => { + expect(() => validateBranchName("branch; rm -rf /")).toThrow(); + expect(() => validateBranchName("test;whoami")).toThrow(); + }); + + it("should reject command chaining with &&", () => { + expect(() => validateBranchName("branch && rm -rf /")).toThrow(); + expect(() => validateBranchName("test&&whoami")).toThrow(); + }); + + it("should reject command chaining with ||", () => { + expect(() => validateBranchName("branch || rm -rf /")).toThrow(); + expect(() => validateBranchName("test||whoami")).toThrow(); + }); + + it("should reject pipe characters", () => { + expect(() => validateBranchName("branch | cat")).toThrow(); + expect(() => validateBranchName("test|grep password")).toThrow(); + }); + + it("should reject redirection operators", () => { + expect(() => validateBranchName("branch > /etc/passwd")).toThrow(); + expect(() => validateBranchName("branch < input")).toThrow(); + expect(() => validateBranchName("branch >> file")).toThrow(); + }); + }); + + describe("option injection attempts", () => { + it("should reject branch names starting with dash", () => { + expect(() => validateBranchName("-x")).toThrow( + /cannot start with a dash/, + ); + expect(() => validateBranchName("--help")).toThrow( + /cannot start with a dash/, + ); + expect(() => validateBranchName("-")).toThrow(/cannot start with a dash/); + expect(() => validateBranchName("--version")).toThrow( + /cannot start with a dash/, + ); + expect(() => validateBranchName("-rf")).toThrow( + /cannot start with a dash/, + ); + }); + }); + + describe("path traversal attempts", () => { + it("should reject double dot sequences", () => { + expect(() => validateBranchName("../../../etc")).toThrow(); + expect(() => validateBranchName("branch/../secret")).toThrow(/'\.\.'$/); + expect(() => validateBranchName("a..b")).toThrow(/'\.\.'$/); + }); + }); + + describe("git-specific invalid patterns", () => { + it("should reject @{ sequence", () => { + expect(() => validateBranchName("branch@{1}")).toThrow(/@{/); + expect(() => validateBranchName("HEAD@{yesterday}")).toThrow(/@{/); + }); + + it("should reject .lock suffix", () => { + expect(() => validateBranchName("branch.lock")).toThrow(/\.lock/); + expect(() => validateBranchName("feature.lock")).toThrow(/\.lock/); + }); + + it("should reject consecutive slashes", () => { + expect(() => validateBranchName("feature//branch")).toThrow( + /consecutive slashes/, + ); + expect(() => validateBranchName("a//b//c")).toThrow( + /consecutive slashes/, + ); + }); + + it("should reject trailing slashes", () => { + expect(() => validateBranchName("feature/")).toThrow( + /cannot end with a slash/, + ); + expect(() => validateBranchName("branch/")).toThrow( + /cannot end with a slash/, + ); + }); + + it("should reject leading periods", () => { + expect(() => validateBranchName(".hidden")).toThrow(); + }); + + it("should reject trailing periods", () => { + expect(() => validateBranchName("branch.")).toThrow( + /cannot start or end with a period/, + ); + }); + + it("should reject special git refspec characters", () => { + expect(() => validateBranchName("branch~1")).toThrow(); + expect(() => validateBranchName("branch^2")).toThrow(); + expect(() => validateBranchName("branch:ref")).toThrow(); + expect(() => validateBranchName("branch?")).toThrow(); + expect(() => validateBranchName("branch*")).toThrow(); + expect(() => validateBranchName("branch[0]")).toThrow(); + expect(() => validateBranchName("branch\\path")).toThrow(); + }); + }); + + describe("control characters and special characters", () => { + it("should reject null bytes", () => { + expect(() => validateBranchName("branch\x00name")).toThrow(); + }); + + it("should reject other control characters", () => { + expect(() => validateBranchName("branch\x01name")).toThrow(); + expect(() => validateBranchName("branch\x1Fname")).toThrow(); + expect(() => validateBranchName("branch\x7Fname")).toThrow(); + }); + + it("should reject spaces", () => { + expect(() => validateBranchName("branch name")).toThrow(); + expect(() => validateBranchName("feature branch")).toThrow(); + }); + + it("should reject newlines and tabs", () => { + expect(() => validateBranchName("branch\nname")).toThrow(); + expect(() => validateBranchName("branch\tname")).toThrow(); + }); + }); + + describe("empty and whitespace", () => { + it("should reject empty strings", () => { + expect(() => validateBranchName("")).toThrow(/cannot be empty/); + }); + + it("should reject whitespace-only strings", () => { + expect(() => validateBranchName(" ")).toThrow(); + expect(() => validateBranchName("\t\n")).toThrow(); + }); + }); + + describe("edge cases", () => { + it("should accept single alphanumeric character", () => { + expect(() => validateBranchName("a")).not.toThrow(); + expect(() => validateBranchName("1")).not.toThrow(); + }); + + it("should reject single special characters", () => { + expect(() => validateBranchName(".")).toThrow(); + expect(() => validateBranchName("/")).toThrow(); + expect(() => validateBranchName("-")).toThrow(); + }); + }); +});