diff --git a/packages/sandbox/daemon/git/ref-name.test.ts b/packages/sandbox/daemon/git/ref-name.test.ts index c969fadab1..81161c1bde 100644 --- a/packages/sandbox/daemon/git/ref-name.test.ts +++ b/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, diff --git a/packages/sandbox/daemon/git/ref-name.ts b/packages/sandbox/daemon/git/ref-name.ts index fdb7407332..6bfea33300 100644 --- a/packages/sandbox/daemon/git/ref-name.ts +++ b/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); } } diff --git a/packages/sandbox/daemon/setup/clone.test.ts b/packages/sandbox/daemon/setup/clone.test.ts index 656520f503..7b92172ee9 100644 --- a/packages/sandbox/daemon/setup/clone.test.ts +++ b/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 { diff --git a/packages/sandbox/daemon/setup/clone.ts b/packages/sandbox/daemon/setup/clone.ts index 0ab6a65deb..494d872c2f 100644 --- a/packages/sandbox/daemon/setup/clone.ts +++ b/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 { // 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 { 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,