Merge pull request #3 from dmno-dev/fix/shell-injection-security

Security hardening: eliminate shell injection vulnerabilities

Author: theoephraim

.bumpy/security-audit.md

@@ -0,0 +1,12 @@
+---
+'@varlock/bumpy': patch
+---
+
+Security hardening: eliminate shell injection vulnerabilities across all CLI commands
+
+- Replace shell string interpolation with `execFile`-based argument arrays (`runArgs`/`runArgsAsync`) throughout the codebase, preventing command injection via branch names, PR numbers, config values, package names, and registry URLs
+- Add input validation for git branch names and PR numbers from environment variables
+- Remove broken `escapeShell` function in favor of shell-free execution
+- Use `sq()` single-quote escaping for template substitutions in user-defined publish commands
+- Restrict dynamic changelog formatter imports to paths within the project root
+- Reduce changeset filename collisions by using three-word random names

.github/workflows/ci.yaml

@@ -10,7 +10,8 @@ jobs:
       - run: bun install
       - run: cd packages/bumpy && bunx varlock load # need ENV types for tsdown config file
       - run: bun run check
-      - run: git config --global user.name "CI" && git config --global user.email "ci@test"
+      # publish tests create temp git repos with commits, which requires a git identity
+      - run: git config --global user.name "CI" && git config --global user.email "ci@example.com"
       - run: bun run test
 
   bumpy-check:

.gitignore

@@ -19,3 +19,5 @@ env.d.ts
 *.tsbuildinfo
 
 ignore
+
+.claude/worktrees

packages/bumpy/src/commands/check.ts

@@ -3,7 +3,7 @@ import { log, colorize } from '../utils/logger.ts';
 import { loadConfig } from '../core/config.ts';
 import { discoverWorkspace } from '../core/workspace.ts';
 import { readChangesets } from '../core/changeset.ts';
-import { tryRun } from '../utils/shell.ts';
+import { tryRunArgs } from '../utils/shell.ts';
 import type { WorkspacePackage } from '../types.ts';
 
 /**
@@ -61,9 +61,9 @@ export async function checkCommand(rootDir: string): Promise<void> {
 /** Get files changed on this branch compared to the base branch */
 function getChangedFiles(rootDir: string, baseBranch: string): string[] {
   // Try merge-base first (works on branches)
-  const mergeBase = tryRun(`git merge-base HEAD origin/${baseBranch}`, { cwd: rootDir });
+  const mergeBase = tryRunArgs(['git', 'merge-base', 'HEAD', `origin/${baseBranch}`], { cwd: rootDir });
   const ref = mergeBase || `origin/${baseBranch}`;
-  const diff = tryRun(`git diff --name-only ${ref}`, { cwd: rootDir });
+  const diff = tryRunArgs(['git', 'diff', '--name-only', ref], { cwd: rootDir });
   if (!diff) return [];
   return diff.split('\n').filter(Boolean);
 }

packages/bumpy/src/commands/ci.ts

@@ -4,16 +4,34 @@ import { discoverWorkspace } from '../core/workspace.ts';
 import { DependencyGraph } from '../core/dep-graph.ts';
 import { readChangesets } from '../core/changeset.ts';
 import { assembleReleasePlan } from '../core/release-plan.ts';
-import { run, tryRun, runAsync } from '../utils/shell.ts';
+import { runArgs, runArgsAsync, tryRunArgs } from '../utils/shell.ts';
 import type { BumpyConfig, ReleasePlan, PlannedRelease } from '../types.ts';
 
