diff options
| -rw-r--r-- | packages/tool-edit-file/src/edit-file.test.ts | 68 | ||||
| -rw-r--r-- | packages/tool-edit-file/src/edit-file.ts | 47 | ||||
| -rw-r--r-- | packages/tool-read-file/src/read-file.test.ts | 98 | ||||
| -rw-r--r-- | packages/tool-read-file/src/read-file.ts | 47 | ||||
| -rw-r--r-- | packages/tool-write-file/src/index.ts | 1 | ||||
| -rw-r--r-- | packages/tool-write-file/src/write-file.test.ts | 60 | ||||
| -rw-r--r-- | packages/tool-write-file/src/write-file.ts | 68 |
7 files changed, 9 insertions, 380 deletions
diff --git a/packages/tool-edit-file/src/edit-file.test.ts b/packages/tool-edit-file/src/edit-file.test.ts index dba3d9e..5ef8376 100644 --- a/packages/tool-edit-file/src/edit-file.test.ts +++ b/packages/tool-edit-file/src/edit-file.test.ts @@ -3,12 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { createLogger, type ToolExecuteContext } from "@dispatch/kernel"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { - computeReplacement, - createEditFileTool, - isPathWithinWorkdir, - validateArgs, -} from "./edit-file.js"; +import { computeReplacement, createEditFileTool, validateArgs } from "./edit-file.js"; function stubCtx(overrides?: Partial<ToolExecuteContext>): ToolExecuteContext { return { @@ -145,24 +140,6 @@ describe("computeReplacement", () => { }); }); -describe("isPathWithinWorkdir", () => { - it("accepts a path within workdir", () => { - expect(isPathWithinWorkdir("/tmp/workdir/file.txt", "/tmp/workdir")).toBe(true); - }); - - it("accepts the workdir itself", () => { - expect(isPathWithinWorkdir("/tmp/workdir", "/tmp/workdir")).toBe(true); - }); - - it("rejects a path outside workdir", () => { - expect(isPathWithinWorkdir("/tmp/other/file.txt", "/tmp/workdir")).toBe(false); - }); - - it("rejects a prefix attack (workdir prefix but different dir)", () => { - expect(isPathWithinWorkdir("/tmp/workdir-evil/file.txt", "/tmp/workdir")).toBe(false); - }); -}); - describe("createEditFileTool", () => { it("replaces a single occurrence", async () => { const filePath = join(workdir, "test.txt"); @@ -251,49 +228,6 @@ describe("createEditFileTool", () => { expect(result.content).toContain("not found"); }); - it("rejects a path outside the working directory", async () => { - const tool = createEditFileTool(workdir); - const result = await tool.execute( - { path: "../escape.txt", oldString: "a", newString: "b" }, - stubCtx(), - ); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - - it("rejects an absolute path outside workdir", async () => { - const tool = createEditFileTool(workdir); - const result = await tool.execute( - { path: "/etc/passwd", oldString: "a", newString: "b" }, - stubCtx(), - ); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - - it("handles symlink escape attempt", async () => { - const outsideDir = await mkdtemp(join(tmpdir(), "outside-")); - const outsideFile = join(outsideDir, "secret.txt"); - await writeFile(outsideFile, "secret data", "utf8"); - - const symlinkPath = join(workdir, "link.txt"); - const { symlink } = await import("node:fs/promises"); - await symlink(outsideFile, symlinkPath); - - const tool = createEditFileTool(workdir); - const result = await tool.execute( - { path: "link.txt", oldString: "secret", newString: "leaked" }, - stubCtx(), - ); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - - await rm(outsideDir, { recursive: true, force: true }); - }); - it("reads file under ctx.cwd when set", async () => { const ctxDir = await mkdtemp(join(tmpdir(), "ctx-cwd-test-")); try { diff --git a/packages/tool-edit-file/src/edit-file.ts b/packages/tool-edit-file/src/edit-file.ts index af630aa..36f99e0 100644 --- a/packages/tool-edit-file/src/edit-file.ts +++ b/packages/tool-edit-file/src/edit-file.ts @@ -1,5 +1,5 @@ -import { readFile, realpath, writeFile } from "node:fs/promises"; -import { resolve, sep } from "node:path"; +import { readFile, writeFile } from "node:fs/promises"; +import { resolve } from "node:path"; import type { ToolContract, ToolResult } from "@dispatch/kernel"; // --- Pure types --- @@ -103,12 +103,6 @@ export function computeReplacement( }; } -/** Pure: check that a resolved absolute path is within the workdir (prefix check). */ -export function isPathWithinWorkdir(resolvedPath: string, workdir: string): boolean { - const normalizedWorkdir = workdir.endsWith(sep) ? workdir : workdir + sep; - return resolvedPath === workdir || resolvedPath.startsWith(normalizedWorkdir); -} - // --- Shell / edge --- /** @@ -156,46 +150,9 @@ export function createEditFileTool(workingDirectory: string): ToolContract { const { path: relPath, oldString, newString, replaceAll } = validated; - // Effective base: per-turn ctx.cwd overrides the baked workdir. const effectiveBase = ctx.cwd ? resolve(ctx.cwd) : workdir; - - // Resolve the requested path against the effective base. const resolvedPath = resolve(effectiveBase, relPath); - // Basic prefix check. - if (!isPathWithinWorkdir(resolvedPath, effectiveBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - - // Symlink hardening: realpath both and re-check containment. - let realResolved: string; - let realBase: string; - try { - [realResolved, realBase] = await Promise.all([ - realpath(resolvedPath), - realpath(effectiveBase), - ]); - } catch (err: unknown) { - const code = (err as NodeJS.ErrnoException).code; - if (code === "ENOENT") { - return { content: `Error: File "${relPath}" not found.`, isError: true }; - } - return { - content: `Error accessing file: ${err instanceof Error ? err.message : String(err)}`, - isError: true, - }; - } - - if (!isPathWithinWorkdir(realResolved, realBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - // Read the file. let content: string; try { diff --git a/packages/tool-read-file/src/read-file.test.ts b/packages/tool-read-file/src/read-file.test.ts index 25b29ff..619ba34 100644 --- a/packages/tool-read-file/src/read-file.test.ts +++ b/packages/tool-read-file/src/read-file.test.ts @@ -6,7 +6,6 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { createReadFileTool, formatDirectoryEntries, - isPathWithinWorkdir, renderLines, sliceLines, validateArgs, @@ -108,24 +107,6 @@ describe("sliceLines", () => { }); }); -describe("isPathWithinWorkdir", () => { - it("accepts a path within workdir", () => { - expect(isPathWithinWorkdir("/tmp/workdir/file.txt", "/tmp/workdir")).toBe(true); - }); - - it("accepts the workdir itself", () => { - expect(isPathWithinWorkdir("/tmp/workdir", "/tmp/workdir")).toBe(true); - }); - - it("rejects a path outside workdir", () => { - expect(isPathWithinWorkdir("/tmp/other/file.txt", "/tmp/workdir")).toBe(false); - }); - - it("rejects a prefix attack (workdir prefix but different dir)", () => { - expect(isPathWithinWorkdir("/tmp/workdir-evil/file.txt", "/tmp/workdir")).toBe(false); - }); -}); - describe("renderLines", () => { it("renders lines with 1-indexed line numbers", () => { const result = renderLines(["a", "b", "c"], 1); @@ -197,22 +178,6 @@ describe("createReadFileTool", () => { expect(result.content).toContain("not found"); }); - it("returns error for path escape via ..", async () => { - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "../escape.txt" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - - it("returns error for absolute path outside workdir", async () => { - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "/etc/passwd" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - it("returns empty-file content for empty file", async () => { const filePath = join(workdir, "empty.txt"); await writeFile(filePath, "", "utf8"); @@ -247,26 +212,6 @@ describe("createReadFileTool", () => { } }); - it("handles symlink escape attempt", async () => { - // Create a symlink inside workdir pointing outside - const outsideDir = await mkdtemp(join(tmpdir(), "outside-")); - const outsideFile = join(outsideDir, "secret.txt"); - await writeFile(outsideFile, "secret data", "utf8"); - - const symlinkPath = join(workdir, "link.txt"); - const { symlink } = await import("node:fs/promises"); - await symlink(outsideFile, symlinkPath); - - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "link.txt" }, stubCtx()); - - // The symlink resolves to outside workdir, so should be rejected - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - - await rm(outsideDir, { recursive: true, force: true }); - }); - it("concurrencySafe is true", () => { const tool = createReadFileTool(workdir); expect(tool.concurrencySafe).toBe(true); @@ -296,41 +241,6 @@ describe("createReadFileTool", () => { } }); - it("rejects path escaping ctx.cwd via ..", async () => { - const ctxDir = await mkdtemp(join(tmpdir(), "ctx-escape-test-")); - try { - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "../escape.txt" }, stubCtx({ cwd: ctxDir })); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - } finally { - await rm(ctxDir, { recursive: true, force: true }); - } - }); - - it("rejects symlink escaping ctx.cwd", async () => { - const ctxDir = await mkdtemp(join(tmpdir(), "ctx-symlink-test-")); - const outsideDir = await mkdtemp(join(tmpdir(), "ctx-outside-")); - try { - const outsideFile = join(outsideDir, "secret.txt"); - await writeFile(outsideFile, "secret data", "utf8"); - - const symlinkPath = join(ctxDir, "link.txt"); - const { symlink } = await import("node:fs/promises"); - await symlink(outsideFile, symlinkPath); - - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "link.txt" }, stubCtx({ cwd: ctxDir })); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - } finally { - await rm(ctxDir, { recursive: true, force: true }); - await rm(outsideDir, { recursive: true, force: true }); - } - }); - it("falls back to baked workdir when ctx.cwd is omitted", async () => { const filePath = join(workdir, "baked-file.txt"); await writeFile(filePath, "from baked workdir", "utf8"); @@ -377,14 +287,6 @@ describe("createReadFileTool", () => { expect(result.content).toBe("2: b\n3: c\n4: d"); }); - it("rejects a directory path outside the working directory (containment still enforced)", async () => { - const tool = createReadFileTool(workdir); - const result = await tool.execute({ path: "../outside-dir" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - it("returns not-found for a nonexistent path", async () => { const tool = createReadFileTool(workdir); const result = await tool.execute({ path: "nonexistent-path" }, stubCtx()); diff --git a/packages/tool-read-file/src/read-file.ts b/packages/tool-read-file/src/read-file.ts index 99b396e..216f165 100644 --- a/packages/tool-read-file/src/read-file.ts +++ b/packages/tool-read-file/src/read-file.ts @@ -1,5 +1,5 @@ -import { readdir, readFile, realpath, stat } from "node:fs/promises"; -import { resolve, sep } from "node:path"; +import { readdir, readFile, stat } from "node:fs/promises"; +import { resolve } from "node:path"; import type { ToolContract, ToolResult } from "@dispatch/kernel"; const DEFAULT_LIMIT = 500; @@ -59,12 +59,6 @@ export function sliceLines( return { lines: sliced, totalLines }; } -/** Pure: check that a resolved absolute path is within the workdir (prefix check). */ -export function isPathWithinWorkdir(resolvedPath: string, workdir: string): boolean { - const normalizedWorkdir = workdir.endsWith(sep) ? workdir : workdir + sep; - return resolvedPath === workdir || resolvedPath.startsWith(normalizedWorkdir); -} - /** Pure: render lines into a string with line numbers. */ export function renderLines(lines: readonly string[], offset: number): string { return lines.map((line, i) => `${offset + i}: ${line}`).join("\n"); @@ -131,46 +125,9 @@ export function createReadFileTool(workingDirectory: string): ToolContract { const { path: relPath, offset, limit } = validated; - // Effective base: per-turn ctx.cwd overrides the baked workdir. const effectiveBase = ctx.cwd ? resolve(ctx.cwd) : workdir; - - // Resolve the requested path against the effective base. const resolvedPath = resolve(effectiveBase, relPath); - // Basic prefix check (catches ".." and absolute paths outside effectiveBase). - if (!isPathWithinWorkdir(resolvedPath, effectiveBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - - // Symlink hardening: realpath both and re-check containment. - let realResolved: string; - let realBase: string; - try { - [realResolved, realBase] = await Promise.all([ - realpath(resolvedPath), - realpath(effectiveBase), - ]); - } catch (err: unknown) { - const code = (err as NodeJS.ErrnoException).code; - if (code === "ENOENT") { - return { content: `Error: File "${relPath}" not found.`, isError: true }; - } - return { - content: `Error reading file: ${err instanceof Error ? err.message : String(err)}`, - isError: true, - }; - } - - if (!isPathWithinWorkdir(realResolved, realBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - // Stat to determine if this is a file or directory. let pathStat: import("node:fs").Stats; try { diff --git a/packages/tool-write-file/src/index.ts b/packages/tool-write-file/src/index.ts index 521a429..8a2cb36 100644 --- a/packages/tool-write-file/src/index.ts +++ b/packages/tool-write-file/src/index.ts @@ -2,6 +2,5 @@ export { extension } from "./extension.js"; export { createWriteFileTool, decideOverwrite, - isPathWithinWorkdir, validateArgs, } from "./write-file.js"; diff --git a/packages/tool-write-file/src/write-file.test.ts b/packages/tool-write-file/src/write-file.test.ts index cf4fa64..6b316bc 100644 --- a/packages/tool-write-file/src/write-file.test.ts +++ b/packages/tool-write-file/src/write-file.test.ts @@ -3,12 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { createLogger, type ToolExecuteContext } from "@dispatch/kernel"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { - createWriteFileTool, - decideOverwrite, - isPathWithinWorkdir, - validateArgs, -} from "./write-file.js"; +import { createWriteFileTool, decideOverwrite, validateArgs } from "./write-file.js"; function stubCtx(overrides?: Partial<ToolExecuteContext>): ToolExecuteContext { return { @@ -75,24 +70,6 @@ describe("decideOverwrite", () => { }); }); -describe("isPathWithinWorkdir", () => { - it("accepts a path within workdir", () => { - expect(isPathWithinWorkdir("/tmp/workdir/file.txt", "/tmp/workdir")).toBe(true); - }); - - it("accepts the workdir itself", () => { - expect(isPathWithinWorkdir("/tmp/workdir", "/tmp/workdir")).toBe(true); - }); - - it("rejects a path outside workdir", () => { - expect(isPathWithinWorkdir("/tmp/other/file.txt", "/tmp/workdir")).toBe(false); - }); - - it("rejects a prefix attack (workdir prefix but different dir)", () => { - expect(isPathWithinWorkdir("/tmp/workdir-evil/file.txt", "/tmp/workdir")).toBe(false); - }); -}); - describe("validateArgs", () => { it("returns validated args for valid input", () => { const result = validateArgs({ path: "foo.txt", content: "hello" }); @@ -192,23 +169,7 @@ describe("createWriteFileTool", () => { const result = await tool.execute({ path: "no/such/dir/file.txt", content: "data" }, stubCtx()); expect(result.isError).toBe(true); - expect(result.content).toContain("Parent directory"); - }); - - it("rejects a path outside the working directory", async () => { - const tool = createWriteFileTool(workdir); - const result = await tool.execute({ path: "../escape.txt", content: "data" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - }); - - it("rejects an absolute path outside workdir", async () => { - const tool = createWriteFileTool(workdir); - const result = await tool.execute({ path: "/tmp/escape.txt", content: "data" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); + expect(result.content).toContain("Error"); }); it("concurrencySafe is false", () => { @@ -253,23 +214,6 @@ describe("createWriteFileTool", () => { } }); - it("handles symlink escape attempt", async () => { - const outsideDir = await mkdtemp(join(tmpdir(), "outside-")); - try { - const symlinkPath = join(workdir, "link.txt"); - const { symlink } = await import("node:fs/promises"); - await symlink(join(outsideDir, "target.txt"), symlinkPath); - - const tool = createWriteFileTool(workdir); - const result = await tool.execute({ path: "link.txt", content: "escape" }, stubCtx()); - - expect(result.isError).toBe(true); - expect(result.content).toContain("outside the working directory"); - } finally { - await rm(outsideDir, { recursive: true, force: true }); - } - }); - it("writes empty content", async () => { const tool = createWriteFileTool(workdir); const result = await tool.execute({ path: "empty.txt", content: "" }, stubCtx()); diff --git a/packages/tool-write-file/src/write-file.ts b/packages/tool-write-file/src/write-file.ts index 16cfc83..1317ce8 100644 --- a/packages/tool-write-file/src/write-file.ts +++ b/packages/tool-write-file/src/write-file.ts @@ -1,5 +1,5 @@ -import { access, lstat, readlink, realpath, stat, writeFile } from "node:fs/promises"; -import { dirname, resolve, sep } from "node:path"; +import { access, stat, writeFile } from "node:fs/promises"; +import { resolve } from "node:path"; import type { ToolContract, ToolResult } from "@dispatch/kernel"; interface ValidatedArgs { @@ -20,12 +20,6 @@ export function decideOverwrite(fileExists: boolean, overwrite: boolean): Overwr return { error: "Error: overwrite: true but the file does not exist." }; } -/** Pure: check that a resolved absolute path is within the workdir (prefix check). */ -export function isPathWithinWorkdir(resolvedPath: string, workdir: string): boolean { - const normalizedWorkdir = workdir.endsWith(sep) ? workdir : workdir + sep; - return resolvedPath === workdir || resolvedPath.startsWith(normalizedWorkdir); -} - /** Pure: validate and coerce args from the model. */ export function validateArgs(args: unknown): ValidatedArgs | { readonly error: string } { if (args === null || args === undefined || typeof args !== "object") { @@ -103,64 +97,6 @@ export function createWriteFileTool(workingDirectory: string): ToolContract { const effectiveBase = ctx.cwd ? resolve(ctx.cwd) : workdir; const resolvedPath = resolve(effectiveBase, relPath); - if (!isPathWithinWorkdir(resolvedPath, effectiveBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - - // Symlink hardening: realpath the parent directory and the base, then re-check. - let realParent: string; - let realBase: string; - try { - const parentDir = dirname(resolvedPath); - [realParent, realBase] = await Promise.all([realpath(parentDir), realpath(effectiveBase)]); - } catch (err: unknown) { - const code = (err as NodeJS.ErrnoException).code; - if (code === "ENOENT") { - return { - content: `Error: Parent directory for "${relPath}" does not exist.`, - isError: true, - }; - } - return { - content: `Error resolving path: ${err instanceof Error ? err.message : String(err)}`, - isError: true, - }; - } - - const realResolvedPath = realParent + sep + resolvedPath.split(sep).at(-1); - if (!isPathWithinWorkdir(realResolvedPath, realBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - - // If the resolved path itself is a symlink, verify the target is contained. - try { - const linkStat = await lstat(resolvedPath); - if (linkStat.isSymbolicLink()) { - const linkTarget = await readlink(resolvedPath); - const resolvedTarget = resolve(dirname(resolvedPath), linkTarget); - if (!isPathWithinWorkdir(resolvedTarget, realBase)) { - return { - content: `Error: Path "${relPath}" is outside the working directory.`, - isError: true, - }; - } - } - } catch (err: unknown) { - const code = (err as NodeJS.ErrnoException).code; - if (code !== "ENOENT") { - return { - content: `Error checking path: ${err instanceof Error ? err.message : String(err)}`, - isError: true, - }; - } - } - // Check existence. let fileExists = false; try { |
