Guard against symlink escape in workspace sandboxing

Amolith and Shelley created 1 month ago

ensureWorkspacePath() previously used path.resolve() which normalizes ..
segments but does not follow symlinks. A symlink inside the workspace
pointing outside (e.g. <workspace>/escape -> /etc) would pass the textual
prefix check and let tools operate beyond the sandbox boundary.

Fix:
- Add safeRealpath() that calls fs.realpathSync() and walks up to the
  nearest existing ancestor on ENOENT (needed for write targets that
  don't exist yet)
- ensureWorkspacePath() now resolves both the workspace and target via
  realpath before checking containment
- The cheap textual check runs first as a fast path; the realpath check
  catches symlink-based escapes

Tests: 9 new cases covering symlink dirs/files pointing outside,
symlinks within workspace (allowed), nested escapes, non-existent write
targets through escaped parents, and writeWorkspaceFile integration.

Co-authored-by: Shelley <[e-mail]>

Change summary

src/agent/tools/path-utils.ts      | 44 +++++++++++++-
test/workspace-containment.test.ts | 93 ++++++++++++++++++++++++++++++++
2 files changed, 132 insertions(+), 5 deletions(-)

Detailed changes

src/agent/tools/path-utils.ts

@@ -1,5 +1,5 @@
-import { accessSync, constants } from "node:fs";
-import { isAbsolute, resolve as resolvePath, sep } from "node:path";
+import { accessSync, constants, realpathSync } from "node:fs";
+import { dirname, isAbsolute, resolve as resolvePath, sep } from "node:path";
 import { ToolInputError } from "../../util/errors.js";
 
 const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g;
@@ -67,13 +67,47 @@ export function resolveReadPath(filePath: string, cwd: string): string {
 	return resolved;
 }
 
+/**
+ * Resolve the real path of `p`, following symlinks. If `p` does not exist,
+ * walk up to the nearest existing ancestor, resolve *that*, and re-append
+ * the remaining segments. This lets us validate write targets that don't
+ * exist yet while still catching symlink escapes in any ancestor directory.
+ */
+function safeRealpath(p: string): string {
+	try {
+		return realpathSync(p);
+	} catch (err: any) {
+		if (err?.code === "ENOENT") {
+			const parent = dirname(p);
+			if (parent === p) {
+				// filesystem root — nothing more to resolve
+				return p;
+			}
+			const realParent = safeRealpath(parent);
+			const tail = p.slice(parent.length);
+			return realParent + tail;
+		}
+		throw err;
+	}
+}
+
 export function ensureWorkspacePath(workspacePath: string, targetPath: string): string {
 	const resolved = resolvePath(workspacePath, targetPath);
+
+	// Quick textual check first (catches the common case cheaply)
 	const root = workspacePath.endsWith(sep) ? workspacePath : `${workspacePath}${sep}`;
+	if (resolved !== workspacePath && !resolved.startsWith(root)) {
+		throw new ToolInputError(`Path escapes workspace: ${targetPath}`);
+	}
 
-	if (resolved === workspacePath || resolved.startsWith(root)) {
-		return resolved;
+	// Resolve symlinks to catch symlink-based escapes
+	const realWorkspace = safeRealpath(workspacePath);
+	const realTarget = safeRealpath(resolved);
+	const realRoot = realWorkspace.endsWith(sep) ? realWorkspace : `${realWorkspace}${sep}`;
+
+	if (realTarget !== realWorkspace && !realTarget.startsWith(realRoot)) {
+		throw new ToolInputError(`Path escapes workspace via symlink: ${targetPath}`);
 	}
 
-	throw new ToolInputError(`Path escapes workspace: ${targetPath}`);
+	return resolved;
 }

test/workspace-containment.test.ts

@@ -1,5 +1,6 @@
 import { describe, test, expect, beforeAll, afterAll } from "bun:test";
 import { mkdtemp, rm, writeFile, mkdir } from "node:fs/promises";
+import { symlinkSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join, resolve } from "node:path";
 import { ensureWorkspacePath } from "../src/agent/tools/index.js";
@@ -235,3 +236,95 @@ describe("writeWorkspaceFile - workspace containment", () => {
 		expect(result.filePath).toBe(join(workspace, "~", "evil.txt"));
 	});
 });
