From 4731605d99b861a591a9575d36e080e2c3ad392d Mon Sep 17 00:00:00 2001 From: Willem Jiang Date: Thu, 21 May 2026 17:51:56 +0800 Subject: [PATCH] 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 --- backend/app/gateway/routers/uploads.py | 28 ++++++++++ .../sandbox/local/local_sandbox_provider.py | 1 + .../deerflow/sandbox/sandbox_provider.py | 1 + backend/tests/test_uploads_router.py | 56 +++++++++++++++++++ 4 files changed, 86 insertions(+) diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 386618725..6834ea2f2 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -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) diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py index d64a1c220..8b6b347ca 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py @@ -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. diff --git a/backend/packages/harness/deerflow/sandbox/sandbox_provider.py b/backend/packages/harness/deerflow/sandbox/sandbox_provider.py index b989f7830..58c52beee 100644 --- a/backend/packages/harness/deerflow/sandbox/sandbox_provider.py +++ b/backend/packages/harness/deerflow/sandbox/sandbox_provider.py @@ -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: diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 7846865b8..003932dcb 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -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)