diff --git a/packages/lib/services/docker-sandbox-provider.ts b/packages/lib/services/docker-sandbox-provider.ts index 631234ee0..5edeecd38 100644 --- a/packages/lib/services/docker-sandbox-provider.ts +++ b/packages/lib/services/docker-sandbox-provider.ts @@ -24,6 +24,10 @@ import { shellQuote, validateAgentId, validateAgentName, + validateContainerName, + validateEnvKey, + validateEnvValue, + validateVolumePath, WEBUI_PORT_MAX, WEBUI_PORT_MIN, } from "./docker-sandbox-utils"; @@ -303,19 +307,14 @@ export class DockerSandboxProvider implements SandboxProvider { MILADY_ALLOWED_ORIGINS: `https://${agentId}.${getAgentBaseDomain()}`, }; - // Validate env var keys to prevent shell command injection via malformed keys - for (const key of Object.keys(allEnv)) { - if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(key)) { - throw new Error(`[docker-sandbox] Invalid environment variable key: "${key}"`); - } + // Validate env keys/values before they are interpolated into remote shell commands. + // Internal env vars must also remain UPPER_SNAKE_CASE so validation stays + // consistent across caller-supplied and provider-generated values. + for (const [key, value] of Object.entries(allEnv)) { + validateEnvKey(key); + validateEnvValue(key, value); } - // Note: Values do not need control-character validation. The shellQuote() function - // wraps each "key=value" pair in single quotes and escapes embedded single quotes as '"'"', - // which makes all values (including those with newlines, tabs, or other control chars) - // safe inside the shell command. Single-quoted strings in bash preserve all characters - // literally except single quotes (which shellQuote already handles). - const envFlags = Object.entries(allEnv) .map(([key, value]) => `-e ${shellQuote(`${key}=${value}`)}`) .join(" "); diff --git a/packages/lib/services/docker-sandbox-utils.ts b/packages/lib/services/docker-sandbox-utils.ts index 85cb12b98..1334bbaf6 100644 --- a/packages/lib/services/docker-sandbox-utils.ts +++ b/packages/lib/services/docker-sandbox-utils.ts @@ -26,6 +26,10 @@ export const BRIDGE_PORT_MIN = 18790; export const BRIDGE_PORT_MAX = 19790; export const WEBUI_PORT_MIN = 20000; export const WEBUI_PORT_MAX = 25000; +export const DOCKER_CONTAINER_NAME_MAX_LENGTH = 128; +export const MILADY_CONTAINER_NAME_PREFIX = "milady-"; +export const MAX_AGENT_ID_LENGTH = + DOCKER_CONTAINER_NAME_MAX_LENGTH - MILADY_CONTAINER_NAME_PREFIX.length; // --------------------------------------------------------------------------- // Shell Quoting @@ -43,14 +47,24 @@ export function shellQuote(value: string): string { // Validation // --------------------------------------------------------------------------- +function hasControlChars(value: string): boolean { + return /[\x00-\x1f\x7f]/.test(value); +} + /** - * Validate an agent ID before using it in shell commands. - * Must be a UUID (hex + hyphens) or alphanumeric with hyphens/underscores. + * Validate an agent ID before using it in Docker-derived names and shell commands. + * Must fit within Docker's 128-char container name limit after the `milady-` + * prefix is applied. */ export function validateAgentId(agentId: string): void { - if (!/^[a-zA-Z0-9_-]{1,128}$/.test(agentId)) { + if ( + agentId.length === 0 || + agentId.length > MAX_AGENT_ID_LENGTH || + hasControlChars(agentId) || + !/^[a-zA-Z0-9_-]+$/.test(agentId) + ) { throw new Error( - `Invalid agent ID "${agentId}": must be 1-128 chars, alphanumeric / hyphens / underscores only.`, + `Invalid agent ID "${agentId}": must be 1-${MAX_AGENT_ID_LENGTH} chars, alphanumeric / hyphens / underscores only.`, ); } } @@ -66,6 +80,56 @@ export function validateAgentName(name: string): void { } } +/** Env keys must be uppercase shell-safe identifiers; lowercase keys are intentionally rejected. */ +export function validateEnvKey(key: string): void { + if (hasControlChars(key) || !/^[A-Z_][A-Z0-9_]*$/.test(key)) { + throw new Error( + `Invalid environment variable key "${key}": must match ^[A-Z_][A-Z0-9_]*$.`, + ); + } +} + +/** + * Env values are shell-safe once single-quoted, but we still reject control + * characters so multi-line payloads and invisible bytes cannot reach the remote + * shell command. Callers should pass a key so production errors are debuggable. + */ +export function validateEnvValue(key: string, value: string): void { + if (hasControlChars(value)) { + throw new Error( + `Invalid environment variable value for key "${key}": contains control characters (newlines and PEM-encoded values are not supported).`, + ); + } +} + +/** Docker container names must be simple shell-safe identifiers. */ +export function validateContainerName(containerName: string): void { + if (hasControlChars(containerName) || !/^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}$/.test(containerName)) { + throw new Error( + `Invalid container name "${containerName}": must match ^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}$.`, + ); + } +} + +/** Docker host volume paths must be absolute, normalized, and shell-safe. */ +export function validateVolumePath(volumePath: string): void { + // First allow only absolute shell-safe path characters, then separately enforce + // normalized-form rules like no traversal, repeated separators, or trailing slash. + if (hasControlChars(volumePath) || !/^\/[A-Za-z0-9._/\-]+$/.test(volumePath)) { + throw new Error(`Invalid volume path "${volumePath}".`); + } + if ( + volumePath.includes("//") || + volumePath.includes("/./") || + volumePath.includes("/../") || + volumePath.endsWith("/.") || + volumePath.endsWith("/..") || + (volumePath.length > 1 && volumePath.endsWith("/")) + ) { + throw new Error(`Invalid volume path "${volumePath}": path must be normalized.`); + } +} + // --------------------------------------------------------------------------- // Port Allocation // --------------------------------------------------------------------------- @@ -107,13 +171,19 @@ export function allocatePort(min: number, max: number, excluded: Set): n * patterns and can collide on the same node). */ export function getContainerName(agentId: string): string { - return `milady-${agentId}`; + validateAgentId(agentId); + const containerName = `${MILADY_CONTAINER_NAME_PREFIX}${agentId}`; + // Keep this derived-output validation as a guardrail if the naming template changes. + validateContainerName(containerName); + return containerName; } /** Volume path on the Docker host for persistent agent data. */ export function getVolumePath(agentId: string): string { validateAgentId(agentId); - return `/data/agents/${agentId}`; + const volumePath = `/data/agents/${agentId}`; + validateVolumePath(volumePath); + return volumePath; } // --------------------------------------------------------------------------- diff --git a/packages/tests/unit/docker-infrastructure.test.ts b/packages/tests/unit/docker-infrastructure.test.ts index 655a6e591..591cb8bcf 100644 --- a/packages/tests/unit/docker-infrastructure.test.ts +++ b/packages/tests/unit/docker-infrastructure.test.ts @@ -9,10 +9,15 @@ import { allocatePort, getContainerName, getVolumePath, + MAX_AGENT_ID_LENGTH, parseDockerNodes, shellQuote, validateAgentId, validateAgentName, + validateContainerName, + validateEnvKey, + validateEnvValue, + validateVolumePath, } from "@/lib/services/docker-sandbox-utils"; // --------------------------------------------------------------------------- @@ -125,8 +130,8 @@ describe("Docker Infrastructure - Pure Functions", () => { expect(() => validateAgentId("a")).not.toThrow(); }); - test("accepts exactly 128 characters", () => { - const id = "a".repeat(128); + test("accepts exactly the Docker-safe maximum length", () => { + const id = "a".repeat(MAX_AGENT_ID_LENGTH); expect(() => validateAgentId(id)).not.toThrow(); }); @@ -134,8 +139,8 @@ describe("Docker Infrastructure - Pure Functions", () => { expect(() => validateAgentId("")).toThrow(/Invalid agent ID/); }); - test("throws for 129 characters (too long)", () => { - const id = "a".repeat(129); + test("throws when the agentId would exceed the Docker container name limit", () => { + const id = "a".repeat(MAX_AGENT_ID_LENGTH + 1); expect(() => validateAgentId(id)).toThrow(/Invalid agent ID/); }); @@ -143,6 +148,10 @@ describe("Docker Infrastructure - Pure Functions", () => { expect(() => validateAgentId("agent;rm -rf /")).toThrow(/Invalid agent ID/); }); + test("throws for trailing newline", () => { + expect(() => validateAgentId("agent_1\n")).toThrow(/Invalid agent ID/); + }); + test("throws for dots", () => { expect(() => validateAgentId("agent.1")).toThrow(/Invalid agent ID/); }); @@ -205,6 +214,115 @@ describe("Docker Infrastructure - Pure Functions", () => { }); }); + // ------------------------------------------------------------------------- + describe("validateEnvKey", () => { + test("accepts uppercase underscore keys", () => { + expect(() => validateEnvKey("JWT_SECRET")).not.toThrow(); + expect(() => validateEnvKey("A1_B2")).not.toThrow(); + }); + + test("rejects lowercase keys", () => { + expect(() => validateEnvKey("jwt_secret")).toThrow(/Invalid environment variable key/); + }); + + test("rejects keys starting with a digit", () => { + expect(() => validateEnvKey("1SECRET")).toThrow(/Invalid environment variable key/); + }); + + test("rejects punctuation", () => { + expect(() => validateEnvKey("JWT-SECRET")).toThrow(/Invalid environment variable key/); + }); + + test("rejects trailing newlines", () => { + expect(() => validateEnvKey("JWT_SECRET\n")).toThrow(/Invalid environment variable key/); + }); + }); + + // ------------------------------------------------------------------------- + describe("validateEnvValue", () => { + test("accepts printable values", () => { + expect(() => validateEnvValue("JWT_SECRET", "hello-world_123")).not.toThrow(); + }); + + test("accepts UUIDs, URLs, and base64-like tokens", () => { + expect(() => + validateEnvValue( + "MIXED_VALUE", + "550e8400-e29b-41d4-a716-446655440000 https://example.com/a?b=c token+/=", + ), + ).not.toThrow(); + }); + + test("rejects null bytes and includes the key name", () => { + expect(() => validateEnvValue("JWT_SECRET", "abc\x00def")).toThrow( + /JWT_SECRET.*control characters/, + ); + }); + + test("rejects newlines and explains PEM-style values are unsupported", () => { + expect(() => validateEnvValue("TLS_CERT", "abc\ndef")).toThrow( + /TLS_CERT.*newlines and PEM-encoded values are not supported/, + ); + }); + + test("rejects tabs", () => { + expect(() => validateEnvValue("JWT_SECRET", "abc\tdef")).toThrow( + /control characters/, + ); + }); + }); + + // ------------------------------------------------------------------------- + describe("container name and volume path validation", () => { + test("getContainerName returns a valid deterministic name", () => { + expect(getContainerName("agent_1")).toBe("milady-agent_1"); + }); + + test("validateContainerName accepts docker-safe names", () => { + expect(() => validateContainerName("milady-agent_1.test-2")).not.toThrow(); + }); + + test("validateContainerName accepts the 128-character boundary", () => { + const containerName = `m${"a".repeat(127)}`; + expect(containerName).toHaveLength(128); + expect(() => validateContainerName(containerName)).not.toThrow(); + }); + + test("validateContainerName rejects shell metacharacters", () => { + expect(() => validateContainerName("bad;name")).toThrow(/Invalid container name/); + }); + + test("validateContainerName rejects trailing newlines", () => { + expect(() => validateContainerName("milady-agent\n")).toThrow(/Invalid container name/); + }); + + test("validateContainerName rejects names longer than 128 characters", () => { + const containerName = `m${"a".repeat(128)}`; + expect(containerName).toHaveLength(129); + expect(() => validateContainerName(containerName)).toThrow(/Invalid container name/); + }); + + test("getVolumePath returns a normalized absolute path", () => { + expect(getVolumePath("agent_1")).toBe("/data/agents/agent_1"); + }); + + test("validateVolumePath rejects traversal", () => { + expect(() => validateVolumePath("/data/agents/../escape")).toThrow(/Invalid volume path/); + }); + + test("validateVolumePath rejects trailing slashes", () => { + expect(() => validateVolumePath("/data/agents/")).toThrow(/path must be normalized/); + }); + + test("validateVolumePath rejects trailing newlines", () => { + expect(() => validateVolumePath("/data/agents/agent_1\n")).toThrow(/Invalid volume path/); + }); + + test("validateVolumePath rejects non-absolute paths", () => { + expect(() => validateVolumePath("data/agents/agent_1")).toThrow(/Invalid volume path/); + }); + }); + // ------------------------------------------------------------------------- describe("allocatePort", () => { test("returns a port within [min, max)", () => { @@ -271,6 +389,11 @@ describe("Docker Infrastructure - Pure Functions", () => { test("works with a short agentId", () => { expect(getContainerName("x")).toBe("milady-x"); }); + + test("rejects an agentId that would exceed the Docker container name limit", () => { + const longId = "a".repeat(MAX_AGENT_ID_LENGTH + 1); + expect(() => getContainerName(longId)).toThrow(/Invalid agent ID/); + }); }); // -------------------------------------------------------------------------