+
+// ─── Symlink containment ────────────────────────────────────────────
+
+describe("symlink containment", () => {
+	let symlinkWorkspace: string;
+	let outsideDir: string;
+
+	beforeAll(async () => {
+		symlinkWorkspace = await mkdtemp(join(tmpdir(), "rumilo-symlink-test-"));
+		outsideDir = await mkdtemp(join(tmpdir(), "rumilo-outside-"));
+
+		// Create a regular file inside workspace
+		await writeFile(join(symlinkWorkspace, "legit.txt"), "safe content");
+
+		// Create a subdirectory inside workspace
+		await mkdir(join(symlinkWorkspace, "subdir"), { recursive: true });
+		await writeFile(join(symlinkWorkspace, "subdir", "inner.txt"), "inner content");
+
+		// Create a file outside workspace
+		await writeFile(join(outsideDir, "secret.txt"), "secret content");
+
+		// Symlink inside workspace pointing outside
+		symlinkSync(outsideDir, join(symlinkWorkspace, "escape-link"));
+
+		// Symlink inside workspace pointing to file outside
+		symlinkSync(join(outsideDir, "secret.txt"), join(symlinkWorkspace, "secret-link.txt"));
+
+		// Symlink inside workspace pointing to a file inside workspace (benign)
+		symlinkSync(join(symlinkWorkspace, "legit.txt"), join(symlinkWorkspace, "good-link.txt"));
+
+		// Nested symlink escape: subdir/nested-escape -> outsideDir
+		symlinkSync(outsideDir, join(symlinkWorkspace, "subdir", "nested-escape"));
+	});
+
+	afterAll(async () => {
+		await rm(symlinkWorkspace, { recursive: true, force: true });
+		await rm(outsideDir, { recursive: true, force: true });
+	});
+
+	test("rejects symlink directory pointing outside workspace", () => {
+		expect(() =>
+			ensureWorkspacePath(symlinkWorkspace, "escape-link/secret.txt"),
+		).toThrow(/escapes workspace via symlink/);
+	});
+
+	test("rejects symlink file pointing outside workspace", () => {
+		expect(() =>
+			ensureWorkspacePath(symlinkWorkspace, "secret-link.txt"),
+		).toThrow(/escapes workspace via symlink/);
+	});
+
+	test("allows symlink pointing within workspace", () => {
+		const result = ensureWorkspacePath(symlinkWorkspace, "good-link.txt");
+		expect(result).toBe(join(symlinkWorkspace, "good-link.txt"));
+	});
+
+	test("rejects nested symlink escape (subdir/nested-escape)", () => {
+		expect(() =>
+			ensureWorkspacePath(symlinkWorkspace, "subdir/nested-escape/secret.txt"),
+		).toThrow(/escapes workspace via symlink/);
+	});
+
+	test("rejects symlink escape via directory symlink alone", () => {
+		expect(() =>
+			ensureWorkspacePath(symlinkWorkspace, "escape-link"),
+		).toThrow(/escapes workspace via symlink/);
+	});
+
+	test("handles non-existent file in real directory (write target)", () => {
+		// File doesn't exist but parent is a real dir inside workspace — should pass
+		const result = ensureWorkspacePath(symlinkWorkspace, "subdir/new-file.txt");
+		expect(result).toBe(join(symlinkWorkspace, "subdir", "new-file.txt"));
+	});
+
+	test("rejects non-existent file under symlink-escaped parent", () => {
+		// Parent is a symlink pointing outside — even though target file doesn't exist
+		expect(() =>
+			ensureWorkspacePath(symlinkWorkspace, "escape-link/new-file.txt"),
+		).toThrow(/escapes workspace via symlink/);
+	});
+
+	test("writeWorkspaceFile rejects path through symlink escape", async () => {
+		await expect(
+			writeWorkspaceFile(symlinkWorkspace, "escape-link/evil.txt", "pwned"),
+		).rejects.toThrow(/escapes workspace via symlink/);
+	});
+
+	test("writeWorkspaceFile allows normal nested write", async () => {
+		const result = await writeWorkspaceFile(symlinkWorkspace, "new-dir/file.txt", "ok");
+		expect(result.filePath).toBe(join(symlinkWorkspace, "new-dir", "file.txt"));
+	});
+});