From cf069f0501c28906c68e331ac53f4dadbcc5e0c0 Mon Sep 17 00:00:00 2001 From: Jaynel Patiarba Date: Tue, 11 Aug 2026 23:54:03 +0800 Subject: [PATCH] fix(cli): prevent shell command injection in deco-migrate clone/copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deco-migrate-cli.ts built `git clone` and `rsync` command strings by interpolating untrusted input (repo URL, --branch value, local source path) and ran them via execSync (/bin/sh -c). A branch like `main; touch pwned` or a URL like `https://…/$(touch pwned)` broke out of the intended command and executed arbitrary commands on the machine running the CLI (dev laptop / CI runner), with that operator's privileges and secrets. Fix: route every command whose arguments derive from untrusted input through a no-shell `runArgv` (spawnSync, shell:false), so each value is a discrete argv element and shell metacharacters are inert. The now-dead `run()` shell helper is removed; execSync remains only for the fixed, operator-controlled `diffAgainstRef` pipelines. Adds deco-migrate-cli.test.ts: unit asserts arg-builders keep injection strings as single literal argv elements, and an end-to-end test runs the real `git` with a malicious --branch/URL and asserts the injected marker file is never created. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../scripts/deco-migrate-cli.test.ts | 81 +++++++++++++ .../blocks-cli/scripts/deco-migrate-cli.ts | 108 ++++++++++++------ 2 files changed, 153 insertions(+), 36 deletions(-) create mode 100644 packages/blocks-cli/scripts/deco-migrate-cli.test.ts diff --git a/packages/blocks-cli/scripts/deco-migrate-cli.test.ts b/packages/blocks-cli/scripts/deco-migrate-cli.test.ts new file mode 100644 index 00000000..9cdb0c01 --- /dev/null +++ b/packages/blocks-cli/scripts/deco-migrate-cli.test.ts @@ -0,0 +1,81 @@ +import { spawnSync } from "node:child_process"; +import { existsSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { buildCloneArgs, buildRsyncArgs } from "./deco-migrate-cli"; + +// Regression guard for the git-clone / rsync command-injection fix. +// Untrusted input (repo URL, --branch value, local source path) used to be +// glued into a shell string handed to `execSync` (`/bin/sh -c`), so a value +// like `main; touch pwned` or `$(touch pwned)` executed as a second command. +// The fix passes every value as a discrete argv element via `spawnSync` with +// no shell, making metacharacters inert. + +describe("buildCloneArgs", () => { + it("keeps a malicious branch as ONE literal argv element (not split)", () => { + const args = buildCloneArgs("https://github.com/org/site", "dest", "main; touch pwned"); + // The whole injection string is a single element, immediately after --branch. + expect(args).toEqual([ + "clone", + "--depth", + "1", + "--branch", + "main; touch pwned", + "https://github.com/org/site", + "dest", + ]); + // No element was fractured on `;` or whitespace. + expect(args.some((a) => a === "touch" || a === "pwned")).toBe(false); + }); + + it("keeps a command-substitution URL as one literal argv element", () => { + const args = buildCloneArgs("https://github.com/org/$(touch pwned)", "dest", null); + expect(args).toContain("https://github.com/org/$(touch pwned)"); + expect(args).not.toContain("--branch"); + }); +}); + +describe("buildRsyncArgs", () => { + it("keeps a malicious source path as one literal argv element", () => { + const args = buildRsyncArgs("/tmp/$(touch pwned)", "dest"); + expect(args).toContain("/tmp/$(touch pwned)/"); + expect(args).toContain("dest/"); + }); +}); + +describe("git clone runs without a shell (end-to-end injection is inert)", () => { + let dir: string; + beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), "deco-migrate-injtest-")); + }); + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + it("does NOT execute an injected command in the --branch value", () => { + const markerName = "pwned.txt"; + const marker = path.join(dir, markerName); + // A non-existent local source so the clone fails fast; the point is the + // injected `touch` in the branch must NEVER run. + const args = buildCloneArgs( + path.join(dir, "no-such-repo"), + path.join(dir, "out"), + `main; touch ${markerName}`, + ); + const result = spawnSync("git", args, { cwd: dir, shell: false, encoding: "utf8" }); + + expect(result.status).not.toBe(0); // clone failed, as expected + expect(existsSync(marker)).toBe(false); // injected command did NOT run + }); + + it("does NOT execute a command-substitution injection in the source URL", () => { + const markerName = "pwned2.txt"; + const marker = path.join(dir, markerName); + const args = buildCloneArgs(`file:///$(touch ${markerName})`, path.join(dir, "out2"), null); + const result = spawnSync("git", args, { cwd: dir, shell: false, encoding: "utf8" }); + + expect(result.status).not.toBe(0); + expect(existsSync(marker)).toBe(false); + }); +}); diff --git a/packages/blocks-cli/scripts/deco-migrate-cli.ts b/packages/blocks-cli/scripts/deco-migrate-cli.ts index 6d6d4e87..8977a84b 100644 --- a/packages/blocks-cli/scripts/deco-migrate-cli.ts +++ b/packages/blocks-cli/scripts/deco-migrate-cli.ts @@ -152,58 +152,81 @@ function extractRepoName(source: string): string { return base || "site"; } -function run(cmd: string, cwd?: string, label?: string): boolean { - if (label) console.log(` ${dim("$")} ${dim(cmd)}`); - try { - execSync(cmd, { - cwd, - stdio: label ? "pipe" : "inherit", - timeout: 120_000, - }); +/** + * Run an external program WITHOUT a shell — `file` + `args` are passed straight + * to execve, so no argument is ever re-parsed for `;`, `|`, `$(...)`, backticks, + * whitespace, etc. Every command whose arguments derive from untrusted input + * (repo URLs, branch names, source paths) MUST go through here, never through a + * shell-interpolated string. + */ +function runArgv(file: string, args: string[], cwd?: string, label?: string): boolean { + if (label) console.log(` ${dim("$")} ${dim([file, ...args].join(" "))}`); + const result = spawnSync(file, args, { + cwd, + stdio: label ? "pipe" : "inherit", + timeout: 120_000, + shell: false, // explicit: never interpret arguments through a shell + }); + if (result.status === 0) { if (label) console.log(` ${icons.success} ${label}`); return true; - } catch (e: any) { - if (label) { - console.log(` ${icons.error} ${label}: ${e.message?.split("\n")[0] || "failed"}`); - } - return false; } + if (label) { + const msg = result.error?.message || result.stderr?.toString().split("\n")[0] || "failed"; + console.log(` ${icons.error} ${label}: ${msg}`); + } + return false; +} + +/** + * Build the argv for `git clone` as a discrete array. `source`, `dest`, and + * `branch` become individual argv elements, so shell metacharacters in any of + * them are inert — git receives them as literal, single arguments. Exported for + * the injection regression test. + */ +export function buildCloneArgs(source: string, dest: string, branch: string | null): string[] { + const args = ["clone", "--depth", "1"]; + if (branch) args.push("--branch", branch); + args.push(source, dest); + return args; +} + +/** Build the argv for the `rsync` copy. See buildCloneArgs for the rationale. */ +export function buildRsyncArgs(source: string, dest: string): string[] { + return [ + "-a", + "--exclude=.git", + "--exclude=node_modules", + "--exclude=_fresh", + "--exclude=.wrangler", + `${source}/`, + `${dest}/`, + ]; } function cloneRepo(source: string, dest: string, branch: string | null): boolean { console.log(`\n Cloning ${cyan(source)}...`); - const branchArg = branch ? ` --branch ${branch}` : ""; - const depthArg = " --depth 1"; - const ok = run( - `git clone${depthArg}${branchArg} "${source}" "${dest}"`, - undefined, - "Clone repository", - ); + const ok = runArgv("git", buildCloneArgs(source, dest, branch), undefined, "Clone repository"); if (!ok) return false; // Strip remote to prevent accidental pushes - run(`git remote remove origin`, dest, "Remove git remote"); + runArgv("git", ["remote", "remove", "origin"], dest, "Remove git remote"); return true; } function copyLocal(source: string, dest: string): boolean { console.log(`\n Copying ${cyan(source)} → ${cyan(dest)}...`); - try { - // Use cp -r, excluding .git and node_modules - execSync( - `rsync -a --exclude='.git' --exclude='node_modules' --exclude='_fresh' --exclude='.wrangler' "${source}/" "${dest}/"`, - { stdio: "pipe", timeout: 120_000 }, - ); - console.log(` ${icons.success} Copied source directory`); - - // Init fresh git so the migration has a clean baseline - run(`git init`, dest); - run(`git add -A && git commit -m "pre-migration snapshot" --allow-empty`, dest); - return true; - } catch (e: any) { - console.log(` ${icons.error} Copy failed: ${e.message?.split("\n")[0]}`); + const copied = runArgv("rsync", buildRsyncArgs(source, dest), undefined, "Copy source directory"); + if (!copied) { + console.log(` ${icons.error} Copy failed`); return false; } + + // Init fresh git so the migration has a clean baseline + runArgv("git", ["init"], dest); + runArgv("git", ["add", "-A"], dest); + runArgv("git", ["commit", "-m", "pre-migration snapshot", "--allow-empty"], dest); + return true; } function runMigration( @@ -441,4 +464,17 @@ async function main() { console.log(""); } -main(); +/** True when this file is the process entry point (invoked directly, not imported). */ +function isMainModule(): boolean { + const entry = process.argv[1]; + if (!entry) return false; + try { + return import.meta.url === new URL(`file://${path.resolve(entry)}`).href; + } catch { + return false; + } +} + +if (isMainModule()) { + main(); +}