From dbeb7232187c45d2ed6fc20030bf27322a2791cc Mon Sep 17 00:00:00 2001
From: affaan-m <124439313+affaan-m@users.noreply.github.com>
Date: Sun, 27 Sep 2026 22:23:33 -0400
Subject: [PATCH] fix(plan-canvas): bind bounded artifact reads to verified
 descriptors

---
 scripts/lib/plan-canvas/server.js | 126 +++++++++++++--
 tests/scripts/plan-canvas.test.js | 252 ++++++++++++++++++++++++++++++
 2 files changed, 362 insertions(+), 16 deletions(-)

diff --git a/scripts/lib/plan-canvas/server.js b/scripts/lib/plan-canvas/server.js
index 7e41774bb..ae0a83468 100644
--- a/scripts/lib/plan-canvas/server.js
+++ b/scripts/lib/plan-canvas/server.js
@@ -123,6 +123,112 @@ function sendHtml(res, statusCode, html, { csp = true } = {}) {
   res.end(html);
 }
 
+// Sibling assets have an explicit 64 MiB resource limit (413 above it), separate
+// from request-body limits. Reads use at most the sampled size plus one sentinel.
+const MAX_ARTIFACT_ASSET_BYTES = 64 * 1024 * 1024;
+
+function assetReadError(code = 'EARTIFACT_UNSAFE') {
+  return Object.assign(new Error('Artifact sibling read refused'), { code });
+}
+
+function sameAssetIdentity(before, after) {
+  return before.dev === after.dev && before.ino === after.ino && before.mode === after.mode;
+}
+
+function sameAssetFile(before, after) {
+  return after.isFile() && sameAssetIdentity(before, after) && before.size === after.size
+    && (before.mtimeNs ?? before.mtimeMs) === (after.mtimeNs ?? after.mtimeMs)
+    && (before.ctimeNs ?? before.ctimeMs) === (after.ctimeNs ?? after.ctimeMs);
+}
+
+function assetByteLength(stats) {
+  if (typeof stats.size !== 'bigint' && !Number.isSafeInteger(stats.size)) throw assetReadError();
+  const size = BigInt(stats.size);
+  if (size < 0n) throw assetReadError();
+  if (size > BigInt(MAX_ARTIFACT_ASSET_BYTES)) throw assetReadError('EARTIFACT_TOO_LARGE');
+  return Number(size);
+}
+
+function snapshotAssetChain(realTarget) {
+  const root = path.parse(realTarget).root;
+  const parts = path.relative(root, realTarget).split(path.sep).filter(Boolean);
+  const paths = [root];
+  for (const part of parts) paths.push(path.join(paths[paths.length - 1], part));
+  return paths.map((entryPath, index) => {
+    const stats = fs.lstatSync(entryPath, { bigint: true });
+    const leaf = index === paths.length - 1;
+    if (stats.isSymbolicLink() || !(leaf ? stats.isFile() : stats.isDirectory())) throw assetReadError();
+    return { path: entryPath, stats, leaf };
+  });
+}
+
+function revalidateAssetPath({ baseDir, resolved, realBase, realTarget, chain }) {
+  if (fs.realpathSync(baseDir) !== realBase || fs.realpathSync(resolved) !== realTarget) throw assetReadError();
+  for (const entry of chain) {
+    const current = fs.lstatSync(entry.path, { bigint: true });
+    if (current.isSymbolicLink() || !(entry.leaf ? sameAssetFile(entry.stats, current)
+      : current.isDirectory() && sameAssetIdentity(entry.stats, current))) throw assetReadError();
+  }
+}
+
+function readAssetDescriptor(fd, size) {
+  const buffer = Buffer.alloc(size + 1);
+  let bytes = 0;
+  while (bytes < buffer.length) {
+    const requested = buffer.length - bytes;
+    const count = fs.readSync(fd, buffer, bytes, requested, bytes);
+    if (!Number.isInteger(count) || count < 0 || count > requested) throw assetReadError();
+    if (count === 0) break;
+    bytes += count;
+  }
+  if (bytes !== size) throw assetReadError();
+  return buffer.subarray(0, bytes);
+}
+
+// Canonical ancestors are sampled from the filesystem root, not just realBase.
+// Revalidation detects observed replacements, but portable pathname operations
+// are not atomic openat confinement: repeated swap/restore or same-inode content
+// races can evade observations. This helper covers siblings only, not the direct
+// session.file read. O_NOFOLLOW protects the leaf only where the flag exists.
+function readSiblingAsset(baseDir, resolved) {
+  const realBase = fs.realpathSync(baseDir);
+  const realTarget = fs.realpathSync(resolved);
+  const relative = path.relative(realBase, realTarget);
+  if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) throw assetReadError();
+  const chain = snapshotAssetChain(realTarget);
+  const expected = chain[chain.length - 1].stats;
+  const size = assetByteLength(expected);
+  const snapshot = { baseDir, resolved, realBase, realTarget, chain };
+  revalidateAssetPath(snapshot);
+  const flags = fs.constants.O_RDONLY | (fs.constants.O_NOFOLLOW || 0) | (fs.constants.O_NONBLOCK || 0);
+  let fd;
+  try {
+    fd = fs.openSync(realTarget, flags);
+  } catch (error) {
+    if (error.code === 'ELOOP' || error.code === 'ENOTDIR') throw assetReadError();
+    throw error;
+  }
+  let primaryError;
+  let data;
+  try {
+    if (!sameAssetFile(expected, fs.fstatSync(fd, { bigint: true }))) throw assetReadError();
+    revalidateAssetPath(snapshot);
+    data = readAssetDescriptor(fd, size);
+    if (!sameAssetFile(expected, fs.fstatSync(fd, { bigint: true }))) throw assetReadError();
+    revalidateAssetPath(snapshot);
+  } catch (error) {
+    primaryError = error;
+  } finally {
+    try { fs.closeSync(fd); } catch (error) {
+      // A close error may mean the fd is already released; never retry it or
+      // replace the primary read/validation failure. Close-only failure refuses.
+      if (!primaryError) primaryError = error;
+    }
+  }
+  if (primaryError) throw primaryError;
+  return { data, realTarget };
+}
+
 function createPlanCanvasServer({
   store,
   host = DEFAULT_HOST,
@@ -532,24 +638,12 @@ function createPlanCanvasServer({
       return sendJson(res, 403, { error: 'asset path escapes artifact directory' });
     }
     let realTarget;
-    try {
-      const realBase = fs.realpathSync(baseDir);
-      realTarget = fs.realpathSync(resolved);
-      if (realTarget !== realBase && !realTarget.startsWith(realBase + path.sep)) {
-        return sendJson(res, 403, { error: 'asset path escapes artifact directory' });
-      }
-    } catch {
-      return sendJson(res, 404, { error: 'asset not found' });
-    }
-    // Residual TOCTOU note: the confinement check and the read below are
-    // separate operations. A local writer able to replace the target or its
-    // parent directory entries can still redirect the read between them.
-    // Static confinement assumes trusted local directory writers; it is not
-    // race-free. The document sandbox below is a separate origin boundary.
     let data;
     try {
-      data = fs.readFileSync(realTarget);
-    } catch {
+      ({ data, realTarget } = readSiblingAsset(baseDir, resolved));
+    } catch (error) {
+      if (error.code === 'EARTIFACT_TOO_LARGE') return sendJson(res, 413, { error: 'asset too large' });
+      if (error.code === 'EARTIFACT_UNSAFE') return sendJson(res, 403, { error: 'asset changed or unsafe' });
       return sendJson(res, 404, { error: 'asset not found' });
     }
     // MIME comes from the link/request name first so a symlinked asset keeps
diff --git a/tests/scripts/plan-canvas.test.js b/tests/scripts/plan-canvas.test.js
index 9b8e760df..bd4ee362b 100644
--- a/tests/scripts/plan-canvas.test.js
+++ b/tests/scripts/plan-canvas.test.js
@@ -202,6 +202,257 @@ async function artifactSecurityTests(test) {
   });
 }
 
+// Deterministic filesystem boundaries, using private regular files only. Native
+// descriptors are owned here even when a spy returns a different private file.
+async function withAssetIo(asset, overrides, callback) {
+  const target = fs.realpathSync(asset);
+  const methods = ['openSync', 'fstatSync', 'lstatSync', 'readSync', 'closeSync'];
+  const original = Object.fromEntries(methods.map(name => [name, fs[name]]));
+  const live = new Set();
+  const calls = { opens: 0, reads: 0, closes: 0, fstats: 0, requested: [], returned: 0, flags: [] };
+  fs.openSync = function (candidate, flags, ...rest) {
+    if (typeof candidate !== 'string' || path.resolve(candidate) !== target) return original.openSync(candidate, flags, ...rest);
+    calls.opens++;
+    calls.flags.push(flags);
+    const fd = overrides.open
+      ? overrides.open({ candidate, flags, original, calls })
+      : original.openSync(candidate, flags, ...rest);
+    live.add(fd);
+    return fd;
+  };
+  fs.fstatSync = function (fd, ...rest) {
+    const stats = original.fstatSync(fd, ...rest);
+    if (!live.has(fd)) return stats;
+    calls.fstats++;
+    return overrides.fstat ? overrides.fstat(stats, calls) : stats;
+  };
+  fs.lstatSync = function (candidate, ...rest) {
+    const stats = original.lstatSync(candidate, ...rest);
+    return overrides.lstat ? overrides.lstat(path.resolve(candidate), stats, calls) : stats;
+  };
+  fs.readSync = function (fd, buffer, offset, length, position) {
+    if (!live.has(fd)) return original.readSync(fd, buffer, offset, length, position);
+    calls.reads++;
+    calls.requested.push({ length, position, capacity: buffer.length });
+    const count = overrides.read
+      ? overrides.read({ fd, buffer, offset, length, position, original, calls })
+      : original.readSync(fd, buffer, offset, length, position);
+    calls.returned += count;
+    return count;
+  };
+  fs.closeSync = function (fd) {
+    if (!live.has(fd)) return original.closeSync(fd);
+    calls.closes++;
+    // Close the actual fixture descriptor before optionally simulating a close
+    // error. The test never leaks an fd to imitate an ambiguous OS error.
+    original.closeSync(fd);
+    live.delete(fd);
+    if (overrides.close) overrides.close(calls);
+  };
+  try {
+    await callback(calls);
+    assert.strictEqual(live.size, 0, 'All returned descriptors must close');
+  } finally {
+    for (const name of methods) fs[name] = original[name];
+    // Cleanup after a failing assertion/implementation; leaks still fail above.
+    for (const fd of live) original.closeSync(fd);
+  }
+}
+
+function changedStats(stats, changes) {
+  return Object.assign(Object.create(Object.getPrototypeOf(stats)), stats, changes);
+}
+
+function assertAssetRefusal(response, status, base, outside) {
+  assert.strictEqual(response.statusCode, status);
+  assert.ok(!response.body.includes('private-fixture-secret'));
+  assert.ok(!response.body.includes(base));
+  assert.ok(!response.body.includes(outside));
+}
+
+async function artifactRaceTests(test) {
+  const withFile = callback => withArtifactHandler(async value => {
+    const parent = path.join(value.base, 'nested');
+    fs.mkdirSync(parent);
+    const asset = path.join(parent, 'asset.txt');
+    fs.writeFileSync(asset, 'inert');
+    fs.writeFileSync(path.join(value.outside, 'asset.txt'), 'private-fixture-secret');
+    await callback({ ...value, parent, asset, fetch: () => value.get('nested/asset.txt') });
+  });
+  await test('sibling reads use one guarded descriptor and at most size plus one bytes', () => withFile(async ({ asset, fetch }) => {
+    await withAssetIo(asset, {}, async calls => {
+      const response = await fetch();
+      assert.strictEqual(response.statusCode, 200);
+      assert.strictEqual(response.body, 'inert');
+      assert.deepStrictEqual([calls.opens, calls.closes], [1, 1]);
+      assert.ok(calls.fstats >= 2);
+      assert.strictEqual(typeof calls.flags[0], 'number');
+      for (const flag of [fs.constants.O_NOFOLLOW || 0, fs.constants.O_NONBLOCK || 0]) {
+        assert.strictEqual(calls.flags[0] & flag, flag);
+      }
+      assert.ok(calls.requested.length > 0);
+      assert.ok(calls.requested.every(call => call.capacity === 6 && call.length <= 6 && Number.isInteger(call.position)));
+      assert.strictEqual(calls.returned, 5);
+    });
+  }));
+  await test('a leaf symlink replacement before native open never reads outside bytes', () => withFile(async ({ asset, base, outside, fetch }) => {
+    const probe = path.join(base, 'probe');
+    createTestSymlink(path.join(outside, 'asset.txt'), probe);
+    fs.unlinkSync(probe);
+    await withAssetIo(asset, { open({ candidate, flags, original }) {
+      fs.unlinkSync(asset);
+      fs.symlinkSync(path.join(outside, 'asset.txt'), asset, 'file');
+      return original.openSync(candidate, flags);
+    } }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.strictEqual(calls.reads, 0);
+      assert.ok(calls.closes === 0 || calls.closes === 1);
+    });
+  }));
+  await test('a substituted descriptor is rejected even with an unchanged pathname', () => withFile(async ({ asset, base, outside, fetch }) => {
+    await withAssetIo(asset, { open({ original }) {
+      return original.openSync(path.join(outside, 'asset.txt'), fs.constants.O_RDONLY);
+    } }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  await test('an intermediate directory symlink swap before open never reads outside bytes', () => withFile(async ({ asset, parent, base, outside, fetch }) => {
+    const probe = path.join(base, 'probe');
+    createTestSymlink(outside, probe, 'dir');
+    fs.unlinkSync(probe);
+    await withAssetIo(asset, { open({ candidate, flags, original }) {
+      fs.renameSync(parent, `${parent}.saved`);
+      fs.symlinkSync(outside, parent, 'dir');
+      return original.openSync(candidate, flags);
+    } }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  await test('parent replacement is refused even with the same leaf inode and simulated stable leaf metadata', () => withFile(async ({ asset, parent, base, outside, fetch }) => {
+    const before = fs.statSync(asset, { bigint: true });
+    const stable = stats => changedStats(stats, { mtimeNs: before.mtimeNs, ctimeNs: before.ctimeNs });
+    await withAssetIo(asset, {
+      open({ candidate, flags, original }) {
+        fs.renameSync(parent, `${parent}.saved`);
+        fs.mkdirSync(parent);
+        fs.renameSync(path.join(`${parent}.saved`, 'asset.txt'), asset);
+        const current = fs.statSync(asset, { bigint: true });
+        assert.deepStrictEqual([current.dev, current.ino], [before.dev, before.ino]);
+        return original.openSync(candidate, flags);
+      },
+      fstat: stable,
+      lstat(candidate, stats) { return candidate === asset ? stable(stats) : stats; }
+    }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  await test('simulated ancestor replacement above the artifact base is refused before read', () => withFile(async ({ asset, base, outside, fetch }) => {
+    await withAssetIo(asset, { lstat(candidate, stats, calls) {
+      return calls.opens > 0 && candidate === path.dirname(base)
+        ? changedStats(stats, { ino: stats.ino + 1n }) : stats;
+    } }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  for (const change of ['descriptor metadata', 'ancestor identity']) {
+    await test(`simulated ${change} change after reading discards the buffered body`, () => withFile(async ({ asset, parent, base, outside, fetch }) => {
+      await withAssetIo(asset, {
+        fstat(stats, calls) { return change === 'descriptor metadata' && calls.reads > 0
+          ? changedStats(stats, { mtimeNs: stats.mtimeNs + 1n }) : stats; },
+        lstat(candidate, stats, calls) { return change === 'ancestor identity' && calls.reads > 0 && candidate === parent
+          ? changedStats(stats, { ino: stats.ino + 1n }) : stats; }
+      }, async calls => {
+        assertAssetRefusal(await fetch(), 403, base, outside);
+        assert.ok(calls.reads > 0);
+        assert.strictEqual(calls.closes, 1);
+      });
+    }));
+  }
+  await test('retargeting an in-root alias after read discards buffered bytes', () => withFile(async ({ asset, base, outside, get }) => {
+    const alias = path.join(base, 'alias.txt');
+    const other = path.join(base, 'other.txt');
+    fs.writeFileSync(other, 'other');
+    createTestSymlink(asset, alias);
+    await withAssetIo(asset, { read({ fd, buffer, offset, length, position, original, calls }) {
+      const count = original.readSync(fd, buffer, offset, length, position);
+      if (calls.reads === 1) { fs.unlinkSync(alias); fs.symlinkSync(other, alias, 'file'); }
+      return count;
+    } }, async calls => {
+      assertAssetRefusal(await get('alias.txt'), 403, base, outside);
+      assert.strictEqual(calls.closes, 1);
+    });
+  }));
+  for (const size of [67108865n, -1n, 1.5, Infinity]) {
+    await test(`invalid or over-limit sampled size ${size} is refused before open`, () => withFile(async ({ asset, base, outside, fetch }) => {
+      await withAssetIo(asset, { lstat(candidate, stats) { return candidate === asset ? changedStats(stats, { size }) : stats; } }, async calls => {
+        const status = size === 67108865n ? 413 : 403;
+        const response = await fetch();
+        assertAssetRefusal(response, status, base, outside);
+        if (status === 413) assert.deepStrictEqual(JSON.parse(response.body), { error: 'asset too large' });
+        assert.deepStrictEqual([calls.opens, calls.reads, calls.closes], [0, 0, 0]);
+      });
+    }));
+  }
+  await test('non-regular opened descriptors are refused without reading', () => withFile(async ({ asset, base, outside, fetch }) => {
+    await withAssetIo(asset, { fstat(stats) { return changedStats(stats, { isFile: () => false }); } }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  for (const kind of ['growth sentinel', 'early EOF']) {
+    await test(`bounded read refuses ${kind}`, () => withFile(async ({ asset, base, outside, fetch }) => {
+      await withAssetIo(asset, { read({ buffer, offset, length }) {
+        if (kind === 'early EOF') return 0;
+        buffer.fill(97, offset, offset + length);
+        return length;
+      } }, async calls => {
+        assertAssetRefusal(await fetch(), 403, base, outside);
+        assert.ok(calls.returned <= 6);
+        assert.ok(calls.requested.every(call => call.length <= 6 && call.capacity === 6));
+        assert.strictEqual(calls.closes, 1);
+      });
+    }));
+  }
+  for (const boundary of ['open', 'fstat', 'read', 'close', 'read and close']) {
+    await test(`${boundary} failure closes only acquired descriptors once and returns no body`, () => withFile(async ({ asset, base, outside, fetch }) => {
+      const failure = code => Object.assign(new Error(`private failure at ${asset}`), { code });
+      const overrides = {};
+      if (boundary === 'open') overrides.open = () => { throw failure('EACCES'); };
+      if (boundary === 'fstat') overrides.fstat = () => { throw failure('EIO'); };
+      if (boundary.includes('read')) overrides.read = () => { throw failure('EIO'); };
+      if (boundary.includes('close')) overrides.close = () => { throw failure(boundary === 'read and close' ? 'ELOOP' : 'EIO'); };
+      await withAssetIo(asset, overrides, async calls => {
+        assertAssetRefusal(await fetch(), 404, base, outside);
+        assert.strictEqual(calls.closes, boundary === 'open' ? 0 : 1);
+      });
+    }));
+  }
+  await test('primary descriptor validation refusal survives a secondary close error', () => withFile(async ({ asset, base, outside, fetch }) => {
+    await withAssetIo(asset, {
+      open({ original }) { return original.openSync(path.join(outside, 'asset.txt'), fs.constants.O_RDONLY); },
+      close() { throw Object.assign(new Error('secondary close error'), { code: 'EIO' }); }
+    }, async calls => {
+      assertAssetRefusal(await fetch(), 403, base, outside);
+      assert.deepStrictEqual([calls.reads, calls.closes], [0, 1]);
+    });
+  }));
+  await test('empty sibling files and static in-root directory aliases remain supported', () => withFile(async ({ asset, parent, base, get }) => {
+    fs.writeFileSync(asset, '');
+    createTestSymlink(parent, path.join(base, 'inside'), 'dir');
+    await withAssetIo(asset, {}, async calls => {
+      const response = await get('inside/asset.txt');
+      assert.strictEqual(response.statusCode, 200);
+      assert.strictEqual(response.body, '');
+      assert.deepStrictEqual([calls.opens, calls.closes, calls.returned], [1, 1, 0]);
+      assert.ok(calls.requested.every(call => call.capacity === 1));
+    });
+  }));
+}
+
 function request(port, method, requestPath, { body = null, headers = {} } = {}) {
   return new Promise((resolve, reject) => {
     const payload = body === null ? null : JSON.stringify(body);
@@ -286,6 +537,7 @@ async function main() {
   const suite = createTestRunner();
   const { test } = suite;
   await artifactSecurityTests(test);
+  await artifactRaceTests(test);
   if (process.argv.includes('--artifact-security-only')) {
     printResults(suite.results);
     return;