From 9226c2d5319ae24cc3f662fb6b47c439d3a28ab0 Mon Sep 17 00:00:00 2001 From: Mauricio Siu Date: Thu, 10 Sep 2026 01:38:43 -0600 Subject: [PATCH] fix(sandbox): block inter-sandbox traffic, restart container when the timeout kill cannot fork, skip sandboxes mid-creation in reconcile --- apps/dokploy/__test__/sandbox/exec.test.ts | 35 +++++- apps/dokploy/__test__/sandbox/network.test.ts | 107 ++++++++++++++++++ apps/dokploy/__test__/sandbox/reaper.test.ts | 18 ++- docs/sandboxes.md | 15 ++- packages/server/src/services/sandbox.ts | 30 ++++- packages/server/src/utils/sandbox/exec.ts | 28 ++++- packages/server/src/utils/sandbox/reaper.ts | 14 ++- 7 files changed, 221 insertions(+), 26 deletions(-) create mode 100644 apps/dokploy/__test__/sandbox/network.test.ts diff --git a/apps/dokploy/__test__/sandbox/exec.test.ts b/apps/dokploy/__test__/sandbox/exec.test.ts index 94e660a66..413f8c84f 100644 --- a/apps/dokploy/__test__/sandbox/exec.test.ts +++ b/apps/dokploy/__test__/sandbox/exec.test.ts @@ -51,6 +51,7 @@ const createDocker = (opts: { const kill = vi.fn(async () => { stream.end(); }); + const start = vi.fn(async () => undefined); const killExecStart = vi.fn(async () => { if (opts.dieOnKill) stream.end(); }); @@ -58,7 +59,7 @@ const createDocker = (opts: { const exec = vi.fn(async (options: { Cmd: string[] }) => { execCreate(options); - if (options.Cmd[2]?.startsWith("for p in /proc/")) { + if (options.Cmd[2]?.startsWith("if grep -qzF x /dev/null")) { return { start: killExecStart, inspect }; } return { @@ -77,10 +78,10 @@ const createDocker = (opts: { }); const docker = { - getContainer: vi.fn(() => ({ exec, kill })), + getContainer: vi.fn(() => ({ exec, kill, start })), modem: { demuxStream }, }; - return { docker, execCreate, execStart, kill, killExecStart, inspect }; + return { docker, execCreate, execStart, kill, start, killExecStart, inspect }; }; describe("runSandboxExec", () => { @@ -168,10 +169,11 @@ describe("runSandboxExec", () => { expect(killExecStart).toHaveBeenCalledWith({ Detach: true }); expect(kill).not.toHaveBeenCalled(); expect(result.containerKilled).toBe(false); + expect(result.containerRestarted).toBe(false); }); - it("kills the container when the process survives the marker kill", async () => { - const { docker, kill } = createDocker({ + it("kills and restarts the container when the process survives the marker kill", async () => { + const { docker, kill, start } = createDocker({ frames: [], exitCode: 0, endAfterMs: 0, @@ -184,14 +186,35 @@ describe("runSandboxExec", () => { expect(result.timedOut).toBe(true); expect(result.exitCode).toBe(SANDBOX_EXEC_TIMEOUT_EXIT_CODE); expect(kill).toHaveBeenCalledWith({ signal: "SIGKILL" }); + expect(start).toHaveBeenCalledTimes(1); expect(result.containerKilled).toBe(true); + expect(result.containerRestarted).toBe(true); + }, 10_000); + + it("reports a failed restart so the sandbox can be marked as error", async () => { + const { docker, start } = createDocker({ + frames: [], + exitCode: 0, + endAfterMs: 0, + dieOnKill: false, + }); + start.mockRejectedValueOnce(new Error("no such container")); + const result = await runSandboxExec(docker as never, "ctr", { + cmd: "sleep 100", + timeoutMs: 50, + }); + expect(result.containerKilled).toBe(true); + expect(result.containerRestarted).toBe(false); }, 10_000); }); describe("buildSandboxKillCommand", () => { it("matches the marker as a fixed string and never fails the shell", () => { const cmd = buildSandboxKillCommand("abc-123"); - expect(cmd).toContain(`grep -qxF ${quote(["DOKPLOY_EXEC_ID=abc-123"])}`); + expect(cmd).toContain(`grep -qzxF ${quote(["DOKPLOY_EXEC_ID=abc-123"])}`); + expect(cmd).toContain( + `tr '\\0' '\\n' < "$f" 2>/dev/null | grep -qxF ${quote(["DOKPLOY_EXEC_ID=abc-123"])}`, + ); expect(parse(cmd)).toContain("DOKPLOY_EXEC_ID=abc-123"); expect(cmd).toContain("kill -9"); expect(cmd.endsWith("; true")).toBe(true); diff --git a/apps/dokploy/__test__/sandbox/network.test.ts b/apps/dokploy/__test__/sandbox/network.test.ts new file mode 100644 index 000000000..5690149d8 --- /dev/null +++ b/apps/dokploy/__test__/sandbox/network.test.ts @@ -0,0 +1,107 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@dokploy/server/utils/servers/remote-docker", () => ({ + getRemoteDocker: vi.fn(), +})); + +import { + ensureSandboxNetwork, + SANDBOX_NETWORK_OPTIONS, +} from "@dokploy/server/services/sandbox"; + +const notFound = () => + Object.assign(new Error("no such network"), { statusCode: 404 }); + +const createDocker = (inspect: () => Promise) => { + const remove = vi.fn(async () => undefined); + const createNetwork = vi.fn(async () => undefined); + const getNetwork = vi.fn(() => ({ inspect, remove })); + return { + docker: { getNetwork, createNetwork }, + remove, + createNetwork, + getNetwork, + }; +}; + +beforeEach(() => { + vi.spyOn(console, "warn").mockImplementation(() => {}); +}); + +describe("ensureSandboxNetwork", () => { + it("creates an internal bridge without inter-container traffic for isolated mode", async () => { + const { docker, createNetwork } = createDocker(async () => { + throw notFound(); + }); + const name = await ensureSandboxNetwork(docker as never, "isolated"); + expect(name).toBe("dokploy-sandboxes-isolated"); + expect(createNetwork).toHaveBeenCalledWith( + expect.objectContaining({ + Name: "dokploy-sandboxes-isolated", + Driver: "bridge", + Internal: true, + Options: SANDBOX_NETWORK_OPTIONS, + }), + ); + expect( + SANDBOX_NETWORK_OPTIONS["com.docker.network.bridge.enable_icc"], + ).toBe("false"); + }); + + it("creates a non-internal bridge for internet mode", async () => { + const { docker, createNetwork } = createDocker(async () => { + throw notFound(); + }); + await ensureSandboxNetwork(docker as never, "internet"); + expect(createNetwork).toHaveBeenCalledWith( + expect.objectContaining({ + Name: "dokploy-sandboxes", + Internal: false, + Options: SANDBOX_NETWORK_OPTIONS, + }), + ); + }); + + it("reuses a network that already blocks inter-container traffic", async () => { + const { docker, createNetwork, remove } = createDocker(async () => ({ + Options: SANDBOX_NETWORK_OPTIONS, + Containers: {}, + })); + await ensureSandboxNetwork(docker as never, "isolated"); + expect(createNetwork).not.toHaveBeenCalled(); + expect(remove).not.toHaveBeenCalled(); + }); + + it("recreates an unused legacy network that allows inter-container traffic", async () => { + const { docker, createNetwork, remove } = createDocker(async () => ({ + Options: {}, + Containers: {}, + })); + await ensureSandboxNetwork(docker as never, "isolated"); + expect(remove).toHaveBeenCalledTimes(1); + expect(createNetwork).toHaveBeenCalledTimes(1); + }); + + it("keeps a legacy network that still has containers attached and warns", async () => { + const { docker, createNetwork, remove } = createDocker(async () => ({ + Options: {}, + Containers: { abc: { Name: "dokploy-sandbox-x" } }, + })); + await ensureSandboxNetwork(docker as never, "isolated"); + expect(remove).not.toHaveBeenCalled(); + expect(createNetwork).not.toHaveBeenCalled(); + expect(console.warn).toHaveBeenCalled(); + }); + + it("tolerates a concurrent create of the same network", async () => { + const { docker, createNetwork } = createDocker(async () => { + throw notFound(); + }); + createNetwork.mockRejectedValueOnce( + Object.assign(new Error("exists"), { statusCode: 409 }), + ); + await expect( + ensureSandboxNetwork(docker as never, "isolated"), + ).resolves.toBe("dokploy-sandboxes-isolated"); + }); +}); diff --git a/apps/dokploy/__test__/sandbox/reaper.test.ts b/apps/dokploy/__test__/sandbox/reaper.test.ts index 27cc73185..c9d1cae07 100644 --- a/apps/dokploy/__test__/sandbox/reaper.test.ts +++ b/apps/dokploy/__test__/sandbox/reaper.test.ts @@ -76,9 +76,9 @@ describe("planSandboxReconcile", () => { it("removes containers without a running row and errors rows without a container", () => { const plan = planSandboxReconcile( [ - { sandboxId: "ok", containerId: "c-ok" }, - { sandboxId: "gone", containerId: "c-gone" }, - { sandboxId: "never-started", containerId: null }, + { sandboxId: "ok", containerId: "c-ok", status: "running" }, + { sandboxId: "gone", containerId: "c-gone", status: "running" }, + { sandboxId: "never-started", containerId: null, status: "running" }, ], [ container("c-ok", "ok"), @@ -92,16 +92,24 @@ describe("planSandboxReconcile", () => { it("removes a stopped container even when its row is running, and errors the row", () => { const plan = planSandboxReconcile( - [{ sandboxId: "sb", containerId: "c1" }], + [{ sandboxId: "sb", containerId: "c1", status: "running" }], [container("c1", "sb", "exited")], ); expect(plan.removeContainers).toEqual(["c1"]); expect(plan.markError).toEqual(["sb"]); }); + it("leaves containers of sandboxes that are still being created alone", () => { + const plan = planSandboxReconcile( + [{ sandboxId: "new", containerId: null, status: "creating" }], + [container("c-new", "new")], + ); + expect(plan).toEqual({ removeContainers: [], markError: [] }); + }); + it("does nothing when everything matches", () => { const plan = planSandboxReconcile( - [{ sandboxId: "sb", containerId: "c1" }], + [{ sandboxId: "sb", containerId: "c1", status: "running" }], [container("c1", "sb")], ); expect(plan).toEqual({ removeContainers: [], markError: [] }); diff --git a/docs/sandboxes.md b/docs/sandboxes.md index 15a444ae2..126fbc19c 100644 --- a/docs/sandboxes.md +++ b/docs/sandboxes.md @@ -183,15 +183,20 @@ Each sandbox container is created with: capability is needed inside the container. - A dedicated bridge network per server: `dokploy-sandboxes-isolated` (`internal: true`, no egress) for `networkMode: "isolated"` (default) or - `dokploy-sandboxes` for `networkMode: "internet"`. Sandboxes are never - attached to `dokploy-network` or to any application network, and nothing is - published through Traefik. + `dokploy-sandboxes` for `networkMode: "internet"`. Both are created with + `com.docker.network.bridge.enable_icc=false`, so sandboxes cannot reach + each other even on the same bridge (a network created by an earlier build + without that option is recreated automatically once no sandbox is attached + to it). Sandboxes are never attached to `dokploy-network` or to any + application network, and nothing is published through Traefik. - Labels `dokploy.sandbox=true`, `dokploy.sandboxId`, `dokploy.projectId` and `dokploy.environmentId` so the reaper/reconcile can find orphans. - Commands run through `docker exec` with a per-command timeout; on timeout the whole process tree started by that exec is killed (matched by a marker env - var, no `procps` needed), falling back to killing the container, in which - case the sandbox is marked `error`. + var, no `procps` needed). If that kill cannot run (for example the command + exhausted `PidsLimit`, so nothing can fork), the container is killed and + started again: every process is gone, files are kept and the sandbox stays + `running`. Only when the restart itself fails is the sandbox marked `error`. Environment variables passed at creation are stored encrypted at rest and are never returned by the API. diff --git a/packages/server/src/services/sandbox.ts b/packages/server/src/services/sandbox.ts index 89484ddae..9ff8befdd 100644 --- a/packages/server/src/services/sandbox.ts +++ b/packages/server/src/services/sandbox.ts @@ -123,23 +123,47 @@ export const ensureSandboxImage = async (docker: Dockerode, image: string) => { }); }; +// Sandboxes share a bridge per server, so inter-container traffic must be +// disabled or two isolated sandboxes could talk to each other. +export const SANDBOX_NETWORK_OPTIONS = { + "com.docker.network.bridge.enable_icc": "false", +}; + export const ensureSandboxNetwork = async ( docker: Dockerode, mode: SandboxNetworkMode, ) => { const name = getSandboxNetworkName(mode); + let existing: { + Options?: Record; + Containers?: Record; + } | null = null; try { - await docker.getNetwork(name).inspect(); - return name; + existing = await docker.getNetwork(name).inspect(); } catch (error) { if (!isNotFound(error)) throw error; } + if (existing) { + if ( + existing.Options?.["com.docker.network.bridge.enable_icc"] === "false" + ) { + return name; + } + if (Object.keys(existing.Containers ?? {}).length > 0) { + console.warn( + `[Sandbox] Network ${name} allows inter-container traffic but is in use; it will be recreated once no sandbox is attached`, + ); + return name; + } + await docker.getNetwork(name).remove(); + } try { await docker.createNetwork({ Name: name, Driver: "bridge", Internal: mode === "isolated", CheckDuplicate: true, + Options: { ...SANDBOX_NETWORK_OPTIONS }, Labels: { [SANDBOX_LABEL]: "true" }, }); } catch (error) { @@ -321,7 +345,7 @@ export const execInSandbox = async ( onStdout: options.onStdout, onStderr: options.onStderr, }); - if (result.containerKilled) { + if (result.containerKilled && !result.containerRestarted) { await updateSandboxById(sandbox.sandboxId, { status: "error" }); } return result; diff --git a/packages/server/src/utils/sandbox/exec.ts b/packages/server/src/utils/sandbox/exec.ts index 24ad2ddb7..ab7d6d816 100644 --- a/packages/server/src/utils/sandbox/exec.ts +++ b/packages/server/src/utils/sandbox/exec.ts @@ -27,6 +27,7 @@ export interface SandboxExecResult { timedOut: boolean; truncated: boolean; containerKilled: boolean; + containerRestarted: boolean; } export const toSandboxEnvArray = (env?: Record | string[]) => @@ -35,9 +36,17 @@ export const toSandboxEnvArray = (env?: Record | string[]) => : Object.entries(env ?? {}).map(([key, value]) => `${key}=${value}`); // Finds every process started by the exec (children included) through the -// marker env var and kills it, without relying on pkill/procps being installed. -export const buildSandboxKillCommand = (marker: string) => - `for p in /proc/[0-9]*; do if tr '\\0' '\\n' < "$p/environ" 2>/dev/null | grep -qxF ${quote([`${SANDBOX_EXEC_MARKER_ENV}=${marker}`])}; then kill -9 "\${p#/proc/}" 2>/dev/null; fi; done; true`; +// marker env var and kills it with the shell builtin `kill`, so each +// iteration needs a single extra pid, which matters when the command +// exhausted PidsLimit. GNU grep reads the NUL-separated environ with -z; +// busybox grep has no -z, so it falls back to tr. +export const buildSandboxKillCommand = (marker: string) => { + const pattern = quote([`${SANDBOX_EXEC_MARKER_ENV}=${marker}`]); + const kill = 'p="${f#/proc/}"; kill -9 "${p%/environ}" 2>/dev/null'; + const withZ = `for f in /proc/[0-9]*/environ; do if grep -qzxF ${pattern} "$f" 2>/dev/null; then ${kill}; fi; done`; + const withTr = `for f in /proc/[0-9]*/environ; do if tr '\\0' '\\n' < "$f" 2>/dev/null | grep -qxF ${pattern}; then ${kill}; fi; done`; + return `if grep -qzF x /dev/null 2>/dev/null; [ $? -ne 2 ]; then ${withZ}; else ${withTr}; fi; true`; +}; const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -114,6 +123,7 @@ export const runSandboxExec = async ( let timedOut = false; let containerKilled = false; + let containerRestarted = false; let timeoutTask: Promise | null = null; const timer = setTimeout(() => { timedOut = true; @@ -127,6 +137,9 @@ export const runSandboxExec = async ( sleep(KILL_GRACE_MS).then(() => false), ]); if (!ended) { + // The marker kill could not run (e.g. PidsLimit exhausted, no + // grep). Killing and starting the container again drops every + // process while keeping the filesystem, so the sandbox stays usable. await container .kill({ signal: "SIGKILL" }) .then(() => { @@ -134,6 +147,14 @@ export const runSandboxExec = async ( }) .catch(() => {}); stream.destroy(); + if (containerKilled) { + await container + .start() + .then(() => { + containerRestarted = true; + }) + .catch(() => {}); + } } })(); }, options.timeoutMs); @@ -158,5 +179,6 @@ export const runSandboxExec = async ( timedOut, truncated: state.truncated, containerKilled, + containerRestarted, }; }; diff --git a/packages/server/src/utils/sandbox/reaper.ts b/packages/server/src/utils/sandbox/reaper.ts index 90cf40732..70f9e1693 100644 --- a/packages/server/src/utils/sandbox/reaper.ts +++ b/packages/server/src/utils/sandbox/reaper.ts @@ -55,7 +55,7 @@ export interface SandboxReconcilePlan { } export const planSandboxReconcile = ( - rows: { sandboxId: string; containerId: string | null }[], + rows: { sandboxId: string; containerId: string | null; status: string }[], containers: { Id: string; State: string; Labels: Record }[], ): SandboxReconcilePlan => { const rowsById = new Map(rows.map((row) => [row.sandboxId, row])); @@ -65,6 +65,8 @@ export const planSandboxReconcile = ( for (const container of containers) { const sandboxId = container.Labels?.[SANDBOX_ID_LABEL]; const row = sandboxId ? rowsById.get(sandboxId) : undefined; + // A sandbox mid-creation has no containerId yet; leave it to createSandbox. + if (row?.status === "creating") continue; if ( !row || row.containerId !== container.Id || @@ -77,7 +79,11 @@ export const planSandboxReconcile = ( } const markError = rows - .filter((row) => !row.containerId || !liveContainerIds.has(row.containerId)) + .filter( + (row) => + row.status === "running" && + (!row.containerId || !liveContainerIds.has(row.containerId)), + ) .map((row) => row.sandboxId); return { removeContainers, markError }; @@ -95,12 +101,12 @@ const reconcileServer = async (serverId: string | null) => { listSandboxContainers(docker), db.query.sandboxes.findMany({ where: and( - eq(sandboxes.status, "running"), + inArray(sandboxes.status, ["running", "creating"]), serverId ? eq(sandboxes.serverId, serverId) : isNull(sandboxes.serverId), ), - columns: { sandboxId: true, containerId: true }, + columns: { sandboxId: true, containerId: true, status: true }, }), ]); const plan = planSandboxReconcile(rows, containers);