fix(security): quote user-controlled docker build args in getDockerCommand

apps/dokploy/__test__/deploy/docker-build-injection.test.ts

@@ -1,7 +1,9 @@
 import { execSync } from "node:child_process";
-import { existsSync, rmSync } from "node:fs";
+import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs";
+import path from "node:path";
+import { getDockerCommand } from "@dokploy/server/utils/builders/docker-file";
 import { parse, quote } from "shell-quote";
-import { describe, expect, it } from "vitest";
+import { afterEach, describe, expect, it } from "vitest";
 
 // Reproduces the escaping applied at the docker build/pull sinks and asserts no
 // payload can break out of the command. `docker`/`cd` are replaced by `:` so the
@@ -64,3 +66,139 @@ describe("docker build/pull command injection", () => {
 		expect(parse(quote(["dist/static"]))).toEqual(["dist/static"]);
 	});
 });
+
+describe("getDockerCommand full-string injection (real builder)", () => {
+	// Generate the real `getDockerCommand(`) output, neutralise the
+	// `docker build` invocation to a shell no-op (`:`) so real docker never
+	// runs, then execute the whole script through /bin/sh. Every other shell
+	// construct — the `cd` guard, `echo`, `|| { … }` error handling,
+	// metacharacter parsing — runs verbatim. The test passes only if no
+	// injected payload can create a marker file on disk, i.e. no
+	// user-controlled field breaks out of the `docker build` argument list.
+	const APP_NAME = `injapp_${process.pid}`;
+	const codeDir = path.join(
+		process.cwd(),
+		".docker",
+		"applications",
+		APP_NAME,
+		"code",
+	);
+	// Slash-free marker name so payloads can stay slash-free, mirroring the
+	// real attack constraint that a literal `/` would trip the `cd` guard.
+	const MARK_NAME = `pwned_df_${process.pid}`;
+
+	const baseApp = (overrides: Record<string, unknown> = {}) =>
+		({
+			appName: APP_NAME,
+			env: null,
+			publishDirectory: null,
+			buildArgs: null,
+			buildSecrets: null,
+			dockerBuildStage: null,
+			cleanCache: false,
+			createEnvFile: false,
+			sourceType: "github",
+			buildType: "dockerfile",
+			buildPath: "",
+			dockerfile: "Dockerfile",
+			dockerContextPath: null,
+			serverId: null,
+			buildServerId: null,
+			gitlabBuildPath: null,
+			bitbucketBuildPath: null,
+			giteaBuildPath: null,
+			dropBuildPath: null,
+			customGitBuildPath: null,
+			environment: { project: { env: null }, env: null },
+			...overrides,
+		}) as any;
+
+	const ensureCodeDir = () => {
+		mkdirSync(codeDir, { recursive: true });
+		writeFileSync(path.join(codeDir, "Dockerfile"), "FROM alpine\n");
+	};
+
+	// Replace only the `docker build -t` token with `:` (a shell no-op) so the
+	// args are parsed by the shell but docker is never invoked. `:` ignores
+	// its args and returns 0, so the `|| { …; exit 1; }` handler does not fire
+	// — any marker that appears must have come from an injected
+	// `;`/`|`/`$()`/backtick breaking out of the `docker build` args.
+	const neutraliseDocker = (cmd: string) =>
+		cmd.replace("docker build -t", ": build -t");
+
+	const runAndCheckSafe = (app: any) => {
+		ensureCodeDir();
+		const mark = path.join(codeDir, MARK_NAME);
+		if (existsSync(mark)) rmSync(mark);
+		try {
+			execSync(neutraliseDocker(getDockerCommand(app)), {
+				shell: "/bin/sh",
+				stdio: "ignore",
+			});
+		} catch {
+			// The no-op stand-in and the `cd` guard may abort the script; only the
+			// marker matters.
+		}
+		const fired = existsSync(mark);
+		if (existsSync(mark)) rmSync(mark);
+		return !fired;
+	};
+
+	afterEach(() => {
+		rmSync(codeDir, { recursive: true, force: true });
+	});
+
+	it("dockerBuildStage (--target) single-field payload does not fire", () => {
+		const app = baseApp({ dockerBuildStage: `x; touch ${MARK_NAME}; #` });
+		expect(runAndCheckSafe(app)).toBe(true);
+	});
+
+	it("dockerfile (-f) slash-free payload does not fire", () => {
+		const app = baseApp({ dockerfile: `Dockerfile; touch ${MARK_NAME}; #` });
+		expect(runAndCheckSafe(app)).toBe(true);
+	});
+
+	it("dockerContextPath + dockerfile two-field payload does not fire", () => {
+		const app = baseApp({
+			dockerContextPath: ".",
+			dockerfile: `Dockerfile; touch ${MARK_NAME}; #`,
+		});
+		expect(runAndCheckSafe(app)).toBe(true);
+	});
+
+	it("customGitBuildPath (*BuildPath) two-field payload does not fire", () => {
+		const app = baseApp({
+			sourceType: "git",
+			customGitBuildPath: `seg; touch ${MARK_NAME}; #`,
+			dockerContextPath: ".",
+		});
+		expect(runAndCheckSafe(app)).toBe(true);
+	});
+
+	it("preserves legitimate build values as single tokens (no over-escaping)", () => {
+		ensureCodeDir();
+		const app = baseApp({
+			sourceType: "git",
+			customGitBuildPath: "repo/sub",
+			dockerfile: "Dockerfile.prod",
+			dockerBuildStage: "builder-node",
+			dockerContextPath: "apps/web",
+		});
+		const cmd = getDockerCommand(app);
+		expect(cmd).toContain("docker build -t");
+		// Legitimate values contain no shell metacharacters, so shell-quote
+		// leaves them untouched and the build still receives the original args.
+		expect(cmd).toContain("--target builder-node");
+		expect(cmd).toContain("Dockerfile.prod");
+		expect(cmd).toContain("repo/sub");
+		expect(cmd).toContain("apps/web");
+		// The build stage must round-trip through the shell as a single literal
+		// token, never as shell operators.
+		const buildLine =
+			cmd
+				.split("\n")
+				.find((l) => l.includes("docker build"))
+				?.trim() ?? "";
+		expect(parse(buildLine)).toContain("builder-node");
+	});
+});

packages/server/src/utils/builders/docker-file.ts

@@ -35,14 +35,14 @@ export const getDockerCommand = (application: ApplicationNested) => {
 		const commandArgs = [
 			"build",
 			"-t",
-			image,
+			quote([image]),
 			"-f",
-			dockerFilePath,
-			dockerContextPath,
+			quote([dockerFilePath]),
+			quote([dockerContextPath]),
 		];
 
 		if (dockerBuildStage) {
-			commandArgs.push("--target", dockerBuildStage);
+			commandArgs.push("--target", quote([dockerBuildStage]));
 		}
 
 		if (cleanCache) {