From bb087d831a10ff71c491a3183068ff02dcf48000 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Tue, 2 Jun 2026 15:30:08 +0100 Subject: [PATCH 1/4] fix: resolve symlinks before path containment check in sandboxed fs.readFile Use fs.realpath() to resolve symlinks before checking whether the path falls within allowed roots, and pass the resolved path to readFile(). This prevents symlink-based path traversal (CWE-22) where a symlink inside an allowed root points to a file outside it. --- src/cognition/emergent/SandboxedToolForge.ts | Bin 20317 -> 20347 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/src/cognition/emergent/SandboxedToolForge.ts b/src/cognition/emergent/SandboxedToolForge.ts index 297bd64af8392c5ef0eb82027ff248cc1ab6ff41..ab71a3a5989f5aa7cca0d47362c196b2bd84449c 100644 GIT binary patch delta 82 zcmcaRkMZ|B#tr}3xpWkYQWJ9u5=$~R|7V}=teRM!m{|fB(*Q~66{QyE=ai*tq-Ex$ X1^@*#H?Q}uV&s4dr)<{sHDCe&cf%kn delta 51 zcmex8kMZt2#tr}3H*<2#b`~v2EXmL-N-fUMDNEHz%gjj)019kA>0QOh0^)7<_BCJv E053HZ7XSbN From eb65e30769f60ebdd7664c1d34948d3fb986209f Mon Sep 17 00:00:00 2001 From: jddunn Date: Fri, 18 Sep 2026 10:01:43 -0700 Subject: [PATCH 2/4] fix(sandbox): check read paths lexically before resolving symlinks The sandboxed fs.readFile resolved the requested path with realpath before the containment check (#13). A path outside the read roots still reached the filesystem, so its ENOENT or EACCES showed whether a host path exists and is readable, and the blocked message printed where a host symlink points. The roots were compared unresolved, so a root that is itself a link (macOS /tmp links to /private/tmp) denied every read beneath it. The lexical check now runs first. A path that passes is resolved and checked again against the roots' own real paths, and the resolved path is what gets read. --- src/cognition/emergent/SandboxedToolForge.ts | 70 ++++++++++++++++-- .../__tests__/sandboxed-forge.test.ts | 73 ++++++++++++++++++- 2 files changed, 135 insertions(+), 8 deletions(-) diff --git a/src/cognition/emergent/SandboxedToolForge.ts b/src/cognition/emergent/SandboxedToolForge.ts index c0db5a0e453..8685db0876b 100644 --- a/src/cognition/emergent/SandboxedToolForge.ts +++ b/src/cognition/emergent/SandboxedToolForge.ts @@ -28,7 +28,9 @@ * * Allowlisted APIs (each requires explicit opt-in via {@link SandboxAPI}): * - `fetch` — HTTP requests, domain-restricted via {@link SandboxedToolForgeConfig.fetchDomainAllowlist}. - * - `fs.readFile` — Read-only file access, path-restricted, max 1 MB. + * - `fs.readFile` — Read-only file access, max 1 MB, restricted to the + * configured roots after symlink resolution (a link inside a root cannot + * be used to reach a file outside one). * - `crypto` — Hashing and HMAC only (`createHash`, `createHmac`). */ @@ -156,6 +158,17 @@ export class SandboxedToolForge { /** Filesystem roots sandboxed reads may access. */ private readonly fsReadRoots: string[]; + /** + * {@link fsReadRoots} with their own symlinks resolved, computed lazily on + * the first sandboxed read and cached for the life of the forge. + * + * Both sides of a containment check have to be real paths: a configured + * root is frequently itself a link (on macOS `/tmp` is a link to + * `/private/tmp`), so comparing a resolved file against an unresolved root + * would deny perfectly legitimate reads. + */ + private realFsReadRootsPromise: Promise | null = null; + /** * Hardened node:vm sandbox shared across all execute() calls. Owns the * codeGeneration restriction, frozen console, and explicit-undefined @@ -216,6 +229,22 @@ export class SandboxedToolForge { * the raw message in that case. The hints map to the 4 most common * LLM forge mistakes observed in production. */ + private resolveReadRoots(): Promise { + this.realFsReadRootsPromise ??= Promise.all( + this.fsReadRoots.map(async (root) => { + // A root that cannot be resolved (it does not exist yet, or is + // unreadable) keeps its lexical form, which simply never matches a + // real path — absent roots must not widen the sandbox. + try { + return await realpath(root); + } catch { + return root; + } + }), + ); + return this.realFsReadRootsPromise; + } + private describeSyntaxError(message: string): string { const m = message || ''; if (/Unexpected token 'const'/.test(m) || /Unexpected token 'let'/.test(m)) { @@ -474,16 +503,43 @@ export class SandboxedToolForge { if (allowlist.includes('fs.readFile')) { extras.fs = { readFile: async (filePath: string) => { - const resolvedPath = await realpath(path.resolve(filePath)); - const allowed = this.fsReadRoots.some((root) => { - return resolvedPath === root || resolvedPath.startsWith(`${root}${path.sep}`); - }); - if (!allowed) { + const resolvedPath = path.resolve(filePath); + const withinRoots = (candidate: string, roots: readonly string[]): boolean => + roots.some((root) => candidate === root || candidate.startsWith(`${root}${path.sep}`)); + + // Lexical pass: rejects the obvious `../../etc/passwd` shape before + // the sandbox pays for any filesystem call. + if (!withinRoots(resolvedPath, this.fsReadRoots)) { throw new Error( `fs.readFile blocked: path "${resolvedPath}" is outside the allowed roots`, ); } - const data = await readFile(resolvedPath); + + // Lexical containment is NOT containment. A symlink sitting inside + // an allowed root resolves to an in-root string while pointing at + // any file on the machine, so string-prefix checking alone reads + // whatever the link targets (CWE-22). Follow the link chain and + // re-check against the roots' own real paths. + // + // The path has already cleared the lexical check, so a resolution + // failure here names something inside an allowed root: surface the + // filesystem's own error (ENOENT and friends) rather than masking + // a missing file as a containment failure. + const realPath = await realpath(resolvedPath); + if (!withinRoots(realPath, await this.resolveReadRoots())) { + throw new Error( + `fs.readFile blocked: path "${resolvedPath}" resolves outside the allowed roots`, + ); + } + + // Read the RESOLVED path, not the caller's string: re-resolving the + // original would walk the link a second time, and the link can be + // repointed between the two walks. (A component of the resolved + // path could still be swapped for a link in that window — closing + // that needs an O_NOFOLLOW handle, which node's fs/promises does + // not expose; it requires local write access inside an allowed + // root, which is already outside this sandbox's threat model.) + const data = await readFile(realPath); if (data.byteLength > 1_048_576) { throw new Error( `fs.readFile blocked: file exceeds 1 MB limit (${data.byteLength} bytes)`, diff --git a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts index 96d245fdb75..84bc9e00400 100644 --- a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts +++ b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts @@ -13,7 +13,10 @@ * 9. Execution time is measured and returned */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterAll } from 'vitest'; +import { mkdtempSync, mkdirSync, symlinkSync, writeFileSync, rmSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { SandboxedToolForge } from '../SandboxedToolForge.js'; import type { SandboxExecutionRequest, SandboxAPI } from '../types.js'; @@ -490,4 +493,72 @@ describe('SandboxedToolForge', () => { // so just assert > 0. expect(result.memoryUsedBytes).toBeGreaterThan(0); }); + + // ------------------------------------------------------------------------- + // fs.readFile containment (CWE-22): a symlink inside an allowed root must + // not reach a file outside one. + // ------------------------------------------------------------------------- + describe('fs.readFile path containment', () => { + // realpathSync: on macOS the OS temp dir is itself a symlink, so the + // fixture paths have to be real before they are handed to the forge as + // roots — otherwise the test would be asserting against the very + // root-resolution behavior it means to exercise. + const base = realpathSync(mkdtempSync(join(tmpdir(), 'forge-fs-'))); + const allowedRoot = join(base, 'allowed'); + const secretDir = join(base, 'secret'); + mkdirSync(allowedRoot, { recursive: true }); + mkdirSync(secretDir, { recursive: true }); + writeFileSync(join(allowedRoot, 'ok.txt'), 'in-root content'); + writeFileSync(join(secretDir, 'secret.txt'), 'SECRET-DO-NOT-READ'); + // The escape: a link that LIVES in the allowed root, so every + // string-prefix check on its own path passes. + symlinkSync(join(secretDir, 'secret.txt'), join(allowedRoot, 'escape.txt')); + + afterAll(() => { + rmSync(base, { recursive: true, force: true }); + }); + + const readRequest = (target: string): SandboxExecutionRequest => + makeRequest( + 'async function execute(input) { return await fs.readFile(input.p); }', + { p: target }, + { allowlist: ['fs.readFile'] }, + ); + + const forgeWithRoot = new SandboxedToolForge({ fsReadRoots: [allowedRoot] }); + + it('reads an ordinary file inside an allowed root', async () => { + const result = await forgeWithRoot.execute(readRequest(join(allowedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + + it('blocks a symlink that lives inside the root but targets a file outside it', async () => { + const result = await forgeWithRoot.execute(readRequest(join(allowedRoot, 'escape.txt'))); + + expect(result.success).toBe(false); + expect(result.error ?? '').toMatch(/resolves outside the allowed roots/); + // The secret must not leak through the output channel either. + expect(JSON.stringify(result.output ?? null)).not.toMatch(/SECRET-DO-NOT-READ/); + }); + + it('still blocks a plainly out-of-root path before touching the filesystem', async () => { + const result = await forgeWithRoot.execute(readRequest(join(secretDir, 'secret.txt'))); + + expect(result.success).toBe(false); + expect(result.error ?? '').toMatch(/is outside the allowed roots/); + }); + + it('allows a root that is itself a symlink (both sides are resolved)', async () => { + const linkedRoot = join(base, 'linked-root'); + symlinkSync(allowedRoot, linkedRoot); + const forgeViaLink = new SandboxedToolForge({ fsReadRoots: [linkedRoot] }); + + const result = await forgeViaLink.execute(readRequest(join(linkedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + }); }); From 14f564264a6dc3201b7644b08064e53e9b11161d Mon Sep 17 00:00:00 2001 From: jddunn Date: Fri, 18 Sep 2026 10:11:24 -0700 Subject: [PATCH 3/4] fix(sandbox): contain read paths with path.relative instead of a string prefix A root that already ends in a separator (the filesystem root, a Windows drive root) was compared against a doubled separator, so every file beneath it was denied. path.relative has no such edge and applies the platform's own case rules. A root set containing an unresolvable entry is no longer cached, so a root that becomes readable later is not stranded; a fully resolved set stays pinned on purpose. --- src/cognition/emergent/SandboxedToolForge.ts | 48 ++++++++++++++-- .../__tests__/sandboxed-forge.test.ts | 57 ++++++++++++++++++- 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/src/cognition/emergent/SandboxedToolForge.ts b/src/cognition/emergent/SandboxedToolForge.ts index 8685db0876b..02b70087cad 100644 --- a/src/cognition/emergent/SandboxedToolForge.ts +++ b/src/cognition/emergent/SandboxedToolForge.ts @@ -230,19 +230,37 @@ export class SandboxedToolForge { * LLM forge mistakes observed in production. */ private resolveReadRoots(): Promise { - this.realFsReadRootsPromise ??= Promise.all( + const pending = this.realFsReadRootsPromise ?? this.readRootsRealPaths(); + this.realFsReadRootsPromise = pending; + return pending; + } + + /** + * Resolve every configured root once. + * + * A fully resolved set is cached for the life of the forge, deliberately: + * re-resolving per read would let a root symlink be repointed underneath a + * running sandbox, which is the move this containment check exists to + * stop. A caller that needs to follow a retargeted root builds a new forge. + * A set containing an unresolvable root is NOT cached, so a root that + * becomes readable later is picked up instead of being stranded. + */ + private async readRootsRealPaths(): Promise { + let allResolved = true; + const resolved = await Promise.all( this.fsReadRoots.map(async (root) => { - // A root that cannot be resolved (it does not exist yet, or is - // unreadable) keeps its lexical form, which simply never matches a - // real path — absent roots must not widen the sandbox. try { return await realpath(root); } catch { + // An unresolvable root keeps its lexical form, which simply never + // matches a real path — an absent root must not widen the sandbox. + allResolved = false; return root; } }), ); - return this.realFsReadRootsPromise; + if (!allResolved) this.realFsReadRootsPromise = null; + return resolved; } private describeSyntaxError(message: string): string { @@ -504,8 +522,26 @@ export class SandboxedToolForge { extras.fs = { readFile: async (filePath: string) => { const resolvedPath = path.resolve(filePath); + // Containment via path.relative, not a string prefix: a root that + // already ends in a separator (the filesystem root `/`, a Windows + // drive root `C:\`) would otherwise be compared against a doubled + // separator and deny every file beneath it. path.relative also + // applies the platform's own case rules. const withinRoots = (candidate: string, roots: readonly string[]): boolean => - roots.some((root) => candidate === root || candidate.startsWith(`${root}${path.sep}`)); + roots.some((root) => { + const relative = path.relative(root, candidate); + // '' means the candidate IS the root. A relative path that + // climbs out ('..' or '../x') or stays absolute (a different + // Windows drive) is outside it. Checking for the '..' segment + // rather than the '..' prefix keeps a sibling named '..foo' + // from reading as an escape. + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); + }); // Lexical pass: rejects the obvious `../../etc/passwd` shape before // the sandbox pays for any filesystem call. diff --git a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts index 84bc9e00400..0315eae7fe3 100644 --- a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts +++ b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts @@ -16,7 +16,7 @@ import { describe, it, expect, afterAll } from 'vitest'; import { mkdtempSync, mkdirSync, symlinkSync, writeFileSync, rmSync, realpathSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, parse } from 'node:path'; import { SandboxedToolForge } from '../SandboxedToolForge.js'; import type { SandboxExecutionRequest, SandboxAPI } from '../types.js'; @@ -550,6 +550,61 @@ describe('SandboxedToolForge', () => { expect(result.error ?? '').toMatch(/is outside the allowed roots/); }); + it('allows reads under a root that ends in a separator (filesystem / drive root)', async () => { + // The prefix form built `//` for a root of `/` and denied everything + // beneath it; path.relative containment has no such edge. + const fsRoot = parse(base).root; + const forgeAtFsRoot = new SandboxedToolForge({ fsReadRoots: [fsRoot] }); + + const result = await forgeAtFsRoot.execute(readRequest(join(allowedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + + it('does not treat a sibling whose name merely starts with ".." as an escape', async () => { + const oddSibling = join(base, '..odd'); + mkdirSync(oddSibling, { recursive: true }); + writeFileSync(join(oddSibling, 'f.txt'), 'sibling content'); + const forgeAtBase = new SandboxedToolForge({ fsReadRoots: [base] }); + + const result = await forgeAtBase.execute(readRequest(join(oddSibling, 'f.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('sibling content'); + }); + + it('pins a resolved root, so repointing the link mid-life cannot move the sandbox', async () => { + // Deliberate: re-resolving the root per read would let an attacker who + // can rewrite the link relocate a running sandbox. The first read fixes + // the real root; the retargeted directory is then outside it. + const movingRoot = join(base, 'moving'); + const firstTarget = join(base, 'target-a'); + const secondTarget = join(base, 'target-b'); + mkdirSync(firstTarget, { recursive: true }); + mkdirSync(secondTarget, { recursive: true }); + writeFileSync(join(firstTarget, 'f.txt'), 'target A'); + writeFileSync(join(secondTarget, 'f.txt'), 'target B'); + symlinkSync(firstTarget, movingRoot); + + const forgeMoving = new SandboxedToolForge({ fsReadRoots: [movingRoot] }); + const first = await forgeMoving.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(first.success).toBe(true); + expect(first.output).toBe('target A'); + + rmSync(movingRoot); + symlinkSync(secondTarget, movingRoot); + + const second = await forgeMoving.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(second.success).toBe(false); + expect(second.error ?? '').toMatch(/resolves outside the allowed roots/); + // A forge built after the change follows the link's new target. + const freshForge = new SandboxedToolForge({ fsReadRoots: [movingRoot] }); + const fresh = await freshForge.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(fresh.success).toBe(true); + expect(fresh.output).toBe('target B'); + }); + it('allows a root that is itself a symlink (both sides are resolved)', async () => { const linkedRoot = join(base, 'linked-root'); symlinkSync(allowedRoot, linkedRoot); From 5cdf53ca16979aea0f99ae329691d39d223c8f17 Mon Sep 17 00:00:00 2001 From: jddunn Date: Fri, 18 Sep 2026 11:26:33 -0700 Subject: [PATCH 4/4] fix(sandbox): pin each resolved read root on its own The root cache was all-or-nothing: One unresolvable root dropped the whole set, so the next read re-resolved the roots that had succeeded. A root symlink repointed in between was then followed to its new target, losing the pinning the cache exists to provide. Roots are now cached one by one: A success is pinned for the life of the forge, a failure is retried, and neither affects a sibling. --- src/cognition/emergent/SandboxedToolForge.ts | 58 +++++++++---------- .../__tests__/sandboxed-forge.test.ts | 27 +++++++++ 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/src/cognition/emergent/SandboxedToolForge.ts b/src/cognition/emergent/SandboxedToolForge.ts index 02b70087cad..682daa1f1f5 100644 --- a/src/cognition/emergent/SandboxedToolForge.ts +++ b/src/cognition/emergent/SandboxedToolForge.ts @@ -159,15 +159,19 @@ export class SandboxedToolForge { private readonly fsReadRoots: string[]; /** - * {@link fsReadRoots} with their own symlinks resolved, computed lazily on - * the first sandboxed read and cached for the life of the forge. + * Real path of each configured root, resolved lazily and cached PER ROOT. * * Both sides of a containment check have to be real paths: a configured * root is frequently itself a link (on macOS `/tmp` is a link to * `/private/tmp`), so comparing a resolved file against an unresolved root * would deny perfectly legitimate reads. + * + * Per root, not per set: a set-wide cache that is dropped whenever ANY + * root fails to resolve would re-resolve the roots that DID succeed, and a + * root symlink repointed in between would then be followed to its new + * target — losing the pinning this cache exists to provide. */ - private realFsReadRootsPromise: Promise | null = null; + private readonly realFsReadRootCache = new Map>(); /** * Hardened node:vm sandbox shared across all execute() calls. Owns the @@ -230,37 +234,33 @@ export class SandboxedToolForge { * LLM forge mistakes observed in production. */ private resolveReadRoots(): Promise { - const pending = this.realFsReadRootsPromise ?? this.readRootsRealPaths(); - this.realFsReadRootsPromise = pending; - return pending; + return Promise.all(this.fsReadRoots.map((root) => this.resolveReadRoot(root))); } /** - * Resolve every configured root once. + * Resolve one configured root, pinning the first success. * - * A fully resolved set is cached for the life of the forge, deliberately: - * re-resolving per read would let a root symlink be repointed underneath a - * running sandbox, which is the move this containment check exists to - * stop. A caller that needs to follow a retargeted root builds a new forge. - * A set containing an unresolvable root is NOT cached, so a root that - * becomes readable later is picked up instead of being stranded. + * A resolved root is pinned for the life of the forge, deliberately: + * re-resolving it per read would let a root symlink be repointed + * underneath a running sandbox, which is the move this containment check + * exists to stop. A caller that needs to follow a retargeted root builds a + * new forge. A FAILED resolution is not pinned, so a root that becomes + * readable later is picked up — and because each root is cached on its + * own, one unreadable root can never un-pin a sibling that resolved. */ - private async readRootsRealPaths(): Promise { - let allResolved = true; - const resolved = await Promise.all( - this.fsReadRoots.map(async (root) => { - try { - return await realpath(root); - } catch { - // An unresolvable root keeps its lexical form, which simply never - // matches a real path — an absent root must not widen the sandbox. - allResolved = false; - return root; - } - }), - ); - if (!allResolved) this.realFsReadRootsPromise = null; - return resolved; + private resolveReadRoot(root: string): Promise { + const cached = this.realFsReadRootCache.get(root); + if (cached !== undefined) return cached; + + const pending = realpath(root).catch(() => { + // Drop the failure so the next read retries this root. Its lexical + // form never matches a real path, so an unresolvable root cannot + // widen the sandbox while it stays unreadable. + this.realFsReadRootCache.delete(root); + return root; + }); + this.realFsReadRootCache.set(root, pending); + return pending; } private describeSyntaxError(message: string): string { diff --git a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts index 0315eae7fe3..208785d2d0a 100644 --- a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts +++ b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts @@ -605,6 +605,33 @@ describe('SandboxedToolForge', () => { expect(fresh.output).toBe('target B'); }); + it('keeps a resolved root pinned even when a sibling root never resolves', async () => { + // A set-wide cache dropped on any failure would re-resolve the symlink + // root on the next read and follow it to its new target. + const movingRoot = join(base, 'moving-mixed'); + const firstTarget = join(base, 'mixed-a'); + const secondTarget = join(base, 'mixed-b'); + mkdirSync(firstTarget, { recursive: true }); + mkdirSync(secondTarget, { recursive: true }); + writeFileSync(join(firstTarget, 'f.txt'), 'mixed A'); + writeFileSync(join(secondTarget, 'f.txt'), 'mixed B'); + symlinkSync(firstTarget, movingRoot); + + const forgeMixed = new SandboxedToolForge({ + fsReadRoots: [movingRoot, join(base, 'never-exists')], + }); + const first = await forgeMixed.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(first.success).toBe(true); + expect(first.output).toBe('mixed A'); + + rmSync(movingRoot); + symlinkSync(secondTarget, movingRoot); + + const second = await forgeMixed.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(second.success).toBe(false); + expect(second.error ?? '').toMatch(/resolves outside the allowed roots/); + }); + it('allows a root that is itself a symlink (both sides are resolved)', async () => { const linkedRoot = join(base, 'linked-root'); symlinkSync(allowedRoot, linkedRoot);