From 6fe157d70235bf1f8a25a77811d8168e3139abb1 Mon Sep 17 00:00:00 2001 From: umutcagand Date: Tue, 1 Sep 2026 09:54:59 +0300 Subject: [PATCH 1/2] fix(sdk): prevent list_directory path escapes --- common/src/testing/mocks/filesystem.ts | 26 ++- common/src/types/filesystem.ts | 8 +- sdk/src/__tests__/list-directory.test.ts | 224 +++++++++++++++++++++++ sdk/src/tools/list-directory.ts | 19 +- 4 files changed, 273 insertions(+), 4 deletions(-) create mode 100644 sdk/src/__tests__/list-directory.test.ts diff --git a/common/src/testing/mocks/filesystem.ts b/common/src/testing/mocks/filesystem.ts index 6c9703622e..bf2af29bde 100644 --- a/common/src/testing/mocks/filesystem.ts +++ b/common/src/testing/mocks/filesystem.ts @@ -2,7 +2,7 @@ import { mock } from 'bun:test' import type { CodebuffFileSystem } from '../../types/filesystem' import type { Mock } from 'bun:test' -import type { PathLike , Stats } from 'node:fs' +import type { PathLike, Stats } from 'node:fs' export interface CreateMockFsOptions { files?: Record @@ -14,6 +14,7 @@ export interface CreateMockFsOptions { path: string, options?: { recursive?: boolean }, ) => Promise + realpathImpl?: (path: string) => Promise statImpl?: (path: string) => Promise } @@ -31,6 +32,7 @@ export interface MockFsWithMocks { options?: { recursive?: boolean }, ) => Promise > + realpath: Mock<(path: PathLike) => Promise> stat: Mock<(path: PathLike) => Promise> } @@ -43,6 +45,7 @@ export function createMockFs(options: CreateMockFsOptions = {}): MockFs { readdirImpl, writeFileImpl, mkdirImpl, + realpathImpl, statImpl, } = options @@ -79,6 +82,20 @@ export function createMockFs(options: CreateMockFsOptions = {}): MockFs { return undefined } + const defaultRealpath = async (path: PathLike): Promise => { + const pathStr = String(path) + const isKnownPath = + pathStr in writtenFiles || + pathStr in directories || + createdDirs.has(pathStr) + + if (!isKnownPath) { + throw new Error(`Path not found: ${pathStr}`) + } + + return pathStr + } + const defaultStat = async (path: PathLike): Promise => { const pathStr = String(path) const isFile = pathStr in writtenFiles @@ -134,6 +151,10 @@ export function createMockFs(options: CreateMockFsOptions = {}): MockFs { mkdirImpl(String(path), opts) : defaultMkdir + const realpathFn = realpathImpl + ? async (path: PathLike) => realpathImpl(String(path)) + : defaultRealpath + const statFn = statImpl ? async (path: PathLike) => statImpl(String(path)) : defaultStat @@ -143,6 +164,7 @@ export function createMockFs(options: CreateMockFsOptions = {}): MockFs { readdir: mock(readdirFn), writeFile: mock(writeFileFn), mkdir: mock(mkdirFn), + realpath: mock(realpathFn), stat: mock(statFn), } as unknown as MockFs } @@ -153,6 +175,7 @@ export function restoreMockFs(mockFs: MockFs): void { mocks.readdir.mockRestore() mocks.writeFile.mockRestore() mocks.mkdir.mockRestore() + mocks.realpath.mockRestore() mocks.stat.mockRestore() } @@ -162,5 +185,6 @@ export function clearMockFs(mockFs: MockFs): void { mocks.readdir.mockClear() mocks.writeFile.mockClear() mocks.mkdir.mockClear() + mocks.realpath.mockClear() mocks.stat.mockClear() } diff --git a/common/src/types/filesystem.ts b/common/src/types/filesystem.ts index 6fa64e1168..4506b5c87e 100644 --- a/common/src/types/filesystem.ts +++ b/common/src/types/filesystem.ts @@ -6,5 +6,11 @@ import type fs from 'fs' */ export type CodebuffFileSystem = Pick< typeof fs.promises, - 'mkdir' | 'readdir' | 'readFile' | 'stat' | 'unlink' | 'writeFile' + | 'mkdir' + | 'readdir' + | 'readFile' + | 'realpath' + | 'stat' + | 'unlink' + | 'writeFile' > diff --git a/sdk/src/__tests__/list-directory.test.ts b/sdk/src/__tests__/list-directory.test.ts new file mode 100644 index 0000000000..5637a75b4c --- /dev/null +++ b/sdk/src/__tests__/list-directory.test.ts @@ -0,0 +1,224 @@ +import { describe, expect, it, mock } from 'bun:test' + +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' + +const PROJECT_ROOT = path.resolve('workspace', 'project') + +function createFs(realpaths: Record) { + const readdir = mock(async (_path: PathLike) => { + 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 + }), + readdir, + } as unknown as CodebuffFileSystem + + return { fs, readdir } +} + +describe('listDirectory', () => { + it('allows listing the project root itself', async () => { + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + }) + + const result = await listDirectory({ + directoryPath: '.', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result[0]).toEqual({ + type: 'json', + value: { + files: ['index.ts'], + directories: [], + path: '.', + }, + }) + expect(readdir).toHaveBeenCalledWith(PROJECT_ROOT, { + withFileTypes: true, + }) + }) + + it('lists a directory inside the project and preserves the requested path', async () => { + const childPath = path.join(PROJECT_ROOT, 'src') + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + [childPath]: childPath, + }) + + const result = await listDirectory({ + directoryPath: 'src', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + files: ['index.ts'], + directories: [], + path: 'src', + }, + }, + ]) + expect(readdir).toHaveBeenCalledWith(childPath, { + withFileTypes: true, + }) + }) + + it('returns the normal list error when the requested directory is missing', async () => { + const missingPath = path.join(PROJECT_ROOT, 'missing') + const readdir = mock(async (_path: PathLike) => [] as Dirent[]) + const fs = { + realpath: mock(async (requestedPath: PathLike) => { + const requestedPathString = String(requestedPath) + if (requestedPathString === missingPath) { + throw new Error( + `ENOENT: no such file or directory, realpath '${missingPath}'`, + ) + } + return requestedPathString + }), + readdir, + } as unknown as CodebuffFileSystem + + const result = await listDirectory({ + directoryPath: 'missing', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + errorMessage: `Failed to list directory: ENOENT: no such file or directory, realpath '${missingPath}'`, + }, + }, + ]) + expect(readdir).not.toHaveBeenCalled() + }) + + it('rejects sibling paths that only share the project prefix', async () => { + const siblingPath = path.resolve(PROJECT_ROOT, '..', 'project-evil') + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + [siblingPath]: siblingPath, + }) + + const result = await listDirectory({ + directoryPath: '../project-evil', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + errorMessage: + "Invalid path: Path '../project-evil' is outside the project directory.", + }, + }, + ]) + expect(readdir).not.toHaveBeenCalled() + }) + + it('rejects the project parent directory', async () => { + const parentPath = path.dirname(PROJECT_ROOT) + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + [parentPath]: parentPath, + }) + + const result = await listDirectory({ + directoryPath: '..', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + errorMessage: + "Invalid path: Path '..' is outside the project directory.", + }, + }, + ]) + expect(readdir).not.toHaveBeenCalled() + }) + + it('rejects directories that escape through a symlink', async () => { + const symlinkPath = path.join(PROJECT_ROOT, 'link') + const outsidePath = path.resolve(PROJECT_ROOT, '..', 'outside') + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + [symlinkPath]: outsidePath, + }) + + const result = await listDirectory({ + directoryPath: 'link', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + errorMessage: + "Invalid path: Path 'link' is outside the project directory.", + }, + }, + ]) + expect(readdir).not.toHaveBeenCalled() + }) + + it('allows a symlink that resolves inside the project', async () => { + const symlinkPath = path.join(PROJECT_ROOT, 'link') + const realTarget = path.join(PROJECT_ROOT, 'src') + const { fs, readdir } = createFs({ + [PROJECT_ROOT]: PROJECT_ROOT, + [symlinkPath]: realTarget, + }) + + const result = await listDirectory({ + directoryPath: 'link', + projectPath: PROJECT_ROOT, + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + files: ['index.ts'], + directories: [], + path: 'link', + }, + }, + ]) + expect(readdir).toHaveBeenCalledWith(realTarget, { + withFileTypes: true, + }) + }) +}) diff --git a/sdk/src/tools/list-directory.ts b/sdk/src/tools/list-directory.ts index 3bf66fa968..53f12b660e 100644 --- a/sdk/src/tools/list-directory.ts +++ b/sdk/src/tools/list-directory.ts @@ -2,6 +2,7 @@ import * as path from 'path' import type { CodebuffToolOutput } from '@codebuff/common/tools/list' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' +import { isPathInside } from '@codebuff/common/util/path' export async function listDirectory(params: { directoryPath: string @@ -11,9 +12,23 @@ export async function listDirectory(params: { const { directoryPath, projectPath, fs } = params try { - const resolvedPath = path.resolve(projectPath, directoryPath) + const projectRoot = path.resolve(projectPath) + const resolvedPath = path.resolve(projectRoot, directoryPath) + const realProjectRoot = await fs.realpath(projectRoot) + const realResolvedPath = await fs.realpath(resolvedPath) - const entries = await fs.readdir(resolvedPath, { + if (!isPathInside(realProjectRoot, realResolvedPath)) { + return [ + { + type: 'json', + value: { + errorMessage: `Invalid path: Path '${directoryPath}' is outside the project directory.`, + }, + }, + ] + } + + const entries = await fs.readdir(realResolvedPath, { withFileTypes: true, }) From c70d5bf9ef15fb1a65601b6dddc60973cbf613b3 Mon Sep 17 00:00:00 2001 From: umutcagand <237324081+c8dhjp4tyv-bit@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:00:22 +0300 Subject: [PATCH 2/2] 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. --- sdk/src/__tests__/list-directory.test.ts | 88 ++++++++++++++++++++++-- sdk/src/tools/list-directory.ts | 32 +++++++++ 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/sdk/src/__tests__/list-directory.test.ts b/sdk/src/__tests__/list-directory.test.ts index 5637a75b4c..04aa5f7b78 100644 --- a/sdk/src/__tests__/list-directory.test.ts +++ b/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) { +function createFs( + realpaths: Record, + 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 + } = {}, +) { const readdir = mock(async (_path: PathLike) => { return [ { @@ -20,15 +28,38 @@ function createFs(realpaths: Record) { ] 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') diff --git a/sdk/src/tools/list-directory.ts b/sdk/src/tools/list-directory.ts index 53f12b660e..879ef8ca09 100644 --- a/sdk/src/tools/list-directory.ts +++ b/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[] = []