+// ---- Validation helpers ----
+
+/** Validate a git branch name to prevent injection */
+function validateBranchName(name: string): string {
+  if (!/^[a-zA-Z0-9_./-]+$/.test(name)) {
+    throw new Error(`Invalid branch name: ${name}`);
+  }
+  return name;
+}
+
+/** Validate a PR number is numeric */
+function validatePrNumber(pr: string): string {
+  if (!/^\d+$/.test(pr)) {
+    throw new Error(`Invalid PR number: ${pr}`);
+  }
+  return pr;
+}
+
 /** Configure git identity for CI commits if not already set */
 function ensureGitIdentity(rootDir: string, config: BumpyConfig): void {
-  const name = tryRun('git config user.name', { cwd: rootDir });
+  const name = tryRunArgs(['git', 'config', 'user.name'], { cwd: rootDir });
   if (!name) {
     const { name: gitName, email: gitEmail } = config.gitUser;
-    run(`git config user.name "${gitName}"`, { cwd: rootDir });
-    run(`git config user.email "${gitEmail}"`, { cwd: rootDir });
+    runArgs(['git', 'config', 'user.name', gitName], { cwd: rootDir });
+    runArgs(['git', 'config', 'user.email', gitEmail], { cwd: rootDir });
     log.dim(`  Using git identity: ${gitName} <${gitEmail}>`);
   }
 }
@@ -119,11 +137,11 @@ async function autoPublish(rootDir: string, config: BumpyConfig, tag?: string):
 
   // Commit the version changes
   log.step('Committing version changes...');
-  run('git add -A', { cwd: rootDir });
-  const status = tryRun('git status --porcelain', { cwd: rootDir });
+  runArgs(['git', 'add', '-A'], { cwd: rootDir });
+  const status = tryRunArgs(['git', 'status', '--porcelain'], { cwd: rootDir });
   if (status) {
-    run('git commit -m "Version packages"', { cwd: rootDir });
-    run('git push', { cwd: rootDir });
+    runArgs(['git', 'commit', '-m', 'Version packages'], { cwd: rootDir });
+    runArgs(['git', 'push'], { cwd: rootDir });
   }
 
   log.step('Running bumpy publish...');
