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 bc700d0e0..cc75baad5 100644 --- a/packages/server/src/utils/process/execAsync.ts +++ b/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) => {