fix(sdk): detect a directory swapped out from under list_directory

Approving the path and reading it are two separate lookups, so the directory
the boundary check approved is not necessarily the one readdir opens: swap a
path component for a symlink pointing outside the project in between and the
listing comes back from wherever the swap pointed, having passed the check.

Node has no readdir on a descriptor, so the read cannot be pinned to the inode
that was approved. Pin identity around it instead - the directory that was
approved, the one that was read, and the one still at that path afterwards
must all be the same inode, and the path must still resolve inside the project.
An attacker who restores the path before the recheck still wins, so this
narrows the window rather than closing it; the comment says so rather than
implying the check is airtight.

Two tests cover it: a directory that changes inode across the read, and a path
that starts resolving outside the project. Both fail without this change - the
second one returns a listing from outside the project.

Author: c8dhjp4tyv-bit

sdk/src/__tests__/list-directory.test.ts

@@ -5,11 +5,19 @@ import path from 'path'
 import { listDirectory } from '../tools/list-directory'
 
 import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem'
-import type { Dirent, PathLike } from 'node:fs'
+import type { Dirent, PathLike, Stats } from 'node:fs'
 
 const PROJECT_ROOT = path.resolve('workspace', 'project')
 
-function createFs(realpaths: Record<string, string>) {
+function createFs(
+  realpaths: Record<string, string>,
+  options: {
+    /** Called for each stat, so a test can make the directory change identity. */
+    identities?: Array<{ dev: number; ino: number }>
+    /** Realpath answers to use once the listing has been read. */
+    realpathsAfterRead?: Record<string, string>
+  } = {},
+) {
   const readdir = mock(async (_path: PathLike) => {
     return [
       {
@@ -20,15 +28,38 @@ function createFs(realpaths: Record<string, string>) {
     ] as Dirent[]
   })
 
+  const identities = options.identities ?? []
+  let statCalls = 0
+  const stat = mock(async (_path: PathLike) => {
+    const identity = identities[statCalls] ?? { dev: 1, ino: 1 }
+    statCalls += 1
+    return identity as unknown as Stats
+  })
+
+  let listed = false
+  readdir.mockImplementation(async (_path: PathLike) => {
+    listed = true
+    return [
+      {
+        name: 'index.ts',
+        isDirectory: () => false,
+        isFile: () => true,
+      },
+    ] as Dirent[]
+  })
+
   const fs = {
     realpath: mock(async (path: PathLike) => {
       const pathString = String(path)
-      return realpaths[pathString] ?? pathString
+      const table =
+        listed && options.realpathsAfterRead ? options.realpathsAfterRead : realpaths
+      return table[pathString] ?? realpaths[pathString] ?? pathString
     }),
     readdir,
+    stat,
   } as unknown as CodebuffFileSystem
 
-  return { fs, readdir }
+  return { fs, readdir, stat }
 }
 
 describe('listDirectory', () => {
@@ -193,6 +224,55 @@ describe('listDirectory', () => {
     expect(readdir).not.toHaveBeenCalled()
   })
 
+  it('refuses a listing whose directory was swapped while it was read', async () => {
+    // The check approved one inode; by the time the read finished the path was a
+    // different one. Returning that listing is the escape the check exists to stop.
+    const { fs } = createFs(
+      { [PROJECT_ROOT]: PROJECT_ROOT },
+      {
+        identities: [
+          { dev: 1, ino: 1 },
+          { dev: 1, ino: 2 },
+        ],
+      },
+    )
+
+    const result = await listDirectory({
+      directoryPath: '.',
+      projectPath: PROJECT_ROOT,
+      fs,
+    })
+
+    expect(result[0]).toEqual({
+      type: 'json',
+      value: {
+        errorMessage: `Invalid path: Path '.' changed while it was being read.`,
+      },
+    })
+  })
+
+  it('refuses a listing whose path started resolving outside the project', async () => {
+    const childPath = path.join(PROJECT_ROOT, 'src')
+    const outsidePath = path.resolve('workspace', 'other', 'src')
+    const { fs } = createFs(
+      { [PROJECT_ROOT]: PROJECT_ROOT, [childPath]: childPath },
+      { realpathsAfterRead: { [childPath]: outsidePath } },
+    )
+
+    const result = await listDirectory({
+      directoryPath: 'src',
+      projectPath: PROJECT_ROOT,
+      fs,
+    })
+
+    expect(result[0]).toEqual({
+      type: 'json',
+      value: {
+        errorMessage: `Invalid path: Path 'src' changed while it was being read.`,
+      },
+    })
+  })
+
   it('allows a symlink that resolves inside the project', async () => {
     const symlinkPath = path.join(PROJECT_ROOT, 'link')
     const realTarget = path.join(PROJECT_ROOT, 'src')

sdk/src/tools/list-directory.ts

@@ -28,10 +28,42 @@ export async function listDirectory(params: {
       ]
     }
 
+    // Checking the path and then reading it are two separate lookups, so the
+    // directory the check approved is not necessarily the one that gets read: a
+    // component of the path can be swapped for a symlink pointing outside the
+    // project in between, and the listing would come back from wherever the swap
+    // pointed. Node has no readdir-on-a-descriptor, so the read cannot be pinned
+    // to the inode that was approved. Pinning identity around it is what is
+    // available: the directory that was approved, the one that was read, and the
+    // one still at that path afterwards must all be the same inode, and the path
+    // must still resolve inside the project. That does not make the swap
+    // impossible - an attacker who restores the path before the recheck still
+    // wins - but it turns the common case from a silent escape into a refusal.
+    const identityBefore = await fs.stat(realResolvedPath)
+
     const entries = await fs.readdir(realResolvedPath, {
       withFileTypes: true,
     })
 
+    const identityAfter = await fs.stat(realResolvedPath)
+    const realResolvedPathAfter = await fs.realpath(realResolvedPath)
+
+    if (
+      identityAfter.dev !== identityBefore.dev ||
+      identityAfter.ino !== identityBefore.ino ||
+      realResolvedPathAfter !== realResolvedPath ||
+      !isPathInside(realProjectRoot, realResolvedPathAfter)
+    ) {
+      return [
+        {
+          type: 'json',
+          value: {
+            errorMessage: `Invalid path: Path '${directoryPath}' changed while it was being read.`,
+          },
+        },
+      ]
+    }
+
     const files: string[] = []
     const directories: string[] = []