@@ -139,21 +157,25 @@ async function createVersionPr(
   config: BumpyConfig,
   branchName?: string,
 ): Promise<void> {
-  const branch = branchName || config.versionPr.branch;
-  const baseBranch = tryRun('git rev-parse --abbrev-ref HEAD', { cwd: rootDir }) || 'main';
+  const branch = validateBranchName(branchName || config.versionPr.branch);
+  const baseBranch = validateBranchName(
+    tryRunArgs(['git', 'rev-parse', '--abbrev-ref', 'HEAD'], { cwd: rootDir }) || 'main',
+  );
 
   // Check if a version PR already exists
-  const existingPr = tryRun(`gh pr list --head "${branch}" --json number --jq ".[0].number"`, { cwd: rootDir });
+  const existingPr = tryRunArgs(['gh', 'pr', 'list', '--head', branch, '--json', 'number', '--jq', '.[0].number'], {
+    cwd: rootDir,
+  });
 
   // Create or update the branch
   log.step(`Creating branch ${branch}...`);
-  const branchExists = tryRun(`git rev-parse --verify ${branch}`, { cwd: rootDir }) !== null;
+  const branchExists = tryRunArgs(['git', 'rev-parse', '--verify', branch], { cwd: rootDir }) !== null;
 
   if (branchExists) {
-    run(`git checkout ${branch}`, { cwd: rootDir });
-    run(`git reset --hard ${baseBranch}`, { cwd: rootDir });
+    runArgs(['git', 'checkout', branch], { cwd: rootDir });
+    runArgs(['git', 'reset', '--hard', baseBranch], { cwd: rootDir });
   } else {
-    run(`git checkout -b ${branch}`, { cwd: rootDir });
+    runArgs(['git', 'checkout', '-b', branch], { cwd: rootDir });
   }
 
   // Run bumpy version
@@ -162,40 +184,41 @@ async function createVersionPr(
   await versionCommand(rootDir);
 
   // Commit and push
-  run('git add -A', { cwd: rootDir });
-  const status = tryRun('git status --porcelain', { cwd: rootDir });
+  runArgs(['git', 'add', '-A'], { cwd: rootDir });
+  const status = tryRunArgs(['git', 'status', '--porcelain'], { cwd: rootDir });
   if (!status) {
     log.info('No version changes to commit.');
-    run(`git checkout ${baseBranch}`, { cwd: rootDir });
+    runArgs(['git', 'checkout', baseBranch], { cwd: rootDir });
     return;
   }
 
   const commitMsg = ['Version packages', '', ...plan.releases.map((r) => `${r.name}@${r.newVersion}`)].join('\n');
-  run('git commit -F -', { cwd: rootDir, input: commitMsg });
-  run(`git push -u origin ${branch} --force`, { cwd: rootDir });
+  runArgs(['git', 'commit', '-F', '-'], { cwd: rootDir, input: commitMsg });
+  runArgs(['git', 'push', '-u', 'origin', branch, '--force'], { cwd: rootDir });
 
   // Create or update PR
   const prBody = formatVersionPrBody(plan, config.versionPr.preamble);
 
   if (existingPr) {
-    log.step(`Updating existing PR #${existingPr}...`);
-    await runAsync(`gh pr edit ${existingPr} --title "${config.versionPr.title}" --body-file -`, {
+    const validPr = validatePrNumber(existingPr);
+    log.step(`Updating existing PR #${validPr}...`);
+    await runArgsAsync(['gh', 'pr', 'edit', validPr, '--title', config.versionPr.title, '--body-file', '-'], {
       cwd: rootDir,
       input: prBody,
     });
-    log.success(`Updated PR #${existingPr}`);
+    log.success(`Updated PR #${validPr}`);
   } else {
     log.step('Creating version PR...');
     const prTitle = config.versionPr.title;
-    const result = await runAsync(
-      `gh pr create --title "${prTitle}" --body-file - --base "${baseBranch}" --head "${branch}"`,
+    const result = await runArgsAsync(
+      ['gh', 'pr', 'create', '--title', prTitle, '--body-file', '-', '--base', baseBranch, '--head', branch],
       { cwd: rootDir, input: prBody },
     );
     log.success(`Created PR: ${result}`);
   }
 
   // Switch back to the base branch
-  run(`git checkout ${baseBranch}`, { cwd: rootDir });
+  runArgs(['git', 'checkout', baseBranch], { cwd: rootDir });
 }
 
 // ---- PR comment helpers ----
@@ -277,23 +300,27 @@ function formatVersionPrBody(plan: ReleasePlan, preamble: string): string {
 const COMMENT_MARKER = '<!-- bumpy-release-plan -->';
 
 async function postOrUpdatePrComment(prNumber: string, body: string, rootDir: string): Promise<void> {
+  const validPr = validatePrNumber(prNumber);
   const markedBody = `${COMMENT_MARKER}\n${body}`;
 
   try {
-    // Find existing bumpy comment
-    const existingComment = tryRun(
-      `gh pr view ${prNumber} --json comments --jq '.comments[] | select(.body | startswith("${COMMENT_MARKER}")) | .id' | head -1`,
-      { cwd: rootDir },
-    );
+    // Find existing bumpy comment using gh with jq
+    const jqFilter = `.comments[] | select(.body | startswith("${COMMENT_MARKER}")) | .id`;
+    const existingComment = tryRunArgs(['gh', 'pr', 'view', validPr, '--json', 'comments', '--jq', jqFilter], {
+      cwd: rootDir,
+    });
+
+    // Take the first result if multiple
+    const commentId = existingComment?.split('\n')[0]?.trim();
 
-    if (existingComment) {
-      await runAsync(`gh api repos/{owner}/{repo}/issues/comments/${existingComment} -X PATCH -f body=@-`, {
-        cwd: rootDir,
-        input: markedBody,
-      });
+    if (commentId) {
+      await runArgsAsync(
+        ['gh', 'api', `repos/{owner}/{repo}/issues/comments/${commentId}`, '-X', 'PATCH', '-f', 'body=@-'],
+        { cwd: rootDir, input: markedBody },
+      );
       log.dim('  Updated PR comment');
     } else {
-      await runAsync(`gh pr comment ${prNumber} --body-file -`, { cwd: rootDir, input: markedBody });
+      await runArgsAsync(['gh', 'pr', 'comment', validPr, '--body-file', '-'], { cwd: rootDir, input: markedBody });
       log.dim('  Posted PR comment');
     }
   } catch (err) {
@@ -308,6 +335,11 @@ function detectPrNumber(): string | null {
     const match = process.env.GITHUB_REF?.match(/refs\/pull\/(\d+)\//);
     if (match) return match[1]!;
   }
-  // Also check for explicit env var
-  return process.env.BUMPY_PR_NUMBER || process.env.PR_NUMBER || null;
+  // Also check for explicit env var — validate it's numeric
+  const envPr = process.env.BUMPY_PR_NUMBER || process.env.PR_NUMBER || null;
+  if (envPr && !/^\d+$/.test(envPr)) {
+    log.warn(`Ignoring invalid PR number from environment: ${envPr}`);
+    return null;
+  }
+  return envPr;
 }

packages/bumpy/src/commands/generate.ts

@@ -1,5 +1,5 @@
 import { log, colorize } from '../utils/logger.ts';
-import { tryRun } from '../utils/shell.ts';
+import { tryRunArgs } from '../utils/shell.ts';
 import { loadConfig } from '../core/config.ts';
 import { discoverPackages } from '../core/workspace.ts';
 import { writeChangeset } from '../core/changeset.ts';
@@ -50,7 +50,7 @@ export async function generateCommand(rootDir: string, opts: GenerateOptions): P
   log.step(`Scanning commits from ${colorize(from, 'cyan')}...`);
 
   // Get commits since ref
-  const rawLog = tryRun(`git log ${from}..HEAD --format="%H%n%s%n%b%n---END---"`, { cwd: rootDir });
+  const rawLog = tryRunArgs(['git', 'log', `${from}..HEAD`, '--format=%H%n%s%n%b%n---END---'], { cwd: rootDir });
 
   if (!rawLog) {
     log.info('No commits found since ' + from);
@@ -239,9 +239,8 @@ function bumpPriority(type: BumpType): number {
 /** Find the most recent version tag in the repo */
 function findLastVersionTag(rootDir: string): string | null {
   // Look for tags matching common patterns: v1.2.3, pkg@1.2.3, etc.
-  const tag = tryRun(
-    'git describe --tags --abbrev=0 --match "v*" 2>/dev/null || git describe --tags --abbrev=0 --match "*@*" 2>/dev/null',
-    { cwd: rootDir },
-  );
+  const tag =
+    tryRunArgs(['git', 'describe', '--tags', '--abbrev=0', '--match', 'v*'], { cwd: rootDir }) ||
+    tryRunArgs(['git', 'describe', '--tags', '--abbrev=0', '--match', '*@*'], { cwd: rootDir });
   return tag || null;
 }

packages/bumpy/src/commands/publish.ts

@@ -158,10 +158,9 @@ async function findUnpublishedPackages(
 }
 
 async function checkIfPublished(name: string, version: string, pkgConfig?: PackageConfig): Promise<boolean> {
-  const { runAsync } = await import('../utils/shell.ts');
-  const { tryRun } = await import('../utils/shell.ts');
+  const { runAsync, runArgsAsync, tryRunArgs } = await import('../utils/shell.ts');
 
-  // 1. Custom check command
+  // 1. Custom check command (user-defined, runs in shell by design)
   if (pkgConfig?.checkPublished) {
     try {
       const result = await runAsync(pkgConfig.checkPublished);
@@ -174,13 +173,14 @@ async function checkIfPublished(name: string, version: string, pkgConfig?: Packa
   // 2. Non-npm packages — check git tags
   if (pkgConfig?.skipNpmPublish || pkgConfig?.publishCommand) {
     const tag = `${name}@${version}`;
-    return tryRun(`git tag -l "${tag}"`) === tag;
+    return tryRunArgs(['git', 'tag', '-l', tag]) === tag;
   }
 
   // 3. Default — check npm registry
   try {
-    const regFlag = pkgConfig?.registry ? `--registry ${pkgConfig.registry}` : '';
-    const result = await runAsync(`npm info "${name}@${version}" version ${regFlag}`.trim());
+    const args = ['npm', 'info', `${name}@${version}`, 'version'];
+    if (pkgConfig?.registry) args.push('--registry', pkgConfig.registry);
+    const result = await runArgsAsync(args);
     return result === version;
   } catch {
     return false;

packages/bumpy/src/commands/version.ts

@@ -5,7 +5,7 @@ import { DependencyGraph } from '../core/dep-graph.ts';
 import { readChangesets } from '../core/changeset.ts';
 import { assembleReleasePlan } from '../core/release-plan.ts';
 import { applyReleasePlan } from '../core/apply-release-plan.ts';
-import { run, tryRun } from '../utils/shell.ts';
+import { runArgs, tryRunArgs } from '../utils/shell.ts';
 import { detectWorkspaces } from '../utils/package-manager.ts';
 
 export async function versionCommand(rootDir: string): Promise<void> {
@@ -46,18 +46,18 @@ export async function versionCommand(rootDir: string): Promise<void> {
   if (config.commit) {
     try {
       // Stage version changes, changelogs, deleted changesets, and lockfile
-      run('git add -A .bumpy/', { cwd: rootDir });
+      runArgs(['git', 'add', '-A', '.bumpy/'], { cwd: rootDir });
       for (const r of plan.releases) {
         const pkg = packages.get(r.name)!;
-        run(`git add "${pkg.relativeDir}/package.json"`, { cwd: rootDir });
-        run(`git add "${pkg.relativeDir}/CHANGELOG.md"`, { cwd: rootDir });
+        runArgs(['git', 'add', '--', `${pkg.relativeDir}/package.json`], { cwd: rootDir });
+        runArgs(['git', 'add', '--', `${pkg.relativeDir}/CHANGELOG.md`], { cwd: rootDir });
       }
       // Stage lockfile if it changed
       for (const lockfile of ['bun.lock', 'bun.lockb', 'pnpm-lock.yaml', 'yarn.lock', 'package-lock.json']) {
-        tryRun(`git add "${lockfile}"`, { cwd: rootDir });
+        tryRunArgs(['git', 'add', '--', lockfile], { cwd: rootDir });
       }
       const msg = ['Version packages', '', ...plan.releases.map((r) => `${r.name}@${r.newVersion}`)].join('\n');
-      run('git commit -F -', { cwd: rootDir, input: msg });
+      runArgs(['git', 'commit', '-F', '-'], { cwd: rootDir, input: msg });
       log.success('Created git commit');
     } catch (e) {
       log.warn(`Git commit failed: ${e}`);
@@ -68,26 +68,26 @@ export async function versionCommand(rootDir: string): Promise<void> {
 /** Run the package manager's install to update the lockfile */
 async function updateLockfile(rootDir: string): Promise<void> {
   const { packageManager } = await detectWorkspaces(rootDir);
-  const installCmd = getInstallCommand(packageManager);
+  const installArgs = getInstallArgs(packageManager);
 
-  log.step(`Updating lockfile (${installCmd})...`);
+  log.step(`Updating lockfile (${installArgs.join(' ')})...`);
   try {
-    run(installCmd, { cwd: rootDir });
+    runArgs(installArgs, { cwd: rootDir });
     log.dim('  Lockfile updated');
   } catch (err) {
     log.warn(`  Lockfile update failed: ${err instanceof Error ? err.message : err}`);
   }
 }
 
-function getInstallCommand(pm: string): string {
+function getInstallArgs(pm: string): string[] {
   switch (pm) {
     case 'pnpm':
-      return 'pnpm install --lockfile-only';
+      return ['pnpm', 'install', '--lockfile-only'];
     case 'bun':
-      return 'bun install';
+      return ['bun', 'install'];
     case 'yarn':
-      return 'yarn install --mode update-lockfile';
+      return ['yarn', 'install', '--mode', 'update-lockfile'];
     default:
-      return 'npm install --package-lock-only';
+      return ['npm', 'install', '--package-lock-only'];
   }
 }