diff --git a/src/agent/tools/exec/__tests__/tools.test.ts b/src/agent/tools/exec/__tests__/tools.test.ts index 6dc1f0af..cebd6241 100644 --- a/src/agent/tools/exec/__tests__/tools.test.ts +++ b/src/agent/tools/exec/__tests__/tools.test.ts @@ -3,20 +3,31 @@ import Database from "better-sqlite3"; import { ensureSchema } from "../../../../memory/schema.js"; import type { ExecConfig } from "../../../../config/schema.js"; import type { ToolContext } from "../../types.js"; +import { createExecRunExecutor, isCommandAllowed } from "../run.js"; +import { createExecInstallExecutor } from "../install.js"; +import { createExecServiceExecutor } from "../service.js"; +import { createExecStatusExecutor } from "../status.js"; // Mock the runner to avoid real command execution vi.mock("../runner.js", () => ({ runCommand: vi.fn(), + spawnInstallCommand: vi.fn(), ensureSandboxDir: vi.fn(), })); -import { runCommand } from "../runner.js"; -import { createExecRunExecutor, isCommandAllowed } from "../run.js"; -import { createExecInstallExecutor } from "../install.js"; -import { createExecServiceExecutor } from "../service.js"; -import { createExecStatusExecutor } from "../status.js"; +// Mock concurrency to avoid real semaphore blocking across tests +vi.mock("../concurrency.js", () => ({ + execConcurrency: { + acquire: vi.fn().mockResolvedValue(undefined), + release: vi.fn(), + count: 0, + }, +})); + +import { runCommand, spawnInstallCommand } from "../runner.js"; const mockRunCommand = vi.mocked(runCommand); +const mockSpawnInstall = vi.mocked(spawnInstallCommand); function createTestDb(): Database.Database { const db = new Database(":memory:"); @@ -60,14 +71,12 @@ function makeExecConfig(overrides?: Partial): ExecConfig { } function makeContext(overrides?: Partial): ToolContext { - const defaultConfig = { telegram: { admin_ids: [42] } } as any; return { bridge: {} as any, db: new Database(":memory:"), chatId: "123", senderId: 42, isGroup: false, - config: defaultConfig, ...overrides, }; } @@ -102,8 +111,8 @@ describe("exec_run", () => { }); expect(mockRunCommand).toHaveBeenCalledWith( "echo hello", - expect.objectContaining({ timeout: 120000, maxOutput: 50000 }), - expect.anything() + { timeout: 120000, maxOutput: 50000 }, + expect.objectContaining({ cwd: expect.any(String) }) ); }); @@ -175,8 +184,53 @@ describe("exec_install", () => { vi.clearAllMocks(); }); - it("constructs correct command for apt", async () => { - mockRunCommand.mockResolvedValue({ + it("rejects URL-based packages", async () => { + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor( + { manager: "pip", packages: "https://evil.com/malware.tar.gz" }, + makeContext() + ); + + expect(result.success).toBe(false); + expect(result.error).toContain("URL-based packages not allowed"); + expect(mockSpawnInstall).not.toHaveBeenCalled(); + }); + + it("rejects archive-based packages", async () => { + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "pip", packages: "malware.whl" }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("archive download not allowed"); + }); + + it("rejects package names with shell metacharacters", async () => { + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "npm", packages: "pkg; rm -rf /" }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("invalid package name"); + }); + + it("rejects too many packages", async () => { + const many = Array.from({ length: 25 }, (_, i) => `pkg${i}`).join(" "); + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "pip", packages: many }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("Too many packages"); + }); + + it("rejects empty package name", async () => { + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "apt", packages: "" }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("No packages specified"); + }); + + it("calls spawnInstallCommand with correct args for apt", async () => { + mockSpawnInstall.mockResolvedValue({ stdout: "installed", stderr: "", exitCode: 0, @@ -187,17 +241,14 @@ describe("exec_install", () => { }); const executor = createExecInstallExecutor(db, makeExecConfig()); - await executor({ manager: "apt", packages: "nginx curl" }, makeContext()); + const result = await executor({ manager: "apt", packages: "nginx curl" }, makeContext()); - expect(mockRunCommand).toHaveBeenCalledWith( - "apt install -y nginx curl", - expect.any(Object), - expect.anything() - ); + expect(result.success).toBe(true); + expect(mockSpawnInstall).toHaveBeenCalledWith("apt", ["nginx", "curl"], 120000, 50000); }); - it("constructs correct command for pip", async () => { - mockRunCommand.mockResolvedValue({ + it("calls spawnInstallCommand with correct args for pip", async () => { + mockSpawnInstall.mockResolvedValue({ stdout: "", stderr: "", exitCode: 0, @@ -208,17 +259,14 @@ describe("exec_install", () => { }); const executor = createExecInstallExecutor(db, makeExecConfig()); - await executor({ manager: "pip", packages: "flask" }, makeContext()); + const result = await executor({ manager: "pip", packages: "flask" }, makeContext()); - expect(mockRunCommand).toHaveBeenCalledWith( - "pip install flask", - expect.any(Object), - expect.anything() - ); + expect(result.success).toBe(true); + expect(mockSpawnInstall).toHaveBeenCalledWith("pip", ["flask"], 120000, 50000); }); - it("constructs correct command for npm", async () => { - mockRunCommand.mockResolvedValue({ + it("calls spawnInstallCommand with correct args for npm", async () => { + mockSpawnInstall.mockResolvedValue({ stdout: "", stderr: "", exitCode: 0, @@ -229,17 +277,14 @@ describe("exec_install", () => { }); const executor = createExecInstallExecutor(db, makeExecConfig()); - await executor({ manager: "npm", packages: "pm2" }, makeContext()); + const result = await executor({ manager: "npm", packages: "pm2" }, makeContext()); - expect(mockRunCommand).toHaveBeenCalledWith( - "npm install -g pm2", - expect.any(Object), - expect.anything() - ); + expect(result.success).toBe(true); + expect(mockSpawnInstall).toHaveBeenCalledWith("npm", ["pm2"], 120000, 50000); }); - it("constructs correct command for docker", async () => { - mockRunCommand.mockResolvedValue({ + it("calls spawnInstallCommand with correct args for docker", async () => { + mockSpawnInstall.mockResolvedValue({ stdout: "", stderr: "", exitCode: 0, @@ -250,17 +295,14 @@ describe("exec_install", () => { }); const executor = createExecInstallExecutor(db, makeExecConfig()); - await executor({ manager: "docker", packages: "nginx:latest" }, makeContext()); + const result = await executor({ manager: "docker", packages: "nginx:latest" }, makeContext()); - expect(mockRunCommand).toHaveBeenCalledWith( - "docker pull nginx:latest", - expect.any(Object), - expect.anything() - ); + expect(result.success).toBe(true); + expect(mockSpawnInstall).toHaveBeenCalledWith("docker", ["nginx:latest"], 120000, 50000); }); it("logs audit entry", async () => { - mockRunCommand.mockResolvedValue({ + mockSpawnInstall.mockResolvedValue({ stdout: "", stderr: "", exitCode: 0, @@ -278,6 +320,34 @@ describe("exec_install", () => { expect(rows[0].tool).toBe("exec_install"); expect(rows[0].command).toBe("apt install -y nginx"); }); + + it("returns error when spawnInstallCommand throws", async () => { + mockSpawnInstall.mockRejectedValue(new Error("spawn ENOENT")); + + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "apt", packages: "nginx" }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("Install failed"); + }); + + it("returns error when install times out", async () => { + mockSpawnInstall.mockResolvedValue({ + stdout: "", + stderr: "", + exitCode: null, + signal: "SIGTERM", + duration: 120000, + truncated: false, + timedOut: true, + }); + + const executor = createExecInstallExecutor(db, makeExecConfig()); + const result = await executor({ manager: "apt", packages: "huge-package" }, makeContext()); + + expect(result.success).toBe(false); + expect(result.error).toContain("timed out"); + }); }); describe("exec_service", () => { @@ -305,7 +375,7 @@ describe("exec_service", () => { expect(mockRunCommand).toHaveBeenCalledWith( "systemctl status nginx", expect.any(Object), - expect.anything() + expect.objectContaining({ cwd: expect.any(String) }) ); }); @@ -347,11 +417,12 @@ describe("isCommandAllowed", () => { }); it("does not allow prefix substring without whitespace boundary", () => { + // 'git' should not match 'gitconfig' without a space after it expect(isCommandAllowed("gitconfig --list", ["git"])).toBe(false); }); it("trims whitespace before matching", () => { - expect(isCommandAllowed(" ls /tmp", ["ls"])).toBe(true); + expect(isCommandAllowed(" ls /tmp", ["ls"])).toBe(true); }); }); @@ -398,7 +469,7 @@ describe("exec_run allowlist mode", () => { expect(mockRunCommand).toHaveBeenCalledWith( "git status", expect.any(Object), - expect.anything() + expect.objectContaining({ cwd: expect.any(String) }) ); }); @@ -497,7 +568,9 @@ describe("exec_status", () => { const result = await executor({} as any, makeContext()); expect(result.success).toBe(true); + // memory should contain the failure message expect(result.data.memory).toContain("failed"); + // other keys should have data expect(result.data.disk).toBe("some data"); }); }); diff --git a/src/agent/tools/exec/install.ts b/src/agent/tools/exec/install.ts index 113b5001..fc828f9c 100644 --- a/src/agent/tools/exec/install.ts +++ b/src/agent/tools/exec/install.ts @@ -1,8 +1,7 @@ import { Type } from "@sinclair/typebox"; import type { Tool, ToolExecutor, ToolResult } from "../types.js"; import type { ExecConfig } from "../../../config/schema.js"; -import { runCommand, ensureSandboxDir } from "./runner.js"; -import { execConcurrency } from "./concurrency.js"; +import { spawnInstallCommand } from "./runner.js"; import { insertAuditEntry, updateAuditEntry } from "./audit.js"; import type Database from "better-sqlite3"; @@ -11,12 +10,52 @@ interface ExecInstallParams { packages: string; } -const INSTALL_COMMANDS: Record string> = { - apt: (pkgs) => `apt install -y ${pkgs}`, - pip: (pkgs) => `pip install ${pkgs}`, - npm: (pkgs) => `npm install -g ${pkgs}`, - docker: (pkgs) => `docker pull ${pkgs}`, -}; +/** Maximum number of packages per install call (prevents resource exhaustion) */ +const MAX_PACKAGES = 20; + +/** + * Validate a single package name token. + * Returns the reason string if invalid, or null if valid. + */ +export function validatePackageToken(token: string): string | null { + if (!token || token.length === 0) return "empty package name"; + if (token.length > 128) return `package name too long (${token.length} chars)`; + + // Reject URL-based package specifications + if (/^https?:\/\//i.test(token)) return `URL-based packages not allowed: "${token}"`; + if (/\.(deb|whl|tar\.gz|tgz|zip)$/i.test(token)) + return `archive download not allowed: "${token}"`; + + // Strict package name regex (alphanumeric, dots, hyphens, underscores, colons for docker tags) + if (!/^[a-zA-Z0-9][a-zA-Z0-9_.\-:]*$/.test(token)) { + return `invalid package name "${token}": only alphanumeric, dots, hyphens, underscores, colons allowed`; + } + + return null; +} + +/** + * Parse and validate a packages string. + * Returns { valid: string[] } on success, or { error: string } on failure. + */ +export function parseAndValidatePackages( + packages: string +): { valid: string[] } | { error: string } { + const trimmed = packages.trim(); + if (!trimmed) return { error: "No packages specified" }; + + const tokens = trimmed.split(/\s+/); + if (tokens.length > MAX_PACKAGES) { + return { error: `Too many packages (${tokens.length}). Maximum is ${MAX_PACKAGES}.` }; + } + + for (const token of tokens) { + const reason = validatePackageToken(token); + if (reason) return { error: reason }; + } + + return { valid: tokens }; +} export const execInstallTool: Tool = { name: "exec_install", @@ -28,7 +67,10 @@ export const execInstallTool: Tool = { { description: "Package manager to use" } ), packages: Type.String({ - description: "Space-separated package names to install (e.g. 'nginx curl')", + description: + "Space-separated package names to install (e.g. 'nginx curl'). " + + "Only alphanumeric characters, dots, hyphens, and underscores are allowed. " + + "URL-based and archive-based packages are rejected.", }), }), }; @@ -41,17 +83,19 @@ export function createExecInstallExecutor( const { manager, packages } = params; const { timeout, max_output } = execConfig.limits; - const buildCommand = INSTALL_COMMANDS[manager]; - if (!buildCommand) { - return { - success: false, - error: `Unsupported package manager: ${manager}. Use apt, pip, npm, or docker.`, - }; + // Validate package names before any command construction + const parsed = parseAndValidatePackages(packages); + if ("error" in parsed) { + return { success: false, error: `Package validation failed: ${parsed.error}` }; } - const command = buildCommand(packages); - - await execConcurrency.acquire(execConfig.security.max_concurrent); + const flagsMap: Record = { + apt: "install -y", + pip: "install", + npm: "install -g", + docker: "pull", + }; + const commandDisplay = `${manager} ${flagsMap[manager]} ${parsed.valid.join(" ")}`; let auditId: number | undefined; if (execConfig.audit.log_commands) { @@ -59,78 +103,59 @@ export function createExecInstallExecutor( userId: context.senderId, username: undefined, tool: "exec_install", - command, + command: commandDisplay, status: "running", truncated: false, }); } + let result; try { - const sandboxDir = execConfig.security.sandbox_dir; - if (sandboxDir) ensureSandboxDir(sandboxDir); - - const security = { - cwd: sandboxDir || undefined, - env: - execConfig.security.env_whitelist.length > 0 - ? buildFilteredEnv(execConfig.security.env_whitelist) - : undefined, - }; - - const result = await runCommand( - command, - { - timeout: timeout * 1000, - maxOutput: max_output, - }, - security - ); - - const status = result.timedOut ? "timeout" : result.exitCode === 0 ? "success" : "failed"; - + result = await spawnInstallCommand(manager, parsed.valid, timeout * 1000, max_output); + } catch (err) { if (auditId !== undefined) { updateAuditEntry(db, auditId, { - status, - exitCode: result.exitCode ?? undefined, - signal: result.signal ?? undefined, - duration: result.duration, - stdout: result.stdout, - stderr: result.stderr, - truncated: result.truncated, + status: "failed", + stderr: err instanceof Error ? err.message : String(err), }); } - return { - success: result.exitCode === 0 && !result.timedOut, - data: { - manager, - packages, - stdout: result.stdout, - stderr: result.stderr, - exitCode: result.exitCode, - duration: result.duration, - truncated: result.truncated, - timedOut: result.timedOut, - }, - ...(result.timedOut - ? { error: `Install timed out after ${timeout}s` } - : result.exitCode !== 0 - ? { error: `Install failed with exit code ${result.exitCode}` } - : {}), + success: false, + error: `Install failed: ${err instanceof Error ? err.message : String(err)}`, }; - } finally { - execConcurrency.release(); } - }; -} -function buildFilteredEnv(envWhitelist: string[]): NodeJS.ProcessEnv { - const allowed = new Set(envWhitelist); - const filtered: NodeJS.ProcessEnv = {}; - for (const [key, value] of Object.entries(process.env)) { - if (allowed.has(key) && value !== undefined) { - filtered[key] = value; + const status = result.timedOut ? "timeout" : result.exitCode === 0 ? "success" : "failed"; + + if (auditId !== undefined) { + updateAuditEntry(db, auditId, { + status, + exitCode: result.exitCode ?? undefined, + signal: result.signal ?? undefined, + duration: result.duration, + stdout: result.stdout, + stderr: result.stderr, + truncated: result.truncated, + }); } - } - return filtered; + + return { + success: result.exitCode === 0 && !result.timedOut, + data: { + manager, + packages: parsed.valid, + stdout: result.stdout, + stderr: result.stderr, + exitCode: result.exitCode, + duration: result.duration, + truncated: result.truncated, + timedOut: result.timedOut, + }, + ...(result.timedOut + ? { error: `Install timed out after ${timeout}s` } + : result.exitCode !== 0 + ? { error: `Install failed with exit code ${result.exitCode}` } + : {}), + }; + }; } diff --git a/src/agent/tools/exec/runner.ts b/src/agent/tools/exec/runner.ts index b3bb8ecb..b22f7822 100644 --- a/src/agent/tools/exec/runner.ts +++ b/src/agent/tools/exec/runner.ts @@ -165,3 +165,110 @@ export function ensureSandboxDir(sandboxDir: string): void { fs.mkdirSync(sandboxDir, { recursive: true, mode: 0o755 }); } } + +/** + * spawnInstallCommand — runs a package manager via spawn with argument arrays. + * Uses sanitizeEnv() for child process env. Respects MAX_CONCURRENT limit. + * This is the injection-safe alternative to string-interpolated shell commands. + */ +export function spawnInstallCommand( + manager: "apt" | "pip" | "npm" | "docker", + packages: string[], + timeout: number, + maxOutput: number +): Promise { + if (activeCount >= MAX_CONCURRENT) { + throw new Error(`Max concurrent processes (${MAX_CONCURRENT}) reached`); + } + activeCount++; + + const argsMap: Record = { + apt: ["install", "-y", ...packages], + pip: ["install", ...packages], + npm: ["install", "-g", ...packages], + docker: ["pull", ...packages], + }; + + const args = argsMap[manager]; + const startTime = Date.now(); + + return new Promise((resolve) => { + let stdout = ""; + let stderr = ""; + let truncated = false; + let timedOut = false; + let resolved = false; + + const env = sanitizeEnv(process.env); + + log.info({ manager, packages }, "Installing packages"); + + const child = spawn(manager, args, { + stdio: ["ignore", "pipe", "pipe"], + env, + }); + + const finish = (exitCode: number | null, signal: string | null) => { + if (resolved) return; + resolved = true; + activeCount--; + clearTimeout(timeoutTimer); + resolve({ + stdout, + stderr, + exitCode, + signal, + duration: Date.now() - startTime, + truncated, + timedOut, + }); + }; + + child.stdout?.setEncoding("utf8"); + child.stderr?.setEncoding("utf8"); + + child.stdout?.on("data", (chunk: string) => { + if (stdout.length < maxOutput) { + stdout += chunk; + if (stdout.length > maxOutput) { + stdout = stdout.slice(0, maxOutput); + truncated = true; + } + } + }); + + child.stderr?.on("data", (chunk: string) => { + if (stderr.length < maxOutput) { + stderr += chunk; + if (stderr.length > maxOutput) { + stderr = stderr.slice(0, maxOutput); + truncated = true; + } + } + }); + + child.on("close", (code, sig) => finish(code, sig)); + child.on("error", (err) => { + log.error({ err }, "Spawn install error"); + stderr += err.message; + finish(1, null); + }); + + const timeoutTimer = setTimeout(() => { + timedOut = true; + log.warn({ manager, packages, timeout }, "Install timed out, killing"); + try { + if (child.pid != null) process.kill(-child.pid, "SIGTERM"); + } catch { + /* dead */ + } + setTimeout(() => { + try { + if (child.pid != null) process.kill(-child.pid, "SIGKILL"); + } catch { + /* dead */ + } + }, 5000); + }, timeout); + }); +}