From 7e84ebd628cf6a05718836504e2734879731d5eb Mon Sep 17 00:00:00 2001 From: Jikun Date: Wed, 14 Oct 2026 20:24:42 +0800 Subject: [PATCH] fix: prevent command injection from funcArgs by spawning func without shell --- src/cli/commands/start/start.spec.ts | 135 +++++++++++++++++++++++++++ src/cli/commands/start/start.ts | 67 ++++++++++++- src/core/utils/cli.spec.ts | 56 ++++++++++- src/core/utils/cli.ts | 92 ++++++++++++++++++ 4 files changed, 345 insertions(+), 5 deletions(-) create mode 100644 src/cli/commands/start/start.spec.ts diff --git a/src/cli/commands/start/start.spec.ts b/src/cli/commands/start/start.spec.ts new file mode 100644 index 000000000..1080cea46 --- /dev/null +++ b/src/cli/commands/start/start.spec.ts @@ -0,0 +1,135 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import type { ConcurrentlyCommandInput, ConcurrentlyOptions, ConcurrentlyResult } from "concurrently"; +import { EventEmitter } from "node:events"; + +const concurrentlyMock = vi.fn(); +const spawnMock = vi.fn(); + +vi.mock("concurrently", () => ({ + concurrently: concurrentlyMock, +})); + +vi.mock("node:child_process", () => ({ + spawn: spawnMock, +})); + +vi.mock("../../../core/utils/logger.js", () => ({ + logger: { + silly: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + log: vi.fn(), + error: vi.fn(), + }, +})); + +vi.mock("../../../core/utils/net.js", () => ({ + isAcceptingTcpConnections: vi.fn().mockResolvedValue(4280), + askNewPort: vi.fn(), + parseUrl: vi.fn(), +})); + +vi.mock("../../../core/func-core-tools.js", () => ({ + getCoreToolsBinary: vi.fn().mockResolvedValue("func"), + getNodeMajorVersion: vi.fn().mockReturnValue(20), + detectTargetCoreToolsVersion: vi.fn().mockReturnValue(4), + isCoreToolsVersionCompatible: vi.fn().mockReturnValue(true), +})); + +vi.mock("../../../core/dataApiBuilder/index.js", () => ({ + getDataApiBuilderBinaryPath: vi.fn(), +})); + +vi.mock("../../../core/utils/json.js", () => ({ + loadPackageJson: vi.fn().mockReturnValue({ version: "test" }), +})); + +function createFakeChildProcess() { + const child = new EventEmitter() as any; + child.pid = 12345; + child.stdin = null; + child.stdout = null; + child.stderr = null; + return child; +} + +describe("swa start command execution safety", () => { + let tempRoot: string; + let originalExit: typeof process.exit; + + beforeEach(() => { + concurrentlyMock.mockReset(); + spawnMock.mockReset(); + spawnMock.mockReturnValue(createFakeChildProcess()); + + tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "swa-start-spec-")); + fs.mkdirSync(path.join(tempRoot, "api"), { recursive: true }); + fs.mkdirSync(path.join(tempRoot, "app"), { recursive: true }); + + originalExit = process.exit; + process.exit = vi.fn() as any; + }); + + afterEach(() => { + process.exit = originalExit; + fs.rmSync(tempRoot, { recursive: true, force: true }); + }); + + it("should spawn Functions with argv and shell disabled for funcArgs", async () => { + let capturedCommands: ConcurrentlyCommandInput[] = []; + let capturedOptions: Partial = {}; + + concurrentlyMock.mockImplementation((commands: ConcurrentlyCommandInput[], options: Partial) => { + capturedCommands = commands; + capturedOptions = options; + const result: ConcurrentlyResult = { + commands: [] as any, + result: Promise.resolve([ + { + command: { name: "swa", command: "node server.js" }, + killed: true, + index: 0, + exitCode: 0, + timings: { startDate: new Date(), endDate: new Date(), durationSeconds: 0 }, + }, + ] as any), + }; + return result; + }); + + const { start } = await import("./start.js"); + await start({ + appLocation: path.join(tempRoot, "app"), + outputLocation: ".", + apiLocation: path.join(tempRoot, "api"), + apiPort: 7071, + host: "localhost", + port: 4280, + devserverTimeout: 60, + ssl: false, + open: false, + swaConfigLocation: path.join(tempRoot, "app"), + funcArgs: "| (echo vulnerable>swa-msrc-144619-windows.marker)", + } as SWACLIConfig); + + const apiCommand = (capturedCommands as any[]).find((command) => command.name === "api")?.command as string; + expect(apiCommand).toContain("__SWA_INTERNAL_FUNC_START__:"); + expect(apiCommand).not.toContain("vulnerable"); + expect(capturedOptions.spawn).toBeTypeOf("function"); + + (capturedOptions.spawn as any)(apiCommand, { env: {} }); + + expect(spawnMock).toHaveBeenCalledTimes(1); + expect(spawnMock).toHaveBeenCalledWith( + "func", + ["start", "--cors", "*", "--port", "7071", "|", "(echo", "vulnerable>swa-msrc-144619-windows.marker)"], + expect.objectContaining({ + shell: false, + windowsVerbatimArguments: false, + cwd: path.join(tempRoot, "api"), + }), + ); + }); +}); diff --git a/src/cli/commands/start/start.ts b/src/cli/commands/start/start.ts index c3552f2c7..ccee3a83c 100644 --- a/src/cli/commands/start/start.ts +++ b/src/cli/commands/start/start.ts @@ -1,11 +1,12 @@ import { concurrently, CloseEvent, ConcurrentlyOptions } from "concurrently"; +import { spawn, SpawnOptions } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { DEFAULT_CONFIG } from "../../../config.js"; import { askNewPort, isAcceptingTcpConnections, parseUrl } from "../../../core/utils/net.js"; import { logger } from "../../../core/utils/logger.js"; -import { createStartupScriptCommand } from "../../../core/utils/cli.js"; +import { createStartupScriptCommand, parseCommandLineArgs } from "../../../core/utils/cli.js"; import { readWorkflowFile } from "../../../core/utils/workflow-config.js"; import { getNodeMajorVersion, @@ -26,6 +27,13 @@ const __dirname = path.dirname(__filename); // This is the path to ./dist/msha/server.js const mshaPath = path.resolve(__dirname, "../../../../dist/msha/server.js"); +const INTERNAL_FUNC_START_PREFIX = "__SWA_INTERNAL_FUNC_START__:"; + +type InternalFuncStartPayload = { + binary: string; + args: string[]; + cwd: string; +}; // import { createRequire } from "node:module"; // const require = createRequire(import.meta.url); @@ -167,6 +175,7 @@ export async function start(options: SWACLIConfig) { } const isApiLocationExistsOnDisk = fs.existsSync(userWorkflowConfig?.apiLocation!); + let apiFuncPayload: InternalFuncStartPayload | undefined = undefined; // handle the API location config let serveApiCommand = "echo 'No API found. Skipping'"; @@ -203,7 +212,20 @@ export async function start(options: SWACLIConfig) { // serve the api if and only if the user provides a folder via the --api-location flag if (isApiLocationExistsOnDisk) { - serveApiCommand = `cd "${userWorkflowConfig.apiLocation}" && "${funcBinary}" start --cors "*" --port ${apiPort} ${funcArgs ?? ""}`; + let parsedFuncArgs: string[] = []; + try { + parsedFuncArgs = parseCommandLineArgs(funcArgs); + } catch (error: unknown) { + logger.error((error as Error).message, true); + } + + apiFuncPayload = { + binary: funcBinary, + cwd: userWorkflowConfig.apiLocation, + args: ["start", "--cors", "*", "--port", `${apiPort}`, ...parsedFuncArgs], + }; + + serveApiCommand = `${INTERNAL_FUNC_START_PREFIX}${Buffer.from(JSON.stringify(apiFuncPayload)).toString("base64url")}`; } } } @@ -344,13 +366,36 @@ export async function start(options: SWACLIConfig) { env: envVarsObj, commands: { swa: concurrentlyCommands.find((c) => c.name === "swa")?.command, - api: concurrentlyCommands.find((c) => c.name === "api")?.command, + api: apiFuncPayload + ? `"${apiFuncPayload.binary}" ${apiFuncPayload.args.join(" ")} (cwd: ${apiFuncPayload.cwd})` + : concurrentlyCommands.find((c) => c.name === "api")?.command, dataApi: concurrentlyCommands.find((c) => c.name == "dataApi")?.command, run: concurrentlyCommands.find((c) => c.name === "run")?.command, }, }); - const concurrentlyOptions: Partial = { restartTries: 0, killOthers: ["failure", "success"], raw: true }; + const concurrentlyOptions: Partial = { + restartTries: 0, + killOthers: ["failure", "success"], + raw: true, + spawn: (command: string, options: SpawnOptions) => { + if (command.startsWith(INTERNAL_FUNC_START_PREFIX)) { + const encodedPayload = command.slice(INTERNAL_FUNC_START_PREFIX.length); + const payload = JSON.parse(Buffer.from(encodedPayload, "base64url").toString("utf8")) as InternalFuncStartPayload; + const env = Object.fromEntries(Object.entries(options.env || {}).map(([key, value]) => [key, value == null ? undefined : String(value)])); + + return spawn(payload.binary, payload.args, { + ...options, + cwd: payload.cwd, + env, + shell: false, + windowsVerbatimArguments: false, + }); + } + + return spawnWithShell(command, options); + }, + }; const { result } = concurrently(concurrentlyCommands, concurrentlyOptions); await result @@ -387,3 +432,17 @@ export async function start(options: SWACLIConfig) { logger.error(err.message, true); }); } + +function spawnWithShell(command: string, options: SpawnOptions) { + const shellOptions: SpawnOptions = { ...options }; + + if (process.platform === "win32") { + const shell = process.env.comspec || "cmd.exe"; + return spawn(shell, ["/d", "/s", "/c", `"${command}"`], { + ...shellOptions, + windowsVerbatimArguments: true, + }); + } + + return spawn("/bin/sh", ["-c", command], shellOptions); +} diff --git a/src/core/utils/cli.spec.ts b/src/core/utils/cli.spec.ts index 45b3c1bcf..9cd65e421 100644 --- a/src/core/utils/cli.spec.ts +++ b/src/core/utils/cli.spec.ts @@ -4,7 +4,7 @@ vi.mock("../constants", () => { }); import path from "node:path"; import { vol } from "memfs"; -import { argv, createStartupScriptCommand, parseServerTimeout } from "./cli.js"; +import { argv, createStartupScriptCommand, parseCommandLineArgs, parseServerTimeout } from "./cli.js"; import { logger } from "./logger.js"; describe("argv()", () => { @@ -64,6 +64,60 @@ describe("createStartupScriptCommand()", () => { expect(cmd).toBe("npm run start#foo --if-present"); }); }); + + describe("parseCommandLineArgs()", () => { + it("should return empty array for empty input", () => { + expect(parseCommandLineArgs()).toStrictEqual([]); + expect(parseCommandLineArgs(" ")).toStrictEqual([]); + }); + + it("should split regular arguments", () => { + expect(parseCommandLineArgs("--verbose --port 7071")).toStrictEqual(["--verbose", "--port", "7071"]); + }); + + it("should preserve quoted arguments", () => { + expect(parseCommandLineArgs('--name "hello world"')).toStrictEqual(["--name", "hello world"]); + expect(parseCommandLineArgs("--name 'hello world'")).toStrictEqual(["--name", "hello world"]); + }); + + it("should preserve shell metacharacters as literal values", () => { + expect(parseCommandLineArgs("| (echo vulnerable>swa-msrc-144619-windows.marker)")).toStrictEqual([ + "|", + "(echo", + "vulnerable>swa-msrc-144619-windows.marker)", + ]); + expect(parseCommandLineArgs("--flag && whoami > out.txt")).toStrictEqual(["--flag", "&&", "whoami", ">", "out.txt"]); + }); + + it("should throw for unbalanced quotes", () => { + expect(() => parseCommandLineArgs('--name "hello')).toThrow("Unbalanced quotes in --func-args."); + }); + + it("should preserve mixed quote content", () => { + expect(parseCommandLineArgs(`--msg "a 'b' c"`)).toStrictEqual(["--msg", "a 'b' c"]); + expect(parseCommandLineArgs(`--msg 'a "b" c'`)).toStrictEqual(["--msg", `a "b" c`]); + }); + + it("should preserve escaped whitespace and trailing backslash", () => { + expect(parseCommandLineArgs(String.raw`--title hello\ world`)).toStrictEqual(["--title", "hello world"]); + expect(parseCommandLineArgs("--path C:\\tmp\\")).toStrictEqual(["--path", "C:tmp\\"]); + }); + + it("should preserve windows paths in quoted values", () => { + expect(parseCommandLineArgs(`--script "C:\\Program Files\\my app\\start.js"`)).toStrictEqual([ + "--script", + `C:\\Program Files\\my app\\start.js`, + ]); + }); + + it("should keep option terminator as a literal token", () => { + expect(parseCommandLineArgs(`-- --literal --not-option`)).toStrictEqual(["--", "--literal", "--not-option"]); + }); + + it("should treat all whitespace as separators when not quoted", () => { + expect(parseCommandLineArgs("--a 1\t--b 2")).toStrictEqual(["--a", "1", "--b", "2"]); + }); + }); describe("yarn", () => { it("should parse yarn scripts (simple)", () => { const cmd = createStartupScriptCommand("yarn:start", {}); diff --git a/src/core/utils/cli.ts b/src/core/utils/cli.ts index 202ea47af..5ec0230b3 100644 --- a/src/core/utils/cli.ts +++ b/src/core/utils/cli.ts @@ -119,3 +119,95 @@ export function parseServerTimeout(time: string) { } return timeValue; } + +/** + * Split a shell-like argument string into argv tokens without executing shell syntax. + * Supports spaces, single/double quotes, and backslash escaping. + * Throws when quotes are not balanced. + */ +export function parseCommandLineArgs(input?: string): string[] { + if (!input?.trim()) { + return []; + } + + const args: string[] = []; + let current = ""; + let quote: "'" | '"' | null = null; + let escaping = false; + let tokenStarted = false; + + const pushToken = () => { + if (tokenStarted) { + args.push(current); + current = ""; + tokenStarted = false; + } + }; + + for (let index = 0; index < input.length; index++) { + const char = input[index]; + + if (escaping) { + current += char; + tokenStarted = true; + escaping = false; + continue; + } + + if (char === "\\") { + if (quote === "'") { + current += char; + tokenStarted = true; + } else if (quote === '"') { + const nextChar = input[index + 1]; + if (nextChar === '"' || nextChar === "\\") { + current += nextChar; + tokenStarted = true; + index++; + } else { + current += char; + tokenStarted = true; + } + } else { + escaping = true; + tokenStarted = true; + } + continue; + } + + if (quote) { + if (char === quote) { + quote = null; + } else { + current += char; + } + tokenStarted = true; + continue; + } + + if (char === "'" || char === '"') { + quote = char; + tokenStarted = true; + continue; + } + + if (/\s/.test(char)) { + pushToken(); + continue; + } + + current += char; + tokenStarted = true; + } + + if (escaping) { + current += "\\"; + } + + if (quote) { + throw new Error(`Unbalanced quotes in --func-args.`); + } + + pushToken(); + return args; +}