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(); + }); + }); +});