fix(sandbox): resolve symlinks before the read-path containment check

src/cognition/emergent/SandboxedToolForge.ts

@@ -28,12 +28,14 @@
  *
  * 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`).
  */
 
 import { createHash, createHmac, randomUUID } from 'node:crypto';
-import { readFile } from 'node:fs/promises';
+import { readFile, realpath } from 'node:fs/promises';
 import * as path from 'node:path';
 import type { SandboxExecutionRequest, SandboxExecutionResult, SandboxAPI } from './types.js';
 import { CodeSandbox } from '../../safety/sandbox/executor/CodeSandbox.js';
@@ -156,6 +158,21 @@ export class SandboxedToolForge {
   /** Filesystem roots sandboxed reads may access. */
   private readonly fsReadRoots: string[];
 
+  /**
+   * 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 readonly realFsReadRootCache = new Map<string, Promise<string>>();
+
   /**
    * Hardened node:vm sandbox shared across all execute() calls. Owns the
    * codeGeneration restriction, frozen console, and explicit-undefined
@@ -216,6 +233,36 @@ 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<string[]> {
+    return Promise.all(this.fsReadRoots.map((root) => this.resolveReadRoot(root)));
+  }
+
+  /**
+   * Resolve one configured root, pinning the first success.
+   *
+   * 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 resolveReadRoot(root: string): Promise<string> {
+    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 {
     const m = message || '';
     if (/Unexpected token 'const'/.test(m) || /Unexpected token 'let'/.test(m)) {
@@ -475,15 +522,60 @@ export class SandboxedToolForge {
       extras.fs = {
         readFile: async (filePath: string) => {
           const resolvedPath = path.resolve(filePath);
-          const allowed = this.fsReadRoots.some((root) => {
-            return resolvedPath === root || resolvedPath.startsWith(`${root}${path.sep}`);
-          });
-          if (!allowed) {
+          // 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) => {
+              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.
+          if (!withinRoots(resolvedPath, this.fsReadRoots)) {
             throw new Error(
               `fs.readFile blocked: path "${resolvedPath}" is outside the allowed roots`,
             );
           }
-          const data = await readFile(filePath);
+
+          // 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)`,

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, parse } from 'node:path';
 import { SandboxedToolForge } from '../SandboxedToolForge.js';
 import type { SandboxExecutionRequest, SandboxAPI } from '../types.js';
 
@@ -490,4 +493,154 @@ 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 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('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);
+      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');
+    });
+  });
 });