fix(sandbox): validate requested branch name before shelling out in clone (#4499)

git.repository.branch is config-supplied and gets interpolated straight into
sh -c git commands in spawnClone (ls-remote/clone/checkout) with no
validation, unlike the sibling checkout path (git/checkout-branch.ts) which
already rejects unsafe ref names before shelling out. Since git allows shell
metacharacters in ref names, a branch like `x;touch pwned` runs as a second
shell command during the very first clone.

Consolidated clone.ts's own isSafeRefName (already used for the
remote-controlled default-branch fetch) into the shared
git/ref-name.ts validator so both call sites share one allowlist, and used
it to validate the requested branch too.

Author: pedrofrxncx

packages/sandbox/daemon/git/ref-name.test.ts

@@ -21,6 +21,7 @@ describe("assertValidRemoteBranchName", () => {
       "main/",
       "main.lock",
       "has space",
+      "a//b",
     ]) {
       expect(() => assertValidRemoteBranchName(name)).toThrow(
         InvalidRemoteBranchNameError,

packages/sandbox/daemon/git/ref-name.ts

@@ -8,17 +8,23 @@ export class InvalidRemoteBranchNameError extends Error {
   }
 }
 
+/** Whether `name` is safe to interpolate as a git CLI branch/ref argument. */
+export function isValidRemoteBranchName(name: string): boolean {
+  return (
+    !!name &&
+    name.length <= 255 &&
+    !name.includes("..") &&
+    !name.includes("//") &&
+    !name.startsWith("/") &&
+    !name.endsWith("/") &&
+    !name.endsWith(".lock") &&
+    REMOTE_BRANCH_NAME.test(name)
+  );
+}
+
 /** Validates a remote branch name before passing it as a git CLI argument. */
 export function assertValidRemoteBranchName(name: string): void {
-  if (
-    !name ||
-    name.length > 255 ||
-    name.includes("..") ||
-    name.startsWith("/") ||
-    name.endsWith("/") ||
-    name.endsWith(".lock") ||
-    !REMOTE_BRANCH_NAME.test(name)
-  ) {
+  if (!isValidRemoteBranchName(name)) {
     throw new InvalidRemoteBranchNameError(name);
   }
 }

packages/sandbox/daemon/setup/clone.test.ts

@@ -11,7 +11,7 @@ import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { computeBranchDivergence } from "../git/branch-divergence";
 import type { Config } from "../types";
-import { isSafeRefName, spawnClone } from "./clone";
+import { spawnClone } from "./clone";
 
 /**
  * Creates a bare "origin" git repo with two branches:
@@ -91,42 +91,6 @@ function makeConfig(
   } as unknown as Config;
 }
 
-describe("isSafeRefName", () => {
-  it("accepts real branch names", () => {
-    for (const name of [
-      "main",
-      "master",
-      "feature/x",
-      "release/1.0",
-      "feat-2.0_a",
-      "user/fix.bug",
-    ]) {
-      expect(isSafeRefName(name)).toBe(true);
-    }
-  });
-
-  it("rejects shell metacharacters and ref-format edge cases", () => {
-    for (const name of [
-      "x;whoami",
-      "a$(id)b",
-      "a`id`b",
-      "a|b",
-      "a&b",
-      "a>b",
-      "a b",
-      "-x",
-      "/x",
-      "x/",
-      "a..b",
-      "a//b",
-      "x.lock",
-      "",
-    ]) {
-      expect(isSafeRefName(name)).toBe(false);
-    }
-  });
-});
-
 describe("spawnClone", () => {
   it("clones the target branch directly when it exists on remote", async () => {
     const { url, root, cleanup } = setupBareRepo();
@@ -248,6 +212,26 @@ describe("spawnClone", () => {
     }
   }, 30_000);
 
+  it("rejects a malicious requested branch instead of shelling it out", async () => {
+    const { url, root, cleanup } = setupBareRepo();
+    try {
+      const repoDir = join(root, "workspace");
+      // `branch` comes from tenant-supplied config and is interpolated into
+      // `sh -c` git commands (ls-remote/clone/checkout) before any other
+      // check runs — an unsafe name must be rejected before it ever reaches
+      // a shell string instead of executing as a second command.
+      const marker = join(root, "INJECTED");
+      const { code } = await spawnClone({
+        config: makeConfig(repoDir, url, `x;touch ${marker}`),
+        onChunk: () => {},
+      });
+      expect(code).not.toBe(0);
+      expect(existsSync(marker)).toBe(false);
+    } finally {
+      cleanup();
+    }
+  }, 30_000);
+
   it("clones default and forks a local branch when target is missing on remote", async () => {
     const { url, root, cleanup } = setupBareRepo();
     try {

packages/sandbox/daemon/setup/clone.ts

@@ -2,6 +2,7 @@ import { sleep } from "@decocms/std";
 import { existsSync, readdirSync } from "node:fs";
 import { isAbsolute } from "node:path";
 import { isSyntheticBranch } from "../constants";
+import { isValidRemoteBranchName } from "../git/ref-name";
 import type { Config } from "../types";
 import { spawnSetupStep } from "./spawn-step";
 
@@ -97,26 +98,6 @@ async function runNetworkStep(cmd: string, deps: CloneDeps): Promise<number> {
 // through to a fork-from-default fallback.
 const LS_REMOTE_NO_MATCH = 2;
 
-/**
- * Conservative ref-name allowlist. `base` below is derived from
- * remote-controlled `ls-remote` output and interpolated into `sh -c` git
- * commands; git permits shell metacharacters (`;`, `$(…)`, backticks, `|`, …)
- * in branch names, so an untrusted remote whose HEAD points at a maliciously
- * named default branch could inject commands. Restrict to characters that
- * appear in real branch names and reject the ref-format edge cases.
- */
-export function isSafeRefName(name: string): boolean {
-  return (
-    /^[A-Za-z0-9._/-]+$/.test(name) &&
-    !name.startsWith("-") &&
-    !name.startsWith("/") &&
-    !name.endsWith("/") &&
-    !name.endsWith(".lock") &&
-    !name.includes("..") &&
-    !name.includes("//")
-  );
-}
-
 /** Like runNetworkStep but also returns the (merged stdout+stderr) output. */
 async function runNetworkStepCapture(
   cmd: string,
@@ -180,7 +161,7 @@ async function fetchBaseBranch(
   // into `sh -c` git commands below — reject anything outside a safe ref-name
   // charset to close the command-injection vector (git allows `;`, `$(…)`,
   // backticks, etc. in branch names).
-  if (!isSafeRefName(base)) {
+  if (!isValidRemoteBranchName(base)) {
     deps.onChunk(
       "setup",
       `\r\n[clone] warning: refusing unsafe base branch name ${JSON.stringify(base)}; divergence vs base unavailable until next fetch\r\n`,
@@ -266,6 +247,17 @@ export async function spawnClone(deps: CloneDeps): Promise<CloneResult> {
   let branchOnRemote: string | null = null;
   let branchToForkLocally: string | null = null;
   if (branch) {
+    // `branch` is config-supplied and interpolated into `sh -c` git commands
+    // below (ls-remote/clone/checkout) — same command-injection vector as
+    // `base` above (git permits shell metacharacters in ref names), so
+    // validate before it ever reaches a shell string.
+    if (!isValidRemoteBranchName(branch)) {
+      deps.onChunk(
+        "setup",
+        `\r\n[clone] refusing invalid branch name ${JSON.stringify(branch)}\r\n`,
+      );
+      return { code: 1 };
+    }
     const probe = await runNetworkStep(
       `${gc} ls-remote --exit-code --heads ${cloneUrl} ${branch}`,
       deps,