From 867070f575e578e6a848b2340c66b2cbcc847971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 26 Jun 2026 18:04:13 -0400 Subject: [PATCH] Contain workspace file ingestion to the bound folder (sec #3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeDeliverable guards against path traversal, but recordWorkspaceFile resolved and readFileSync'd its path with no containment check — safe only because today's lone caller hands it the already-sanitized result. Any future caller passing a raw agent path (../../.ssh/id_rsa) could read an arbitrary file into the org's file store. Extract the resolve+reject-".."/absolute guard into one workspace-path helper and route both the writer and the reader through it, so the defense lives on the function rather than the caller. --- .../src/main/services/deliverable-writer.ts | 19 ++++-------- .../main/services/files/file-object-store.ts | 5 +-- .../src/main/services/files/workspace-path.ts | 21 +++++++++++++ .../tests/unit/workspace-path.test.ts | 31 +++++++++++++++++++ 4 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 AgentCompany-New/src/main/services/files/workspace-path.ts create mode 100644 AgentCompany-New/tests/unit/workspace-path.test.ts diff --git a/AgentCompany-New/src/main/services/deliverable-writer.ts b/AgentCompany-New/src/main/services/deliverable-writer.ts index f797a66c..614ac712 100644 --- a/AgentCompany-New/src/main/services/deliverable-writer.ts +++ b/AgentCompany-New/src/main/services/deliverable-writer.ts @@ -1,5 +1,6 @@ import { mkdirSync, writeFileSync } from "node:fs"; -import { dirname, isAbsolute, relative, resolve, sep } from "node:path"; +import { dirname } from "node:path"; +import { resolveWorkspacePath } from "./files/workspace-path"; /** * Writes an agent deliverable into the run's bound workspace folder. @@ -9,16 +10,8 @@ import { dirname, isAbsolute, relative, resolve, sep } from "node:path"; * write inside the folder the operator bound to the company. */ export function writeDeliverable(input: { workspaceRoot: string; relativePath: string; content: string }): string { - const root = resolve(input.workspaceRoot); - const requested = input.relativePath.trim().replace(/^[\\/]+/, ""); - if (!requested) throw new Error("Deliverable path is required."); - if (isAbsolute(requested)) throw new Error("Deliverable path must be relative to the workspace."); - const target = resolve(root, requested); - const rel = relative(root, target); - if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) { - throw new Error("Deliverable path escapes the workspace root."); - } - mkdirSync(dirname(target), { recursive: true }); - writeFileSync(target, input.content, "utf8"); - return rel.split(sep).join("/"); + const { absolute, relativePosix } = resolveWorkspacePath(input.workspaceRoot, input.relativePath); + mkdirSync(dirname(absolute), { recursive: true }); + writeFileSync(absolute, input.content, "utf8"); + return relativePosix; } diff --git a/AgentCompany-New/src/main/services/files/file-object-store.ts b/AgentCompany-New/src/main/services/files/file-object-store.ts index 47af2f15..fbf6384b 100644 --- a/AgentCompany-New/src/main/services/files/file-object-store.ts +++ b/AgentCompany-New/src/main/services/files/file-object-store.ts @@ -1,9 +1,10 @@ import { createHash } from "node:crypto"; import { readFileSync } from "node:fs"; -import { basename, extname, resolve } from "node:path"; +import { basename, extname } from "node:path"; import type { DatabaseSync } from "node:sqlite"; import { FILE_SCAN_STATE, type FileScanState } from "../../../shared/domain"; import { createId } from "../../../shared/ids"; +import { resolveWorkspacePath } from "./workspace-path"; export interface FileScanResult { state: FileScanState; @@ -149,7 +150,7 @@ export function recordWorkspaceFile(db: DatabaseSync, input: { workspaceRoot: string; relativePath: string; }): string { - const absolute = resolve(input.workspaceRoot, input.relativePath); + const { absolute } = resolveWorkspacePath(input.workspaceRoot, input.relativePath); const bytes = readFileSync(absolute); const name = basename(input.relativePath); const scan = scanFileContent(name, bytes); diff --git a/AgentCompany-New/src/main/services/files/workspace-path.ts b/AgentCompany-New/src/main/services/files/workspace-path.ts new file mode 100644 index 00000000..3d17c204 --- /dev/null +++ b/AgentCompany-New/src/main/services/files/workspace-path.ts @@ -0,0 +1,21 @@ +import { isAbsolute, relative, resolve, sep } from "node:path"; + +/** + * Resolves a caller-supplied relative path against a workspace root and rejects + * anything that escapes it (absolute paths or `..` traversal). Both writing a + * deliverable and ingesting one back as a file object go through here, so an + * agent-supplied path can only ever land inside the folder the operator bound + * to the company — never `../../.ssh/id_rsa`. + */ +export function resolveWorkspacePath(workspaceRoot: string, relativePath: string): { absolute: string; relativePosix: string } { + const root = resolve(workspaceRoot); + const requested = relativePath.trim().replace(/^[\\/]+/, ""); + if (!requested) throw new Error("Workspace path is required."); + if (isAbsolute(requested)) throw new Error("Workspace path must be relative to the workspace."); + const absolute = resolve(root, requested); + const rel = relative(root, absolute); + if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) { + throw new Error("Workspace path escapes the workspace root."); + } + return { absolute, relativePosix: rel.split(sep).join("/") }; +} diff --git a/AgentCompany-New/tests/unit/workspace-path.test.ts b/AgentCompany-New/tests/unit/workspace-path.test.ts new file mode 100644 index 00000000..40302e99 --- /dev/null +++ b/AgentCompany-New/tests/unit/workspace-path.test.ts @@ -0,0 +1,31 @@ +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { describe, expect, test } from "vitest"; +import { resolveWorkspacePath } from "../../src/main/services/files/workspace-path"; + +describe("resolveWorkspacePath", () => { + const root = join(tmpdir(), "ac-workspace-root"); + + test("keeps a normal relative path inside the workspace", () => { + const { absolute, relativePosix } = resolveWorkspacePath(root, "plan/business-plan.md"); + expect(relativePosix).toBe("plan/business-plan.md"); + expect(absolute.startsWith(resolve(root))).toBe(true); + }); + + test("rejects paths that escape the workspace via traversal", () => { + expect(() => resolveWorkspacePath(root, "../../etc/passwd")).toThrow("escapes the workspace root"); + expect(() => resolveWorkspacePath(root, "plan/../../../secret.key")).toThrow("escapes the workspace root"); + }); + + test("normalises a leading slash and refuses real drive-absolute paths", () => { + // A leading slash is treated as workspace-relative, not a filesystem root. + expect(resolveWorkspacePath(root, "/notes.md").relativePosix).toBe("notes.md"); + if (process.platform === "win32") { + expect(() => resolveWorkspacePath(root, "C:\\Windows\\System32")).toThrow("must be relative"); + } + }); + + test("rejects an empty path", () => { + expect(() => resolveWorkspacePath(root, " ")).toThrow("required"); + }); +});