Merge pull request #257 from RonaldJEN/main

Fix file download path encoding and host volume validation errors

Author: ninan-nn

sdks/sandbox/python/src/opensandbox/adapters/filesystem_adapter.py

@@ -25,6 +25,7 @@
 from collections.abc import AsyncIterator
 from io import IOBase, TextIOBase
 from typing import TypedDict
+from urllib.parse import quote
 
 import httpx
 
@@ -52,7 +53,7 @@
 
 class _DownloadRequest(TypedDict):
     url: str
-    params: dict[str, str]
+    params: dict[str, str] | None
     headers: dict[str, str]
 
 
@@ -158,11 +159,13 @@ async def read_bytes(
             request_data = self._build_download_request(path, range_header)
             client = await self._get_httpx_client()
 
-            response = await client.get(
-                request_data["url"],
-                params=request_data["params"],
-                headers=request_data["headers"],
-            )
+            request_kwargs: dict[str, dict[str, str]] = {
+                "headers": request_data["headers"],
+            }
+            if request_data["params"] is not None:
+                request_kwargs["params"] = request_data["params"]
+
+            response = await client.get(request_data["url"], **request_kwargs)
             response.raise_for_status()
             return response.content
         except Exception as e:
@@ -186,12 +189,13 @@ async def read_bytes_stream(
             params = request_data["params"]
             headers = request_data["headers"]
 
-            request = client.build_request(
-                "GET",
-                url,
-                params=params,
-                headers=headers,
-            )
+            request_kwargs: dict[str, dict[str, str]] = {
+                "headers": headers,
+            }
+            if params is not None:
+                request_kwargs["params"] = params
+
+            request = client.build_request("GET", url, **request_kwargs)
 
             response = await client.send(request, stream=True)
 
@@ -476,15 +480,15 @@ def _build_download_request(
         Returns:
             Dictionary containing URL, parameters, and headers for the request
         """
-        url = self._get_execd_url(self.FILESYSTEM_DOWNLOAD_PATH)
-        params = {"path": path}
+        encoded_path = quote(path, safe="/")
+        url = f"{self._get_execd_url(self.FILESYSTEM_DOWNLOAD_PATH)}?path={encoded_path}"
         headers: dict[str, str] = {}
 
         if range_header:
             headers["Range"] = range_header
 
         return {
             "url": url,
-            "params": params,
+            "params": None,
             "headers": headers,
         }

sdks/sandbox/python/src/opensandbox/sync/adapters/filesystem_adapter.py

@@ -22,6 +22,7 @@
 from collections.abc import Iterator
 from io import IOBase, TextIOBase
 from typing import TypedDict
+from urllib.parse import quote
 
 import httpx
 
@@ -49,7 +50,7 @@
 
 class _DownloadRequest(TypedDict):
     url: str
-    params: dict[str, str]
+    params: dict[str, str] | None
     headers: dict[str, str]
 
 
@@ -87,12 +88,12 @@ def _get_execd_url(self, path: str) -> str:
         return f"{self.connection_config.protocol}://{self.execd_endpoint.endpoint}{path}"
 
     def _build_download_request(self, path: str, range_header: str | None = None) -> _DownloadRequest:
-        url = self._get_execd_url(self.FILESYSTEM_DOWNLOAD_PATH)
-        params = {"path": path}
+        encoded_path = quote(path, safe="/")
+        url = f"{self._get_execd_url(self.FILESYSTEM_DOWNLOAD_PATH)}?path={encoded_path}"
         headers: dict[str, str] = {}
         if range_header:
             headers["Range"] = range_header
-        return {"url": url, "params": params, "headers": headers}
+        return {"url": url, "params": None, "headers": headers}
 
     def read_file(
         self,
@@ -108,11 +109,13 @@ def read_bytes(self, path: str, *, range_header: str | None = None) -> bytes:
         logger.debug("Reading file as bytes: %s", path)
         try:
             request_data = self._build_download_request(path, range_header)
-            response = self._httpx_client.get(
-                request_data["url"],
-                params=request_data["params"],
-                headers=request_data["headers"],
-            )
+            request_kwargs: dict[str, dict[str, str]] = {
+                "headers": request_data["headers"],
+            }
+            if request_data["params"] is not None:
+                request_kwargs["params"] = request_data["params"]
+
+            response = self._httpx_client.get(request_data["url"], **request_kwargs)
             response.raise_for_status()
             return response.content
         except Exception as e:
@@ -128,7 +131,11 @@ def read_bytes_stream(
         params = request_data["params"]
         headers = request_data["headers"]
 
-        request = self._httpx_client.build_request("GET", url, params=params, headers=headers)
+        request_kwargs: dict[str, dict[str, str]] = {"headers": headers}
+        if params is not None:
+            request_kwargs["params"] = params
+
+        request = self._httpx_client.build_request("GET", url, **request_kwargs)
         response = self._httpx_client.send(request, stream=True)
 
         if response.status_code >= 300:

server/src/api/lifecycle.py

@@ -432,9 +432,11 @@ async def proxy_sandbox_endpoint_request(request: Request, sandbox_id: str, port
 
     target_host = endpoint.endpoint
     query_string = request.url.query
-    target_url = f"http://{target_host}/{full_path}"
-    if query_string:
-        target_url = f"{target_url}?{query_string}"
+    target_url = (
+        f"http://{target_host}/{full_path}?{query_string}"
+        if query_string
+        else f"http://{target_host}/{full_path}"
+    )
 
     client: httpx.AsyncClient = request.app.state.http_client
 

server/src/services/constants.py

@@ -69,6 +69,7 @@ class SandboxErrorCodes:
     INVALID_PVC_NAME = "VOLUME::INVALID_PVC_NAME"
     UNSUPPORTED_VOLUME_BACKEND = "VOLUME::UNSUPPORTED_BACKEND"
     HOST_PATH_NOT_FOUND = "VOLUME::HOST_PATH_NOT_FOUND"
+    HOST_PATH_CREATE_FAILED = "VOLUME::HOST_PATH_CREATE_FAILED"
     PVC_VOLUME_NOT_FOUND = "VOLUME::PVC_NOT_FOUND"
     PVC_VOLUME_INSPECT_FAILED = "VOLUME::PVC_INSPECT_FAILED"
     PVC_SUBPATH_UNSUPPORTED_DRIVER = "VOLUME::PVC_SUBPATH_UNSUPPORTED_DRIVER"

server/src/services/docker.py

@@ -976,14 +976,15 @@ def _validate_host_volume(volume, allowed_prefixes: Optional[list[str]]) -> None
         Docker-specific validation for host bind mount volumes.
 
         Validates that the resolved host path (host.path + optional subPath)
-        exists on the filesystem and remains within allowed prefixes.
+        remains within allowed prefixes, then ensures the directory exists on
+        the filesystem — creating it automatically if it does not.
 
         Args:
             volume: Volume with host backend.
             allowed_prefixes: Optional allowlist of host path prefixes.
 
         Raises:
-            HTTPException: When the resolved path is invalid or missing.
+            HTTPException: When the resolved path is invalid or cannot be created.
         """
         resolved_path = volume.host.path
         if volume.sub_path:
@@ -996,14 +997,16 @@ def _validate_host_volume(volume, allowed_prefixes: Optional[list[str]]) -> None
         if allowed_prefixes and resolved_path != volume.host.path:
             ensure_valid_host_path(resolved_path, allowed_prefixes)
 
-        if not os.path.exists(resolved_path):
+        try:
+            os.makedirs(resolved_path, exist_ok=True)
+        except OSError as e:
             raise HTTPException(
-                status_code=status.HTTP_400_BAD_REQUEST,
+                status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
                 detail={
-                    "code": SandboxErrorCodes.HOST_PATH_NOT_FOUND,
+                    "code": SandboxErrorCodes.HOST_PATH_CREATE_FAILED,
                     "message": (
-                        f"Volume '{volume.name}': resolved host path '{resolved_path}' "
-                        "does not exist. Host paths must exist before sandbox creation."
+                        f"Volume '{volume.name}': could not ensure host path "
+                        f"directory exists at '{resolved_path}': {type(e).__name__}"
                     ),
                 },
             )

server/tests/test_docker_service.py

@@ -988,7 +988,7 @@ def test_pvc_subpath_binds_resolved_to_mountpoint(self, mock_docker):
         assert binds[0] == "/var/lib/docker/volumes/my-vol/_data/datasets/train:/mnt/train:ro"
 
     def test_host_path_not_found_rejected(self, mock_docker):
-        """Host path that does not exist should be rejected."""
+        """Host path create failure should return 500 with HOST_PATH_CREATE_FAILED."""
         mock_client = MagicMock()
         mock_client.containers.list.return_value = []
         mock_docker.from_env.return_value = mock_client
@@ -1012,11 +1012,12 @@ def test_host_path_not_found_rejected(self, mock_docker):
             ],
         )
 
-        with pytest.raises(HTTPException) as exc_info:
-            service.create_sandbox(request)
+        with patch("src.services.docker.os.makedirs", side_effect=PermissionError("denied")):
+            with pytest.raises(HTTPException) as exc_info:
+                service.create_sandbox(request)
 
-        assert exc_info.value.status_code == status.HTTP_400_BAD_REQUEST
-        assert exc_info.value.detail["code"] == SandboxErrorCodes.HOST_PATH_NOT_FOUND
+        assert exc_info.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+        assert exc_info.value.detail["code"] == SandboxErrorCodes.HOST_PATH_CREATE_FAILED
 
     def test_host_path_not_in_allowlist_rejected(self, mock_docker):
         """Host path not in allowlist should be rejected."""
@@ -1167,17 +1168,21 @@ def test_host_volume_with_subpath_resolved_correctly(self, mock_docker):
             assert len(binds) == 1
             assert binds[0] == f"{sub_dir}:/mnt/work:ro"
 
-    def test_host_subpath_not_found_rejected(self, mock_docker):
-        """Host volume with non-existent subPath should be rejected."""
+    def test_host_subpath_auto_created(self, mock_docker):
+        """Host volume with non-existent subPath should be auto-created."""
         mock_client = MagicMock()
         mock_client.containers.list.return_value = []
+        mock_client.api.create_host_config.return_value = {}
+        mock_client.api.create_container.return_value = {"Id": "cid"}
+        mock_client.containers.get.return_value = MagicMock()
         mock_docker.from_env.return_value = mock_client
 
         service = DockerSandboxService(config=_app_config())
 
         import tempfile
 
         with tempfile.TemporaryDirectory() as tmpdir:
+            sub = "auto-created-sub"
             request = CreateSandboxRequest(
                 image=ImageSpec(uri="python:3.11"),
                 timeout=120,
@@ -1191,16 +1196,31 @@ def test_host_subpath_not_found_rejected(self, mock_docker):
                         host=Host(path=tmpdir),
                         mount_path="/mnt/work",
                         read_only=False,
-                        sub_path="nonexistent-sub",
+                        sub_path=sub,
                     )
                 ],
             )
 
-            with pytest.raises(HTTPException) as exc_info:
-                service.create_sandbox(request)
+            import os
+
+            resolved = os.path.join(tmpdir, sub)
+            assert not os.path.exists(resolved)
 
-            assert exc_info.value.status_code == status.HTTP_400_BAD_REQUEST
-            assert exc_info.value.detail["code"] == SandboxErrorCodes.HOST_PATH_NOT_FOUND
+            # create_sandbox will proceed past volume validation (subpath
+            # auto-created) but will fail later during container provisioning
+            # (mock doesn't cover the full flow).  We only care that the
+            # directory was created — NOT that it raised HOST_PATH_CREATE_FAILED.
+            try:
+                service.create_sandbox(request)
+            except HTTPException as e:
+                # If it's our own create-failed error, the auto-create didn't
+                # work — let the test fail explicitly.
+                if e.detail.get("code") == SandboxErrorCodes.HOST_PATH_CREATE_FAILED:
+                    raise
+            except Exception:
+                pass  # other provisioning errors are expected
+
+            assert os.path.isdir(resolved)
 
     def test_empty_allowlist_permits_any_host_path(self, mock_docker):
         """Empty allowed_host_paths (default) should permit any valid host path."""