-
Notifications
You must be signed in to change notification settings - Fork 93
base path restriction for FileEditor #1421
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -60,10 +60,12 @@ class FileEditor: | |
| _max_file_size: int | ||
| _encoding_manager: EncodingManager | ||
| _cwd: str | ||
| _base_path: Path | None | ||
|
|
||
| def __init__( | ||
| self, | ||
| workspace_root: str | None = None, | ||
| base_path: str | None = None, | ||
| max_file_size_mb: int | None = None, | ||
| ): | ||
| """Initialize the editor. | ||
|
|
@@ -74,6 +76,9 @@ def __init__( | |
| workspace_root: Root directory that serves as the current working | ||
| directory for relative path suggestions. Must be an absolute path. | ||
| If None, no path suggestions will be provided for relative paths. | ||
| base_path: Base directory that restricts all file operations. When set, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a small question, is there a reason why this isn't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, even I had the same hunch i.e. merging both of these parameters together... but the original issue demanded a separate parameter and since this is my first contribution, decided to follow the proposed idea. Let me know if this needs to change though...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, I think the issue is written with the help of the agent too, so who knows, we can take it as starting point, but change it freely if it makes sense. I think maybe workspace root makes sense |
||
| all file paths must be within this directory. Must be an absolute path. | ||
| If None, no path restrictions are enforced. | ||
| """ | ||
| self._history_manager = FileHistoryManager(max_history_per_file=10) | ||
| self._max_file_size = ( | ||
|
|
@@ -83,6 +88,17 @@ def __init__( | |
| # Initialize encoding manager | ||
| self._encoding_manager = EncodingManager() | ||
|
|
||
| # Set base_path for security enforcement | ||
| if base_path is not None: | ||
| base_path_obj = Path(base_path) | ||
| # Ensure base_path is an absolute path | ||
| if not base_path_obj.is_absolute(): | ||
| base_path_obj = base_path_obj.resolve() | ||
| self._base_path = base_path_obj | ||
| logger.info(f"FileEditor base path restriction enabled: {self._base_path}") | ||
| else: | ||
| self._base_path = None | ||
|
|
||
| # Set cwd (current working directory) if workspace_root is provided | ||
| if workspace_root is not None: | ||
| workspace_path = Path(workspace_root) | ||
|
|
@@ -551,7 +567,8 @@ def validate_path(self, command: CommandLiteral, path: Path) -> None: | |
|
|
||
| Validates: | ||
| 1. Path is absolute | ||
| 2. Path and command are compatible | ||
| 2. Path is within base_path (if set) | ||
| 3. Path and command are compatible | ||
| """ | ||
| # Check if its an absolute path | ||
| if not path.is_absolute(): | ||
|
|
@@ -571,6 +588,22 @@ def validate_path(self, command: CommandLiteral, path: Path) -> None: | |
| suggestion_message, | ||
| ) | ||
|
|
||
| # Check if path is within base_path (if set) | ||
| if self._base_path is not None: | ||
| # Resolve the path to handle symlinks and relative components | ||
| resolved_path = path.resolve(strict=False) | ||
|
|
||
| # Check if resolved path is within base_path | ||
| try: | ||
| resolved_path.relative_to(self._base_path) | ||
| except ValueError: | ||
| raise EditorToolParameterInvalidError( | ||
| "path", | ||
| str(path), | ||
| f"Path is outside the allowed base path. All file operations must " | ||
| f"be within: {self._base_path}", | ||
| ) | ||
|
|
||
| # Check if path and command are compatible | ||
| if command == "create" and path.exists(): | ||
| raise EditorToolParameterInvalidError( | ||
|
|
||
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.
If I understand correctly,
base_pathwill always be set, is that right? Even though it's defined as an optionalThere 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.
Yes, you're right. In this specific code path,
base_pathwill always havea value (either the provided one or
workspace.working_diras fallback).However, I kept
base_pathas optional in FileEditorTool's signature because:The fallback here ensures a sensible default for this specific use case
while keeping the tool flexible for other scenarios.
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.
Just to clarify, you're right we need to do this, to implement the restriction, but I do think you're right it should be optional too, though. The use case I have in mind is with the OpenHands-CLI, and I think it should be possible for the CLI user to enable or disable this.
This bit of code creates the executor... which I think all callers need? It's not clear to me if we have enough flexibility for the CLI to just say e.g. "
--always-approvemeans yolo, we'll disable path enforcement"? Please correct me if I'm wrong! It's just a tiny question, nothing else, I'd like to understand this.