fix(sandbox): allow MCP filesystem server paths in local bash commands (#1527)
* feat/bug-fix: copy the allowed path configurations in MCP filesystem tools to bash tool. With updated unit test * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
parent
25df82cbfd
commit
9e5ba74ecd
|
|
@ -212,6 +212,35 @@ def _resolve_acp_workspace_path(path: str, thread_id: str | None = None) -> str:
|
||||||
return str(resolved_path)
|
return str(resolved_path)
|
||||||
|
|
||||||
|
|
||||||
|
def _get_mcp_allowed_paths() -> list[str]:
|
||||||
|
"""Get the list of allowed paths from MCP config for file system server."""
|
||||||
|
allowed_paths = []
|
||||||
|
try:
|
||||||
|
from deerflow.config.extensions_config import get_extensions_config
|
||||||
|
|
||||||
|
extensions_config = get_extensions_config()
|
||||||
|
|
||||||
|
for _, server in extensions_config.mcp_servers.items():
|
||||||
|
if not server.enabled:
|
||||||
|
continue
|
||||||
|
|
||||||
|
# Only check the filesystem server
|
||||||
|
args = server.args or []
|
||||||
|
# Check if args has server-filesystem package
|
||||||
|
has_filesystem = any("server-filesystem" in arg for arg in args)
|
||||||
|
if not has_filesystem:
|
||||||
|
continue
|
||||||
|
# Unpack the allowed file system paths in config
|
||||||
|
for arg in args:
|
||||||
|
if not arg.startswith("-") and arg.startswith("/"):
|
||||||
|
allowed_paths.append(arg.rstrip("/") + "/")
|
||||||
|
|
||||||
|
except Exception:
|
||||||
|
pass
|
||||||
|
|
||||||
|
return allowed_paths
|
||||||
|
|
||||||
|
|
||||||
def _path_variants(path: str) -> set[str]:
|
def _path_variants(path: str) -> set[str]:
|
||||||
return {path, path.replace("\\", "/"), path.replace("/", "\\")}
|
return {path, path.replace("\\", "/"), path.replace("/", "\\")}
|
||||||
|
|
||||||
|
|
@ -481,8 +510,14 @@ def validate_local_bash_command_paths(command: str, thread_data: ThreadDataState
|
||||||
raise SandboxRuntimeError("Thread data not available for local sandbox")
|
raise SandboxRuntimeError("Thread data not available for local sandbox")
|
||||||
|
|
||||||
unsafe_paths: list[str] = []
|
unsafe_paths: list[str] = []
|
||||||
|
allowed_paths = _get_mcp_allowed_paths()
|
||||||
|
|
||||||
for absolute_path in _ABSOLUTE_PATH_PATTERN.findall(command):
|
for absolute_path in _ABSOLUTE_PATH_PATTERN.findall(command):
|
||||||
|
# Check for MCP filesystem server allowed paths
|
||||||
|
if any(absolute_path.startswith(path) or absolute_path == path.rstrip("/") for path in allowed_paths):
|
||||||
|
_reject_path_traversal(absolute_path)
|
||||||
|
continue
|
||||||
|
|
||||||
if absolute_path == VIRTUAL_PATH_PREFIX or absolute_path.startswith(f"{VIRTUAL_PATH_PREFIX}/"):
|
if absolute_path == VIRTUAL_PATH_PREFIX or absolute_path.startswith(f"{VIRTUAL_PATH_PREFIX}/"):
|
||||||
_reject_path_traversal(absolute_path)
|
_reject_path_traversal(absolute_path)
|
||||||
continue
|
continue
|
||||||
|
|
|
||||||
|
|
@ -423,3 +423,40 @@ def test_mask_local_paths_in_output_hides_acp_workspace_host_paths() -> None:
|
||||||
|
|
||||||
assert acp_host not in masked
|
assert acp_host not in masked
|
||||||
assert "/mnt/acp-workspace/hello.py" in masked
|
assert "/mnt/acp-workspace/hello.py" in masked
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_local_bash_command_paths_allows_mcp_filesystem_paths() -> None:
|
||||||
|
"""Bash commands referencing MCP filesystem server paths should be allowed."""
|
||||||
|
from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig
|
||||||
|
|
||||||
|
mock_config = ExtensionsConfig(
|
||||||
|
mcp_servers={
|
||||||
|
"filesystem": McpServerConfig(
|
||||||
|
enabled=True,
|
||||||
|
command="npx",
|
||||||
|
args=["-y", "@modelcontextprotocol/server-filesystem", "/mnt/d/workspace"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
with patch("deerflow.config.extensions_config.get_extensions_config", return_value=mock_config):
|
||||||
|
# Should not raise - MCP filesystem paths are allowed
|
||||||
|
validate_local_bash_command_paths("ls /mnt/d/workspace", _THREAD_DATA)
|
||||||
|
validate_local_bash_command_paths("cat /mnt/d/workspace/subdir/file.txt", _THREAD_DATA)
|
||||||
|
|
||||||
|
# Path traversal should still be blocked
|
||||||
|
with pytest.raises(PermissionError, match="path traversal"):
|
||||||
|
validate_local_bash_command_paths("cat /mnt/d/workspace/../../etc/passwd", _THREAD_DATA)
|
||||||
|
|
||||||
|
# Disabled servers should not expose paths
|
||||||
|
disabled_config = ExtensionsConfig(
|
||||||
|
mcp_servers={
|
||||||
|
"filesystem": McpServerConfig(
|
||||||
|
enabled=False,
|
||||||
|
command="npx",
|
||||||
|
args=["-y", "@modelcontextprotocol/server-filesystem", "/mnt/d/workspace"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
with patch("deerflow.config.extensions_config.get_extensions_config", return_value=disabled_config):
|
||||||
|
with pytest.raises(PermissionError, match="Unsafe absolute paths"):
|
||||||
|
validate_local_bash_command_paths("ls /mnt/d/workspace", _THREAD_DATA)
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue