Skip to content

feat: add FileSystemTool with path whitelist#2206

Open
baobaodawang-creater wants to merge 1 commit intohuggingface:mainfrom
baobaodawang-creater:feature/filesystem-tool
Open

feat: add FileSystemTool with path whitelist#2206
baobaodawang-creater wants to merge 1 commit intohuggingface:mainfrom
baobaodawang-creater:feature/filesystem-tool

Conversation

@baobaodawang-creater
Copy link
Copy Markdown

Summary

  • add a FileSystemTool implementation with a path whitelist
  • register the new tool in default tools
  • add unit tests covering the new filesystem tool

Copy link
Copy Markdown

@VANDRANKI VANDRANKI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whitelist design is solid. Using resolve() before relative_to() correctly neutralises .. traversal and symlinks that escape the allowed tree - both are handled at the OS level before the check runs. Tests cover the main cases cleanly.

Two things worth considering before merge:

No file size limit on read_file
An agent could call read_file on a large binary or log file and load its entire contents into memory. A simple size cap (configurable, e.g. max_file_size_bytes=10*1024*1024) would prevent accidental or adversarial memory exhaustion.

No test for symlinks that escape the allowed tree
The traversal test covers ../ in the path string. Worth adding a case where a symlink inside the allowed dir points outside it - just to confirm the resolve() defence covers that path too.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants