From 56d3e4d9e80f5800a4501fc728652e31e97c5729 Mon Sep 17 00:00:00 2001 From: Tamir Date: Thu, 29 Oct 2026 16:36:13 +0200 Subject: [PATCH] security: check the inference request path with the API path rules [CLOUD-2523] `InferenceClient._build_url` checked `path` only for `.` and `..` segments. A path built from untrusted input could add query parameters or a fragment to a request that carries the inference key. The forms `..;/` and `..%252f` also passed the check. The checks of `HTTPClient._add_base_url` are now in `verda.helpers.check_request_path`. Both clients call it. The function rejects `?`, `#`, `.` and `..` segments, and characters outside an allowlist. `HTTPClient` uses the same allowlist as before. `InferenceClient` also allows `:`, because TensorFlow Serving uses it (`my_model:predict`). `run_sync` and `run` on `InferenceClient` and `Deployment` now take a `params` argument, so that all inference calls can send query parameters. **Breaking changes:** - `InferenceClient` rejects a query string or fragment in `path`. Use `params`. - `InferenceClient` rejects characters in `path` other than letters, digits, `-._~`, `/` and `:`. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 ++ CLAUDE.md | 4 +- .../inference_client/test_inference_client.py | 153 ++++++++++++++++-- verda/containers/_containers.py | 10 +- verda/helpers.py | 36 +++++ verda/http_client/_http_client.py | 19 +-- verda/inference_client/_inference_client.py | 29 +++- 7 files changed, 227 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de0e581..b2f007c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- `InferenceClient` now checks the request `path` with the same rules as the main API client. A path built from untrusted input could inject query parameters or a fragment into a request that carries the inference key, and dot-segment forms such as `..;/` and `..%252f` passed the old check. `path` may now contain only letters, digits, `-._~`, `/` and `:`, and must not contain a `.` or `..` segment. Anything else raises `InferenceClientError` before the request is sent. + +### Added + +- `InferenceClient.run_sync`, `InferenceClient.run`, `Deployment.run_sync` and `Deployment.run` accept a `params` argument for query string parameters, like the other `InferenceClient` methods. + +### Changed + +- **Breaking:** `InferenceClient` no longer accepts a query string or fragment in `path`. Pass query data as `params` instead. For example, replace `client.get('v1/models?limit=10')` with `client.get('v1/models', params={'limit': 10})`, and `client.run_sync(data, path='v1/completions?stream=true')` with `client.run_sync(data, path='v1/completions', params={'stream': 'true'})`. +- **Breaking:** `InferenceClient` now accepts only letters, digits, `-._~`, `/` and `:` in `path`. A path with a percent-encoded value (such as `%20` or `%2F`), a space, a non-ASCII character or another character such as `@`, `+`, `,`, `=` or `;` raises `InferenceClientError`. Send such values in the request body or in `params`. + ## [1.25.0] - 2026-10-23 ### Deprecated diff --git a/CLAUDE.md b/CLAUDE.md index bbabe2d..a5b31b4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -107,7 +107,9 @@ All five verbs delegate to a single private `_request`, which is where the url i The check is `_encode_path_segment` in `verda/http_client/_http_client.py`; `path_params` is the only supported way to put a caller-supplied value into a path. `_add_base_url` re-asserts the same allowlist on the finished path, as a backstop for a call site that skips `path_params`. -`verda.helpers.has_relative_path_segment` strips the query string and decodes escapes and encoded separators. It is for `InferenceClient` only, whose `path` spans several segments and may carry a query string. Do not use it in the http client, and do not re-implement it. +The finished-path check is `verda.helpers.check_request_path`. It rejects `?`, `#`, any character outside an allowlist, and `.`/`..` segments. `HTTPClient._add_base_url` calls it with the default allowlist (unreserved set plus `/`). `InferenceClient._build_url` calls it on the caller's multi-segment `path` with `_ALLOWED_IN_INFERENCE_PATH`, which also allows `:` (TensorFlow Serving's `my_model:predict`). Query data for both clients goes in `params`, never in the path. Do not re-implement the check. + +`verda.helpers.has_relative_path_segment` strips the query string and decodes escapes and encoded separators. It is for `InferenceClient`'s `endpoint_base_url` only. Do not use it to check a request path. ## Code style diff --git a/tests/unit_tests/inference_client/test_inference_client.py b/tests/unit_tests/inference_client/test_inference_client.py index dd24e13..ca06799 100644 --- a/tests/unit_tests/inference_client/test_inference_client.py +++ b/tests/unit_tests/inference_client/test_inference_client.py @@ -13,11 +13,15 @@ # limitations under the License. import pytest +import responses # https://github.com/getsentry/responses +from responses import matchers from verda.inference_client import InferenceClient, InferenceClientError BASE_URL = 'https://inference.example.com/v1/my-deployment' +VERBS = ['get', 'post', 'put', 'delete', 'patch', 'head', 'options'] + @pytest.fixture def inference_client(): @@ -36,6 +40,10 @@ class TestBuildUrl: # multi-segment paths are the documented contract and must keep working ('v1/models/predict', f'{BASE_URL}/v1/models/predict'), ('/v1/models/predict', f'{BASE_URL}/v1/models/predict'), + ('v1/chat/completions', f'{BASE_URL}/v1/chat/completions'), + ('v1/models/', f'{BASE_URL}/v1/models/'), + # TensorFlow Serving names the method after a ':' + ('v1/models/my_model:predict', f'{BASE_URL}/v1/models/my_model:predict'), ], ) def test_ordinary_paths_are_joined_unchanged(self, inference_client, path, expected): @@ -49,29 +57,154 @@ def test_ordinary_paths_are_joined_unchanged(self, inference_client, path, expec 'a/../../b', './x', '..', - # requests decodes percent-encoded unreserved characters before sending, - # so these reach the wire as real dot-segments. - '%2e%2e/%2e%2e/v1/victim', - '%2E%2E/other-deployment', ], ) def test_paths_that_escape_the_deployment_are_rejected(self, inference_client, path): # endpoint_base_url ends with this deployment's name, so a dot-segment walks # the request onto a different deployment while still carrying the caller's # inference key. - with pytest.raises(InferenceClientError, match='relative path segment'): + with pytest.raises(InferenceClientError, match='escape the API base path'): inference_client._build_url(path) @pytest.mark.parametrize( 'path', - ['predict?filter=a/../b', 'predict#a/../b', 'predict?q=..'], + ['chat?x=1', 'v1#frag', 'predict?filter=a/../b', 'predict?q=..'], ) - def test_dot_segments_inside_a_query_string_are_not_path_traversal( + def test_a_query_string_or_fragment_in_the_path_is_rejected(self, inference_client, path): + # Query data is passed as `params`, so a '?' or '#' here can only come from a + # value interpolated into the path, injecting parameters into a request that + # carries the inference key. + with pytest.raises(InferenceClientError, match='query string or fragment'): + inference_client._build_url(path) + + @pytest.mark.parametrize( + 'path', + [ + # requests decodes percent-encoded unreserved characters before sending, + # so these reach the wire as real dot-segments. + '%2e%2e/x', + '%2e%2e/%2e%2e/v1/victim', + '%2E%2E/other-deployment', + # a decoding check misses these, but a normalising proxy may not + '..%252fx', + '..%2Fx', + '..;/x', + 'a\\b', + 'a;b', + 'v1/my model', + ], + ) + def test_an_escape_or_a_character_outside_the_allowlist_is_rejected( self, inference_client, path ): - # Only the path is resolved by a URL parser; a value inside the query string - # is not, so rejecting it would break callers passing an inline query. - assert inference_client._build_url(path) == f'{BASE_URL}/{path}' + with pytest.raises(InferenceClientError, match='unencoded value'): + inference_client._build_url(path) + + +class TestRequestPaths: + @pytest.mark.parametrize('verb', VERBS) + @responses.activate + def test_params_are_sent_as_the_query_string(self, inference_client, verb): + # arrange + responses.add( + verb.upper(), + f'{BASE_URL}/v1/models', + match=[matchers.query_param_matcher({'filter': 'a/../b'})], + ) + + # act + response = getattr(inference_client, verb)('v1/models', params={'filter': 'a/../b'}) + + # assert + assert response.status_code == 200 + + @pytest.mark.parametrize('verb', VERBS) + @responses.activate + def test_a_rejected_path_sends_no_request(self, inference_client, verb): + with pytest.raises(InferenceClientError, match='query string or fragment'): + getattr(inference_client, verb)('chat?x=1') + + assert len(responses.calls) == 0 + + @responses.activate + def test_run_sync_posts_to_the_path(self, inference_client): + # arrange + responses.add(responses.POST, f'{BASE_URL}/v1/chat/completions', json={'ok': True}) + + # act + response = inference_client.run_sync({'prompt': 'hi'}, path='v1/chat/completions') + + # assert + assert response.status_code == 200 + + @responses.activate + def test_run_posts_to_the_path(self, inference_client): + # arrange + responses.add(responses.POST, f'{BASE_URL}/v1/chat/completions', json={'Id': 'abc'}) + + # act + execution = inference_client.run({'prompt': 'hi'}, path='v1/chat/completions') + + # assert + assert execution.id == 'abc' + + @responses.activate + def test_run_sync_sends_params_as_the_query_string(self, inference_client): + # arrange + responses.add( + responses.POST, + f'{BASE_URL}/v1/completions', + json={'ok': True}, + match=[matchers.query_param_matcher({'stream': 'true'})], + ) + + # act + response = inference_client.run_sync( + {'prompt': 'hi'}, path='v1/completions', params={'stream': 'true'} + ) + + # assert + assert response.status_code == 200 + + @pytest.mark.parametrize('no_response', [False, True]) + @responses.activate + def test_run_sends_params_as_the_query_string(self, inference_client, no_response): + # arrange + responses.add( + responses.POST, + f'{BASE_URL}/v1/completions', + json={'Id': 'abc'}, + match=[matchers.query_param_matcher({'stream': 'true'})], + ) + + # act + inference_client.run( + {'prompt': 'hi'}, + path='v1/completions', + no_response=no_response, + params={'stream': 'true'}, + ) + + # assert + assert len(responses.calls) == 1 + + @responses.activate + def test_health_gets_the_health_path(self, inference_client): + # arrange + responses.add(responses.GET, f'{BASE_URL}/health') + + # act + response = inference_client.health() + + # assert + assert response.status_code == 200 + + @responses.activate + def test_run_sync_rejects_a_query_string_in_the_path(self, inference_client): + with pytest.raises(InferenceClientError, match='query string or fragment'): + inference_client.run_sync({'prompt': 'hi'}, path='v1/completions?stream=true') + + assert len(responses.calls) == 0 class TestEndpointBaseUrl: diff --git a/verda/containers/_containers.py b/verda/containers/_containers.py index 3eb3d82..a051331 100644 --- a/verda/containers/_containers.py +++ b/verda/containers/_containers.py @@ -491,6 +491,7 @@ def run_sync( headers: dict[str, str] | None = None, http_method: str = 'POST', stream: bool = False, + params: dict[str, Any] | None = None, ) -> InferenceResponse: """Runs a synchronous inference request. @@ -501,6 +502,7 @@ def run_sync( headers: Optional headers to include in the request. http_method: The HTTP method to use for the request. stream: Whether to stream the response. + params: Optional query string parameters. Returns: InferenceResponse: The response from the inference request. @@ -510,7 +512,7 @@ def run_sync( """ self._validate_inference_client() return self._inference_client.run_sync( - data, path, timeout_seconds, headers, http_method, stream + data, path, timeout_seconds, headers, http_method, stream, params=params ) def run( @@ -521,6 +523,7 @@ def run( headers: dict[str, str] | None = None, http_method: str = 'POST', stream: bool = False, + params: dict[str, Any] | None = None, ): """Runs an asynchronous inference request. @@ -531,6 +534,7 @@ def run( headers: Optional headers to include in the request. http_method: The HTTP method to use for the request. stream: Whether to stream the response. + params: Optional query string parameters. Returns: The response from the inference request. @@ -539,7 +543,9 @@ def run( ValueError: If the inference client is not initialized. """ self._validate_inference_client() - return self._inference_client.run(data, path, timeout_seconds, headers, http_method, stream) + return self._inference_client.run( + data, path, timeout_seconds, headers, http_method, stream, params=params + ) def health(self): """Checks the health of the deployed application. diff --git a/verda/helpers.py b/verda/helpers.py index b4981db..ae7d40b 100644 --- a/verda/helpers.py +++ b/verda/helpers.py @@ -21,6 +21,9 @@ # retargeted at a different endpoint. _RELATIVE_SEGMENTS = frozenset({'.', '..'}) +# The RFC 3986 unreserved set plus the '/' between path segments. +_ALLOWED_IN_PATH = re.compile(r'^[A-Za-z0-9._~/-]*$') + # A percent-escape of an unreserved character (RFC 3986, section 2.3). `requests` # decodes these while preparing a request, so '%2E' becomes '.' before it is sent. _ESCAPED_UNRESERVED = re.compile(r'%([0-9A-Fa-f]{2})') @@ -80,6 +83,39 @@ def has_relative_path_segment(path: str) -> bool: return bool(_RELATIVE_SEGMENTS.intersection(decoded.split('/'))) +def check_request_path(path: str, allowed: re.Pattern[str] = _ALLOWED_IN_PATH) -> None: + r"""Check a finished request path cannot be retargeted or carry a query string. + + Rejects rather than decodes: ``requests`` decodes ``%2E`` back to ``.`` before + sending, and an intermediary that unescapes ``%2F`` or treats ``\`` as a separator + before normalising the path restores a traversal. Refusing ``%`` and ``\`` + outright also covers forms a decoder would miss, such as the double-encoded + ``..%252f``, and the allowlist refuses matrix parameters such as ``..;``. + + Args: + path: A url path, relative to a trusted base url. + allowed: Pattern the whole path must match. Defaults to the unreserved set + plus ``/``. + + Raises: + ValueError: If the path contains a query string or fragment, a character the + pattern does not allow, or a relative path segment. + """ + # Query data is passed separately as `params`. + if '?' in path or '#' in path: + raise ValueError(f'request path must not contain a query string or fragment: {path!r}') + + # Catches '%', which requests may decode into a separator or a dot, and '\', + # which some servers normalise to '/'. + if not allowed.match(path): + raise ValueError(f'request path must not contain an unencoded value: {path!r}') + + if _RELATIVE_SEGMENTS.intersection(path.split('/')): + raise ValueError( + f'refusing to send a request whose path would escape the API base path: {path!r}' + ) + + def stringify_class_object_properties(class_object: type) -> str: """Generates a json string representation of a class object's properties and values. diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index 4c9c71e..86bf149 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -21,7 +21,7 @@ from verda._version import __version__ from verda.exceptions import APIException -from verda.helpers import _RELATIVE_SEGMENTS +from verda.helpers import _RELATIVE_SEGMENTS, check_request_path # A `{name}` placeholder in a relative url template. _PLACEHOLDER = re.compile(r'\{(\w+)\}') @@ -30,9 +30,6 @@ # position is a slug, an id or a machine type: 'my-deployment', '1A100.22V', a UUID. _UNSAFE_IN_SEGMENT = re.compile(r'[^A-Za-z0-9._~-]') -# The unreserved set plus the '/' between the segments of an endpoint constant. -_ALLOWED_IN_PATH = re.compile(r'^[A-Za-z0-9._~/-]*$') - def _encode_path_segment(value: object) -> str: """Check a caller-supplied value is safe as a single URL path segment. @@ -421,22 +418,10 @@ def _add_base_url(self, url: str) -> str: Raises: ValueError: If the path could escape the API base path. """ - # Query data is passed separately as `params`. - if '?' in url or '#' in url: - raise ValueError(f'request path must not contain a query string or fragment: {url!r}') - - # Catches '%', which requests may decode into a separator or a dot, and '\', - # which some servers normalise to '/'. - if not _ALLOWED_IN_PATH.match(url): - raise ValueError(f'request path must not contain an unencoded value: {url!r}') + check_request_path(url) # An empty segment collapses a delete-one call onto the collection endpoint. if any(segment == '' for segment in url.split('/')[1:]): raise ValueError(f'request path must not contain an empty path segment: {url!r}') - if _RELATIVE_SEGMENTS.intersection(url.split('/')): - raise ValueError( - f'refusing to send a request whose path would escape the API base path: {url!r}' - ) - return self._base_url + url diff --git a/verda/inference_client/_inference_client.py b/verda/inference_client/_inference_client.py index c458b15..6ed8561 100644 --- a/verda/inference_client/_inference_client.py +++ b/verda/inference_client/_inference_client.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re from collections.abc import Generator from dataclasses import dataclass from enum import Enum @@ -22,7 +23,11 @@ from dataclasses_json import Undefined, dataclass_json # type: ignore from requests.structures import CaseInsensitiveDict -from verda.helpers import has_relative_path_segment +from verda.helpers import check_request_path, has_relative_path_segment + +# The unreserved set plus '/', as for the API, and ':', which some inference servers +# use to name a method on a resource (TensorFlow Serving's 'v1/models/my_model:predict'). +_ALLOWED_IN_INFERENCE_PATH = re.compile(r'^[A-Za-z0-9._~/:-]*$') class InferenceClientError(Exception): @@ -215,7 +220,8 @@ def _build_url(self, path: str) -> str: ``path`` may span several segments, so it is not checked as one. It must still stay under the deployment's base url: ``endpoint_base_url`` ends with this deployment's name, so a relative segment would walk the request onto a - different deployment while still carrying the caller's inference key. + different deployment while still carrying the caller's inference key. Query + data is passed separately as ``params``. Args: path: API path relative to the deployment's endpoint. @@ -224,13 +230,15 @@ def _build_url(self, path: str) -> str: The full request URL. Raises: - InferenceClientError: If the path contains a relative path segment. + InferenceClientError: If the path contains a query string or fragment, a + character outside the unreserved set, '/' and ':', or a relative path + segment. """ relative = path.lstrip('/') - if has_relative_path_segment(relative): - raise InferenceClientError( - f'path must not contain a relative path segment, got {path!r}' - ) + try: + check_request_path(relative, _ALLOWED_IN_INFERENCE_PATH) + except ValueError as e: + raise InferenceClientError(str(e)) from e return f'{self.endpoint_base_url}/{relative}' @@ -290,6 +298,7 @@ def run_sync( headers: dict[str, str] | None = None, http_method: str = 'POST', stream: bool = False, + params: dict[str, Any] | None = None, ): """Make a synchronous request to the inference endpoint. @@ -300,6 +309,7 @@ def run_sync( headers: Optional headers to include in the request http_method: HTTP method to use. Defaults to "POST". stream: Whether to stream the response. Defaults to False. + params: Optional query string parameters Returns: InferenceResponse: Object containing the response data. @@ -311,6 +321,7 @@ def run_sync( http_method, path, json=data, + params=params, timeout_seconds=timeout_seconds, headers=headers, stream=stream, @@ -331,6 +342,7 @@ def run( headers: dict[str, str] | None = None, http_method: str = 'POST', no_response: bool = False, + params: dict[str, Any] | None = None, ): """Make an asynchronous request to the inference endpoint. @@ -341,6 +353,7 @@ def run( headers: Optional headers to include in the request http_method: HTTP method to use. Defaults to "POST". no_response: If True, don't wait for response. Defaults to False. + params: Optional query string parameters Returns: AsyncInferenceExecution: Object to track the async execution status. @@ -358,6 +371,7 @@ def run( http_method, path, json=data, + params=params, timeout_seconds=timeout_seconds, headers=headers, ) @@ -369,6 +383,7 @@ def run( http_method, path, json=data, + params=params, timeout_seconds=timeout_seconds, headers=headers, )