From b5c5faf9f0577eb00fe8e0874f55cf3c71943eb5 Mon Sep 17 00:00:00 2001 From: Gabriel Lima <20707866+Gabrielgvl@users.noreply.github.com> Date: Sat, 17 Oct 2026 12:18:12 -0300 Subject: [PATCH] fix(security): pass registry login passwords through stdin Use execFile arguments for local login and the SSH stream for remote login. Discard execution errors after sanitizing their messages so echoed credentials cannot survive in nested error fields. Co-Authored-By: Claude Opus 5.5 --- .../__test__/registry/docker-login.test.ts | 205 ++++++++++++++++++ apps/dokploy/server/api/routers/registry.ts | 60 ++--- packages/server/src/services/registry.ts | 66 ++++-- packages/server/src/services/rollbacks.ts | 26 +-- .../server/src/utils/process/execAsync.ts | 18 +- 5 files changed, 285 insertions(+), 90 deletions(-) create mode 100644 apps/dokploy/__test__/registry/docker-login.test.ts diff --git a/apps/dokploy/__test__/registry/docker-login.test.ts b/apps/dokploy/__test__/registry/docker-login.test.ts new file mode 100644 index 000000000..7ca9c46ec --- /dev/null +++ b/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"); + }, + ); +}); diff --git a/apps/dokploy/server/api/routers/registry.ts b/apps/dokploy/server/api/routers/registry.ts index c93da5a22..ce6e2b4b8 100644 --- a/apps/dokploy/server/api/routers/registry.ts +++ b/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) { diff --git a/packages/server/src/services/registry.ts b/packages/server/src/services/registry.ts index e395dd4c9..784a769eb 100644 --- a/packages/server/src/services/registry.ts +++ b/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 { + 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)); diff --git a/packages/server/src/services/rollbacks.ts b/packages/server/src/services/rollbacks.ts index 8837ae912..15bfdd419 100644 --- a/packages/server/src/services/rollbacks.ts +++ b/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); diff --git a/packages/server/src/utils/process/execAsync.ts b/packages/server/src/utils/process/execAsync.ts index 37b0b7f91..5593b1999 100644 --- a/packages/server/src/utils/process/execAsync.ts +++ b/packages/server/src/utils/process/execAsync.ts @@ -121,11 +121,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 = ""; @@ -149,6 +144,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); + } }); }; @@ -156,6 +156,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); @@ -209,6 +210,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) => {