Skip to content

fix(sdk): prevent list_directory path escapes - #1098

Open
c8dhjp4tyv-bit wants to merge 6 commits into
CodebuffAI:mainfrom
c8dhjp4tyv-bit:fix/sdk-list-directory-path-boundary
Open

fix(sdk): prevent list_directory path escapes#1098
c8dhjp4tyv-bit wants to merge 6 commits into
CodebuffAI:mainfrom
c8dhjp4tyv-bit:fix/sdk-list-directory-path-boundary

Conversation

@c8dhjp4tyv-bit

Copy link
Copy Markdown

Summary

  • Normalize the project root before resolving the requested directory.
  • Compare the real project root and target paths before listing.
  • Reuse the shared path-containment helper so parent paths and sibling-prefix paths are rejected.
  • Extend the filesystem abstraction and test mock with realpath.
  • Add regression tests for the project root, valid child directories, parent traversal, sibling-prefix traversal, and symlink escapes.

Fixes #463

Validation

  • bun run --cwd common typecheck
  • bun run --cwd sdk typecheck
  • bun run --cwd sdk test — 518 passed, 0 failed
  • bun test common/src/__tests__/project-file-tree.test.ts — 12 passed, 0 failed
  • git diff --check

@c8dhjp4tyv-bit
c8dhjp4tyv-bit force-pushed the fix/sdk-list-directory-path-boundary branch from 10e1ba9 to b00d9e9 Compare August 23, 2026 21:30
@codebuff-team

Copy link
Copy Markdown
Contributor

Good bug fix. path.resolve(projectPath, directoryPath) alone doesn't stop ../project-evil (shares prefix but escapes) or symlinks that resolve outside the project root, and this PR closes both holes by resolving both sides through fs.realpath and checking containment with the existing isPathInside helper before calling readdir. That matches the project's stated bug (#463).

The test suite in sdk/src/__tests__/list-directory.test.ts is thorough: it exercises project-root listing, valid child dirs, .. traversal, sibling-prefix traversal, and both an escaping and a non-escaping symlink. That's exactly the kind of edge-case coverage a security fix needs.

A couple of things worth double-checking before porting:

  1. fs.realpath(resolvedPath) will throw if the directory doesn't exist (ENOENT) rather than surfacing a clean "invalid path" error — worth confirming the existing catch block in list-directory.ts produces a sane error message for that case, since previously a missing directory would fail inside readdir with the same behavior, but now it fails one line earlier at realpath.
  2. Adding realpath to CodebuffFileSystem (common/src/types/filesystem.ts) means every other implementer/mocker of that interface across the codebase needs to supply it — you've updated the shared mock in common/src/testing/mocks/filesystem.ts, but it's worth greping for other places that construct a CodebuffFileSystem-shaped object by hand (not through the mock) to make sure nothing breaks at the type level.

Modest, well-scoped diff with tests — this is the kind of PR that's easy to review and port by hand.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Aug 24, 2026

Copy link
Copy Markdown
Author

Followed up on both review questions in cb86793.

  • Added a regression test for a missing requested directory. It pins the existing behavior after the new realpath step: the tool returns the normal Failed to list directory: ENOENT... JSON error and never calls readdir.
  • I also checked the CodebuffFileSystem usages/hand-built mocks. The shared mock is updated with realpath, and this PR's existing common + sdk typechecks already exercise the interface change. No additional concrete implementation was found that needs a production change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: list-directory path traversal — missing path.sep in startsWith check

2 participants