From 552f8423c439ebb53f9c73eaeb147942a539da43 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 27 Jul 2026 17:22:44 +0300 Subject: [PATCH 1/2] fix: percent-encode storage resource paths on the wire (#124) DIAL Core parses every resource path (in the URL and in JSON bodies such as sourceUrl/destinationUrl) as a percent-encoded URL. A raw reserved character (space, #, ?, [, ...) either got truncated by urlparse or reached Core unencoded, making 'new URI(...)' throw and Core answer 404/500. Add percent_encode_resource_url() plus DialStorageResourceMixin .get_encoded_api_path(), which decode-then-encode each segment so decoded ('my file.txt') and already-encoded ('my%20file.txt', as returned by the API) inputs converge without double-encoding. Apply it across files (upload, download, delete, move_to, copy_to, get_metadata), the shared metadata.get (also covers conversations and direct low-level calls), prompts, and resource_permissions.grant. --- aidial_client/helpers/storage_resource.py | 27 ++++++- aidial_client/resources/files.py | 74 +++++++++++-------- aidial_client/resources/metadata.py | 15 +++- aidial_client/resources/prompts.py | 16 ++-- .../resources/resource_permissions.py | 13 +++- tests/resources/files/test_download.py | 49 +++++++++++- tests/resources/files/test_metadata.py | 24 ++++++ tests/resources/files/test_move_copy.py | 64 ++++++++++++++++ 8 files changed, 236 insertions(+), 46 deletions(-) diff --git a/aidial_client/helpers/storage_resource.py b/aidial_client/helpers/storage_resource.py index 27c9501..e67e717 100644 --- a/aidial_client/helpers/storage_resource.py +++ b/aidial_client/helpers/storage_resource.py @@ -1,6 +1,6 @@ from pathlib import PurePosixPath from typing import Literal, cast, get_args -from urllib.parse import urljoin, urlparse +from urllib.parse import quote, unquote, urljoin, urlparse, urlsplit from aidial_client._compatibility.pydantic_v1 import BaseModel from aidial_client._constants import API_PREFIX @@ -12,6 +12,22 @@ StorageResourceType = Literal["files", "conversations", "prompts"] +def percent_encode_resource_url(url: str) -> str: + """ + Percent-encode each path segment so reserved characters (space, ``#``, + ``?``, ``[`` …) reach DIAL Core encoded instead of making it answer 500. + Segments are decoded first, so a decoded path (``my file.txt``) and an + already-encoded one (``my%20file.txt``, as returned by the API) converge + without double-encoding. Absolute URLs come from the API already encoded and + are returned untouched. + """ + if urlsplit(url).netloc: + return url + + segments = url.split("/") + return "/".join(quote(unquote(seg), safe="") for seg in segments) + + def _is_directory(s: str) -> bool: return s[-1] == "/" @@ -153,6 +169,15 @@ def get_api_path(self, url: str) -> str: """ return self.get_storage_resource(url).api_path + def get_encoded_api_path(self, url: str) -> str: + """ + Relative api path with every segment percent-encoded for the wire. + + Encodes before parsing so reserved characters (notably ``#`` and ``?``, + which ``urlparse`` would otherwise drop as fragment/query) survive. + """ + return self.get_api_path(percent_encode_resource_url(url)) + def get_display_name(self, url: str) -> str: """ Get the display name of the resource from the URL diff --git a/aidial_client/resources/files.py b/aidial_client/resources/files.py index ce3de02..e1c2b40 100644 --- a/aidial_client/resources/files.py +++ b/aidial_client/resources/files.py @@ -2,7 +2,7 @@ from contextlib import asynccontextmanager from pathlib import PurePosixPath from typing import Literal -from urllib.parse import urljoin +from urllib.parse import unquote, urljoin import httpx @@ -18,13 +18,41 @@ FinalRequestOptions, ) from aidial_client._utils._dict import remove_none -from aidial_client.helpers.storage_resource import DialStorageResourceMixin +from aidial_client.helpers.storage_resource import ( + DialStorageResourceMixin, + percent_encode_resource_url, +) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata from aidial_client.types.file import FileDownloadResponse from aidial_client.types.metadata import FileItem, FileMetadata +def _prepare_file_download( + resource: DialStorageResourceMixin, + url: str | PurePosixPath, + etag_if_match: str | None, +) -> tuple[FinalRequestOptions, str]: + """Build a download request from an encoded path, decoded filename.""" + options, filename = resource._prepare_download_request( + percent_encode_resource_url(str(url)), etag_if_match + ) + return options, unquote(filename) + + +def _move_copy_body( + resource: DialStorageResourceMixin, + source: str | PurePosixPath, + destination: str | PurePosixPath, + overwrite: bool, +) -> dict[str, object]: + return { + "sourceUrl": resource.get_encoded_api_path(str(source)), + "destinationUrl": resource.get_encoded_api_path(str(destination)), + "overwrite": overwrite, + } + + def _files_error_processor( http_status_error: httpx.HTTPStatusError, ) -> DialException | None: @@ -54,7 +82,7 @@ def upload( cast_to=FileItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), files={"file": file}, headers=remove_none( { @@ -71,7 +99,7 @@ def download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> FileDownloadResponse: - options, filename = self._prepare_download_request(url, etag_if_match) + options, filename = _prepare_file_download(self, url, etag_if_match) response = self.http_client.request( cast_to=httpx.Response, options=options, @@ -88,7 +116,7 @@ def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), headers=remove_none( { "If-Match": etag_if_match, @@ -109,11 +137,7 @@ def move_to( options=FinalRequestOptions( method="POST", url=urljoin(API_PREFIX, "ops/resource/move"), - json_data={ - "sourceUrl": self.get_api_path(str(source)), - "destinationUrl": self.get_api_path(str(destination)), - "overwrite": overwrite, - }, + json_data=_move_copy_body(self, source, destination, overwrite), ), on_http_error=_files_error_processor, ) @@ -129,11 +153,7 @@ def copy_to( options=FinalRequestOptions( method="POST", url=urljoin(API_PREFIX, "ops/resource/copy"), - json_data={ - "sourceUrl": self.get_api_path(str(source)), - "destinationUrl": self.get_api_path(str(destination)), - "overwrite": overwrite, - }, + json_data=_move_copy_body(self, source, destination, overwrite), ), on_http_error=_files_error_processor, ) @@ -147,7 +167,7 @@ def get_metadata( ) -> FileMetadata: return self.metadata.get( resource="files", - relative_url=self.get_api_path(str(url)), + relative_url=self.get_encoded_api_path(str(url)), limit=limit, token=token, ) @@ -168,7 +188,7 @@ async def upload( cast_to=FileItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), files={"file": file}, headers=remove_none( { @@ -185,7 +205,7 @@ async def download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> FileDownloadResponse: - options, filename = self._prepare_download_request(url, etag_if_match) + options, filename = _prepare_file_download(self, url, etag_if_match) response = await self.http_client.request( cast_to=httpx.Response, options=options, @@ -199,7 +219,7 @@ async def stream_download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> AsyncIterator[FileDownloadResponse]: - options, filename = self._prepare_download_request(url, etag_if_match) + options, filename = _prepare_file_download(self, url, etag_if_match) async with self.http_client.stream( options=options, on_http_error=_files_error_processor, @@ -215,7 +235,7 @@ async def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), headers=remove_none( { "If-Match": etag_if_match, @@ -236,11 +256,7 @@ async def move_to( options=FinalRequestOptions( method="POST", url=urljoin(API_PREFIX, "ops/resource/move"), - json_data={ - "sourceUrl": self.get_api_path(str(source)), - "destinationUrl": self.get_api_path(str(destination)), - "overwrite": overwrite, - }, + json_data=_move_copy_body(self, source, destination, overwrite), ), on_http_error=_files_error_processor, ) @@ -256,11 +272,7 @@ async def copy_to( options=FinalRequestOptions( method="POST", url=urljoin(API_PREFIX, "ops/resource/copy"), - json_data={ - "sourceUrl": self.get_api_path(str(source)), - "destinationUrl": self.get_api_path(str(destination)), - "overwrite": overwrite, - }, + json_data=_move_copy_body(self, source, destination, overwrite), ), on_http_error=_files_error_processor, ) @@ -274,7 +286,7 @@ async def get_metadata( ) -> FileMetadata: return await self.metadata.get( resource="files", - relative_url=self.get_api_path(str(url)), + relative_url=self.get_encoded_api_path(str(url)), limit=limit, token=token, ) diff --git a/aidial_client/resources/metadata.py b/aidial_client/resources/metadata.py index acf460a..86faf2a 100644 --- a/aidial_client/resources/metadata.py +++ b/aidial_client/resources/metadata.py @@ -6,7 +6,10 @@ from aidial_client._constants import METADATA_PREFIX from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none -from aidial_client.helpers.storage_resource import StorageResourceType +from aidial_client.helpers.storage_resource import ( + StorageResourceType, + percent_encode_resource_url, +) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.types.metadata import ( ConversationMetadata, @@ -71,7 +74,10 @@ def get( cast_to=_get_cast_to(resource), options=FinalRequestOptions( method="GET", - url=urljoin(METADATA_PREFIX, relative_url), + url=urljoin( + METADATA_PREFIX, + percent_encode_resource_url(relative_url), + ), params=remove_none({"limit": limit, "token": token}), ), ) @@ -120,7 +126,10 @@ async def get( cast_to=_get_cast_to(resource), options=FinalRequestOptions( method="GET", - url=urljoin(METADATA_PREFIX, relative_url), + url=urljoin( + METADATA_PREFIX, + percent_encode_resource_url(relative_url), + ), params=remove_none({"limit": limit, "token": token}), ), ) diff --git a/aidial_client/resources/prompts.py b/aidial_client/resources/prompts.py index 94b1c2a..6e9130b 100644 --- a/aidial_client/resources/prompts.py +++ b/aidial_client/resources/prompts.py @@ -56,7 +56,7 @@ def save( cast_to=PromptItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), json_data=_prompt_to_json(prompt), headers=remove_none( { @@ -74,7 +74,7 @@ def get(self, url: str | PurePosixPath) -> Prompt: cast_to=Prompt, options=FinalRequestOptions( method="GET", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), ), on_http_error=_prompts_error_processor, ) @@ -88,7 +88,7 @@ def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), headers=remove_none( { "If-Match": etag_if_match, @@ -101,7 +101,7 @@ def delete( def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: return self.metadata.get( resource="prompts", - relative_url=self.get_api_path(str(url)), + relative_url=self.get_encoded_api_path(str(url)), ) @@ -120,7 +120,7 @@ async def save( cast_to=PromptItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), json_data=_prompt_to_json(prompt), headers=remove_none( { @@ -138,7 +138,7 @@ async def get(self, url: str | PurePosixPath) -> Prompt: cast_to=Prompt, options=FinalRequestOptions( method="GET", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), ), on_http_error=_prompts_error_processor, ) @@ -152,7 +152,7 @@ async def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(str(url))), + url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))), headers=remove_none( { "If-Match": etag_if_match, @@ -165,5 +165,5 @@ async def delete( async def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: return await self.metadata.get( resource="prompts", - relative_url=self.get_api_path(str(url)), + relative_url=self.get_encoded_api_path(str(url)), ) diff --git a/aidial_client/resources/resource_permissions.py b/aidial_client/resources/resource_permissions.py index 494aa41..939dd3c 100644 --- a/aidial_client/resources/resource_permissions.py +++ b/aidial_client/resources/resource_permissions.py @@ -1,5 +1,8 @@ from aidial_client._internal_types._generic import NoneType from aidial_client._internal_types._http_request import FinalRequestOptions +from aidial_client.helpers.storage_resource import ( + percent_encode_resource_url, +) from aidial_client.resources.base import AsyncResource, Resource _GRANT_URL = "v1/ops/resource/per-request-permissions/grant" @@ -21,7 +24,10 @@ def grant( url=_GRANT_URL, json_data={ "resourcePermissions": [ - {"url": url, "permissions": permissions} + { + "url": percent_encode_resource_url(url), + "permissions": permissions, + } for url in resources ], "receiver": receiver, @@ -46,7 +52,10 @@ async def grant( url=_GRANT_URL, json_data={ "resourcePermissions": [ - {"url": url, "permissions": permissions} + { + "url": percent_encode_resource_url(url), + "permissions": permissions, + } for url in resources ], "receiver": receiver, diff --git a/tests/resources/files/test_download.py b/tests/resources/files/test_download.py index d5320d3..439bc38 100644 --- a/tests/resources/files/test_download.py +++ b/tests/resources/files/test_download.py @@ -1,14 +1,61 @@ from typing import Any, cast -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, Mock import httpx import pytest +from aidial_client import Dial from aidial_client._client import AsyncDial from aidial_client._exception import InvalidDialURLError from tests.client_mock import MockStreamIterator +def _make_capturing_client(captured: list[httpx.Request]) -> Dial: + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + return httpx.Response(status_code=200, request=request, content=b"x") + + client._http_client._internal_http_client.send = cast(Any, send_mock) + client._get_my_bucket = cast(Any, Mock(return_value="test-bucket")) + return client + + +@pytest.mark.parametrize( + "raw, encoded", + [ + ("my file.txt", "my%20file.txt"), + ("a#b.txt", "a%23b.txt"), + ("a?b.txt", "a%3Fb.txt"), + ("tag[1].txt", "tag%5B1%5D.txt"), + ], +) +def test_download_encodes_url_and_decodes_filename(raw: str, encoded: str): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + response = client.files.download(f"files/test-bucket/{raw}") + + assert ( + captured[0].url.raw_path.decode() == f"/v1/files/test-bucket/{encoded}" + ) + assert response.filename == raw + + +def test_download_accepts_already_encoded_url(): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + response = client.files.download("files/test-bucket/my%20file.txt") + + assert ( + captured[0].url.raw_path.decode() + == "/v1/files/test-bucket/my%20file.txt" + ) + assert response.filename == "my file.txt" + + @pytest.mark.asyncio async def test_stream_download_async_streams_and_closes_response(): captured_requests: list[httpx.Request] = [] diff --git a/tests/resources/files/test_metadata.py b/tests/resources/files/test_metadata.py index 8c41751..4601b82 100644 --- a/tests/resources/files/test_metadata.py +++ b/tests/resources/files/test_metadata.py @@ -86,6 +86,30 @@ def test_get_metadata(): assert r.next_token == "next-page-token" # noqa: S105 +def test_get_metadata_encodes_reserved_characters(): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + client.files.get_metadata(url="files/test-bucket/c#py (1)?.md") + + assert captured[0].url.raw_path.decode() == ( + "/v1/metadata/files/test-bucket/c%23py%20%281%29%3F.md" + ) + + +def test_low_level_metadata_get_encodes_reserved_characters(): + # The generic client.metadata.get(...) is also used directly (bypassing + # files.get_metadata), so it must encode the path too. + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + client.metadata.get("files", "files/test-bucket/c#py (1)?.md") + + assert captured[0].url.raw_path.decode() == ( + "/v1/metadata/files/test-bucket/c%23py%20%281%29%3F.md" + ) + + def test_get_metadata_sends_pagination_params(): captured: list[httpx.Request] = [] client = _make_capturing_client(captured) diff --git a/tests/resources/files/test_move_copy.py b/tests/resources/files/test_move_copy.py index f517db8..d2733f5 100644 --- a/tests/resources/files/test_move_copy.py +++ b/tests/resources/files/test_move_copy.py @@ -103,6 +103,70 @@ def test_accepts_pureposixpath_and_absolute_urls(method: str): assert body["destinationUrl"] == "files/test-bucket/final/file.txt" +# Every reserved character must reach DIAL Core percent-encoded, otherwise +# `new URI(...)` on the server throws and it answers 500. `#` and `?` also must +# survive (a naive urlparse would drop them as fragment/query). +RESERVED_CASES = [ + ("my file.txt", "my%20file.txt"), + ("50% off.txt", "50%25%20off.txt"), + ("a#b.txt", "a%23b.txt"), + ("a?b.txt", "a%3Fb.txt"), + ("a&b.txt", "a%26b.txt"), + ("tag[1].txt", "tag%5B1%5D.txt"), + ("naïve.txt", "na%C3%AFve.txt"), +] +parametrize_reserved = pytest.mark.parametrize("raw, encoded", RESERVED_CASES) + + +@parametrize_method +@parametrize_reserved +def test_encodes_reserved_characters_in_body( + method: str, raw: str, encoded: str +): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + getattr(client.files, method)( + source=f"files/test-bucket/{raw}", + destination="files/test-bucket/dst.txt", + ) + + assert _body(captured[0])["sourceUrl"] == f"files/test-bucket/{encoded}" + + +@parametrize_method +@parametrize_reserved +@pytest.mark.asyncio +async def test_async_encodes_reserved_characters_in_body( + method: str, raw: str, encoded: str +): + captured: list[httpx.Request] = [] + client = _make_async_capturing_client(captured) + + await getattr(client.files, method)( + source=f"files/test-bucket/{raw}", + destination="files/test-bucket/dst.txt", + ) + + assert _body(captured[0])["sourceUrl"] == f"files/test-bucket/{encoded}" + + +@parametrize_method +def test_already_encoded_input_is_not_double_encoded(method: str): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + # A url as returned by the API (percent-encoded) must round-trip unchanged. + getattr(client.files, method)( + source="files/test-bucket/my%20file.txt", + destination="files/test-bucket/50%25%20off.txt", + ) + + body = _body(captured[0]) + assert body["sourceUrl"] == "files/test-bucket/my%20file.txt" + assert body["destinationUrl"] == "files/test-bucket/50%25%20off.txt" + + @parametrize_method @pytest.mark.parametrize( "bad_arg", From c6b6848cf5aa86ee06c92e348b4e3fbe2ac9b949 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 27 Jul 2026 18:05:40 +0300 Subject: [PATCH 2/2] refactor: fold download encoding into _prepare_download_request Drop the _prepare_file_download wrapper; encode the url and decode the returned filename directly in the shared (files-only) _prepare_download_request. --- aidial_client/helpers/storage_resource.py | 7 +++++-- aidial_client/resources/files.py | 25 +++++------------------ 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/aidial_client/helpers/storage_resource.py b/aidial_client/helpers/storage_resource.py index e67e717..4702595 100644 --- a/aidial_client/helpers/storage_resource.py +++ b/aidial_client/helpers/storage_resource.py @@ -189,7 +189,9 @@ def _prepare_download_request( url: str | PurePosixPath, etag_if_match: str | None, ) -> tuple[FinalRequestOptions, str]: - storage_resource = self.get_storage_resource(str(url)) + storage_resource = self.get_storage_resource( + percent_encode_resource_url(str(url)) + ) if storage_resource.filename is None: raise InvalidDialURLError("URL points to a directory, not a file") @@ -204,4 +206,5 @@ def _prepare_download_request( ), ) - return options, storage_resource.filename + # api_path is percent-encoded; return a human-readable filename. + return options, unquote(storage_resource.filename) diff --git a/aidial_client/resources/files.py b/aidial_client/resources/files.py index e1c2b40..2ba4e8f 100644 --- a/aidial_client/resources/files.py +++ b/aidial_client/resources/files.py @@ -2,7 +2,7 @@ from contextlib import asynccontextmanager from pathlib import PurePosixPath from typing import Literal -from urllib.parse import unquote, urljoin +from urllib.parse import urljoin import httpx @@ -18,28 +18,13 @@ FinalRequestOptions, ) from aidial_client._utils._dict import remove_none -from aidial_client.helpers.storage_resource import ( - DialStorageResourceMixin, - percent_encode_resource_url, -) +from aidial_client.helpers.storage_resource import DialStorageResourceMixin from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata from aidial_client.types.file import FileDownloadResponse from aidial_client.types.metadata import FileItem, FileMetadata -def _prepare_file_download( - resource: DialStorageResourceMixin, - url: str | PurePosixPath, - etag_if_match: str | None, -) -> tuple[FinalRequestOptions, str]: - """Build a download request from an encoded path, decoded filename.""" - options, filename = resource._prepare_download_request( - percent_encode_resource_url(str(url)), etag_if_match - ) - return options, unquote(filename) - - def _move_copy_body( resource: DialStorageResourceMixin, source: str | PurePosixPath, @@ -99,7 +84,7 @@ def download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> FileDownloadResponse: - options, filename = _prepare_file_download(self, url, etag_if_match) + options, filename = self._prepare_download_request(url, etag_if_match) response = self.http_client.request( cast_to=httpx.Response, options=options, @@ -205,7 +190,7 @@ async def download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> FileDownloadResponse: - options, filename = _prepare_file_download(self, url, etag_if_match) + options, filename = self._prepare_download_request(url, etag_if_match) response = await self.http_client.request( cast_to=httpx.Response, options=options, @@ -219,7 +204,7 @@ async def stream_download( url: str | PurePosixPath, etag_if_match: str | None = None, ) -> AsyncIterator[FileDownloadResponse]: - options, filename = _prepare_file_download(self, url, etag_if_match) + options, filename = self._prepare_download_request(url, etag_if_match) async with self.http_client.stream( options=options, on_http_error=_files_error_processor,