Merge pull request #5701 from Gabrielgvl/fix/registry-login-stdin

fix(security): pass registry passwords to docker login via stdin

apps/dokploy/__test__/registry/docker-login.test.ts

@@ -0,0 +1,205 @@
+import { EventEmitter } from "node:events";
+import { Duplex, PassThrough, Writable } from "node:stream";
+import { inspect } from "node:util";
+import { dockerLogin } from "@dokploy/server/services/registry";
+import { execAsyncRemote } from "@dokploy/server/utils/process/execAsync";
+import { parse } from "shell-quote";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+const mocks = vi.hoisted(() => ({
+	exec: vi.fn(),
+	execFile: vi.fn(),
+	remoteExec: vi.fn(),
+	end: vi.fn(),
+}));
+
+vi.mock("node:child_process", async () => {
+	const { promisify } = await import("node:util");
+	return {
+		exec: Object.assign(mocks.exec, {
+			[promisify.custom]: (command: string) =>
+				new Promise((resolve, reject) => {
+					mocks.exec(
+						command,
+						(error: Error | null, stdout: string, stderr: string) => {
+							if (error) reject(Object.assign(error, { stdout, stderr }));
+							else resolve({ stdout, stderr });
+						},
+					);
+				}),
+		}),
+		execFile: mocks.execFile,
+	};
+});
+vi.mock("@dokploy/server/services/server", () => ({
+	findServerById: vi.fn(async () => ({ sshKeyId: "test-key" })),
+}));
+vi.mock("ssh2", async () => {
+	const { EventEmitter } = await import("node:events");
+	return {
+		Client: class extends EventEmitter {
+			exec = mocks.remoteExec;
+			end = mocks.end;
+			connect() {
+				queueMicrotask(() => this.emit("ready"));
+				return this;
+			}
+		},
+	};
+});
+
+const password = "fixture'\";$(false)\nsecret";
+const registryUrl = "registry.example.com; false";
+const username = "user'$(false)";
+let localInput: string;
+let remoteInput: string;
+let localStdin: Writable;
+let remoteStream: Duplex & { stderr: PassThrough };
+let exitCode: number;
+let stdout: string;
+let stderr: string;
+
+beforeEach(() => {
+	vi.clearAllMocks();
+	localInput = "";
+	remoteInput = "";
+	exitCode = 0;
+	stdout = "Login Succeeded";
+	stderr = "";
+
+	mocks.exec.mockImplementation((command, ...args) => {
+		const callback = args.at(-1);
+		queueMicrotask(() => {
+			callback(
+				exitCode === 0
+					? null
+					: Object.assign(new Error(`Command failed: ${command}`), {
+							code: exitCode,
+						}),
+				stdout,
+				stderr,
+			);
+		});
+	});
+	mocks.execFile.mockImplementation(() => {
+		const child = Object.assign(new EventEmitter(), {
+			stdout: new PassThrough(),
+			stderr: new PassThrough(),
+			stdin: new Writable({
+				write(chunk, _encoding, callback) {
+					localInput += chunk.toString();
+					callback();
+				},
+			}),
+		});
+		localStdin = child.stdin;
+		queueMicrotask(() => {
+			child.stdout.write(stdout);
+			child.stderr.write(stderr);
+			child.emit("close", exitCode);
+		});
+		return child;
+	});
+	mocks.remoteExec.mockImplementation((_command, callback) => {
+		remoteStream = Object.assign(
+			new Duplex({
+				read() {},
+				write(chunk, _encoding, done) {
+					remoteInput += chunk.toString();
+					done();
+				},
+			}),
+			{ stderr: new PassThrough() },
+		);
+		callback(null, remoteStream);
+		queueMicrotask(() => {
+			remoteStream.emit("data", stdout);
+			remoteStream.stderr.write(stderr);
+			remoteStream.emit("close", exitCode);
+		});
+	});
+});
+
+describe("docker login credentials", () => {
+	it("passes local credentials only on stdin, without a shell", async () => {
+		await dockerLogin(registryUrl, username, password);
+
+		expect(mocks.exec).not.toHaveBeenCalled();
+		expect(mocks.execFile).toHaveBeenCalledExactlyOnceWith("docker", [
+			"login",
+			registryUrl,
+			"--username",
+			username,
+			"--password-stdin",
+		]);
+		expect(localInput).toBe(password);
+		expect(localStdin.writableEnded).toBe(true);
+	});
+
+	it("escapes remote arguments and sends the password only on stream stdin", async () => {
+		await dockerLogin(registryUrl, username, password, "test-server");
+
+		const command = mocks.remoteExec.mock.calls[0]?.[0];
+		expect(command).not.toContain(password);
+		expect(parse(command)).toEqual([
+			"docker",
+			"login",
+			registryUrl,
+			"--username",
+			username,
+			"--password-stdin",
+		]);
+		expect(remoteInput).toBe(password);
+		expect(remoteStream.writableEnded).toBe(true);
+		expect(mocks.end).toHaveBeenCalledOnce();
+	});
+
+	it("ends local stdin even for an empty password", async () => {
+		await dockerLogin("", "user", "");
+
+		expect(localInput).toBe("");
+		expect(localStdin.writableEnded).toBe(true);
+	});
+
+	it("keeps remote execution without input unchanged", async () => {
+		const onData = vi.fn();
+		stderr = "warning";
+
+		await expect(
+			execAsyncRemote("test-server", "echo hello", onData),
+		).resolves.toEqual({
+			stdout,
+			stderr,
+		});
+		expect(remoteStream.writableEnded).toBe(false);
+		expect(onData.mock.calls).toEqual([[stdout], [stderr]]);
+	});
+
+	it.each([undefined, "test-server"])(
+		"does not retain passwords echoed by a failed login on %s",
+		async (serverId) => {
+			exitCode = 1;
+			stdout = `stdout: ${password}`;
+			stderr = `denied: ${password}`;
+
+			const error = await dockerLogin(
+				registryUrl,
+				username,
+				password,
+				serverId,
+			).catch((error: unknown) => error);
+
+			expect(error).toBeInstanceOf(Error);
+			expect(error).toHaveProperty(
+				"message",
+				expect.not.stringContaining(password),
+			);
+			expect(inspect(error, { depth: null })).not.toContain(password);
+			expect(JSON.stringify(error)).not.toContain(
+				JSON.stringify(password).slice(1, -1),
+			);
+			expect(error).not.toHaveProperty("originalError");
+			expect(error).not.toHaveProperty("cause");
+		},
+	);
+});

