mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-05-21 15:36:48 +00:00
fix(sandbox): add group/other read permissions to uploaded files for Docker sandbox (#3127)
When using AIO sandbox with LocalContainerBackend, uploaded files are created with 0o600 (owner-only) permissions by the gateway process running as root. The sandbox process inside the Docker container runs as a non-root user and cannot read these bind-mounted files, causing a "Permission denied" error on read_file. Add `needs_upload_permission_adjustment` attribute to SandboxProvider (default True) to indicate that uploaded files need chmod adjustment. LocalSandboxProvider opts out (same user). A new `_make_file_sandbox_readable` function adds S_IRGRP | S_IROTH bits after files are written, changing permissions from 0o600 to 0o644 so the sandbox can read the uploads. fixes #3127
This commit is contained in:
@@ -74,6 +74,25 @@ def _make_file_sandbox_writable(file_path: os.PathLike[str] | str) -> None:
|
||||
os.chmod(file_path, writable_mode, **chmod_kwargs)
|
||||
|
||||
|
||||
def _make_file_sandbox_readable(file_path: os.PathLike[str] | str) -> None:
|
||||
"""Ensure uploaded files are readable by the sandbox process.
|
||||
|
||||
For Docker sandboxes (AIO), the gateway writes files as root with 0o600
|
||||
permissions, then bind-mounts the host directory into the container. The
|
||||
sandbox process inside the container runs as a non-root user and cannot
|
||||
read those files without group/other read bits. This function adds
|
||||
``S_IRGRP | S_IROTH`` so the sandbox can read the uploaded content.
|
||||
"""
|
||||
file_stat = os.lstat(file_path)
|
||||
if stat.S_ISLNK(file_stat.st_mode):
|
||||
logger.warning("Skipping sandbox chmod for symlinked upload path: %s", file_path)
|
||||
return
|
||||
|
||||
readable_mode = stat.S_IMODE(file_stat.st_mode) | stat.S_IRGRP | stat.S_IROTH
|
||||
chmod_kwargs = {"follow_symlinks": False} if os.chmod in os.supports_follow_symlinks else {}
|
||||
os.chmod(file_path, readable_mode, **chmod_kwargs)
|
||||
|
||||
|
||||
def _uses_thread_data_mounts(sandbox_provider: SandboxProvider) -> bool:
|
||||
return bool(getattr(sandbox_provider, "uses_thread_data_mounts", False))
|
||||
|
||||
@@ -276,6 +295,15 @@ async def upload_files(
|
||||
_cleanup_uploaded_paths(written_paths)
|
||||
raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}: {str(e)}")
|
||||
|
||||
# When the sandbox uses bind-mounted thread data directories (e.g. AIO with
|
||||
# LocalContainerBackend), uploaded files are visible inside the container but
|
||||
# retain the 0o600 permissions set by the gateway. The sandbox process runs
|
||||
# as a different user and cannot read them. Adjust permissions to add
|
||||
# group/other read bits so the sandbox can access the files.
|
||||
if not sync_to_sandbox and getattr(sandbox_provider, "needs_upload_permission_adjustment", True):
|
||||
for file_path in written_paths:
|
||||
_make_file_sandbox_readable(file_path)
|
||||
|
||||
if sync_to_sandbox:
|
||||
for file_path, virtual_path in sandbox_sync_targets:
|
||||
_make_file_sandbox_writable(file_path)
|
||||
|
||||
@@ -63,6 +63,7 @@ class LocalSandboxProvider(SandboxProvider):
|
||||
"""
|
||||
|
||||
uses_thread_data_mounts = True
|
||||
needs_upload_permission_adjustment = False
|
||||
|
||||
def __init__(self, max_cached_threads: int = DEFAULT_MAX_CACHED_THREAD_SANDBOXES):
|
||||
"""Initialize the local sandbox provider with static path mappings.
|
||||
|
||||
@@ -10,6 +10,7 @@ class SandboxProvider(ABC):
|
||||
"""Abstract base class for sandbox providers"""
|
||||
|
||||
uses_thread_data_mounts: bool = False
|
||||
needs_upload_permission_adjustment: bool = True
|
||||
|
||||
@abstractmethod
|
||||
def acquire(self, thread_id: str | None = None) -> str:
|
||||
|
||||
@@ -218,6 +218,7 @@ def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path):
|
||||
|
||||
provider = MagicMock()
|
||||
provider.uses_thread_data_mounts = True
|
||||
provider.needs_upload_permission_adjustment = False
|
||||
provider.acquire.return_value = "local"
|
||||
sandbox = MagicMock()
|
||||
provider.get.return_value = sandbox
|
||||
@@ -227,12 +228,14 @@ def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path):
|
||||
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
|
||||
patch.object(uploads, "get_sandbox_provider", return_value=provider),
|
||||
patch.object(uploads, "_make_file_sandbox_writable") as make_writable,
|
||||
patch.object(uploads, "_make_file_sandbox_readable") as make_readable,
|
||||
):
|
||||
file = UploadFile(filename="notes.txt", file=BytesIO(b"hello uploads"))
|
||||
result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace()))
|
||||
|
||||
assert result.success is True
|
||||
make_writable.assert_not_called()
|
||||
make_readable.assert_not_called()
|
||||
|
||||
|
||||
def test_upload_files_acquires_non_local_sandbox_before_writing(tmp_path):
|
||||
@@ -430,6 +433,59 @@ def test_make_file_sandbox_writable_skips_symlinks(tmp_path):
|
||||
chmod.assert_not_called()
|
||||
|
||||
|
||||
def test_make_file_sandbox_readable_adds_read_bits_for_regular_files(tmp_path):
|
||||
file_path = tmp_path / "data.csv"
|
||||
file_path.write_bytes(b"csv-data")
|
||||
# Simulate the 0o600 permissions set by open_upload_file_no_symlink
|
||||
file_path.chmod(0o600)
|
||||
|
||||
uploads._make_file_sandbox_readable(file_path)
|
||||
|
||||
updated_mode = stat.S_IMODE(file_path.stat().st_mode)
|
||||
assert updated_mode & stat.S_IRUSR
|
||||
assert updated_mode & stat.S_IRGRP
|
||||
assert updated_mode & stat.S_IROTH
|
||||
|
||||
|
||||
def test_make_file_sandbox_readable_skips_symlinks(tmp_path):
|
||||
file_path = tmp_path / "target-link.txt"
|
||||
file_path.write_text("hello", encoding="utf-8")
|
||||
symlink_stat = MagicMock(st_mode=stat.S_IFLNK)
|
||||
|
||||
with (
|
||||
patch.object(uploads.os, "lstat", return_value=symlink_stat),
|
||||
patch.object(uploads.os, "chmod") as chmod,
|
||||
):
|
||||
uploads._make_file_sandbox_readable(file_path)
|
||||
|
||||
chmod.assert_not_called()
|
||||
|
||||
|
||||
def test_upload_files_adjusts_read_permissions_for_mounted_non_local_sandbox(tmp_path):
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir(parents=True)
|
||||
|
||||
# AIO sandbox with LocalContainerBackend: uses_thread_data_mounts=True
|
||||
# but needs_upload_permission_adjustment=True (default)
|
||||
provider = MagicMock()
|
||||
provider.uses_thread_data_mounts = True
|
||||
provider.needs_upload_permission_adjustment = True
|
||||
|
||||
with (
|
||||
patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir),
|
||||
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
|
||||
patch.object(uploads, "get_sandbox_provider", return_value=provider),
|
||||
patch.object(uploads, "_make_file_sandbox_readable") as make_readable,
|
||||
):
|
||||
file = UploadFile(filename="notes.txt", file=BytesIO(b"hello uploads"))
|
||||
result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace()))
|
||||
|
||||
assert result.success is True
|
||||
make_readable.assert_called_once()
|
||||
called_path = make_readable.call_args[0][0]
|
||||
assert called_path.name == "notes.txt"
|
||||
|
||||
|
||||
def test_upload_files_rejects_dotdot_and_dot_filenames(tmp_path):
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir(parents=True)
|
||||
|
||||
Reference in New Issue
Block a user