Add mount_path parameter for flexibility and security

[M7Saad]

Problem

CodeInterpreterTool mounts current working directory with read/write access by default, which may not always be intended and can be a security concern.

Changes

• Added mount_path parameter to specify which directory to mount instead of always mounting current working directory
• Updated README with caution notice about default read/write access and documentation for the new mount_path parameter

[joaomdmoura]

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #352: Enhance CodeInterpreterTool with Configurable Mount Path

Overview

The introduction of the mount_path parameter in CodeInterpreterTool represents a significant improvement in maintaining security while executing code within Docker containers. By restricting file system access to specific directories, this feature enhances the safety of code execution.

Documentation Changes

Positive Aspects

• Clarity: The README includes a clear warning regarding the default mount behavior, ensuring users are made aware of potential risks.
• Informative Example: The documentation provides an example for utilizing the mount_path parameter, which is exceptionally helpful for users integrating this feature.
• Structured Updates: The documentation is well-organized, making it user-friendly.

Suggestions

1. Additional Examples: Include more examples demonstrating both relative and absolute path usage to bolster user understanding.
2. Security Best Practices: Introduce a section on security best practices related to mount_path, possibly including:

   ### Security Best Practices
   - Always utilize the most restrictive mount_path possible.
   - Avoid mounting directories that include sensitive information.
   - Example paths:
     ```python
     CodeInterpreterTool(mount_path="./data")  # Specific data directory
     CodeInterpreterTool(mount_path="./tmp")   # Temporary directory

Code Changes

Security Improvements

• Implementation of path validation prevents directory traversal attack vectors, which significantly enhances security.
• Inclusion of checks to ensure that the specified mount path exists and is indeed a directory.
• The code restricts access to paths predominantly within the project root.

Code Quality Issues

1. Error Handling:
   The current error handling is overly broad and could be improved. Adjusting it to catch specific exceptions will provide clarity in troubleshooting:

   except (ValueError, OSError) as e:
       Printer.print(
           f"Warning: Error resolving mount path '{self.mount_path}': {str(e)}.",
           color="yellow",
       )
       logger.warning(f"Mount path resolution failed: {str(e)}")

2. Missing Type Hints:
   Incorporating type hints for mount_path will enhance code readability:

   mount_path: Optional[str] = None

3. Defined Constants:
   Defining constants at the class level enhances maintainability and clarity:

   DEFAULT_CONTAINER_NAME = "code-interpreter"
   WORKSPACE_MOUNT_POINT = "/workspace"

4. Improved Documentation:
   Enrich the docstring for _init_docker_container to accurately reflect its parameters and exceptions:

   def _init_docker_container(self) -> Container:
       """Initializes and returns a Docker container for code execution.
       
       Returns:
           Container: A Docker container object ready for code execution.
       
       Raises:
           DockerException: If Docker operations fail.
           ValueError: If mount_path validation fails.
       """

Suggested Additional Features

• Mount Path Validation Method: Creating a dedicated method for validating mount paths would be beneficial:

  def _validate_mount_path(self, path: Path) -> Path:
      """Validates the provided mount path.
      
      Args:
          path: The mount path to validate
          
      Returns:
          Path: The validated absolute path
          
      Raises:
          ValueError: If path is invalid or outside project root
      """
      # Implementation goes here...

Performance Considerations

• The implementation performs path resolution only once during container initialization which should not impose notable performance overhead.

Security Assessment

Strengths

• The path validation mechanism significantly fortifies security by preventing unauthorized access or directory traversal.
• Informative warning messages serve as alerts for invalid paths, improving user feedback.

Recommendations

1. Mandatory Mount Path: Making the mount_path a required parameter can further enforce security standards.
2. Read-Only Mount Feature: Implementing an option for read-only mounts may prevent unauthorized modifications to files during execution.
3. Allowlist/Blocklist for Paths:

   class CodeInterpreterTool(BaseTool):
       mount_path: Optional[str] = None
       mount_readonly: bool = False
       allowed_paths: List[str] = []
       blocked_paths: List[str] = ["/etc", "/root", "/home"]

Testing Recommendations

Ensure the addition of comprehensive test cases to cover:

1. Invalid mount paths.
2. Attempts at directory traversal.
3. Non-existent path scenarios.
4. Validating read/write operations in the mounted directory.

Example Test:

def test_mount_path_validation():
    tool = CodeInterpreterTool(mount_path="../invalid")
    with pytest.raises(ValueError):
        tool._init_docker_container()
        
def test_mount_path_permissions():
    tool = CodeInterpreterTool(mount_path="./data")
    container = tool._init_docker_container()
    assert "/workspace" in container.attrs["Mounts"][0]["Destination"]

Final Recommendations

1. Implement all suggested security enhancements to further fortify access control.
2. Develop a comprehensive test suite to ensure the functionality and security integrity of the feature.
3. Expand documentation to encompass clearly defined security best practices.
4. Consider implementing more configurable mount options, such as read-only or temporary mounts.
5. Strengthen logging for security-related events for better traceability and accountability.
6. Introduce a mount path allowlist/blocklist to reinforce security.

The changes introduced in this PR mark substantial progress towards securing CodeInterpreterTool. Applying the aforementioned recommendations will further augment the tool's resilience and usability.