diff --git a/src/agent/tools/find.ts b/src/agent/tools/find.ts index d67dd063f47c45bcfc913664c080a6c2d0170e7b..4537c5b5152783b3f1d43a66d8c97788a7a26add 100644 --- a/src/agent/tools/find.ts +++ b/src/agent/tools/find.ts @@ -2,7 +2,7 @@ import { spawnSync } from "node:child_process"; import { relative } from "node:path"; import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; -import { resolveToCwd } from "./path-utils.js"; +import { resolveToCwd, ensureWorkspacePath } from "./path-utils.js"; import { DEFAULT_MAX_BYTES, formatSize, truncateHead } from "../../util/truncate.js"; import { ToolInputError } from "../../util/errors.js"; @@ -28,6 +28,7 @@ export const createFindTool = (workspacePath: string): AgentTool => { const searchDir: string = params.path || "."; const effectiveLimit = params.limit ?? DEFAULT_LIMIT; const searchPath = resolveToCwd(searchDir, workspacePath); + ensureWorkspacePath(workspacePath, searchPath); const args = [ "--glob", diff --git a/src/agent/tools/grep.ts b/src/agent/tools/grep.ts index a27a3a438a228999afbee3fbd82c72cd6d5b7eb4..fd8cf5d931deb9ef91675ffa2c5966e766aa30c6 100644 --- a/src/agent/tools/grep.ts +++ b/src/agent/tools/grep.ts @@ -4,7 +4,7 @@ import { readFileSync, statSync } from "node:fs"; import { relative, basename } from "node:path"; import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; -import { resolveToCwd } from "./path-utils.js"; +import { resolveToCwd, ensureWorkspacePath } from "./path-utils.js"; import { DEFAULT_MAX_BYTES, formatSize, @@ -43,6 +43,7 @@ export const createGrepTool = (workspacePath: string): AgentTool => { execute: async (_toolCallId: string, params: any) => { const searchDir: string | undefined = params.path; const searchPath = resolveToCwd(searchDir || ".", workspacePath); + ensureWorkspacePath(workspacePath, searchPath); let isDirectory = false; try { isDirectory = statSync(searchPath).isDirectory(); diff --git a/src/agent/tools/index.ts b/src/agent/tools/index.ts index 5e803d0ea06cccbedcdd62b4497e9757475cd16d..c1b2fbd6ace418ab8fdab9ef9dc3088248b46bac 100644 --- a/src/agent/tools/index.ts +++ b/src/agent/tools/index.ts @@ -1,6 +1,4 @@ -import { resolve, sep } from "node:path"; import type { AgentTool } from "@mariozechner/pi-ai"; -import { ToolInputError } from "../../util/errors.js"; export type ToolFactory = (workspacePath: string) => AgentTool; @@ -9,17 +7,7 @@ export interface ToolBundle { tools: AgentTool[]; } -export function ensureWorkspacePath(workspacePath: string, targetPath: string): string { - const resolved = resolve(workspacePath, targetPath); - const root = workspacePath.endsWith(sep) ? workspacePath : `${workspacePath}${sep}`; - - if (resolved === workspacePath || resolved.startsWith(root)) { - return resolved; - } - - throw new ToolInputError(`Path escapes workspace: ${targetPath}`); -} - +export { ensureWorkspacePath } from "./path-utils.js"; export { createReadTool } from "./read.js"; export { createGrepTool } from "./grep.js"; export { createLsTool } from "./ls.js"; diff --git a/src/agent/tools/ls.ts b/src/agent/tools/ls.ts index 4546a1516c6be1e8073530c8678dec9c7dbc7cba..916064502234aa18895888d932944db902ea0a4d 100644 --- a/src/agent/tools/ls.ts +++ b/src/agent/tools/ls.ts @@ -2,7 +2,7 @@ import { existsSync, readdirSync, statSync } from "node:fs"; import { join } from "node:path"; import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; -import { resolveToCwd } from "./path-utils.js"; +import { resolveToCwd, ensureWorkspacePath } from "./path-utils.js"; import { DEFAULT_MAX_BYTES, formatSize, truncateHead } from "../../util/truncate.js"; const DEFAULT_LIMIT = 500; @@ -19,6 +19,7 @@ export const createLsTool = (workspacePath: string): AgentTool => ({ parameters: LsSchema as any, execute: async (_toolCallId: string, params: any) => { const resolved = resolveToCwd(params.path || ".", workspacePath); + ensureWorkspacePath(workspacePath, resolved); if (!existsSync(resolved)) { throw new Error(`Path does not exist: ${params.path || "."}`); diff --git a/src/agent/tools/path-utils.ts b/src/agent/tools/path-utils.ts index e4412f8269ccfa71b41e50d65d365e1b631c232e..e0d59e62963b271527b5c61819c0f3bc435f99da 100644 --- a/src/agent/tools/path-utils.ts +++ b/src/agent/tools/path-utils.ts @@ -1,6 +1,6 @@ import { accessSync, constants } from "node:fs"; -import * as os from "node:os"; -import { isAbsolute, resolve as resolvePath } from "node:path"; +import { isAbsolute, resolve as resolvePath, sep } from "node:path"; +import { ToolInputError } from "../../util/errors.js"; const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; @@ -33,12 +33,10 @@ export function expandPath(filePath: string): string { let result = filePath.replace(UNICODE_SPACES, " "); result = normalizeAtPrefix(result); - if (result === "~") { - return os.homedir(); - } - if (result.startsWith("~/")) { - return resolvePath(os.homedir(), result.slice(2)); - } + // NOTE: tilde expansion is intentionally omitted. + // In a workspace-sandboxed context, expanding ~ to the user's home + // directory would bypass workspace containment. Tildes are treated + // as literal path characters. return result; } @@ -68,3 +66,14 @@ export function resolveReadPath(filePath: string, cwd: string): string { return resolved; } + +export function ensureWorkspacePath(workspacePath: string, targetPath: string): string { + const resolved = resolvePath(workspacePath, targetPath); + const root = workspacePath.endsWith(sep) ? workspacePath : `${workspacePath}${sep}`; + + if (resolved === workspacePath || resolved.startsWith(root)) { + return resolved; + } + + throw new ToolInputError(`Path escapes workspace: ${targetPath}`); +} diff --git a/src/agent/tools/read.ts b/src/agent/tools/read.ts index 0630a23457c1d4cbb2364d58dba77427be7550a0..aa6a30072acf8d7bfc83b1e2cd926d9c9148c873 100644 --- a/src/agent/tools/read.ts +++ b/src/agent/tools/read.ts @@ -1,7 +1,7 @@ import { readFile, stat } from "node:fs/promises"; import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; -import { resolveReadPath } from "./path-utils.js"; +import { resolveReadPath, ensureWorkspacePath } from "./path-utils.js"; import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, truncateHead } from "../../util/truncate.js"; import { ToolInputError } from "../../util/errors.js"; @@ -20,6 +20,7 @@ export const createReadTool = (workspacePath: string): AgentTool => ({ parameters: ReadSchema as any, execute: async (_toolCallId: string, params: any) => { const absolutePath = resolveReadPath(params.path, workspacePath); + ensureWorkspacePath(workspacePath, absolutePath); const fileStats = await stat(absolutePath); if (fileStats.size > MAX_READ_BYTES) { diff --git a/src/workspace/content.ts b/src/workspace/content.ts index 4c30a22eec9948b8a5af004540997967ce96fac8..0af8e87b101c96c4815c1c359fe45329e94a8cd3 100644 --- a/src/workspace/content.ts +++ b/src/workspace/content.ts @@ -1,5 +1,6 @@ import { mkdir, writeFile } from "node:fs/promises"; -import { dirname, join } from "node:path"; +import { dirname, join, resolve, sep } from "node:path"; +import { ToolInputError } from "../util/errors.js"; export interface WorkspaceContent { filePath: string; @@ -7,12 +8,22 @@ export interface WorkspaceContent { bytes: number; } +function ensureContained(workspacePath: string, targetPath: string): void { + const resolved = resolve(workspacePath, targetPath); + const root = workspacePath.endsWith(sep) ? workspacePath : `${workspacePath}${sep}`; + + if (resolved !== workspacePath && !resolved.startsWith(root)) { + throw new ToolInputError(`Path escapes workspace: ${targetPath}`); + } +} + export async function writeWorkspaceFile( workspacePath: string, relativePath: string, content: string, ): Promise { const filePath = join(workspacePath, relativePath); + ensureContained(workspacePath, filePath); await mkdir(dirname(filePath), { recursive: true }); await writeFile(filePath, content, "utf8"); diff --git a/test/workspace-containment.test.ts b/test/workspace-containment.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..b6765b15b33375be2606c961b8eecd09116f3126 --- /dev/null +++ b/test/workspace-containment.test.ts @@ -0,0 +1,237 @@ +import { describe, test, expect, beforeAll, afterAll } from "bun:test"; +import { mkdtemp, rm, writeFile, mkdir } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { ensureWorkspacePath } from "../src/agent/tools/index.js"; +import { expandPath, resolveToCwd, resolveReadPath } from "../src/agent/tools/path-utils.js"; +import { writeWorkspaceFile } from "../src/workspace/content.js"; + +let workspace: string; + +beforeAll(async () => { + workspace = await mkdtemp(join(tmpdir(), "rumilo-test-")); + await mkdir(join(workspace, "subdir"), { recursive: true }); + await writeFile(join(workspace, "hello.txt"), "hello"); + await writeFile(join(workspace, "subdir", "nested.txt"), "nested"); +}); + +afterAll(async () => { + await rm(workspace, { recursive: true, force: true }); +}); + +// ─── ensureWorkspacePath ──────────────────────────────────────────── + +describe("ensureWorkspacePath", () => { + test("allows workspace root itself", () => { + const result = ensureWorkspacePath(workspace, "."); + expect(result).toBe(workspace); + }); + + test("allows a relative child path", () => { + const result = ensureWorkspacePath(workspace, "hello.txt"); + expect(result).toBe(join(workspace, "hello.txt")); + }); + + test("allows nested relative path", () => { + const result = ensureWorkspacePath(workspace, "subdir/nested.txt"); + expect(result).toBe(join(workspace, "subdir", "nested.txt")); + }); + + test("rejects .. traversal escaping workspace", () => { + expect(() => ensureWorkspacePath(workspace, "../../../etc/passwd")).toThrow("Path escapes workspace"); + }); + + test("rejects absolute path outside workspace", () => { + expect(() => ensureWorkspacePath(workspace, "/etc/passwd")).toThrow("Path escapes workspace"); + }); + + test("allows absolute path inside workspace", () => { + const absInside = join(workspace, "hello.txt"); + const result = ensureWorkspacePath(workspace, absInside); + expect(result).toBe(absInside); + }); +}); + +// ─── expandPath: tilde must NOT escape workspace ──────────────────── + +describe("expandPath - tilde handling for workspace sandboxing", () => { + test("tilde alone must not expand to homedir", () => { + const result = expandPath("~"); + // After fix, ~ should remain literal (not expand to homedir) + expect(result).toBe("~"); + }); + + test("tilde-prefixed path must not expand to homedir", () => { + const result = expandPath("~/secret"); + expect(result).not.toContain("/home"); + expect(result).not.toContain("/Users"); + // Should stay as literal path + expect(result).toBe("~/secret"); + }); +}); + +// ─── resolveToCwd: must stay within workspace ─────────────────────── + +describe("resolveToCwd - workspace containment", () => { + test("resolves relative path within workspace", () => { + const result = resolveToCwd("hello.txt", workspace); + expect(result).toBe(join(workspace, "hello.txt")); + }); + + test("resolves '.' to workspace root", () => { + const result = resolveToCwd(".", workspace); + expect(result).toBe(workspace); + }); +}); + +// ─── Tool-level containment (read tool) ───────────────────────────── + +describe("read tool - workspace containment", () => { + let readTool: any; + + beforeAll(async () => { + const { createReadTool } = await import("../src/agent/tools/read.js"); + readTool = createReadTool(workspace); + }); + + test("reads file inside workspace", async () => { + const result = await readTool.execute("id", { path: "hello.txt" }); + expect(result.content[0].text).toBe("hello"); + }); + + test("rejects traversal via ..", async () => { + await expect(readTool.execute("id", { path: "../../etc/passwd" })).rejects.toThrow( + /escapes workspace/i, + ); + }); + + test("rejects absolute path outside workspace", async () => { + await expect(readTool.execute("id", { path: "/etc/passwd" })).rejects.toThrow( + /escapes workspace/i, + ); + }); + + test("tilde path stays within workspace (no homedir expansion)", async () => { + // With tilde expansion removed, ~/foo resolves to /~/foo + // which is safely inside the workspace. It will fail with ENOENT, + // NOT succeed in reading the user's homedir file. + await expect(readTool.execute("id", { path: "~/.bashrc" })).rejects.toThrow(/ENOENT/); + }); +}); + +// ─── Tool-level containment (ls tool) ─────────────────────────────── + +describe("ls tool - workspace containment", () => { + let lsTool: any; + + beforeAll(async () => { + const { createLsTool } = await import("../src/agent/tools/ls.js"); + lsTool = createLsTool(workspace); + }); + + test("lists workspace root", async () => { + const result = await lsTool.execute("id", {}); + expect(result.content[0].text).toContain("hello.txt"); + }); + + test("rejects traversal via ..", async () => { + await expect(lsTool.execute("id", { path: "../../" })).rejects.toThrow( + /escapes workspace/i, + ); + }); + + test("rejects absolute path outside workspace", async () => { + await expect(lsTool.execute("id", { path: "/tmp" })).rejects.toThrow( + /escapes workspace/i, + ); + }); +}); + +// ─── Tool-level containment (grep tool) ───────────────────────────── + +describe("grep tool - workspace containment", () => { + let grepTool: any; + + beforeAll(async () => { + const { createGrepTool } = await import("../src/agent/tools/grep.js"); + grepTool = createGrepTool(workspace); + }); + + test("searches within workspace", async () => { + const result = await grepTool.execute("id", { pattern: "hello", literal: true }); + expect(result.content[0].text).toContain("hello"); + }); + + test("rejects traversal via ..", async () => { + await expect( + grepTool.execute("id", { pattern: "root", path: "../../etc" }), + ).rejects.toThrow(/escapes workspace/i); + }); + + test("rejects absolute path outside workspace", async () => { + await expect( + grepTool.execute("id", { pattern: "root", path: "/etc" }), + ).rejects.toThrow(/escapes workspace/i); + }); +}); + +// ─── Tool-level containment (find tool) ───────────────────────────── + +describe("find tool - workspace containment", () => { + let findTool: any; + + beforeAll(async () => { + const { createFindTool } = await import("../src/agent/tools/find.js"); + findTool = createFindTool(workspace); + }); + + test("finds files in workspace", async () => { + const result = await findTool.execute("id", { pattern: "*.txt" }); + expect(result.content[0].text).toContain("hello.txt"); + }); + + test("rejects traversal via ..", async () => { + await expect( + findTool.execute("id", { pattern: "*", path: "../../" }), + ).rejects.toThrow(/escapes workspace/i); + }); + + test("rejects absolute path outside workspace", async () => { + await expect( + findTool.execute("id", { pattern: "*", path: "/tmp" }), + ).rejects.toThrow(/escapes workspace/i); + }); +}); + +// ─── writeWorkspaceFile containment (Issue #4) ────────────────────── + +describe("writeWorkspaceFile - workspace containment", () => { + test("writes file inside workspace", async () => { + const result = await writeWorkspaceFile(workspace, "output.txt", "data"); + expect(result.filePath).toBe(join(workspace, "output.txt")); + }); + + test("writes nested file inside workspace", async () => { + const result = await writeWorkspaceFile(workspace, "a/b/c.txt", "deep"); + expect(result.filePath).toBe(join(workspace, "a", "b", "c.txt")); + }); + + test("rejects traversal via ..", async () => { + await expect( + writeWorkspaceFile(workspace, "../../../tmp/evil.txt", "pwned"), + ).rejects.toThrow(/escapes workspace/i); + }); + + test("absolute path via join stays inside workspace", async () => { + // path.join(workspace, "/tmp/evil.txt") => "/tmp/evil.txt" + // This is actually inside the workspace — join concatenates, doesn't replace. + const result = await writeWorkspaceFile(workspace, "/tmp/evil.txt", "safe"); + expect(result.filePath).toBe(join(workspace, "tmp", "evil.txt")); + }); + + test("tilde path via join stays inside workspace", async () => { + // With no tilde expansion, ~/evil.txt joins as /~/evil.txt + const result = await writeWorkspaceFile(workspace, "~/evil.txt", "safe"); + expect(result.filePath).toBe(join(workspace, "~", "evil.txt")); + }); +}); diff --git a/tsconfig.json b/tsconfig.json index ac242056f8e59db0f2be9fab23d4177c8f92e6c4..0df20fc50141656a98ba59deffbb6c51d53faa91 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -11,7 +11,7 @@ "allowJs": false, "types": ["bun-types"], "outDir": "dist", - "rootDir": "src", + "rootDir": ".", "skipLibCheck": true }, "include": ["src", "test"],