Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { randomUUID } from "node:crypto";
import {
existsSync,
mkdirSync,
readFileSync,
rmSync,
symlinkSync,
writeFileSync,
Expand Down Expand Up @@ -69,19 +70,17 @@ describe("bug-hunt-2: symlink and additional sandbox probes", () => {
).resolves.toMatchObject({ kind: "text", content: "PII" });
});

test("writeFile through a symlinked dir into outside the workspace is rejected", async () => {
const link = join(repo.repoPath, "evil-link");
test("writeFile writes through a symlinked dir that points outside the workspace", async () => {
const link = join(repo.repoPath, "external-link");
symlinkSync(outsideDir, link);

await expect(
host.trpc.filesystem.writeFile.mutate({
workspaceId,
absolutePath: join(link, "planted.txt"),
content: "should-not-write",
options: { create: true, overwrite: true },
}),
).rejects.toThrow();
expect(existsSync(join(outsideDir, "planted.txt"))).toBe(false);
await host.trpc.filesystem.writeFile.mutate({
workspaceId,
absolutePath: join(link, "edited.txt"),
content: "edited",
options: { create: true, overwrite: true },
});
expect(readFileSync(join(outsideDir, "edited.txt"), "utf8")).toBe("edited");
});

test("createDirectory rejects '..' traversal", async () => {
Expand Down
35 changes: 19 additions & 16 deletions packages/host-service/test/integration/bug-hunt.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
* bug worth fixing.
*
* The filesystem section also pins the intended sandbox policy in both
* directions: reads are host-wide (viewing files a terminal/agent referenced
* outside the workspace), mutations are confined to the workspace root. An
* "allows" test failing means the read policy regressed, not that a defense
* appeared.
* directions: reads and file edits are host-wide (files a terminal/agent
* referenced outside the workspace), structural mutations are confined to the
* workspace root. An "allows"/"edits" test failing means that policy
* regressed, not that a defense appeared.
*
* Categories:
* - sandbox / path traversal in workspace-fs operations
Expand All @@ -22,6 +22,7 @@ import { randomUUID } from "node:crypto";
import {
existsSync,
mkdirSync,
readFileSync,
rmSync,
symlinkSync,
writeFileSync,
Expand All @@ -33,7 +34,7 @@ import { projects, workspaces } from "../../src/db/schema";
import { createTestHost, type TestHost } from "../helpers/createTestHost";
import { createGitFixture, type GitFixture } from "../helpers/git-fixture";

describe("bug-hunt: filesystem sandbox (mutations confined, reads host-wide)", () => {
describe("bug-hunt: filesystem sandbox (structural mutations confined, reads and edits host-wide)", () => {
let host: TestHost;
let repo: GitFixture;
const projectId = randomUUID();
Expand Down Expand Up @@ -62,18 +63,20 @@ describe("bug-hunt: filesystem sandbox (mutations confined, reads host-wide)", (
repo.dispose();
});

test("writeFile rejects '..' traversal escaping the workspace root", async () => {
const escapeWritePath = `${repo.repoPath}/../escape.txt`;
await expect(
host.trpc.filesystem.writeFile.mutate({
test("writeFile edits paths outside the workspace root", async () => {
const sibling = join(repo.repoPath, "..", `outside-write-${randomUUID()}`);
writeFileSync(sibling, "before");
try {
await host.trpc.filesystem.writeFile.mutate({
workspaceId,
absolutePath: escapeWritePath,
content: "should not exist",
options: { create: true, overwrite: true },
}),
).rejects.toThrow();
// Sibling of repoPath must not have been written.
expect(existsSync(escapeWritePath)).toBe(false);
absolutePath: sibling,
content: "after",
options: { create: false, overwrite: true },
});
expect(readFileSync(sibling, "utf8")).toBe("after");
} finally {
rmSync(sibling, { force: true });
}
});

test("readFile allows viewing paths outside the workspace root", async () => {
Expand Down
41 changes: 15 additions & 26 deletions packages/workspace-fs/src/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,18 +188,15 @@ describe("readFile", () => {
});

describe("writeFile", () => {
it("rejects paths outside the workspace root", async () => {
const rootPath = await createTempRoot();
it("writes files outside the workspace root", async () => {
const outsideRoot = await createTempRoot();
const absolutePath = path.join(outsideRoot, "escape.txt");
const absolutePath = path.join(outsideRoot, "outside.txt");
await fs.writeFile(absolutePath, "before");

await expect(
writeFile({
rootPath,
absolutePath,
content: "should not exist",
}),
).rejects.toThrow("outside workspace root");
const result = await writeFile({ absolutePath, content: "after" });

expect(result.ok).toEqual(true);
expect(await fs.readFile(absolutePath, "utf-8")).toEqual("after");
});

it("returns a conflict when revision does not match", async () => {
Expand All @@ -208,7 +205,6 @@ describe("writeFile", () => {
await fs.writeFile(absolutePath, "current");

const result = await writeFile({
rootPath,
absolutePath,
content: "next",
precondition: { ifMatch: "stale-revision" },
Expand All @@ -233,7 +229,6 @@ describe("writeFile", () => {
});

const result = await writeFile({
rootPath,
absolutePath,
content: "updated",
precondition: { ifMatch: readResult.revision },
Expand All @@ -249,7 +244,6 @@ describe("writeFile", () => {
await fs.writeFile(absolutePath, "content");

const result = await writeFile({
rootPath,
absolutePath,
content: "new content",
options: { create: true, overwrite: false },
Expand All @@ -266,7 +260,6 @@ describe("writeFile", () => {
const absolutePath = path.join(rootPath, "missing.txt");

const result = await writeFile({
rootPath,
absolutePath,
content: "content",
options: { create: false, overwrite: true },
Expand All @@ -292,13 +285,11 @@ describe("writeFile", () => {

const [firstResult, secondResult] = await Promise.all([
writeFile({
rootPath,
absolutePath,
content: "first",
precondition: { ifMatch: revision },
}),
writeFile({
rootPath,
absolutePath,
content: "second",
precondition: { ifMatch: revision },
Expand All @@ -321,7 +312,6 @@ describe("writeFile", () => {
await fs.symlink(realPath, absolutePath);

const result = await writeFile({
rootPath,
absolutePath,
content: "after",
});
Expand All @@ -345,7 +335,6 @@ describe("writeFile", () => {
encoding: "utf-8",
});
const result = await writeFile({
rootPath,
absolutePath,
content: "after",
precondition: { ifMatch: readResult.revision },
Expand All @@ -356,26 +345,26 @@ describe("writeFile", () => {
expect(await fs.readFile(realPath, "utf-8")).toEqual("after");
});

it("still refuses a symlink that escapes the workspace root", async () => {
it("writes through an in-root symlink that resolves outside the root", async () => {
const rootPath = await createTempRoot();
const outsideRoot = await createTempRoot();
const outsidePath = path.join(outsideRoot, "secret.txt");
await fs.writeFile(outsidePath, "secret");
const outsidePath = path.join(outsideRoot, "target.txt");
await fs.writeFile(outsidePath, "before");
const absolutePath = path.join(rootPath, "link.txt");
await fs.symlink(outsidePath, absolutePath);

await expect(
writeFile({ rootPath, absolutePath, content: "leaked" }),
).rejects.toThrow("outside workspace root");
expect(await fs.readFile(outsidePath, "utf-8")).toEqual("secret");
const result = await writeFile({ absolutePath, content: "after" });

expect(result.ok).toEqual(true);
expect((await fs.lstat(absolutePath)).isSymbolicLink()).toEqual(true);
expect(await fs.readFile(outsidePath, "utf-8")).toEqual("after");
});

it("writes Uint8Array content", async () => {
const rootPath = await createTempRoot();
const absolutePath = path.join(rootPath, "binary.bin");

const result = await writeFile({
rootPath,
absolutePath,
content: new Uint8Array([0x48, 0x65, 0x6c, 0x6c, 0x6f]),
});
Expand Down
19 changes: 6 additions & 13 deletions packages/workspace-fs/src/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,19 +346,16 @@ async function resolveWriteTarget(absolutePath: string): Promise<string> {
}

async function writeAtomically({
rootPath,
absolutePath,
content,
encoding,
}: {
rootPath: string;
absolutePath: string;
content: string | Uint8Array;
encoding?: string;
}): Promise<void> {
const targetPath = await resolveWriteTarget(absolutePath);
const tempPath = `${targetPath}.superset-tmp-${randomUUID()}`;
await assertParentWithinRoot(rootPath, tempPath);

let sourceMode: number | undefined;
try {
Expand Down Expand Up @@ -388,11 +385,11 @@ async function writeAtomically({
// per-entry stat calls bounds how much zombie work continues after an abort.
const LIST_DIRECTORY_STAT_BATCH_SIZE = 16;

// Read-only operations (listDirectory, readFile, getMetadata) are not
// confined to the workspace root: terminals and agents routinely reference
// files anywhere on the host, and viewing them is within the caller's trust
// model (statPath/browseHost already expose arbitrary host paths). Mutations
// remain strictly confined to the root.
// listDirectory, readFile, getMetadata and writeFile are not confined to the
// workspace root: terminals and agents routinely reference files anywhere on
// the host, and viewing or editing them is within the caller's trust model
// (the same caller can run any command in a terminal). Structural mutations
// (create, delete, move, copy) remain confined to the root.
export async function listDirectory({
absolutePath,
signal,
Expand Down Expand Up @@ -536,22 +533,19 @@ export async function getMetadata({
}

export async function writeFile({
rootPath,
absolutePath,
content,
encoding,
options,
precondition,
}: {
rootPath: string;
absolutePath: string;
content: string | Uint8Array;
encoding?: string;
options?: { create: boolean; overwrite: boolean };
precondition?: { ifMatch: string };
}): Promise<FsWriteResult> {
const targetPath = ensureWithinRoot({ rootPath, absolutePath });
await assertRealpathWithinRoot(rootPath, targetPath);
const targetPath = normalizeAbsolutePath(absolutePath);

const create = options?.create ?? true;
const overwrite = options?.overwrite ?? true;
Expand Down Expand Up @@ -604,7 +598,6 @@ export async function writeFile({
}

await writeAtomically({
rootPath,
absolutePath: targetPath,
content,
encoding,
Expand Down
1 change: 0 additions & 1 deletion packages/workspace-fs/src/host/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ export function createFsHostService(

async writeFile(input) {
return await writeFile({
rootPath,
absolutePath: input.absolutePath,
content: input.content,
encoding: input.encoding,
Expand Down
Loading