apps/dokploy/server/api/routers/registry.ts

@@ -1,11 +1,9 @@
 import {
 	createRegistry,
-	execAsyncRemote,
-	execFileAsync,
+	dockerLogin,
 	findRegistryById,
 	IS_CLOUD,
 	removeRegistry,
-	safeDockerLoginCommand,
 	updateRegistry,
 } from "@dokploy/server";
 import { db } from "@dokploy/server/db";
@@ -105,14 +103,6 @@ export const registryRouter = createTRPCRouter({
 		.input(apiTestRegistry)
 		.mutation(async ({ input }) => {
 			try {
-				const args = [
-					"login",
-					input.registryUrl,
-					"--username",
-					input.username,
-					"--password-stdin",
-				];
-
 				if (IS_CLOUD && !input.serverId) {
 					throw new TRPCError({
 						code: "NOT_FOUND",
@@ -120,20 +110,12 @@ export const registryRouter = createTRPCRouter({
 					});
 				}
 
-				if (input.serverId && input.serverId !== "none") {
-					await execAsyncRemote(
-						input.serverId,
-						safeDockerLoginCommand(
-							input.registryUrl,
-							input.username,
-							input.password,
-						),
-					);
-				} else {
-					await execFileAsync("docker", args, {
-						input: Buffer.from(input.password).toString(),
-					});
-				}
+				await dockerLogin(
+					input.registryUrl,
+					input.username,
+					input.password,
+					input.serverId,
+				);
 
 				return true;
 			} catch (error) {
@@ -169,14 +151,6 @@ export const registryRouter = createTRPCRouter({
 					});
 				}
 
-				const args = [
-					"login",
-					registryData.registryUrl,
-					"--username",
-					registryData.username,
-					"--password-stdin",
-				];
-
 				if (IS_CLOUD && !input.serverId) {
 					throw new TRPCError({
 						code: "NOT_FOUND",
@@ -184,20 +158,12 @@ export const registryRouter = createTRPCRouter({
 					});
 				}
 
-				if (input.serverId && input.serverId !== "none") {
-					await execAsyncRemote(
-						input.serverId,
-						safeDockerLoginCommand(
-							registryData.registryUrl,
-							registryData.username,
-							registryData.password,
-						),
-					);
-				} else {
-					await execFileAsync("docker", args, {
-						input: Buffer.from(registryData.password).toString(),
-					});
-				}
+				await dockerLogin(
+					registryData.registryUrl,
+					registryData.username,
+					registryData.password,
+					input.serverId,
+				);
 
 				return true;
 			} catch (error) {

packages/server/src/services/registry.ts

@@ -3,6 +3,7 @@ import { type apiCreateRegistry, registry } from "@dokploy/server/db/schema";
 import {
 	execAsync,
 	execAsyncRemote,
+	execFileAsync,
 } from "@dokploy/server/utils/process/execAsync";
 import { TRPCError } from "@trpc/server";
 import { eq } from "drizzle-orm";
@@ -27,6 +28,32 @@ export function safeDockerLoginCommand(
 	return `printf %s ${escapedPassword} | docker login ${escapedRegistry} -u ${escapedUser} --password-stdin`;
 }
 
+export async function dockerLogin(
+	registryUrl: string | undefined,
+	username: string | undefined,
+	password: string | undefined,
+	serverId?: string | null,
+): Promise<void> {
+	const args = [
+		"login",
+		registryUrl ?? "",
+		"--username",
+		username ?? "",
+		"--password-stdin",
+	];
+	try {
+		if (serverId && serverId !== "none") {
+			const command = `docker login ${shEscape(registryUrl)} --username ${shEscape(username)} --password-stdin`;
+			await execAsyncRemote(serverId, command, undefined, password ?? "");
+		} else {
+			await execFileAsync("docker", args, { input: password ?? "" });
+		}
+	} catch (error) {
+		// Do not retain an execution error that may contain echoed credentials.
+		throw new Error(sanitizeRegistryError(error, password));
+	}
+}
+
 function sanitizeRegistryError(
 	error: unknown,
 	password: string | null | undefined,
@@ -64,16 +91,17 @@ export const createRegistry = async (
 				message: "Select a server to add the registry",
 			});
 		}
-		const loginCommand = safeDockerLoginCommand(
-			input.registryUrl,
-			input.username,
-			input.password,
-		);
 		try {
-			if (input.serverId && input.serverId !== "none") {
-				await execAsyncRemote(input.serverId, loginCommand);
-			} else if (newRegistry.registryType === "cloud") {
-				await execAsync(loginCommand);
+			if (
+				(input.serverId && input.serverId !== "none") ||
+				newRegistry.registryType === "cloud"
+			) {
+				await dockerLogin(
+					input.registryUrl,
+					input.username,
+					input.password,
+					input.serverId,
+				);
 			}
 		} catch (error) {
 			const sanitized = sanitizeRegistryError(error, input.password);
@@ -127,12 +155,6 @@ export const updateRegistry = async (
 			.returning()
 			.then((res) => res[0]);
 
-		const loginCommand = safeDockerLoginCommand(
-			response?.registryUrl,
-			response?.username,
-			response?.password,
-		);
-
 		if (
 			IS_CLOUD &&
 			!registryData?.serverId &&
@@ -145,10 +167,16 @@ export const updateRegistry = async (
 		}
 
 		try {
-			if (registryData?.serverId && registryData?.serverId !== "none") {
-				await execAsyncRemote(registryData.serverId, loginCommand);
-			} else if (response?.registryType === "cloud") {
-				await execAsync(loginCommand);
+			if (
+				(registryData.serverId && registryData.serverId !== "none") ||
+				response?.registryType === "cloud"
+			) {
+				await dockerLogin(
+					response?.registryUrl,
+					response?.username,
+					response?.password,
+					registryData.serverId,
+				);
 			}
 		} catch (execError) {
 			throw new Error(sanitizeRegistryError(execError, response?.password));

packages/server/src/services/rollbacks.ts

@@ -26,9 +26,9 @@ import { resolveServiceNetworks } from "./network";
 import type { Port } from "./port";
 import type { Project } from "./project";
 import {
+	dockerLogin,
 	findRegistryByIdWithCredentials,
 	type Registry,
-	safeDockerLoginCommand,
 } from "./registry";
 
 export const createRollback = async (
@@ -182,23 +182,6 @@ export const rollback = async (rollbackId: string) => {
 	);
 };
 
-const dockerLoginForRegistry = async (
-	registry: Registry,
-	serverId?: string | null,
-) => {
-	const loginCommand = safeDockerLoginCommand(
-		registry.registryUrl,
-		registry.username,
-		registry.password,
-	);
-
-	if (serverId) {
-		await execAsyncRemote(serverId, loginCommand);
-	} else {
-		await execAsync(loginCommand);
-	}
-};
-
 const rollbackApplication = async (
 	appName: string,
 	image: string,
@@ -225,7 +208,12 @@ const rollbackApplication = async (
 	// alone is not sufficient — Docker Swarm also relies on the daemon's
 	// cached credentials (~/.docker/config.json) to distribute auth to nodes.
 	if (rollbackRegistry) {
-		await dockerLoginForRegistry(rollbackRegistry, serverId);
+		await dockerLogin(
+			rollbackRegistry.registryUrl,
+			rollbackRegistry.username,
+			rollbackRegistry.password,
+			serverId,
+		);
 	}
 
 	const docker = await getRemoteDocker(serverId);

packages/server/src/utils/process/execAsync.ts

@@ -131,11 +131,6 @@ export const execFileAsync = async (
 ): Promise<{ stdout: string; stderr: string }> => {
 	const child = execFile(command, args);
 
-	if (options.input && child.stdin) {
-		child.stdin.write(options.input);
-		child.stdin.end();
-	}
-
 	return new Promise((resolve, reject) => {
 		let stdout = "";
 		let stderr = "";
@@ -159,6 +154,11 @@ export const execFileAsync = async (
 		});
 
 		child.on("error", reject);
+
+		if (options.input !== undefined && child.stdin) {
+			child.stdin.on("error", reject);
+			child.stdin.end(options.input);
+		}
 	});
 };
 
@@ -166,6 +166,7 @@ export const execAsyncRemote = async (
 	serverId: string | null,
 	command: string,
 	onData?: (data: string) => void,
+	input?: string,
 ): Promise<{ stdout: string; stderr: string }> => {
 	if (!serverId) return { stdout: "", stderr: "" };
 	const server = await findServerById(serverId);
@@ -219,6 +220,13 @@ export const execAsyncRemote = async (
 							stderr += data.toString();
 							onData?.(data.toString());
 						});
+					if (input !== undefined) {
+						stream.on("error", (error: Error) => {
+							conn.end();
+							reject(error);
+						});
+						stream.end(input);
+					}
 				});
 			})
 			.on("error", (err) => {