Compare commits

...

3 Commits

Author SHA1 Message Date
Willem Jiang bc8aab0ade 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.
2026-05-23 16:31:30 +08:00
Willem Jiang cfd9c61b9a Merge branch 'main' into fix-3127 2026-05-22 21:56:04 +08:00
Willem Jiang 4731605d99 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
2026-05-21 17:51:56 +08:00
4 changed files with 91 additions and 1 deletions
+30 -1
View File
@@ -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)
@@ -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:
+59
View File
@@ -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)