-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(tools): allow file operations outside working directory #657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,12 @@ | |
|
|
||
|
|
||
| class Params(BaseModel): | ||
| path: str = Field(description="The absolute path to the file to write") | ||
| path: str = Field( | ||
| description=( | ||
| "The path to the file to write. Absolute paths are required when writing files " | ||
| "outside the working directory." | ||
| ) | ||
| ) | ||
| content: str = Field(description="The content to write to the file") | ||
| mode: Literal["overwrite", "append"] = Field( | ||
| description=( | ||
|
|
@@ -39,41 +44,35 @@ def __init__(self, builtin_args: BuiltinSystemPromptArgs, approval: Approval): | |
|
|
||
| async def _validate_path(self, path: KaosPath) -> ToolError | None: | ||
| """Validate that the path is safe to write.""" | ||
| # Check for path traversal attempts | ||
| resolved_path = path.canonical() | ||
|
|
||
| # Ensure the path is within work directory | ||
| if not is_within_directory(resolved_path, self._work_dir): | ||
| if not is_within_directory(resolved_path, self._work_dir) and not path.is_absolute(): | ||
| return ToolError( | ||
| message=( | ||
| f"`{path}` is outside the working directory. " | ||
| "You can only write files within the working directory." | ||
| f"`{path}` is not an absolute path. " | ||
| "You must provide an absolute path to write a file " | ||
| "outside the working directory." | ||
| ), | ||
| brief="Path outside working directory", | ||
| brief="Invalid path", | ||
| ) | ||
| return None | ||
|
|
||
| @override | ||
| async def __call__(self, params: Params) -> ToolReturnValue: | ||
| # TODO: checks: | ||
| # - check if the path may contain secrets | ||
| # - check if the file format is writable | ||
| try: | ||
| p = KaosPath(params.path) | ||
| if not params.path: | ||
| return ToolError( | ||
| message="File path cannot be empty.", | ||
| brief="Empty file path", | ||
| ) | ||
|
|
||
| if not p.is_absolute(): | ||
| return ToolError( | ||
| message=( | ||
| f"`{params.path}` is not an absolute path. " | ||
| "You must provide an absolute path to write a file." | ||
| ), | ||
| brief="Invalid path", | ||
| ) | ||
| try: | ||
| p = KaosPath(params.path).expanduser() | ||
|
|
||
| # Validate path safety | ||
| path_error = await self._validate_path(p) | ||
| if path_error: | ||
| return path_error | ||
| if err := await self._validate_path(p): | ||
| return err | ||
| p = p.canonical() | ||
|
Comment on lines
+71
to
+75
|
||
|
|
||
| if not await p.parent.exists(): | ||
| return ToolError( | ||
|
|
@@ -101,17 +100,23 @@ async def __call__(self, params: Params) -> ToolReturnValue: | |
| ) | ||
| diff_blocks: list[DisplayBlock] = list( | ||
| build_diff_blocks( | ||
| params.path, | ||
| str(p), | ||
| old_text or "", | ||
| new_text, | ||
| ) | ||
| ) | ||
|
|
||
| action = ( | ||
| FileActions.EDIT | ||
| if is_within_directory(p, self._work_dir) | ||
| else FileActions.EDIT_OUTSIDE | ||
| ) | ||
|
|
||
| # Request approval | ||
| if not await self._approval.request( | ||
| self.name, | ||
| FileActions.EDIT, | ||
| f"Write file `{params.path}`", | ||
| action, | ||
| f"Write file `{p}`", | ||
| display=diff_blocks, | ||
| ): | ||
| return ToolRejectedError() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is redundant path canonicalization. The path is canonicalized inside
_validate_pathat line 50, but then canonicalized again at line 82 after validation. Additionally, the validation is called with the expanded but non-canonical path, which could lead to inconsistent behavior. Consider either canonicalizing before validation or removing the duplicate canonicalization after validation.