Contain workspace file ingestion to the bound folder (sec #3)
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.
This commit is contained in:
1 parent
6367512052
commit
867070f575
4 files changed
+61
-15
No files matched your search
@@ -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;
|
||||
}
|
||||
@@ -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);
|
||||
|
||||
@@ -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("/") };
|
||||
}
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user