fix(security): harden Milady Docker provisioning against env injection

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(" ");

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<number>): 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;
 }
 
 // ---------------------------------------------------------------------------

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,24 +130,28 @@ 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();
     });
 
     test("throws for empty string", () => {
       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/);
     });
 
     test("throws for shell injection chars (semicolon, space)", () => {
       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/);
+    });
   });
 
   // -------------------------------------------------------------------------