Skip to content

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

Open
c8dhjp4tyv-bit wants to merge 1 commit into
CodebuffAI:mainfrom
c8dhjp4tyv-bit:recreate/1098-sdk-list-directory-path-boundary
Open

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

Conversation

@c8dhjp4tyv-bit

Copy link
Copy Markdown

Recreated on the rewritten main after #1098 was auto-closed during repository maintenance. This carries the same reviewed change set on the new history.

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

Prior validation before the history rewrite:

  • 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

@codebuff-team

Copy link
Copy Markdown
Contributor

This looks like a real, well-scoped security fix. list-directory.ts previously did path.resolve(projectPath, directoryPath) and handed the result straight to readdir with no containment check, so ../-style traversal (and symlink escapes) would work. The fix:

  • Resolves both the project root and the target path through fs.realpath before comparing, which correctly handles symlink escapes (the link -> ../outside test case is a good catch).
  • Reuses isPathInside from @codebuff/common/util/path rather than reinventing containment logic.
  • Extends CodebuffFileSystem and the mock fs (common/src/testing/mocks/filesystem.ts) with realpath, keeping the abstraction consistent for future tools.
  • Adds targeted regression tests in sdk/src/__tests__/list-directory.test.ts covering root listing, valid child, missing directory (readdir not called), sibling-prefix rejection, parent rejection, and both symlink-escape and symlink-inside cases.

One thing worth double-checking on port: using realpath on the requested path before validating containment means a legitimate but nonexistent path (e.g. a directory the caller is about to create) will throw ENOENT before the containment check runs, changing the error message shape slightly from before — the test for the missing-directory case documents this, so it's likely intentional and acceptable, but flag it to whoever ports this so the change in error message is expected on the private side.

No forbidden paths touched (sdk/, common/ only). Good, minimal diff for the actual behavior change, with tests matching the existing bun:test conventions. This is worth porting as-is.

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

Copy link
Copy Markdown
Author

Rechecked the realpath ordering point. The missing-directory case is intentional and explicitly regression-tested: realpath returns the normal ENOENT-style list error before readdir, while traversal/sibling-prefix/symlink escapes are rejected by containment checks. Targeted list-directory suite: 7 passed, 0 failed. No additional code change was necessary.

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