From f9b7071304178ba9f42836af7fdf8cc9ca48d61c Mon Sep 17 00:00:00 2001 From: Willem Jiang Date: Mon, 25 May 2026 09:26:18 +0800 Subject: [PATCH] fix(sandbox): add group/other read permissions to uploaded files for Docker sandbox (#3127) (#3134) * 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 * fix(uploads): unconditionally adjust file permissions for sandbox access The conditional check meant uploaded files retained 0o600 permissions in some Docker sandbox configurations, preventing the sandbox process (UID 1000) from reading them. Always add group/other read bits so every sandbox setup can access uploaded content. Also add read bits to the sync-path writable helper as defense in depth. --- backend/app/gateway/routers/uploads.py | 31 +++++++++- .../sandbox/local/local_sandbox_provider.py | 1 + .../deerflow/sandbox/sandbox_provider.py | 1 + backend/tests/test_uploads_router.py | 59 +++++++++++++++++++ 4 files changed, 91 insertions(+), 1 deletion(-) diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 386618725..9e75a35cd 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -69,11 +69,30 @@ def _make_file_sandbox_writable(file_path: os.PathLike[str] | str) -> None: logger.warning("Skipping sandbox chmod for symlinked upload path: %s", file_path) return - writable_mode = stat.S_IMODE(file_stat.st_mode) | stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH + writable_mode = stat.S_IMODE(file_stat.st_mode) | stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH | stat.S_IRGRP | stat.S_IROTH chmod_kwargs = {"follow_symlinks": False} if os.chmod in os.supports_follow_symlinks else {} 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,16 @@ async def upload_files( _cleanup_uploaded_paths(written_paths) raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}: {str(e)}") + # Uploaded files are created with 0o600 permissions (owner read/write only). + # In Docker sandbox deployments the gateway writes as root but the sandbox + # process runs as a non-root user (typically UID 1000). Without group/other + # read bits the sandbox cannot access the files — whether the uploads + # directory is bind-mounted into the container or synced via + # sandbox.update_file. Always add group/other read bits so every sandbox + # configuration can read the uploaded content. + 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 1bcdb2eb7..46804d321 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -219,6 +219,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 @@ -228,12 +229,17 @@ 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() + # Readable adjustment is now always applied regardless of sandbox type + make_readable.assert_called_once() + called_path = make_readable.call_args[0][0] + assert called_path.name == "notes.txt" def test_upload_files_acquires_non_local_sandbox_before_writing(tmp_path): @@ -431,6 +437,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)