From 3ea0c5ec6549e8dfc92022058e2dbdbef5817491 Mon Sep 17 00:00:00 2001 From: Tamir Date: Fri, 7 Aug 2026 09:58:29 +0300 Subject: [PATCH 1/7] security: fix path traversal and query injection in resource names Caller-supplied resource names and IDs are no longer interpolated directly into request paths. This prevents malicious input containing `../` from retargeting API calls, and `?` or `#` from injecting query parameters or fragments. Values are now percent-encoded as single path segments, and `.` `..` or empty values are rejected. The HTTP client also includes a backstop to refuse requests that attempt to escape the API base path. --- CHANGELOG.md | 6 + CLAUDE.md | 29 +++ .../http_client/test_http_client.py | 186 ++++++++++++++ tests/unit_tests/test_path_traversal.py | 239 ++++++++++++++++++ verda/clusters/_clusters.py | 4 +- verda/containers/_containers.py | 77 ++++-- verda/http_client/_http_client.py | 151 ++++++++++- verda/instances/_instances.py | 4 +- verda/job_deployments/_job_deployments.py | 33 ++- verda/ssh_keys/_ssh_keys.py | 5 +- verda/startup_scripts/_startup_scripts.py | 5 +- verda/volumes/_volumes.py | 8 +- 12 files changed, 703 insertions(+), 44 deletions(-) create mode 100644 tests/unit_tests/test_path_traversal.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4561af6..4c1c0ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- Fixed a path traversal issue where a resource name or id containing `../` was resolved while the request was prepared, retargeting the call at a different API endpoint under the SDK's own credentials (for example `containers.delete_deployment('../../v1/instances')` issued `DELETE /v1/instances`). This also prevents a name from injecting query parameters, such as overriding the `force` flag of `containers.delete_secret`. + + Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. Values that cannot be made safe by encoding — `.`, `..`, and empty values — raise `ValueError`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. + ### Added - `LongTermService` with `get_cluster_periods()` and `get_instance_periods()` methods diff --git a/CLAUDE.md b/CLAUDE.md index 525f30d..1924da7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,6 +71,34 @@ verda// - `__init__.py` files do NOT have the Apache 2.0 license header. All other `.py` files do. - Implementation files are prefixed with `_` (e.g., `_instances.py`, `_volumes.py`). +## Making API requests + +Service modules call the shared `HTTPClient` (`verda/http_client/`), which exposes `get`, `post`, `put`, `patch`, and `delete`. + +**Never interpolate a caller-supplied value into the request path.** Resource names and IDs arrive from application input. A value containing `../` is resolved while the request is prepared and retargets the call at a different API endpoint under the SDK's own credentials; a value containing `?` injects query parameters. + +Pass such values as `path_params`. The client percent-encodes each one as exactly one path segment before substituting it: + +```python +# correct +response = self.client.get( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/status', + path_params={'deployment_name': deployment_name}, +) + +# wrong -- the name can escape its path segment +response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/status') +``` + +- The url is a template: a trusted endpoint constant concatenated with a literal containing `{name}` placeholders. Keep it a plain string, never an f-string, so a value cannot be interpolated by accident. +- Name each placeholder after the parameter it carries (`{deployment_name}`, `{id}`, `{job_name}`). +- `path_params` goes last in the call, after any positional `json` body or `params` query dict. +- Paths with no caller input need no `path_params` (e.g. `self.client.get(INSTANCES_ENDPOINT)`). + +The client raises `ValueError` for values that cannot be safely encoded — empty values, and the relative segments `.` and `..` — and for a template placeholder left without a value. A placeholder missing from `path_params` raises `KeyError`. + +Encoding lives in `_encode_path_segment` in `verda/http_client/_http_client.py`. It is private on purpose: `path_params` is the only supported way to put a caller-supplied value into a path, so service modules never hand-roll encoding. As a backstop, `_add_base_url` refuses to send a request whose path would escape the API base path. + ## Code style ### Formatting and linting @@ -143,6 +171,7 @@ Ensure two blank lines between the header and the first top-level `class`/`def` - **API error tests:** use `pytest.raises(APIException)` and verify `.code` and `.message` - **Request matching:** use `responses.add()` with `matchers.json_params_matcher()` to verify request payloads - **Test data:** define constants and mock payloads as module-level variables at top of test file +- **Path traversal regression:** `tests/unit_tests/test_path_traversal.py` drives every method that takes a resource name or id against hostile values. When adding such a method, add it to the `_call_sites` table there. ## Git and branching diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index 9acbc32..ba5f6ff 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -12,12 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re from unittest.mock import Mock import pytest import responses # https://github.com/getsentry/responses from verda.exceptions import APIException +from verda.http_client._http_client import _encode_path_segment INVALID_REQUEST = 'invalid_request' INVALID_REQUEST_MESSAGE = 'Your existence is invalid' @@ -26,6 +28,129 @@ UNAUTHORIZED_REQUEST_MESSAGE = 'Access token is missing or invalid' +@pytest.mark.parametrize( + 'value', + ['my-deployment', 'a1b2c3', 'name_with.dots-and~tilde', 'UPPER123'], +) +def test_encode_path_segment_leaves_ordinary_names_unchanged(value): + assert _encode_path_segment(value) == value + + +@pytest.mark.parametrize( + ('value', 'expected'), + [ + ('../../v1/balance', '..%2F..%2Fv1%2Fbalance'), + ('../ssh-keys', '..%2Fssh-keys'), + ('a/b', 'a%2Fb'), + ('nested/../../escape', 'nested%2F..%2F..%2Fescape'), + ], +) +def test_encode_path_segment_encodes_path_separators(value, expected): + assert _encode_path_segment(value) == expected + + +@pytest.mark.parametrize( + ('value', 'expected'), + [ + ('x?force=true&', 'x%3Fforce%3Dtrue%26'), + ('name#fragment', 'name%23fragment'), + ('a b', 'a%20b'), + ], +) +def test_encode_path_segment_encodes_query_and_fragment_delimiters(value, expected): + assert _encode_path_segment(value) == expected + + +@pytest.mark.parametrize('value', ['.', '..']) +def test_encode_path_segment_rejects_relative_segments(value): + # RFC 3986 dot-segments cannot be neutralised by encoding: `quote` leaves '.' + # alone (it is unreserved) and encoding it as '%2E' does not help either, because + # `requests.utils.requote_uri` decodes percent-encoded unreserved characters back + # before the request goes on the wire. + with pytest.raises(ValueError, match='relative path segment'): + _encode_path_segment(value) + + +@pytest.mark.parametrize('value', ['%2E', '%2e%2E', '..%2F..']) +def test_encode_path_segment_escapes_percent_so_encoded_dots_stay_literal(value): + # A literal '%' is itself encoded to '%25', so these can never decode back into + # a dot-segment on the wire. + assert _encode_path_segment(value) == value.replace('%', '%25') + + +@pytest.mark.parametrize('value', ['', None]) +def test_encode_path_segment_rejects_empty_values(value): + # An empty segment collapses the URL onto the collection endpoint, which turns + # a delete-one call into a delete-all call. + with pytest.raises(ValueError, match='must be a non-empty string'): + _encode_path_segment(value) + + +def test_encode_path_segment_allows_dots_inside_a_name(): + assert _encode_path_segment('v1.2.3') == 'v1.2.3' + assert _encode_path_segment('..leading') == '..leading' + + +class TestBuildPath: + """`path_params` values are encoded by the client, so call sites cannot forget.""" + + def test_template_without_path_params_is_returned_unchanged(self, http_client): + assert http_client._build_path('/instances', None) == '/instances' + assert http_client._build_path('/long-term/periods/clusters', {}) == ( + '/long-term/periods/clusters' + ) + + def test_ordinary_values_are_substituted(self, http_client): + path = http_client._build_path( + '/container-deployments/{name}/status', {'name': 'my-deployment'} + ) + assert path == '/container-deployments/my-deployment/status' + + def test_multiple_params_are_substituted(self, http_client): + path = http_client._build_path( + '/container-deployments/{name}/replicas/{replica}', + {'name': 'dep', 'replica': 'r-1'}, + ) + assert path == '/container-deployments/dep/replicas/r-1' + + @pytest.mark.parametrize( + ('value', 'encoded'), + [ + ('../../v1/instances', '..%2F..%2Fv1%2Finstances'), + ('a/b', 'a%2Fb'), + ('x?force=true&', 'x%3Fforce%3Dtrue%26'), + ('name#frag', 'name%23frag'), + ('a b', 'a%20b'), + ], + ) + def test_hostile_values_are_encoded_into_one_segment(self, http_client, value, encoded): + path = http_client._build_path('/secrets/{name}', {'name': value}) + assert path == f'/secrets/{encoded}' + + @pytest.mark.parametrize('value', ['.', '..', '', None]) + def test_values_that_cannot_be_encoded_are_rejected(self, http_client, value): + with pytest.raises(ValueError, match='path segment must'): + http_client._build_path('/secrets/{name}', {'name': value}) + + def test_placeholder_without_a_value_is_rejected(self, http_client): + # Otherwise the literal '{id}' would be sent as the resource id. + with pytest.raises(ValueError, match='unsubstituted placeholder'): + http_client._build_path('/instances/{id}', None) + + def test_unused_path_param_is_rejected(self, http_client): + # A value the caller believed was being used but that the template ignores. + with pytest.raises(KeyError): + http_client._build_path('/instances/{id}', {'wrong_name': 'x'}) + + def test_substituted_path_keeps_the_template_segment_count(self, http_client): + # An encoded value can never introduce a separator; assert the invariant holds + # so a future change to the encoding cannot silently add a path segment. + template = '/container-deployments/{name}/status' + for value in ['a/b/c', '../../x', 'plain']: + path = http_client._build_path(template, {'name': value}) + assert path.count('/') == template.count('/') + + class TestHttpClient: def test_add_base_url(self, http_client): # arrange @@ -39,6 +164,40 @@ def test_add_base_url(self, http_client): assert base == http_client._base_url assert url == base + path + @pytest.mark.parametrize( + 'path', + [ + '/container-deployments/../../v1/instances', + '/container-deployments/..', + '/instances/../ssh-keys', + '/volumes/.', + '/scripts/./x', + # requests decodes percent-encoded unreserved characters before sending, + # so these reach the server as real dot-segments. + '/container-deployments/%2E%2E', + '/volumes/%2e', + ], + ) + def test_add_base_url_rejects_relative_path_segments(self, http_client, path): + # A call site that forgets to encode its path segment must not be able to + # retarget the request at a different endpoint. + with pytest.raises(ValueError, match='escape the API base path'): + http_client._add_base_url(path) + + @pytest.mark.parametrize( + 'path', + [ + '/container-deployments/my-deployment', + '/container-deployments/..%2F..%2Fv1%2Finstances', + '/container-deployments/%252E%252E', + '/volumes/name.with.dots', + '/volumes/..leading', + '/long-term/periods/clusters', + ], + ) + def test_add_base_url_allows_encoded_and_ordinary_paths(self, http_client, path): + assert http_client._add_base_url(path) == http_client._base_url + path + def test_generate_bearer_header(self, http_client): bearer_string = http_client._generate_bearer_header() access_token = http_client._auth_service._access_token @@ -73,6 +232,33 @@ def test_generate_headers(self, http_client): assert headers['Authorization'] == authorization_string assert headers['User-Agent'] == user_agent_string + @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) + @responses.activate + def test_request_methods_encode_path_params(self, http_client, method): + # arrange + responses.add(getattr(responses, method.upper()), re.compile(r'.*'), json={}, status=200) + + # act + getattr(http_client, method)( + '/container-deployments/{name}/status', path_params={'name': '../../v1/instances'} + ) + + # assert + assert responses.calls[0].request.path_url == ( + '/v1/container-deployments/..%2F..%2Fv1%2Finstances/status' + ) + + @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) + @responses.activate + def test_request_methods_reject_unsafe_path_params_before_sending(self, http_client, method): + # arrange + responses.add(getattr(responses, method.upper()), re.compile(r'.*'), json={}, status=200) + + # act / assert + with pytest.raises(ValueError, match='path segment must'): + getattr(http_client, method)('/secrets/{name}', path_params={'name': '..'}) + assert not responses.calls + def test_refresh_token_if_expired_refresh_successful(self, http_client): # act http_client._refresh_token_if_expired() diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py new file mode 100644 index 0000000..77dfd4f --- /dev/null +++ b/tests/unit_tests/test_path_traversal.py @@ -0,0 +1,239 @@ +# Copyright 2026 Verda Cloud Oy +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Regression tests: a caller-supplied name must stay inside its own path segment. + +Resource names and IDs reach the SDK from application input. When they are +interpolated into the request path unencoded, `requests` resolves any relative +segments they contain while preparing the URL, which retargets the call at a +different API endpoint using the SDK's own credentials. +""" + +import re + +import pytest +import responses + +from verda.clusters import ClustersService +from verda.containers import ContainersService +from verda.instances import InstancesService +from verda.job_deployments import JobDeploymentsService +from verda.ssh_keys import SSHKeysService +from verda.startup_scripts import StartupScriptsService +from verda.volumes import VolumesService + +BASE_PATH = '/v1' + +# Hostile name -> the single path segment it must be reduced to on the wire. +# Expectations are written out literally rather than derived from the SDK helper, so +# the test fails if the encoding rule itself regresses. +HOSTILE_NAMES = [ + ('../../v1/instances', '..%2F..%2Fv1%2Finstances'), + ('../../../v1/instances', '..%2F..%2F..%2Fv1%2Finstances'), + ('../ssh-keys', '..%2Fssh-keys'), + ('../../v1/balance', '..%2F..%2Fv1%2Fbalance'), + ('nested/../../escape', 'nested%2F..%2F..%2Fescape'), + # query-string / fragment smuggling + ('x?force=true&', 'x%3Fforce%3Dtrue%26'), + ('name#frag', 'name%23frag'), + # a literal '%' must be escaped so it cannot decode into a dot-segment later + ('%2E%2E', '%252E%252E'), +] + +# Names that cannot be made safe by encoding and must be refused outright: +# '.' and '..' are RFC 3986 dot-segments, and an empty name collapses the URL onto +# the collection endpoint (turning delete-one into delete-all). +REJECTED_NAMES = ['.', '..', '', None] + +ANY_URL = re.compile(r'.*') + + +def _register_catch_all(): + """Answer any request so the prepared URL can be inspected afterwards.""" + for method in (responses.GET, responses.POST, responses.PUT, responses.PATCH, responses.DELETE): + responses.add(method, ANY_URL, json={}, status=200) + + +def _call_sites(services, name): + """Every single-resource call site that interpolates a caller-supplied value. + + Returns (label, callable, expected_path_prefix) triples. + """ + containers = services['containers'] + jobs = services['jobs'] + deployments = f'{BASE_PATH}/container-deployments/' + job_deployments = f'{BASE_PATH}/job-deployments/' + + return [ + ( + 'containers.get_deployment_by_name', + lambda: containers.get_deployment_by_name(name), + deployments, + ), + ('containers.delete_deployment', lambda: containers.delete_deployment(name), deployments), + ( + 'containers.get_deployment_status', + lambda: containers.get_deployment_status(name), + deployments, + ), + ('containers.restart_deployment', lambda: containers.restart_deployment(name), deployments), + ( + 'containers.get_deployment_scaling_options', + lambda: containers.get_deployment_scaling_options(name), + deployments, + ), + ( + 'containers.get_deployment_replicas', + lambda: containers.get_deployment_replicas(name), + deployments, + ), + ( + 'containers.purge_deployment_queue', + lambda: containers.purge_deployment_queue(name), + deployments, + ), + ('containers.pause_deployment', lambda: containers.pause_deployment(name), deployments), + ('containers.resume_deployment', lambda: containers.resume_deployment(name), deployments), + ( + 'containers.get_deployment_environment_variables', + lambda: containers.get_deployment_environment_variables(name), + deployments, + ), + ( + 'containers.delete_secret', + lambda: containers.delete_secret(name), + f'{BASE_PATH}/secrets/', + ), + ( + 'containers.delete_registry_credentials', + lambda: containers.delete_registry_credentials(name), + f'{BASE_PATH}/container-registry-credentials/', + ), + ( + 'containers.delete_fileset_secret', + lambda: containers.delete_fileset_secret(name), + f'{BASE_PATH}/file-secrets/', + ), + ( + 'instances.get_by_id', + lambda: services['instances'].get_by_id(name), + f'{BASE_PATH}/instances/', + ), + ('volumes.get_by_id', lambda: services['volumes'].get_by_id(name), f'{BASE_PATH}/volumes/'), + ( + 'volumes.delete_by_id', + lambda: services['volumes'].delete_by_id(name), + f'{BASE_PATH}/volumes/', + ), + ( + 'ssh_keys.get_by_id', + lambda: services['ssh_keys'].get_by_id(name), + f'{BASE_PATH}/sshkeys/', + ), + ( + 'ssh_keys.delete_by_id', + lambda: services['ssh_keys'].delete_by_id(name), + f'{BASE_PATH}/sshkeys/', + ), + ( + 'startup_scripts.get_by_id', + lambda: services['startup_scripts'].get_by_id(name), + f'{BASE_PATH}/scripts/', + ), + ( + 'startup_scripts.delete_by_id', + lambda: services['startup_scripts'].delete_by_id(name), + f'{BASE_PATH}/scripts/', + ), + ( + 'clusters.get_by_id', + lambda: services['clusters'].get_by_id(name), + f'{BASE_PATH}/clusters/', + ), + ('jobs.get_by_name', lambda: jobs.get_by_name(name), job_deployments), + ('jobs.delete', lambda: jobs.delete(name), job_deployments), + ('jobs.get_status', lambda: jobs.get_status(name), job_deployments), + ('jobs.get_scaling_options', lambda: jobs.get_scaling_options(name), job_deployments), + ('jobs.pause', lambda: jobs.pause(name), job_deployments), + ('jobs.resume', lambda: jobs.resume(name), job_deployments), + ('jobs.purge_queue', lambda: jobs.purge_queue(name), job_deployments), + ] + + +class TestPathTraversal: + @pytest.fixture + def services(self, http_client): + return { + 'containers': ContainersService(http_client), + 'instances': InstancesService(http_client), + 'volumes': VolumesService(http_client), + 'ssh_keys': SSHKeysService(http_client), + 'startup_scripts': StartupScriptsService(http_client), + 'clusters': ClustersService(http_client), + 'jobs': JobDeploymentsService(http_client), + } + + @pytest.mark.parametrize(('name', 'encoded'), HOSTILE_NAMES) + def test_hostile_name_stays_within_its_endpoint(self, services, name, encoded): + for label, call, expected_prefix in _call_sites(services, name): + with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: + for method in ('GET', 'POST', 'PUT', 'PATCH', 'DELETE'): + mock.add(method, ANY_URL, json={}, status=200) + try: + call() + except Exception: # response-shape errors are irrelevant here + pass + + sent = [c.request.path_url for c in mock.calls] + assert sent, f'{label}({name!r}) sent no request' + for path in sent: + assert path.startswith(expected_prefix + encoded), ( + f'{label}({name!r}) escaped its endpoint: {path}' + ) + + @pytest.mark.parametrize('name', REJECTED_NAMES) + def test_unsafe_name_is_rejected_before_a_request_is_sent(self, services, name): + for label, call, _ in _call_sites(services, name): + with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: + for method in ('GET', 'POST', 'PUT', 'PATCH', 'DELETE'): + mock.add(method, ANY_URL, json={}, status=200) + with pytest.raises(ValueError, match='path segment must'): + call() + assert not mock.calls, f'{label}({name!r}) sent a request anyway' + + def test_ordinary_names_still_reach_the_documented_route(self, services): + for label, call, expected_prefix in _call_sites(services, 'my-resource-1'): + with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: + for method in ('GET', 'POST', 'PUT', 'PATCH', 'DELETE'): + mock.add(method, ANY_URL, json={}, status=200) + try: + call() + except Exception: # response shape is irrelevant here + pass + + for path in [c.request.path_url for c in mock.calls]: + assert path.startswith(f'{expected_prefix}my-resource-1'), ( + f'{label} changed shape for an ordinary name: {path}' + ) + + def test_query_string_injection_cannot_override_the_force_flag(self, services): + # delete_secret passes params={'force': ...}; the name must not be able to + # smuggle an earlier `force=true` into the query string. + with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: + mock.add('DELETE', ANY_URL, json={}, status=200) + services['containers'].delete_secret('x?force=true&', force=False) + + path = mock.calls[0].request.path_url + assert path.count('force=') == 1, f'force flag was polluted: {path}' + assert path.endswith('force=false'), path diff --git a/verda/clusters/_clusters.py b/verda/clusters/_clusters.py index 26cb5e1..16708d9 100644 --- a/verda/clusters/_clusters.py +++ b/verda/clusters/_clusters.py @@ -141,7 +141,9 @@ def get_by_id(self, id: str) -> Cluster: Raises: HTTPError: If the cluster is not found or other API error occurs. """ - cluster_dict = self._http_client.get(CLUSTERS_ENDPOINT + f'/{id}').json() + cluster_dict = self._http_client.get( + CLUSTERS_ENDPOINT + '/{id}', path_params={'id': id} + ).json() return Cluster.from_dict(cluster_dict, infer_missing=True) def create( diff --git a/verda/containers/_containers.py b/verda/containers/_containers.py index e8af8cc..3eb3d82 100644 --- a/verda/containers/_containers.py +++ b/verda/containers/_containers.py @@ -801,7 +801,10 @@ def get_deployment_by_name(self, deployment_name: str) -> Deployment: Returns: Deployment: The requested deployment. """ - response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}') + response = self.client.get( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}', + path_params={'deployment_name': deployment_name}, + ) return Deployment.from_dict_with_inference_key(response.json(), self._inference_key) # Function alias @@ -830,7 +833,9 @@ def update_deployment(self, deployment_name: str, deployment: Deployment) -> Dep Deployment: The updated deployment. """ response = self.client.patch( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}', deployment.to_dict() + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}', + deployment.to_dict(), + path_params={'deployment_name': deployment_name}, ) return Deployment.from_dict_with_inference_key(response.json(), self._inference_key) @@ -840,7 +845,10 @@ def delete_deployment(self, deployment_name: str) -> None: Args: deployment_name: Name of the deployment to delete. """ - self.client.delete(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}') + self.client.delete( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}', + path_params={'deployment_name': deployment_name}, + ) def get_deployment_status(self, deployment_name: str) -> ContainerDeploymentStatus: """Retrieves the current status of a deployment. @@ -851,7 +859,10 @@ def get_deployment_status(self, deployment_name: str) -> ContainerDeploymentStat Returns: ContainerDeploymentStatus: Current status of the deployment. """ - response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/status') + response = self.client.get( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/status', + path_params={'deployment_name': deployment_name}, + ) return ContainerDeploymentStatus(response.json()['status']) def restart_deployment(self, deployment_name: str) -> None: @@ -860,7 +871,10 @@ def restart_deployment(self, deployment_name: str) -> None: Args: deployment_name: Name of the deployment to restart. """ - self.client.post(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/restart') + self.client.post( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/restart', + path_params={'deployment_name': deployment_name}, + ) def get_deployment_scaling_options(self, deployment_name: str) -> ScalingOptions: """Retrieves the scaling options for a deployment. @@ -871,7 +885,10 @@ def get_deployment_scaling_options(self, deployment_name: str) -> ScalingOptions Returns: ScalingOptions: Current scaling options for the deployment. """ - response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/scaling') + response = self.client.get( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/scaling', + path_params={'deployment_name': deployment_name}, + ) return ScalingOptions.from_dict(response.json()) def update_deployment_scaling_options( @@ -887,8 +904,9 @@ def update_deployment_scaling_options( ScalingOptions: Updated scaling options for the deployment. """ response = self.client.patch( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/scaling', + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/scaling', scaling_options.to_dict(), + path_params={'deployment_name': deployment_name}, ) return ScalingOptions.from_dict(response.json()) @@ -901,7 +919,10 @@ def get_deployment_replicas(self, deployment_name: str) -> list[ReplicaInfo]: Returns: list[ReplicaInfo]: List of replica information. """ - response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/replicas') + response = self.client.get( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/replicas', + path_params={'deployment_name': deployment_name}, + ) return [ReplicaInfo.from_dict(replica) for replica in response.json()['list']] def purge_deployment_queue(self, deployment_name: str) -> None: @@ -910,7 +931,10 @@ def purge_deployment_queue(self, deployment_name: str) -> None: Args: deployment_name: Name of the deployment. """ - self.client.post(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/purge-queue') + self.client.post( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/purge-queue', + path_params={'deployment_name': deployment_name}, + ) def pause_deployment(self, deployment_name: str) -> None: """Pauses a deployment. @@ -918,7 +942,10 @@ def pause_deployment(self, deployment_name: str) -> None: Args: deployment_name: Name of the deployment to pause. """ - self.client.post(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/pause') + self.client.post( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/pause', + path_params={'deployment_name': deployment_name}, + ) def resume_deployment(self, deployment_name: str) -> None: """Resumes a paused deployment. @@ -926,7 +953,10 @@ def resume_deployment(self, deployment_name: str) -> None: Args: deployment_name: Name of the deployment to resume. """ - self.client.post(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/resume') + self.client.post( + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/resume', + path_params={'deployment_name': deployment_name}, + ) def get_deployment_environment_variables(self, deployment_name: str) -> dict[str, list[EnvVar]]: """Retrieves environment variables for a deployment. @@ -938,7 +968,8 @@ def get_deployment_environment_variables(self, deployment_name: str) -> dict[str dict[str, list[EnvVar]]: Dictionary mapping container names to their environment variables. """ response = self.client.get( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/environment-variables' + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/environment-variables', + path_params={'deployment_name': deployment_name}, ) result = {} for item in response.json(): @@ -961,11 +992,12 @@ def add_deployment_environment_variables( dict[str, list[EnvVar]]: Updated environment variables for all containers. """ response = self.client.post( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/environment-variables', + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/environment-variables', { 'container_name': container_name, 'env': [env_var.to_dict() for env_var in env_vars], }, + path_params={'deployment_name': deployment_name}, ) result = {} for item in response.json(): @@ -988,11 +1020,12 @@ def update_deployment_environment_variables( dict[str, list[EnvVar]]: Updated environment variables for all containers. """ response = self.client.patch( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/environment-variables', + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/environment-variables', { 'container_name': container_name, 'env': [env_var.to_dict() for env_var in env_vars], }, + path_params={'deployment_name': deployment_name}, ) result = {} item = response.json() @@ -1015,8 +1048,9 @@ def delete_deployment_environment_variables( dict[str, list[EnvVar]]: Updated environment variables for all containers. """ response = self.client.delete( - f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/environment-variables', + CONTAINER_DEPLOYMENTS_ENDPOINT + '/{deployment_name}/environment-variables', {'container_name': container_name, 'env': env_var_names}, + path_params={'deployment_name': deployment_name}, ) result = {} for item in response.json(): @@ -1077,7 +1111,9 @@ def delete_secret(self, secret_name: str, force: bool = False) -> None: force: Whether to force delete even if secret is in use. """ self.client.delete( - f'{SECRETS_ENDPOINT}/{secret_name}', params={'force': str(force).lower()} + SECRETS_ENDPOINT + '/{secret_name}', + params={'force': str(force).lower()}, + path_params={'secret_name': secret_name}, ) def get_registry_credentials(self) -> list[RegistryCredential]: @@ -1104,7 +1140,10 @@ def delete_registry_credentials(self, credentials_name: str) -> None: Args: credentials_name: Name of the credentials to delete. """ - self.client.delete(f'{CONTAINER_REGISTRY_CREDENTIALS_ENDPOINT}/{credentials_name}') + self.client.delete( + CONTAINER_REGISTRY_CREDENTIALS_ENDPOINT + '/{credentials_name}', + path_params={'credentials_name': credentials_name}, + ) def get_fileset_secrets(self) -> list[Secret]: """Retrieves all fileset secrets. @@ -1121,7 +1160,9 @@ def delete_fileset_secret(self, secret_name: str) -> None: Args: secret_name: Name of the secret to delete. """ - self.client.delete(f'{FILESET_SECRETS_ENDPOINT}/{secret_name}') + self.client.delete( + FILESET_SECRETS_ENDPOINT + '/{secret_name}', path_params={'secret_name': secret_name} + ) def create_fileset_secret_from_file_paths( self, secret_name: str, file_paths: list[str] diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index dabedc9..9393b73 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -13,12 +13,46 @@ # limitations under the License. import json +from urllib.parse import quote import requests +from requests.exceptions import InvalidURL +from requests.utils import unquote_unreserved from verda._version import __version__ from verda.exceptions import APIException +# Path segments a URL parser resolves relative to the preceding segment (RFC 3986, +# section 5.2.4). Reaching the wire with one of these means the request has been +# retargeted at a different endpoint. +_RELATIVE_SEGMENTS = frozenset({'.', '..'}) + + +def _encode_path_segment(value: str) -> str: + """Encode a caller-supplied value for safe use as a single URL path segment. + + Resource names and ids are interpolated into request paths. An unencoded value + containing ``/`` is resolved by ``requests`` while the request is prepared, which + silently retargets the call at a different API endpoint under the caller's own + credentials. Encoding with ``safe=''`` also neutralises ``?`` and ``#``, so a + name cannot inject query parameters either. + + :param value: a resource name or id supplied by the caller + :type value: str + :raises ValueError: if the value is not a non-empty string, or is a relative + path segment that no amount of encoding can make safe + :return: the value encoded as exactly one path segment + :rtype: str + """ + if not isinstance(value, str) or not value: + raise ValueError(f'path segment must be a non-empty string, got {value!r}') + if value in _RELATIVE_SEGMENTS: + # Encoding these as '%2E'/'%2E%2E' does not help: requests decodes + # percent-encoded unreserved characters again before sending the request. + raise ValueError(f'path segment must not be a relative path segment, got {value!r}') + + return quote(value, safe='') + def handle_error(response: requests.Response) -> None: """Checks for the response status code and raises an exception if it's 400 or higher. @@ -48,7 +82,12 @@ def __init__(self, auth_service, base_url: str) -> None: self._auth_service.authenticate() def post( - self, url: str, json: dict | None = None, params: dict | None = None, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a POST request. @@ -62,6 +101,9 @@ def post( :type json: dict, optional :param params: Dictionary of querystring data to attach to the Request, defaults to None :type params: dict, optional + :param path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment, defaults to None + :type path_params: dict, optional :raises APIException: an api exception with message and error type code @@ -70,7 +112,7 @@ def post( """ self._refresh_token_if_expired() - url = self._add_base_url(url) + url = self._add_base_url(self._build_path(url, path_params)) headers = self._generate_headers() response = requests.post(url, json=json, headers=headers, params=params, **kwargs) @@ -79,7 +121,12 @@ def post( return response def put( - self, url: str, json: dict | None = None, params: dict | None = None, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a PUT request. @@ -93,6 +140,9 @@ def put( :type json: dict, optional :param params: Dictionary of querystring data to attach to the Request, defaults to None :type params: dict, optional + :param path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment, defaults to None + :type path_params: dict, optional :raises APIException: an api exception with message and error type code @@ -101,7 +151,7 @@ def put( """ self._refresh_token_if_expired() - url = self._add_base_url(url) + url = self._add_base_url(self._build_path(url, path_params)) headers = self._generate_headers() response = requests.put(url, json=json, headers=headers, params=params, **kwargs) @@ -109,7 +159,13 @@ def put( return response - def get(self, url: str, params: dict | None = None, **kwargs) -> requests.Response: + def get( + self, + url: str, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, + ) -> requests.Response: """Sends a GET request. A wrapper for the requests.get method. @@ -120,6 +176,9 @@ def get(self, url: str, params: dict | None = None, **kwargs) -> requests.Respon :type url: str :param params: Dictionary of querystring data to attach to the Request, defaults to None :type params: dict, optional + :param path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment, defaults to None + :type path_params: dict, optional :raises APIException: an api exception with message and error type code @@ -128,7 +187,7 @@ def get(self, url: str, params: dict | None = None, **kwargs) -> requests.Respon """ self._refresh_token_if_expired() - url = self._add_base_url(url) + url = self._add_base_url(self._build_path(url, path_params)) headers = self._generate_headers() response = requests.get(url, params=params, headers=headers, **kwargs) @@ -137,7 +196,12 @@ def get(self, url: str, params: dict | None = None, **kwargs) -> requests.Respon return response def patch( - self, url: str, json: dict | None = None, params: dict | None = None, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a PATCH request. @@ -151,6 +215,9 @@ def patch( :type json: dict, optional :param params: Dictionary of querystring data to attach to the Request, defaults to None :type params: dict, optional + :param path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment, defaults to None + :type path_params: dict, optional :raises APIException: an api exception with message and error type code @@ -159,7 +226,7 @@ def patch( """ self._refresh_token_if_expired() - url = self._add_base_url(url) + url = self._add_base_url(self._build_path(url, path_params)) headers = self._generate_headers() response = requests.patch(url, json=json, headers=headers, params=params, **kwargs) @@ -168,7 +235,12 @@ def patch( return response def delete( - self, url: str, json: dict | None = None, params: dict | None = None, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a DELETE request. @@ -182,6 +254,9 @@ def delete( :type json: dict, optional :param params: Dictionary of querystring data to attach to the Request, defaults to None :type params: dict, optional + :param path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment, defaults to None + :type path_params: dict, optional :raises APIException: an api exception with message and error type code @@ -190,7 +265,7 @@ def delete( """ self._refresh_token_if_expired() - url = self._add_base_url(url) + url = self._add_base_url(self._build_path(url, path_params)) headers = self._generate_headers() response = requests.delete(url, headers=headers, json=json, params=params, **kwargs) @@ -244,6 +319,43 @@ def _generate_user_agent(self) -> str: return f'datacrunch-python-v{self._version}-{client_id_truncated}' + def _build_path(self, url: str, path_params: dict | None) -> str: + """Substitutes caller-supplied values into a relative url template. + + Each value is encoded as exactly one path segment, so a resource name or id + cannot introduce a path separator, walk out of its segment, or start a query + string. Call sites pass values as data rather than interpolating them, which + means the encoding cannot be forgotten. + + Example: + ``_build_path('/instances/{id}', {'id': 'a/b'})`` returns ``'/instances/a%2Fb'`` + + :param url: a relative url, optionally containing ``{name}`` placeholders + :type url: str + :param path_params: values to substitute into the placeholders + :type path_params: dict, optional + :raises ValueError: if a value cannot be safely encoded as a path segment, or + if the url still contains a placeholder that was never given a value + :raises KeyError: if the url has a placeholder missing from ``path_params`` + :return: the relative url with every value encoded and substituted + :rtype: str + """ + if not path_params: + if '{' in url: + raise ValueError(f'url has an unsubstituted placeholder, got {url!r}') + return url + + encoded = {key: _encode_path_segment(value) for key, value in path_params.items()} + path = url.format(**encoded) + + # An encoded value can never contain a separator, so substitution must not + # change the shape of the route. Belt and braces against a future change to + # the encoding rules. + if path.count('/') != url.count('/'): + raise ValueError(f'path parameter added a path segment to {url!r}') + + return path + def _add_base_url(self, url: str) -> str: """Adds the base url to the relative url. @@ -252,9 +364,28 @@ def _add_base_url(self, url: str) -> str: and the base url is 'https://api.verda.com/v1' then this method will return 'https://api.verda.com/v1/balance' + Acts as a backstop for the per-segment encoding done by the service modules: + a relative path segment that reached this point would be resolved away by + ``requests`` (or by the server), pointing the request at a different endpoint. + :param url: a relative url path :type url: str + :raises ValueError: if the path could escape the API base path :return: the full url path :rtype: str """ + path = url.split('?', 1)[0].split('#', 1)[0] + + # requests decodes percent-encoded unreserved characters before sending, which + # turns '%2E' back into '.'. Inspect the path the way the server will see it. + try: + path = unquote_unreserved(path) + except InvalidURL: + pass + + if _RELATIVE_SEGMENTS.intersection(path.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/instances/_instances.py b/verda/instances/_instances.py index 6460a7d..173c61b 100644 --- a/verda/instances/_instances.py +++ b/verda/instances/_instances.py @@ -145,7 +145,9 @@ def get_by_id(self, id: str) -> Instance: Raises: HTTPError: If the instance is not found or other API error occurs. """ - instance_dict = self._http_client.get(INSTANCES_ENDPOINT + f'/{id}').json() + instance_dict = self._http_client.get( + INSTANCES_ENDPOINT + '/{id}', path_params={'id': id} + ).json() return Instance.from_dict(instance_dict, infer_missing=True) def create( diff --git a/verda/job_deployments/_job_deployments.py b/verda/job_deployments/_job_deployments.py index 2f62571..1ffb635 100644 --- a/verda/job_deployments/_job_deployments.py +++ b/verda/job_deployments/_job_deployments.py @@ -83,7 +83,9 @@ def get(self) -> list[JobDeploymentSummary]: def get_by_name(self, job_name: str) -> JobDeployment: """Return a job deployment by name.""" - response = self._http_client.get(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}') + response = self._http_client.get( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}', path_params={'job_name': job_name} + ) return JobDeployment.from_dict(response.json(), infer_missing=True) def create(self, deployment: JobDeployment) -> JobDeployment: @@ -97,34 +99,49 @@ def create(self, deployment: JobDeployment) -> JobDeployment: def update(self, job_name: str, deployment: JobDeployment) -> JobDeployment: """Update an existing job deployment.""" response = self._http_client.patch( - f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}', + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}', json=strip_none_values(deployment.to_dict()), + path_params={'job_name': job_name}, ) return JobDeployment.from_dict(response.json(), infer_missing=True) def delete(self, job_name: str, timeout: float | None = None) -> None: """Delete a job deployment.""" params = {'timeout': timeout} if timeout is not None else None - self._http_client.delete(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}', params=params) + self._http_client.delete( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}', + params=params, + path_params={'job_name': job_name}, + ) def get_status(self, job_name: str) -> JobDeploymentStatus: """Return the current status for a job deployment.""" - response = self._http_client.get(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}/status') + response = self._http_client.get( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}/status', path_params={'job_name': job_name} + ) return JobDeploymentStatus(response.json()['status']) def get_scaling_options(self, job_name: str) -> JobScalingOptions: """Return scaling options for a job deployment.""" - response = self._http_client.get(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}/scaling') + response = self._http_client.get( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}/scaling', path_params={'job_name': job_name} + ) return JobScalingOptions.from_dict(response.json()) def pause(self, job_name: str) -> None: """Pause a job deployment.""" - self._http_client.post(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}/pause') + self._http_client.post( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}/pause', path_params={'job_name': job_name} + ) def resume(self, job_name: str) -> None: """Resume a job deployment.""" - self._http_client.post(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}/resume') + self._http_client.post( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}/resume', path_params={'job_name': job_name} + ) def purge_queue(self, job_name: str) -> None: """Purge the job deployment queue.""" - self._http_client.post(f'{JOB_DEPLOYMENTS_ENDPOINT}/{job_name}/purge-queue') + self._http_client.post( + JOB_DEPLOYMENTS_ENDPOINT + '/{job_name}/purge-queue', path_params={'job_name': job_name} + ) diff --git a/verda/ssh_keys/_ssh_keys.py b/verda/ssh_keys/_ssh_keys.py index cfb0933..a991abf 100644 --- a/verda/ssh_keys/_ssh_keys.py +++ b/verda/ssh_keys/_ssh_keys.py @@ -85,7 +85,8 @@ def get_by_id(self, id: str) -> SSHKey: :return: SSHKey object :rtype: SSHKey """ - key_dict = self._http_client.get(SSHKEYS_ENDPOINT + f'/{id}').json()[0] + response = self._http_client.get(SSHKEYS_ENDPOINT + '/{id}', path_params={'id': id}) + key_dict = response.json()[0] key_object = SSHKey(key_dict['id'], key_dict['name'], key_dict['key']) return key_object @@ -105,7 +106,7 @@ def delete_by_id(self, id: str) -> None: :param id: SSH key id :type id: str """ - self._http_client.delete(SSHKEYS_ENDPOINT + f'/{id}') + self._http_client.delete(SSHKEYS_ENDPOINT + '/{id}', path_params={'id': id}) return def create(self, name: str, key: str) -> SSHKey: diff --git a/verda/startup_scripts/_startup_scripts.py b/verda/startup_scripts/_startup_scripts.py index 0dbe19f..3a126a9 100644 --- a/verda/startup_scripts/_startup_scripts.py +++ b/verda/startup_scripts/_startup_scripts.py @@ -86,7 +86,8 @@ def get_by_id(self, id) -> StartupScript: :return: startup script object :rtype: StartupScript """ - script = self._http_client.get(STARTUP_SCRIPTS_ENDPOINT + f'/{id}').json()[0] + response = self._http_client.get(STARTUP_SCRIPTS_ENDPOINT + '/{id}', path_params={'id': id}) + script = response.json()[0] return StartupScript(script['id'], script['name'], script['script']) @@ -106,7 +107,7 @@ def delete_by_id(self, id: str) -> None: :param id: startup script id :type id: str """ - self._http_client.delete(STARTUP_SCRIPTS_ENDPOINT + f'/{id}') + self._http_client.delete(STARTUP_SCRIPTS_ENDPOINT + '/{id}', path_params={'id': id}) return def create(self, name: str, script: str) -> StartupScript: diff --git a/verda/volumes/_volumes.py b/verda/volumes/_volumes.py index 20bf164..fbf5623 100644 --- a/verda/volumes/_volumes.py +++ b/verda/volumes/_volumes.py @@ -108,7 +108,9 @@ def get_by_id(self, id: str) -> Volume: :return: Volume details object :rtype: Volume """ - volume_dict = self._http_client.get(VOLUMES_ENDPOINT + f'/{id}').json() + volume_dict = self._http_client.get( + VOLUMES_ENDPOINT + '/{id}', path_params={'id': id} + ).json() return Volume.from_dict(volume_dict) @@ -257,7 +259,9 @@ def delete_by_id(self, volume_id: str, is_permanent: bool = False) -> None: :type is_permanent: bool, optional """ payload = {'is_permanent': is_permanent} - self._http_client.delete(VOLUMES_ENDPOINT + f'/{volume_id}', json=payload) + self._http_client.delete( + VOLUMES_ENDPOINT + '/{volume_id}', json=payload, path_params={'volume_id': volume_id} + ) return def delete(self, id_list: list[str] | str, is_permanent: bool = False) -> None: From 0dd792a62219292082996cc139f20f3c17911006 Mon Sep 17 00:00:00 2001 From: Tamir Date: Fri, 7 Aug 2026 10:23:49 +0300 Subject: [PATCH 2/7] fix: improve path parameter validation and broaden security coverage Expands the path traversal protection to cover additional service methods, including `instances.is_available()` and `clusters.is_available()`, and various container and job deployment operations. Enhances path parameter validation to reject unused keys, preventing silent errors from misspelled or stale parameters. Non-string path values are now gracefully coerced to strings for backward compatibility. Reorders token refresh to occur after path validation, preventing unnecessary network calls when path parameters are invalid. --- CHANGELOG.md | 2 +- CLAUDE.md | 4 +- .../http_client/test_http_client.py | 40 +++++++++- tests/unit_tests/test_path_traversal.py | 75 +++++++++++++++++-- verda/clusters/_clusters.py | 10 ++- verda/http_client/_http_client.py | 62 +++++++++++---- verda/instances/_instances.py | 10 ++- 7 files changed, 171 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c1c0ec..b3de3ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed a path traversal issue where a resource name or id containing `../` was resolved while the request was prepared, retargeting the call at a different API endpoint under the SDK's own credentials (for example `containers.delete_deployment('../../v1/instances')` issued `DELETE /v1/instances`). This also prevents a name from injecting query parameters, such as overriding the `force` flag of `containers.delete_secret`. - Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. Values that cannot be made safe by encoding — `.`, `..`, and empty values — raise `ValueError`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. + Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. This covers `instances.is_available()` and `clusters.is_available()`, where the affected value was the `instance_type`/`cluster_type` query. Values that cannot be made safe by encoding — `.`, `..`, and empty values — raise `ValueError`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 1924da7..518fc83 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,7 +95,9 @@ response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/ - `path_params` goes last in the call, after any positional `json` body or `params` query dict. - Paths with no caller input need no `path_params` (e.g. `self.client.get(INSTANCES_ENDPOINT)`). -The client raises `ValueError` for values that cannot be safely encoded — empty values, and the relative segments `.` and `..` — and for a template placeholder left without a value. A placeholder missing from `path_params` raises `KeyError`. +Endpoint paths belong in a module-level `_ENDPOINT` constant, never an inline string literal — a literal hides the call site from the greps and audits used to check this rule. + +The client raises `ValueError` for values that cannot be safely encoded — empty values, and the relative segments `.` and `..` — for a template placeholder left without a value, and for a `path_params` key the template does not use. A placeholder with no value at all raises `KeyError`. Non-string values (`int`, `UUID`) are coerced with `str()`. Encoding lives in `_encode_path_segment` in `verda/http_client/_http_client.py`. It is private on purpose: `path_params` is the only supported way to put a caller-supplied value into a path, so service modules never hand-roll encoding. As a backstop, `_add_base_url` refuses to send a request whose path would escape the API base path. diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index ba5f6ff..3475e3c 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -13,6 +13,7 @@ # limitations under the License. import re +import uuid from unittest.mock import Mock import pytest @@ -137,11 +138,32 @@ def test_placeholder_without_a_value_is_rejected(self, http_client): with pytest.raises(ValueError, match='unsubstituted placeholder'): http_client._build_path('/instances/{id}', None) - def test_unused_path_param_is_rejected(self, http_client): - # A value the caller believed was being used but that the template ignores. + def test_placeholder_missing_from_path_params_raises(self, http_client): + # 'a' is supplied and used, 'b' has no value at all. with pytest.raises(KeyError): + http_client._build_path('/x/{a}/{b}', {'a': '1'}) + + def test_misspelled_key_reports_the_unused_parameter(self, http_client): + # Both wrong at once; the unused-key message names the actual mistake. + with pytest.raises(ValueError, match='unused path parameter'): http_client._build_path('/instances/{id}', {'wrong_name': 'x'}) + def test_path_param_the_template_does_not_use_is_rejected(self, http_client): + # A stale key left behind after a template was renamed: the caller believes + # the value is being sent, but it silently is not. + with pytest.raises(ValueError, match='unused path parameter'): + http_client._build_path('/instances/{id}', {'id': 'x', 'stale': 'y'}) + + @pytest.mark.parametrize( + ('value', 'expected'), + [(123, '123'), (uuid.UUID('0c41e387-8b12-4b4b-9c1e-000000000001'), None)], + ) + def test_non_string_values_are_coerced_not_rejected(self, http_client, value, expected): + # Before path_params these were interpolated by an f-string, so rejecting them + # would be an undocumented breaking change for callers passing ints or UUIDs. + path = http_client._build_path('/instances/{id}', {'id': value}) + assert path == f'/instances/{expected or value}' + def test_substituted_path_keeps_the_template_segment_count(self, http_client): # An encoded value can never introduce a separator; assert the invariant holds # so a future change to the encoding cannot silently add a path segment. @@ -259,6 +281,20 @@ def test_request_methods_reject_unsafe_path_params_before_sending(self, http_cli getattr(http_client, method)('/secrets/{name}', path_params={'name': '..'}) assert not responses.calls + @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) + def test_rejected_path_params_do_not_trigger_a_token_refresh(self, http_client, method): + # arrange - the fixture reports an expired token, so a refresh would fire + http_client._auth_service.refresh.reset_mock() + http_client._auth_service.authenticate.reset_mock() + + # act + with pytest.raises(ValueError, match='path segment must'): + getattr(http_client, method)('/secrets/{name}', path_params={'name': '..'}) + + # assert - an invalid name must not cost an auth round-trip + http_client._auth_service.refresh.assert_not_called() + http_client._auth_service.authenticate.assert_not_called() + def test_refresh_token_if_expired_refresh_successful(self, http_client): # act http_client._refresh_token_if_expired() diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py index 77dfd4f..6cadd4e 100644 --- a/tests/unit_tests/test_path_traversal.py +++ b/tests/unit_tests/test_path_traversal.py @@ -26,9 +26,20 @@ import responses from verda.clusters import ClustersService -from verda.containers import ContainersService +from verda.containers import ( + ComputeResource, + Container, + ContainersService, + Deployment, + EnvVar, + EnvVarType, + QueueLoadScalingTrigger, + ScalingOptions, + ScalingPolicy, + ScalingTriggers, +) from verda.instances import InstancesService -from verda.job_deployments import JobDeploymentsService +from verda.job_deployments import JobDeployment, JobDeploymentsService from verda.ssh_keys import SSHKeysService from verda.startup_scripts import StartupScriptsService from verda.volumes import VolumesService @@ -58,11 +69,22 @@ ANY_URL = re.compile(r'.*') - -def _register_catch_all(): - """Answer any request so the prepared URL can be inspected afterwards.""" - for method in (responses.GET, responses.POST, responses.PUT, responses.PATCH, responses.DELETE): - responses.add(method, ANY_URL, json={}, status=200) +# Bodies for the methods that need more than a name; their content is irrelevant, +# only the request path is under test. +_CONTAINER = Container(image='img', exposed_port=80) +_COMPUTE = ComputeResource(name='General Purpose 2D:2v', size=1) +_DEPLOYMENT = Deployment(name='d', containers=[_CONTAINER], compute=_COMPUTE) +_JOB = JobDeployment(name='j', containers=[_CONTAINER], compute=_COMPUTE) +_SCALING = ScalingOptions( + min_replica_count=1, + max_replica_count=5, + scale_down_policy=ScalingPolicy(delay_seconds=300), + scale_up_policy=ScalingPolicy(delay_seconds=60), + queue_message_ttl_seconds=3600, + concurrent_requests_per_replica=10, + scaling_triggers=ScalingTriggers(queue_load=QueueLoadScalingTrigger(threshold=0.75)), +) +_ENV_VARS = [EnvVar(name='K', value_or_reference_to_secret='v', type=EnvVarType.PLAIN)] def _call_sites(services, name): @@ -161,6 +183,42 @@ def _call_sites(services, name): lambda: services['clusters'].get_by_id(name), f'{BASE_PATH}/clusters/', ), + ( + 'containers.update_deployment', + lambda: containers.update_deployment(name, _DEPLOYMENT), + deployments, + ), + ( + 'containers.update_deployment_scaling_options', + lambda: containers.update_deployment_scaling_options(name, _SCALING), + deployments, + ), + ( + 'containers.add_deployment_environment_variables', + lambda: containers.add_deployment_environment_variables(name, 'c', _ENV_VARS), + deployments, + ), + ( + 'containers.update_deployment_environment_variables', + lambda: containers.update_deployment_environment_variables(name, 'c', _ENV_VARS), + deployments, + ), + ( + 'containers.delete_deployment_environment_variables', + lambda: containers.delete_deployment_environment_variables(name, 'c', ['K']), + deployments, + ), + ( + 'instances.is_available', + lambda: services['instances'].is_available(name), + f'{BASE_PATH}/instance-availability/', + ), + ( + 'clusters.is_available', + lambda: services['clusters'].is_available(name), + f'{BASE_PATH}/cluster-availability/', + ), + ('jobs.update', lambda: jobs.update(name, _JOB), job_deployments), ('jobs.get_by_name', lambda: jobs.get_by_name(name), job_deployments), ('jobs.delete', lambda: jobs.delete(name), job_deployments), ('jobs.get_status', lambda: jobs.get_status(name), job_deployments), @@ -222,6 +280,9 @@ def test_ordinary_names_still_reach_the_documented_route(self, services): except Exception: # response shape is irrelevant here pass + # Without this the test passes vacuously if the call raises before + # sending -- exactly the regression it exists to catch. + assert mock.calls, f'{label} sent no request for an ordinary name' for path in [c.request.path_url for c in mock.calls]: assert path.startswith(f'{expected_prefix}my-resource-1'), ( f'{label} changed shape for an ordinary name: {path}' diff --git a/verda/clusters/_clusters.py b/verda/clusters/_clusters.py index 16708d9..1e95382 100644 --- a/verda/clusters/_clusters.py +++ b/verda/clusters/_clusters.py @@ -23,6 +23,7 @@ from verda.http_client import HTTPClient CLUSTERS_ENDPOINT = '/clusters' +CLUSTER_AVAILABILITY_ENDPOINT = '/cluster-availability' # Default shared volume size is 30TB DEFAULT_SHARED_VOLUME_SIZE = 30000 @@ -278,8 +279,11 @@ def is_available( True if the cluster type is available, False otherwise. """ query_params = {'location_code': location_code} - url = f'/cluster-availability/{cluster_type}' - response = self._http_client.get(url, query_params).text + response = self._http_client.get( + CLUSTER_AVAILABILITY_ENDPOINT + '/{cluster_type}', + query_params, + path_params={'cluster_type': cluster_type}, + ).text return response == 'true' def get_availabilities(self, location_code: str | None = None) -> list[str]: @@ -292,7 +296,7 @@ def get_availabilities(self, location_code: str | None = None) -> list[str]: List of available cluster types and their details. """ query_params = {'location_code': location_code} - response = self._http_client.get('/cluster-availability', params=query_params).json() + response = self._http_client.get(CLUSTER_AVAILABILITY_ENDPOINT, params=query_params).json() availabilities = response[0]['availabilities'] return availabilities diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index 9393b73..3e93951 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import re from urllib.parse import quote import requests @@ -27,6 +28,9 @@ # retargeted at a different endpoint. _RELATIVE_SEGMENTS = frozenset({'.', '..'}) +# A `{name}` placeholder in a relative url template. +_PLACEHOLDER = re.compile(r'\{(\w+)\}') + def _encode_path_segment(value: str) -> str: """Encode a caller-supplied value for safe use as a single URL path segment. @@ -37,14 +41,22 @@ def _encode_path_segment(value: str) -> str: credentials. Encoding with ``safe=''`` also neutralises ``?`` and ``#``, so a name cannot inject query parameters either. + Non-string values are coerced with ``str()``, matching what the f-string + interpolation this replaced used to do, so ids passed as ``int`` or ``UUID`` + keep working. + :param value: a resource name or id supplied by the caller :type value: str - :raises ValueError: if the value is not a non-empty string, or is a relative - path segment that no amount of encoding can make safe + :raises ValueError: if the value is empty, or is a relative path segment that no + amount of encoding can make safe :return: the value encoded as exactly one path segment :rtype: str """ - if not isinstance(value, str) or not value: + if value is None: + raise ValueError(f'path segment must be a non-empty string, got {value!r}') + if not isinstance(value, str): + value = str(value) + if not value: raise ValueError(f'path segment must be a non-empty string, got {value!r}') if value in _RELATIVE_SEGMENTS: # Encoding these as '%2E'/'%2E%2E' does not help: requests decodes @@ -105,14 +117,17 @@ def post( each encoded as a single path segment, defaults to None :type path_params: dict, optional + :raises ValueError: if a path parameter cannot be safely encoded as a single + path segment, or the url and ``path_params`` do not match :raises APIException: an api exception with message and error type code :return: Response object :rtype: requests.Response """ - self._refresh_token_if_expired() - + # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) + + self._refresh_token_if_expired() headers = self._generate_headers() response = requests.post(url, json=json, headers=headers, params=params, **kwargs) @@ -144,14 +159,17 @@ def put( each encoded as a single path segment, defaults to None :type path_params: dict, optional + :raises ValueError: if a path parameter cannot be safely encoded as a single + path segment, or the url and ``path_params`` do not match :raises APIException: an api exception with message and error type code :return: Response object :rtype: requests.Response """ - self._refresh_token_if_expired() - + # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) + + self._refresh_token_if_expired() headers = self._generate_headers() response = requests.put(url, json=json, headers=headers, params=params, **kwargs) @@ -180,14 +198,17 @@ def get( each encoded as a single path segment, defaults to None :type path_params: dict, optional + :raises ValueError: if a path parameter cannot be safely encoded as a single + path segment, or the url and ``path_params`` do not match :raises APIException: an api exception with message and error type code :return: Response object :rtype: requests.Response """ - self._refresh_token_if_expired() - + # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) + + self._refresh_token_if_expired() headers = self._generate_headers() response = requests.get(url, params=params, headers=headers, **kwargs) @@ -219,14 +240,17 @@ def patch( each encoded as a single path segment, defaults to None :type path_params: dict, optional + :raises ValueError: if a path parameter cannot be safely encoded as a single + path segment, or the url and ``path_params`` do not match :raises APIException: an api exception with message and error type code :return: Response object :rtype: requests.Response """ - self._refresh_token_if_expired() - + # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) + + self._refresh_token_if_expired() headers = self._generate_headers() response = requests.patch(url, json=json, headers=headers, params=params, **kwargs) @@ -258,14 +282,17 @@ def delete( each encoded as a single path segment, defaults to None :type path_params: dict, optional + :raises ValueError: if a path parameter cannot be safely encoded as a single + path segment, or the url and ``path_params`` do not match :raises APIException: an api exception with message and error type code :return: Response object :rtype: requests.Response """ - self._refresh_token_if_expired() - + # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) + + self._refresh_token_if_expired() headers = self._generate_headers() response = requests.delete(url, headers=headers, json=json, params=params, **kwargs) @@ -334,8 +361,9 @@ def _build_path(self, url: str, path_params: dict | None) -> str: :type url: str :param path_params: values to substitute into the placeholders :type path_params: dict, optional - :raises ValueError: if a value cannot be safely encoded as a path segment, or - if the url still contains a placeholder that was never given a value + :raises ValueError: if a value cannot be safely encoded as a path segment, if + the url still contains a placeholder that was never given a value, or if + ``path_params`` carries a key the url does not use :raises KeyError: if the url has a placeholder missing from ``path_params`` :return: the relative url with every value encoded and substituted :rtype: str @@ -345,6 +373,10 @@ def _build_path(self, url: str, path_params: dict | None) -> str: raise ValueError(f'url has an unsubstituted placeholder, got {url!r}') return url + unused = set(path_params) - set(_PLACEHOLDER.findall(url)) + if unused: + raise ValueError(f'unused path parameter {sorted(unused)} for url {url!r}') + encoded = {key: _encode_path_segment(value) for key, value in path_params.items()} path = url.format(**encoded) diff --git a/verda/instances/_instances.py b/verda/instances/_instances.py index 173c61b..7535aec 100644 --- a/verda/instances/_instances.py +++ b/verda/instances/_instances.py @@ -23,6 +23,7 @@ from verda.constants import InstanceStatus, Locations INSTANCES_ENDPOINT = '/instances' +INSTANCE_AVAILABILITY_ENDPOINT = '/instance-availability' Contract = Literal['LONG_TERM', 'PAY_AS_YOU_GO', 'SPOT'] Pricing = Literal['DYNAMIC_PRICE', 'FIXED_PRICE'] @@ -298,8 +299,11 @@ def is_available( """ is_spot = str(is_spot).lower() query_params = {'isSpot': is_spot, 'location_code': location_code} - url = f'/instance-availability/{instance_type}' - return self._http_client.get(url, query_params).json() + return self._http_client.get( + INSTANCE_AVAILABILITY_ENDPOINT + '/{instance_type}', + query_params, + path_params={'instance_type': instance_type}, + ).json() def get_availabilities( self, is_spot: bool | None = None, location_code: str | None = None @@ -315,4 +319,4 @@ def get_availabilities( """ is_spot = str(is_spot).lower() if is_spot is not None else None query_params = {'isSpot': is_spot, 'location_code': location_code} - return self._http_client.get('/instance-availability', params=query_params).json() + return self._http_client.get(INSTANCE_AVAILABILITY_ENDPOINT, params=query_params).json() From c84a7f863f3848f266cc5939087899a2a456ac38 Mon Sep 17 00:00:00 2001 From: Tamir Date: Fri, 7 Aug 2026 10:54:26 +0300 Subject: [PATCH 3/7] security: harden path traversal protection by rejecting unsafe segments Strengthens path traversal protection by explicitly rejecting resource names or IDs that contain relative path segments (`.` or `..`), are empty, or `None`. Previously, such values were merely percent-encoded, which is insufficient as `requests` decodes unreserved characters or intermediaries unescape encoded slashes, re-introducing vulnerabilities. This also expands protection to `InferenceClient` paths, ensuring they cannot escape their deployment's base URL. All HTTP verb methods now delegate to a central `_request` method, standardizing path validation and ensuring comprehensive coverage. This introduces two breaking changes: - Resource names or IDs containing a relative path segment, an empty value, or `None` now raise `ValueError`. - Resource names and IDs are always percent-encoded by the client; pre-encoded values will be double-encoded. Callers should pass raw names. --- CHANGELOG.md | 6 +- CLAUDE.md | 6 +- .../http_client/test_http_client.py | 57 ++- tests/unit_tests/inference_client/__init__.py | 13 + .../inference_client/test_inference_client.py | 59 +++ tests/unit_tests/test_path_traversal.py | 29 +- verda/clusters/_clusters.py | 3 +- verda/http_client/_http_client.py | 338 ++++++++++-------- verda/inference_client/_inference_client.py | 36 +- 9 files changed, 363 insertions(+), 184 deletions(-) create mode 100644 tests/unit_tests/inference_client/__init__.py create mode 100644 tests/unit_tests/inference_client/test_inference_client.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b3de3ab..6726c95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed a path traversal issue where a resource name or id containing `../` was resolved while the request was prepared, retargeting the call at a different API endpoint under the SDK's own credentials (for example `containers.delete_deployment('../../v1/instances')` issued `DELETE /v1/instances`). This also prevents a name from injecting query parameters, such as overriding the `force` flag of `containers.delete_secret`. - Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. This covers `instances.is_available()` and `clusters.is_available()`, where the affected value was the `instance_type`/`cluster_type` query. Values that cannot be made safe by encoding — `.`, `..`, and empty values — raise `ValueError`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. + Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a keyword-only `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. This covers `instances.is_available()` and `clusters.is_available()`, where the affected value was the `instance_type`/`cluster_type`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. + + `InferenceClient` paths are validated too: `path` may still span several segments, but it can no longer walk out of the deployment's base url. ### Added @@ -22,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Breaking:** a resource name or id containing a relative path segment (`.` or `..` between slashes), an empty value, or `None` now raises `ValueError` instead of being sent. Encoding alone is not sufficient for these: `%2E` is decoded back to `.` before the request is sent, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. +- **Breaking:** resource names and ids are now percent-encoded, so a value that was already URL-encoded by the caller is encoded again — `get_deployment_by_name('my%20deployment')` now looks up a deployment literally named `my%20deployment` rather than `my deployment`. Pass the raw name instead. - Refactored `Image` model to use `@dataclass` and `@dataclass_json` for consistency with `Instance` and `Volume` - License changed from MIT to Apache 2.0 diff --git a/CLAUDE.md b/CLAUDE.md index 518fc83..a921d13 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,7 +97,11 @@ response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/ Endpoint paths belong in a module-level `_ENDPOINT` constant, never an inline string literal — a literal hides the call site from the greps and audits used to check this rule. -The client raises `ValueError` for values that cannot be safely encoded — empty values, and the relative segments `.` and `..` — for a template placeholder left without a value, and for a `path_params` key the template does not use. A placeholder with no value at all raises `KeyError`. Non-string values (`int`, `UUID`) are coerced with `str()`. +The client raises `ValueError` for values it cannot make safe — empty/`None`, and any value with a relative path segment (`.` or `..` between slashes) — and for any template/`path_params` mismatch. Non-string values (`int`, `UUID`) are coerced with `str()`. + +Dot-segments are rejected rather than encoded because encoding does not hold end to end: `requests` decodes `%2E` back to `.` before sending, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. A slash on its own is fine and is encoded (`docker.io/myorg` → `docker.io%2Fmyorg`). + +All five verbs delegate to a single private `_request`, which is where the url is built and validated. Add new verbs by delegating to it, never by calling `requests` directly. Encoding lives in `_encode_path_segment` in `verda/http_client/_http_client.py`. It is private on purpose: `path_params` is the only supported way to put a caller-supplied value into a path, so service modules never hand-roll encoding. As a backstop, `_add_base_url` refuses to send a request whose path would escape the API base path. diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index 3475e3c..3ad4762 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -40,16 +40,28 @@ def test_encode_path_segment_leaves_ordinary_names_unchanged(value): @pytest.mark.parametrize( ('value', 'expected'), [ - ('../../v1/balance', '..%2F..%2Fv1%2Fbalance'), - ('../ssh-keys', '..%2Fssh-keys'), ('a/b', 'a%2Fb'), - ('nested/../../escape', 'nested%2F..%2F..%2Fescape'), + ('docker.io/myorg', 'docker.io%2Fmyorg'), ], ) def test_encode_path_segment_encodes_path_separators(value, expected): assert _encode_path_segment(value) == expected +@pytest.mark.parametrize( + 'value', + ['../../v1/balance', '../ssh-keys', 'nested/../../escape', 'a/./b', 'a/..'], +) +def test_encode_path_segment_rejects_values_containing_dot_segments(value): + # Encoding '/' as '%2F' is not sufficient on its own: any intermediary that + # unescapes encoded slashes before normalising the path (Envoy's + # UNESCAPE_AND_FORWARD, some gateways and servlet containers) turns + # '..%2F..%2Fv1%2Finstances' back into a working traversal. A resource name has + # no legitimate reason to contain a dot-segment, so refuse it outright. + with pytest.raises(ValueError, match='relative path segment'): + _encode_path_segment(value) + + @pytest.mark.parametrize( ('value', 'expected'), [ @@ -117,7 +129,6 @@ def test_multiple_params_are_substituted(self, http_client): @pytest.mark.parametrize( ('value', 'encoded'), [ - ('../../v1/instances', '..%2F..%2Fv1%2Finstances'), ('a/b', 'a%2Fb'), ('x?force=true&', 'x%3Fforce%3Dtrue%26'), ('name#frag', 'name%23frag'), @@ -128,6 +139,11 @@ def test_hostile_values_are_encoded_into_one_segment(self, http_client, value, e path = http_client._build_path('/secrets/{name}', {'name': value}) assert path == f'/secrets/{encoded}' + @pytest.mark.parametrize('value', ['../../v1/instances', 'nested/../escape']) + def test_values_containing_dot_segments_are_rejected(self, http_client, value): + with pytest.raises(ValueError, match='relative path segment'): + http_client._build_path('/secrets/{name}', {'name': value}) + @pytest.mark.parametrize('value', ['.', '..', '', None]) def test_values_that_cannot_be_encoded_are_rejected(self, http_client, value): with pytest.raises(ValueError, match='path segment must'): @@ -139,10 +155,17 @@ def test_placeholder_without_a_value_is_rejected(self, http_client): http_client._build_path('/instances/{id}', None) def test_placeholder_missing_from_path_params_raises(self, http_client): - # 'a' is supplied and used, 'b' has no value at all. - with pytest.raises(KeyError): + # 'a' is supplied and used, 'b' has no value at all. Every template/params + # mismatch reports as ValueError, matching what the request methods document. + with pytest.raises(ValueError, match='no value given for placeholder'): http_client._build_path('/x/{a}/{b}', {'a': '1'}) + def test_malformed_percent_escape_fails_closed(self, http_client): + # The dot-segment check cannot be performed on a path that will not normalise, + # so the request must be refused rather than sent unchecked. + with pytest.raises(ValueError, match='malformed percent-escape'): + http_client._add_base_url('/x/%2E%2E/%zz') + def test_misspelled_key_reports_the_unused_parameter(self, http_client): # Both wrong at once; the unused-key message names the actual mistake. with pytest.raises(ValueError, match='unused path parameter'): @@ -164,13 +187,17 @@ def test_non_string_values_are_coerced_not_rejected(self, http_client, value, ex path = http_client._build_path('/instances/{id}', {'id': value}) assert path == f'/instances/{expected or value}' - def test_substituted_path_keeps_the_template_segment_count(self, http_client): - # An encoded value can never introduce a separator; assert the invariant holds - # so a future change to the encoding cannot silently add a path segment. - template = '/container-deployments/{name}/status' - for value in ['a/b/c', '../../x', 'plain']: - path = http_client._build_path(template, {'name': value}) - assert path.count('/') == template.count('/') + def test_segment_count_guard_catches_an_encoder_that_leaks_a_separator( + self, http_client, monkeypatch + ): + # The guard is unreachable while the encoder is correct, so drive it with a + # deliberately broken encoder. This tests the guard itself rather than + # re-testing urllib's quote(). + monkeypatch.setattr( + 'verda.http_client._http_client._encode_path_segment', lambda value: value + ) + with pytest.raises(ValueError, match='added a path segment'): + http_client._build_path('/container-deployments/{name}/status', {'name': 'a/b'}) class TestHttpClient: @@ -262,12 +289,12 @@ def test_request_methods_encode_path_params(self, http_client, method): # act getattr(http_client, method)( - '/container-deployments/{name}/status', path_params={'name': '../../v1/instances'} + '/container-deployments/{name}/status', path_params={'name': 'x?force=true&'} ) # assert assert responses.calls[0].request.path_url == ( - '/v1/container-deployments/..%2F..%2Fv1%2Finstances/status' + '/v1/container-deployments/x%3Fforce%3Dtrue%26/status' ) @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) diff --git a/tests/unit_tests/inference_client/__init__.py b/tests/unit_tests/inference_client/__init__.py new file mode 100644 index 0000000..6184622 --- /dev/null +++ b/tests/unit_tests/inference_client/__init__.py @@ -0,0 +1,13 @@ +# Copyright 2026 Verda Cloud Oy +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. diff --git a/tests/unit_tests/inference_client/test_inference_client.py b/tests/unit_tests/inference_client/test_inference_client.py new file mode 100644 index 0000000..130393a --- /dev/null +++ b/tests/unit_tests/inference_client/test_inference_client.py @@ -0,0 +1,59 @@ +# Copyright 2026 Verda Cloud Oy +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pytest + +from verda.inference_client import InferenceClient, InferenceClientError + +BASE_URL = 'https://inference.example.com/v1/my-deployment' + + +@pytest.fixture +def inference_client(): + return InferenceClient(inference_key='key-123', endpoint_base_url=BASE_URL) + + +class TestBuildUrl: + """`path` is a caller-chosen multi-segment path, but it must stay under the base.""" + + @pytest.mark.parametrize( + ('path', 'expected'), + [ + ('', f'{BASE_URL}/'), + ('predict', f'{BASE_URL}/predict'), + ('/predict', f'{BASE_URL}/predict'), + # 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'), + ], + ) + def test_ordinary_paths_are_joined_unchanged(self, inference_client, path, expected): + assert inference_client._build_url(path) == expected + + @pytest.mark.parametrize( + 'path', + [ + '../other-deployment', + '../../v1/other-deployment', + 'a/../../b', + './x', + '..', + ], + ) + 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'): + inference_client._build_url(path) diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py index 6cadd4e..70321d1 100644 --- a/tests/unit_tests/test_path_traversal.py +++ b/tests/unit_tests/test_path_traversal.py @@ -50,22 +50,35 @@ # Expectations are written out literally rather than derived from the SDK helper, so # the test fails if the encoding rule itself regresses. HOSTILE_NAMES = [ - ('../../v1/instances', '..%2F..%2Fv1%2Finstances'), - ('../../../v1/instances', '..%2F..%2F..%2Fv1%2Finstances'), - ('../ssh-keys', '..%2Fssh-keys'), - ('../../v1/balance', '..%2F..%2Fv1%2Fbalance'), - ('nested/../../escape', 'nested%2F..%2F..%2Fescape'), # query-string / fragment smuggling ('x?force=true&', 'x%3Fforce%3Dtrue%26'), ('name#frag', 'name%23frag'), # a literal '%' must be escaped so it cannot decode into a dot-segment later ('%2E%2E', '%252E%252E'), + ('..%2F..', '..%252F..'), + # a slash with no dot-segment is encoded, not refused: registry credential + # names legitimately look like 'docker.io/myorg' + ('a/b', 'a%2Fb'), ] # Names that cannot be made safe by encoding and must be refused outright: -# '.' and '..' are RFC 3986 dot-segments, and an empty name collapses the URL onto -# the collection endpoint (turning delete-one into delete-all). -REJECTED_NAMES = ['.', '..', '', None] +# - '.' and '..' are RFC 3986 dot-segments, and '%2E' is decoded back to '.' by +# requests before the request is sent +# - a dot-segment anywhere in the value survives '%2F' encoding if any intermediary +# unescapes encoded slashes before normalising the path +# - an empty name collapses the URL onto the collection endpoint, turning a +# delete-one call into a delete-all call +REJECTED_NAMES = [ + '.', + '..', + '', + None, + '../../v1/instances', + '../../../v1/instances', + '../ssh-keys', + '../../v1/balance', + 'nested/../../escape', +] ANY_URL = re.compile(r'.*') diff --git a/verda/clusters/_clusters.py b/verda/clusters/_clusters.py index 1e95382..702a9de 100644 --- a/verda/clusters/_clusters.py +++ b/verda/clusters/_clusters.py @@ -24,6 +24,7 @@ CLUSTERS_ENDPOINT = '/clusters' CLUSTER_AVAILABILITY_ENDPOINT = '/cluster-availability' +CLUSTER_IMAGES_ENDPOINT = '/images/cluster' # Default shared volume size is 30TB DEFAULT_SHARED_VOLUME_SIZE = 30000 @@ -313,5 +314,5 @@ def get_cluster_images( List of available images for the given cluster type. """ query_params = {'instance_type': cluster_type} - images = self._http_client.get('/images/cluster', params=query_params).json() + images = self._http_client.get(CLUSTER_IMAGES_ENDPOINT, params=query_params).json() return [image['image_type'] for image in images] diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index 3e93951..944fc42 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -45,12 +45,15 @@ def _encode_path_segment(value: str) -> str: interpolation this replaced used to do, so ids passed as ``int`` or ``UUID`` keep working. - :param value: a resource name or id supplied by the caller - :type value: str - :raises ValueError: if the value is empty, or is a relative path segment that no - amount of encoding can make safe - :return: the value encoded as exactly one path segment - :rtype: str + Args: + value: A resource name or id supplied by the caller. + + Returns: + The value encoded as exactly one path segment. + + Raises: + ValueError: If the value is empty, or contains a relative path segment that no + amount of encoding can make safe. """ if value is None: raise ValueError(f'path segment must be a non-empty string, got {value!r}') @@ -58,10 +61,14 @@ def _encode_path_segment(value: str) -> str: value = str(value) if not value: raise ValueError(f'path segment must be a non-empty string, got {value!r}') - if value in _RELATIVE_SEGMENTS: - # Encoding these as '%2E'/'%2E%2E' does not help: requests decodes - # percent-encoded unreserved characters again before sending the request. - raise ValueError(f'path segment must not be a relative path segment, got {value!r}') + # Encoding is not sufficient for dot-segments. '%2E' is decoded back to '.' by + # requests before the request is sent, and encoding '/' as '%2F' only holds as + # long as nothing between here and the server unescapes it -- an intermediary + # that unescapes encoded slashes before normalising the path (for example + # Envoy's UNESCAPE_AND_FORWARD) would turn '..%2F..%2Fx' back into a traversal. + # A resource name has no legitimate reason to contain a dot-segment. + if _RELATIVE_SEGMENTS.intersection(value.split('/')): + raise ValueError(f'path segment must not contain a relative path segment, got {value!r}') return quote(value, safe='') @@ -69,8 +76,11 @@ def _encode_path_segment(value: str) -> str: def handle_error(response: requests.Response) -> None: """Checks for the response status code and raises an exception if it's 400 or higher. - :param response: the API call response - :raises APIException: an api exception with message and error type code + Args: + response: The API call response. + + Raises: + APIException: An api exception with message and error type code. """ if not response.ok: data = json.loads(response.text) @@ -98,6 +108,7 @@ def post( url: str, json: dict | None = None, params: dict | None = None, + *, path_params: dict | None = None, **kwargs, ) -> requests.Response: @@ -107,39 +118,32 @@ def post( Builds the url, uses custom headers, refresh tokens if needed. - :param url: relative url of the API endpoint - :type url: str - :param json: A JSON serializable Python object to send in the body of the Request, defaults to None - :type json: dict, optional - :param params: Dictionary of querystring data to attach to the Request, defaults to None - :type params: dict, optional - :param path_params: Values substituted into ``{name}`` placeholders in the url, - each encoded as a single path segment, defaults to None - :type path_params: dict, optional - - :raises ValueError: if a path parameter cannot be safely encoded as a single - path segment, or the url and ``path_params`` do not match - :raises APIException: an api exception with message and error type code - - :return: Response object - :rtype: requests.Response + Args: + url: Relative url of the API endpoint. + json: A JSON serializable Python object to send in the body of the request. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment. + **kwargs: Additional keyword arguments passed through to ``requests``. + + Returns: + The response object. + + Raises: + ValueError: If a path parameter cannot be safely encoded as a single path + segment, or the url and ``path_params`` do not match. + APIException: An api exception with message and error type code. """ - # Validate before the refresh so a rejected name costs no auth round-trip. - url = self._add_base_url(self._build_path(url, path_params)) - - self._refresh_token_if_expired() - headers = self._generate_headers() - - response = requests.post(url, json=json, headers=headers, params=params, **kwargs) - handle_error(response) - - return response + return self._request( + 'POST', url, json=json, params=params, path_params=path_params, **kwargs + ) def put( self, url: str, json: dict | None = None, params: dict | None = None, + *, path_params: dict | None = None, **kwargs, ) -> requests.Response: @@ -149,38 +153,31 @@ def put( Builds the url, uses custom headers, refresh tokens if needed. - :param url: relative url of the API endpoint - :type url: str - :param json: A JSON serializable Python object to send in the body of the Request, defaults to None - :type json: dict, optional - :param params: Dictionary of querystring data to attach to the Request, defaults to None - :type params: dict, optional - :param path_params: Values substituted into ``{name}`` placeholders in the url, - each encoded as a single path segment, defaults to None - :type path_params: dict, optional - - :raises ValueError: if a path parameter cannot be safely encoded as a single - path segment, or the url and ``path_params`` do not match - :raises APIException: an api exception with message and error type code - - :return: Response object - :rtype: requests.Response + Args: + url: Relative url of the API endpoint. + json: A JSON serializable Python object to send in the body of the request. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment. + **kwargs: Additional keyword arguments passed through to ``requests``. + + Returns: + The response object. + + Raises: + ValueError: If a path parameter cannot be safely encoded as a single path + segment, or the url and ``path_params`` do not match. + APIException: An api exception with message and error type code. """ - # Validate before the refresh so a rejected name costs no auth round-trip. - url = self._add_base_url(self._build_path(url, path_params)) - - self._refresh_token_if_expired() - headers = self._generate_headers() - - response = requests.put(url, json=json, headers=headers, params=params, **kwargs) - handle_error(response) - - return response + return self._request( + 'PUT', url, json=json, params=params, path_params=path_params, **kwargs + ) def get( self, url: str, params: dict | None = None, + *, path_params: dict | None = None, **kwargs, ) -> requests.Response: @@ -190,37 +187,29 @@ def get( Builds the url, uses custom headers, refresh tokens if needed. - :param url: relative url of the API endpoint - :type url: str - :param params: Dictionary of querystring data to attach to the Request, defaults to None - :type params: dict, optional - :param path_params: Values substituted into ``{name}`` placeholders in the url, - each encoded as a single path segment, defaults to None - :type path_params: dict, optional + Args: + url: Relative url of the API endpoint. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment. + **kwargs: Additional keyword arguments passed through to ``requests``. - :raises ValueError: if a path parameter cannot be safely encoded as a single - path segment, or the url and ``path_params`` do not match - :raises APIException: an api exception with message and error type code + Returns: + The response object. - :return: Response object - :rtype: requests.Response + Raises: + ValueError: If a path parameter cannot be safely encoded as a single path + segment, or the url and ``path_params`` do not match. + APIException: An api exception with message and error type code. """ - # Validate before the refresh so a rejected name costs no auth round-trip. - url = self._add_base_url(self._build_path(url, path_params)) - - self._refresh_token_if_expired() - headers = self._generate_headers() - - response = requests.get(url, params=params, headers=headers, **kwargs) - handle_error(response) - - return response + return self._request('GET', url, params=params, path_params=path_params, **kwargs) def patch( self, url: str, json: dict | None = None, params: dict | None = None, + *, path_params: dict | None = None, **kwargs, ) -> requests.Response: @@ -230,39 +219,32 @@ def patch( Builds the url, uses custom headers, refresh tokens if needed. - :param url: relative url of the API endpoint - :type url: str - :param json: A JSON serializable Python object to send in the body of the Request, defaults to None - :type json: dict, optional - :param params: Dictionary of querystring data to attach to the Request, defaults to None - :type params: dict, optional - :param path_params: Values substituted into ``{name}`` placeholders in the url, - each encoded as a single path segment, defaults to None - :type path_params: dict, optional - - :raises ValueError: if a path parameter cannot be safely encoded as a single - path segment, or the url and ``path_params`` do not match - :raises APIException: an api exception with message and error type code - - :return: Response object - :rtype: requests.Response + Args: + url: Relative url of the API endpoint. + json: A JSON serializable Python object to send in the body of the request. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment. + **kwargs: Additional keyword arguments passed through to ``requests``. + + Returns: + The response object. + + Raises: + ValueError: If a path parameter cannot be safely encoded as a single path + segment, or the url and ``path_params`` do not match. + APIException: An api exception with message and error type code. """ - # Validate before the refresh so a rejected name costs no auth round-trip. - url = self._add_base_url(self._build_path(url, path_params)) - - self._refresh_token_if_expired() - headers = self._generate_headers() - - response = requests.patch(url, json=json, headers=headers, params=params, **kwargs) - handle_error(response) - - return response + return self._request( + 'PATCH', url, json=json, params=params, path_params=path_params, **kwargs + ) def delete( self, url: str, json: dict | None = None, params: dict | None = None, + *, path_params: dict | None = None, **kwargs, ) -> requests.Response: @@ -272,30 +254,63 @@ def delete( Builds the url, uses custom headers, refresh tokens if needed. - :param url: relative url of the API endpoint - :type url: str - :param json: A JSON serializable Python object to send in the body of the Request, defaults to None - :type json: dict, optional - :param params: Dictionary of querystring data to attach to the Request, defaults to None - :type params: dict, optional - :param path_params: Values substituted into ``{name}`` placeholders in the url, - each encoded as a single path segment, defaults to None - :type path_params: dict, optional - - :raises ValueError: if a path parameter cannot be safely encoded as a single - path segment, or the url and ``path_params`` do not match - :raises APIException: an api exception with message and error type code - - :return: Response object - :rtype: requests.Response + Args: + url: Relative url of the API endpoint. + json: A JSON serializable Python object to send in the body of the request. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url, + each encoded as a single path segment. + **kwargs: Additional keyword arguments passed through to ``requests``. + + Returns: + The response object. + + Raises: + ValueError: If a path parameter cannot be safely encoded as a single path + segment, or the url and ``path_params`` do not match. + APIException: An api exception with message and error type code. + """ + return self._request( + 'DELETE', url, json=json, params=params, path_params=path_params, **kwargs + ) + + def _request( + self, + method: str, + url: str, + json: dict | None = None, + params: dict | None = None, + path_params: dict | None = None, + **kwargs, + ) -> requests.Response: + """Sends a request, building and validating the url first. + + Every verb goes through here, so the path-parameter encoding cannot be + skipped by adding a new one. + + Args: + method: HTTP method name. + url: Relative url of the API endpoint. + json: A JSON serializable Python object to send in the body of the request. + params: Dictionary of querystring data to attach to the request. + path_params: Values substituted into ``{name}`` placeholders in the url. + **kwargs: Additional keyword arguments passed through to ``requests``. + + Returns: + The response object. + + Raises: + ValueError: If the url and ``path_params`` do not produce a safe path. + APIException: An api exception with message and error type code. """ # Validate before the refresh so a rejected name costs no auth round-trip. url = self._add_base_url(self._build_path(url, path_params)) self._refresh_token_if_expired() - headers = self._generate_headers() - response = requests.delete(url, headers=headers, json=json, params=params, **kwargs) + response = requests.request( + method, url, json=json, headers=self._generate_headers(), params=params, **kwargs + ) handle_error(response) return response @@ -303,9 +318,11 @@ def delete( def _refresh_token_if_expired(self) -> None: """Refreshes the access token if it expired. - Uses the refresh token to refresh, and if the refresh token is also expired, uses the client credentials. + Uses the refresh token to refresh, and if the refresh token is also expired, + uses the client credentials. - :raises APIException: an api exception with message and error type code + Raises: + APIException: An api exception with message and error type code. """ if self._auth_service.is_expired(): # try to refresh. if refresh token has expired, reauthenticate @@ -317,8 +334,8 @@ def _refresh_token_if_expired(self) -> None: def _generate_headers(self) -> dict: """Generate the default headers for every request. - :return: dict with request headers - :rtype: dict + Returns: + Dict with request headers. """ headers = { 'Authorization': self._generate_bearer_header(), @@ -330,16 +347,16 @@ def _generate_headers(self) -> dict: def _generate_bearer_header(self) -> str: """Generate the authorization header Bearer string. - :return: Authorization header Bearer string - :rtype: str + Returns: + Authorization header Bearer string. """ return f'Bearer {self._auth_service._access_token}' def _generate_user_agent(self) -> str: """Generate the user agent string. - :return: user agent string - :rtype: str + Returns: + User agent string. """ # get the first 10 chars of the client id client_id_truncated = self._auth_service._client_id[:10] @@ -357,16 +374,17 @@ def _build_path(self, url: str, path_params: dict | None) -> str: Example: ``_build_path('/instances/{id}', {'id': 'a/b'})`` returns ``'/instances/a%2Fb'`` - :param url: a relative url, optionally containing ``{name}`` placeholders - :type url: str - :param path_params: values to substitute into the placeholders - :type path_params: dict, optional - :raises ValueError: if a value cannot be safely encoded as a path segment, if - the url still contains a placeholder that was never given a value, or if - ``path_params`` carries a key the url does not use - :raises KeyError: if the url has a placeholder missing from ``path_params`` - :return: the relative url with every value encoded and substituted - :rtype: str + Args: + url: A relative url, optionally containing ``{name}`` placeholders. + path_params: Values to substitute into the placeholders. + + Returns: + The relative url with every value encoded and substituted. + + Raises: + ValueError: If a value cannot be safely encoded as a path segment, if the + url still contains a placeholder that was never given a value, or if + ``path_params`` carries a key the url does not use. """ if not path_params: if '{' in url: @@ -378,7 +396,10 @@ def _build_path(self, url: str, path_params: dict | None) -> str: raise ValueError(f'unused path parameter {sorted(unused)} for url {url!r}') encoded = {key: _encode_path_segment(value) for key, value in path_params.items()} - path = url.format(**encoded) + try: + path = url.format(**encoded) + except KeyError as error: + raise ValueError(f'no value given for placeholder {error} in url {url!r}') from error # An encoded value can never contain a separator, so substitution must not # change the shape of the route. Belt and braces against a future change to @@ -400,20 +421,25 @@ def _add_base_url(self, url: str) -> str: a relative path segment that reached this point would be resolved away by ``requests`` (or by the server), pointing the request at a different endpoint. - :param url: a relative url path - :type url: str - :raises ValueError: if the path could escape the API base path - :return: the full url path - :rtype: str + Args: + url: A relative url path. + + Returns: + The full url path. + + Raises: + ValueError: If the path could escape the API base path. """ path = url.split('?', 1)[0].split('#', 1)[0] # requests decodes percent-encoded unreserved characters before sending, which # turns '%2E' back into '.'. Inspect the path the way the server will see it. + # A malformed escape means the check cannot be performed, so fail closed + # rather than validating the un-normalised path. try: path = unquote_unreserved(path) - except InvalidURL: - pass + except InvalidURL as error: + raise ValueError(f'request path has a malformed percent-escape: {url!r}') from error if _RELATIVE_SEGMENTS.intersection(path.split('/')): raise ValueError( diff --git a/verda/inference_client/_inference_client.py b/verda/inference_client/_inference_client.py index c341ecd..d4fbc76 100644 --- a/verda/inference_client/_inference_client.py +++ b/verda/inference_client/_inference_client.py @@ -22,6 +22,10 @@ from dataclasses_json import Undefined, dataclass_json # type: ignore from requests.structures import CaseInsensitiveDict +# Path segments a URL parser resolves relative to the preceding segment (RFC 3986, +# section 5.2.4). +_RELATIVE_SEGMENTS = frozenset({'.', '..'}) + class InferenceClientError(Exception): """Base exception for InferenceClient errors.""" @@ -144,6 +148,12 @@ def __init__( self.endpoint_base_url = endpoint_base_url.rstrip('/') self.base_domain = self.endpoint_base_url[: self.endpoint_base_url.rindex('/')] self.deployment_name = self.endpoint_base_url[self.endpoint_base_url.rindex('/') + 1 :] + # deployment_name is interpolated into the async status/result urls, so a + # dot-segment here would retarget those at the parent path. + if self.deployment_name in _RELATIVE_SEGMENTS: + raise InferenceClientError( + f'endpoint_base_url must end in a deployment name, got {endpoint_base_url!r}' + ) self.timeout_seconds = timeout_seconds self._session = requests.Session() self._global_headers = { @@ -193,8 +203,30 @@ def remove_global_header(self, key: str) -> None: del self._global_headers[key] def _build_url(self, path: str) -> str: - """Construct the full URL by joining the base URL with the path.""" - return f'{self.endpoint_base_url}/{path.lstrip("/")}' + """Construct the full URL by joining the base URL with the path. + + ``path`` is a caller-chosen API path and may span several segments, so it is + not encoded 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. + + Args: + path: API path relative to the deployment's endpoint. + + Returns: + The full request URL. + + Raises: + InferenceClientError: If the path contains a relative path segment. + """ + relative = path.lstrip('/') + if _RELATIVE_SEGMENTS.intersection(relative.split('/')): + raise InferenceClientError( + f'path must not contain a relative path segment, got {path!r}' + ) + + return f'{self.endpoint_base_url}/{relative}' def _build_request_headers( self, request_headers: dict[str, str] | None = None From 5a439c52fe487caa07cf9eea0e132e4f3651f8e7 Mon Sep 17 00:00:00 2001 From: Tamir Date: Fri, 7 Aug 2026 11:25:59 +0300 Subject: [PATCH 4/7] security: centralize and strengthen path validation Moves path traversal detection to `verda.helpers.has_relative_path_segment` to unify checks across `HTTPClient` and `InferenceClient`, ensuring consistent protection against `.` and `..` segments, even when percent-encoded. Expands `_encode_path_segment` to explicitly reject path parameters that are not `str`, `int`, or `UUID`, preventing ambiguous coercion (e.g., `bytes` to `"b'abc'"`) that would result in confusing 404s. Hardens `_add_base_url` to reject paths containing query strings or fragments, preventing URL component injection. Adds a dynamic test to ensure all service methods accepting path parameters are covered by traversal tests. **Breaking changes:** - A path value that is not `str`, `int`, or `UUID` now raises `ValueError`. - A `/` within a resource name is now consistently encoded as `%2F`, ensuring it remains a single path segment. Servers that reject or refuse to decode encoded slashes may break. --- CHANGELOG.md | 2 + CLAUDE.md | 4 ++ .../http_client/test_http_client.py | 40 +++++++++++---- .../inference_client/test_inference_client.py | 31 ++++++++++++ tests/unit_tests/test_path_traversal.py | 43 ++++++++++++++++ verda/helpers.py | 50 +++++++++++++++++++ verda/http_client/_http_client.py | 40 ++++++--------- verda/inference_client/_inference_client.py | 20 ++++---- 8 files changed, 186 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6726c95..8268cf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Breaking:** a resource name or id containing a relative path segment (`.` or `..` between slashes), an empty value, or `None` now raises `ValueError` instead of being sent. Encoding alone is not sufficient for these: `%2E` is decoded back to `.` before the request is sent, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. - **Breaking:** resource names and ids are now percent-encoded, so a value that was already URL-encoded by the caller is encoded again — `get_deployment_by_name('my%20deployment')` now looks up a deployment literally named `my%20deployment` rather than `my deployment`. Pass the raw name instead. +- **Breaking:** a `/` inside a resource name is now encoded as `%2F` and stays one path segment, where it previously split the route — `delete_registry_credentials('docker.io/myorg')` sends `DELETE /v1/container-registry-credentials/docker.io%2Fmyorg`. Servers that reject or refuse to decode encoded slashes (nginx and Apache with `AllowEncodedSlashes off`) will not match such a name. +- **Breaking:** a path value that is not a `str`, `int` or `UUID` now raises `ValueError` rather than being coerced with `str()` into a nonsense path segment. - Refactored `Image` model to use `@dataclass` and `@dataclass_json` for consistency with `Instance` and `Volume` - License changed from MIT to Apache 2.0 diff --git a/CLAUDE.md b/CLAUDE.md index a921d13..803af4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,6 +101,10 @@ The client raises `ValueError` for values it cannot make safe — empty/`None`, Dot-segments are rejected rather than encoded because encoding does not hold end to end: `requests` decodes `%2E` back to `.` before sending, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. A slash on its own is fine and is encoded (`docker.io/myorg` → `docker.io%2Fmyorg`). +The one rule for "would a URL parser resolve this away?" lives in `verda.helpers.has_relative_path_segment`, which strips any query string and decodes the escapes `requests` decodes. Use it — do not re-implement the check. `InferenceClient` once carried a second copy that missed `%2E%2E`. + +`tests/unit_tests/test_path_traversal.py` enforces its own completeness: it reads the source for methods passing `path_params` and fails if any is absent from its `_call_sites` table. + All five verbs delegate to a single private `_request`, which is where the url is built and validated. Add new verbs by delegating to it, never by calling `requests` directly. Encoding lives in `_encode_path_segment` in `verda/http_client/_http_client.py`. It is private on purpose: `path_params` is the only supported way to put a caller-supplied value into a path, so service modules never hand-roll encoding. As a backstop, `_add_base_url` refuses to send a request whose path would escape the API base path. diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index 3ad4762..590b22c 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -160,12 +160,6 @@ def test_placeholder_missing_from_path_params_raises(self, http_client): with pytest.raises(ValueError, match='no value given for placeholder'): http_client._build_path('/x/{a}/{b}', {'a': '1'}) - def test_malformed_percent_escape_fails_closed(self, http_client): - # The dot-segment check cannot be performed on a path that will not normalise, - # so the request must be refused rather than sent unchecked. - with pytest.raises(ValueError, match='malformed percent-escape'): - http_client._add_base_url('/x/%2E%2E/%zz') - def test_misspelled_key_reports_the_unused_parameter(self, http_client): # Both wrong at once; the unused-key message names the actual mistake. with pytest.raises(ValueError, match='unused path parameter'): @@ -179,13 +173,25 @@ def test_path_param_the_template_does_not_use_is_rejected(self, http_client): @pytest.mark.parametrize( ('value', 'expected'), - [(123, '123'), (uuid.UUID('0c41e387-8b12-4b4b-9c1e-000000000001'), None)], + [ + (123, '123'), + ( + uuid.UUID('0c41e387-8b12-4b4b-9c1e-000000000001'), + '0c41e387-8b12-4b4b-9c1e-000000000001', + ), + ], ) def test_non_string_values_are_coerced_not_rejected(self, http_client, value, expected): # Before path_params these were interpolated by an f-string, so rejecting them # would be an undocumented breaking change for callers passing ints or UUIDs. - path = http_client._build_path('/instances/{id}', {'id': value}) - assert path == f'/instances/{expected or value}' + assert http_client._build_path('/instances/{id}', {'id': value}) == f'/instances/{expected}' + + @pytest.mark.parametrize('value', [b'abc', ['a'], {'k': 'v'}, 3.5]) + def test_types_that_are_not_identifiers_are_rejected(self, http_client, value): + # str() on these yields a nonsense segment (b'abc' -> "b'abc'"), producing a + # confusing 404 instead of an error at the call site. + with pytest.raises(ValueError, match='path segment must be'): + http_client._build_path('/instances/{id}', {'id': value}) def test_segment_count_guard_catches_an_encoder_that_leaks_a_separator( self, http_client, monkeypatch @@ -233,6 +239,22 @@ def test_add_base_url_rejects_relative_path_segments(self, http_client, path): with pytest.raises(ValueError, match='escape the API base path'): http_client._add_base_url(path) + def test_add_base_url_still_detects_traversal_beside_a_malformed_escape(self, http_client): + # A malformed escape must not stop the dot-segment check from running. + with pytest.raises(ValueError, match='escape the API base path'): + http_client._add_base_url('/x/%2E%2E/%zz') + + @pytest.mark.parametrize( + 'path', + ['/secrets/x?force=true&', '/secrets/x#frag'], + ) + def test_add_base_url_rejects_a_query_string_in_the_path(self, http_client, path): + # Query data is passed separately as `params`, so a '?' reaching here means an + # unencoded value was interpolated into the path -- the injection half of the + # traversal bug, which the dot-segment check alone does not catch. + with pytest.raises(ValueError, match='query string or fragment'): + http_client._add_base_url(path) + @pytest.mark.parametrize( 'path', [ diff --git a/tests/unit_tests/inference_client/test_inference_client.py b/tests/unit_tests/inference_client/test_inference_client.py index 130393a..f53a3b6 100644 --- a/tests/unit_tests/inference_client/test_inference_client.py +++ b/tests/unit_tests/inference_client/test_inference_client.py @@ -49,6 +49,10 @@ 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): @@ -57,3 +61,30 @@ def test_paths_that_escape_the_deployment_are_rejected(self, inference_client, p # inference key. with pytest.raises(InferenceClientError, match='relative path segment'): inference_client._build_url(path) + + @pytest.mark.parametrize( + 'path', + ['predict?filter=a/../b', 'predict#a/../b', 'predict?q=..'], + ) + def test_dot_segments_inside_a_query_string_are_not_path_traversal( + 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}' + + +class TestEndpointBaseUrl: + @pytest.mark.parametrize( + 'base_url', + [ + 'https://inf.example.com/v1/..', + 'https://inf.example.com/v1/%2E%2E', + 'https://inf.example.com/v1/../admin/dep', + ], + ) + def test_base_url_containing_a_dot_segment_is_rejected(self, base_url): + # deployment_name and base_domain are sliced out of this and interpolated + # into the async status/result urls without further checks. + with pytest.raises(InferenceClientError, match='relative path segment'): + InferenceClient(inference_key='k', endpoint_base_url=base_url) diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py index 70321d1..c0b8bb4 100644 --- a/tests/unit_tests/test_path_traversal.py +++ b/tests/unit_tests/test_path_traversal.py @@ -20,11 +20,14 @@ different API endpoint using the SDK's own credentials. """ +import ast +import pathlib import re import pytest import responses +import verda from verda.clusters import ClustersService from verda.containers import ( ComputeResource, @@ -100,6 +103,28 @@ _ENV_VARS = [EnvVar(name='K', value_or_reference_to_secret='v', type=EnvVarType.PLAIN)] +def _service_methods_taking_a_path_param() -> set[tuple[str, str]]: + """(class, method) for every service method that passes a value as a path param. + + Derived from the source rather than hand-listed, so a newly added method cannot + quietly escape the regression table below. + """ + found = set() + for path in sorted(pathlib.Path(verda.__file__).parent.rglob('*.py')): + for node in ast.walk(ast.parse(path.read_text())): + if not isinstance(node, ast.ClassDef) or not node.name.endswith('Service'): + continue + for method in node.body: + if not isinstance(method, ast.FunctionDef): + continue + for call in ast.walk(method): + if isinstance(call, ast.Call) and any( + keyword.arg == 'path_params' for keyword in call.keywords + ): + found.add((node.name, method.name)) + return found + + def _call_sites(services, name): """Every single-resource call site that interpolates a caller-supplied value. @@ -255,6 +280,24 @@ def services(self, http_client): 'jobs': JobDeploymentsService(http_client), } + def test_every_method_taking_a_path_param_is_covered(self, services): + # The table below is hand-maintained; this is what stops it drifting. Two + # methods (instances/clusters.is_available) were once missed exactly this way. + # arrange + covered = set() + for label, _, _ in _call_sites(services, 'placeholder'): + service_key, method_name = label.split('.', 1) + covered.add((type(services[service_key]).__name__, method_name)) + + # act + missing = _service_methods_taking_a_path_param() - covered + + # assert + assert not missing, ( + f'these methods take a caller-supplied path value but are not exercised ' + f'by this file; add them to _call_sites: {sorted(missing)}' + ) + @pytest.mark.parametrize(('name', 'encoded'), HOSTILE_NAMES) def test_hostile_name_stays_within_its_endpoint(self, services, name, encoded): for label, call, expected_prefix in _call_sites(services, name): diff --git a/verda/helpers.py b/verda/helpers.py index a0ae6d7..2935251 100644 --- a/verda/helpers.py +++ b/verda/helpers.py @@ -13,8 +13,58 @@ # limitations under the License. import json +import re from typing import Any +# Path segments a URL parser resolves relative to the preceding segment (RFC 3986, +# section 5.2.4). Reaching the wire with one of these means the request has been +# retargeted at a different endpoint. +_RELATIVE_SEGMENTS = frozenset({'.', '..'}) + +# 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})') +_UNRESERVED = frozenset('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~') + + +def _decode_unreserved(path: str) -> str: + """Decode the percent-escapes that ``requests`` decodes before sending a request. + + Mirrors ``requests.utils.unquote_unreserved``, which is not part of the public + ``requests`` API, so a future release cannot break importing this package. + + Args: + path: A url path, possibly percent-encoded. + + Returns: + The path with escapes of unreserved characters decoded. + """ + + def replace(match: re.Match) -> str: + char = chr(int(match.group(1), 16)) + return char if char in _UNRESERVED else match.group(0) + + return _ESCAPED_UNRESERVED.sub(replace, path) + + +def has_relative_path_segment(path: str) -> bool: + """Whether a url path contains a segment a URL parser would resolve away. + + Any query string or fragment is stripped first: only the path is resolved, so a + dot-segment inside a query value is not traversal. Percent-escapes of unreserved + characters are then decoded, because ``requests`` decodes them before sending and + a check against the raw string would miss ``%2E%2E``. + + Args: + path: A url path to inspect. + + Returns: + True if resolving the path would move it above one of its own segments. + """ + path = path.split('?', 1)[0].split('#', 1)[0] + + return bool(_RELATIVE_SEGMENTS.intersection(_decode_unreserved(path).split('/'))) + 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 944fc42..b4dba85 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -15,24 +15,19 @@ import json import re from urllib.parse import quote +from uuid import UUID import requests -from requests.exceptions import InvalidURL -from requests.utils import unquote_unreserved from verda._version import __version__ from verda.exceptions import APIException - -# Path segments a URL parser resolves relative to the preceding segment (RFC 3986, -# section 5.2.4). Reaching the wire with one of these means the request has been -# retargeted at a different endpoint. -_RELATIVE_SEGMENTS = frozenset({'.', '..'}) +from verda.helpers import _RELATIVE_SEGMENTS, has_relative_path_segment # A `{name}` placeholder in a relative url template. _PLACEHOLDER = re.compile(r'\{(\w+)\}') -def _encode_path_segment(value: str) -> str: +def _encode_path_segment(value: object) -> str: """Encode a caller-supplied value for safe use as a single URL path segment. Resource names and ids are interpolated into request paths. An unencoded value @@ -41,9 +36,10 @@ def _encode_path_segment(value: str) -> str: credentials. Encoding with ``safe=''`` also neutralises ``?`` and ``#``, so a name cannot inject query parameters either. - Non-string values are coerced with ``str()``, matching what the f-string - interpolation this replaced used to do, so ids passed as ``int`` or ``UUID`` - keep working. + ``int`` and ``UUID`` are coerced with ``str()``, matching what the f-string + interpolation this replaced used to do, so ids passed as either keep working. Any + other type is refused: ``str()`` on it would yield a nonsense segment (``b'abc'`` + becomes ``"b'abc'"``) and a confusing 404 in place of an error at the call site. Args: value: A resource name or id supplied by the caller. @@ -52,10 +48,10 @@ def _encode_path_segment(value: str) -> str: The value encoded as exactly one path segment. Raises: - ValueError: If the value is empty, or contains a relative path segment that no - amount of encoding can make safe. + ValueError: If the value is empty, is not a str, int or UUID, or contains a + relative path segment that no amount of encoding can make safe. """ - if value is None: + if not isinstance(value, str | int | UUID) or isinstance(value, bool): raise ValueError(f'path segment must be a non-empty string, got {value!r}') if not isinstance(value, str): value = str(value) @@ -430,18 +426,12 @@ def _add_base_url(self, url: str) -> str: Raises: ValueError: If the path could escape the API base path. """ - path = url.split('?', 1)[0].split('#', 1)[0] - - # requests decodes percent-encoded unreserved characters before sending, which - # turns '%2E' back into '.'. Inspect the path the way the server will see it. - # A malformed escape means the check cannot be performed, so fail closed - # rather than validating the un-normalised path. - try: - path = unquote_unreserved(path) - except InvalidURL as error: - raise ValueError(f'request path has a malformed percent-escape: {url!r}') from error + # Query data is passed separately as `params`, so a '?' or '#' here means a + # caller-supplied value was interpolated into the path without encoding. + if '?' in url or '#' in url: + raise ValueError(f'request path must not contain a query string or fragment: {url!r}') - if _RELATIVE_SEGMENTS.intersection(path.split('/')): + if has_relative_path_segment(url): raise ValueError( f'refusing to send a request whose path would escape the API base path: {url!r}' ) diff --git a/verda/inference_client/_inference_client.py b/verda/inference_client/_inference_client.py index d4fbc76..802fff0 100644 --- a/verda/inference_client/_inference_client.py +++ b/verda/inference_client/_inference_client.py @@ -22,9 +22,7 @@ from dataclasses_json import Undefined, dataclass_json # type: ignore from requests.structures import CaseInsensitiveDict -# Path segments a URL parser resolves relative to the preceding segment (RFC 3986, -# section 5.2.4). -_RELATIVE_SEGMENTS = frozenset({'.', '..'}) +from verda.helpers import has_relative_path_segment class InferenceClientError(Exception): @@ -143,17 +141,19 @@ def __init__( parsed_url = urlparse(endpoint_base_url) if not parsed_url.scheme or not parsed_url.netloc: raise InferenceClientError('endpoint_base_url must be a valid URL') + # base_domain and deployment_name are sliced out of this url and interpolated + # into the async status/result urls, so the whole path must be free of + # segments a parser would resolve away. + if has_relative_path_segment(parsed_url.path): + raise InferenceClientError( + f'endpoint_base_url must not contain a relative path segment, ' + f'got {endpoint_base_url!r}' + ) self.inference_key = inference_key self.endpoint_base_url = endpoint_base_url.rstrip('/') self.base_domain = self.endpoint_base_url[: self.endpoint_base_url.rindex('/')] self.deployment_name = self.endpoint_base_url[self.endpoint_base_url.rindex('/') + 1 :] - # deployment_name is interpolated into the async status/result urls, so a - # dot-segment here would retarget those at the parent path. - if self.deployment_name in _RELATIVE_SEGMENTS: - raise InferenceClientError( - f'endpoint_base_url must end in a deployment name, got {endpoint_base_url!r}' - ) self.timeout_seconds = timeout_seconds self._session = requests.Session() self._global_headers = { @@ -221,7 +221,7 @@ def _build_url(self, path: str) -> str: InferenceClientError: If the path contains a relative path segment. """ relative = path.lstrip('/') - if _RELATIVE_SEGMENTS.intersection(relative.split('/')): + if has_relative_path_segment(relative): raise InferenceClientError( f'path must not contain a relative path segment, got {path!r}' ) From c6c3e1bba2b25558b40c98248bdc49136e71d632 Mon Sep 17 00:00:00 2001 From: Tamir Date: Fri, 7 Aug 2026 15:22:58 +0300 Subject: [PATCH 5/7] security: reject traversal hidden behind an encoded separator --- CLAUDE.md | 4 +- .../http_client/test_http_client.py | 38 +++++++++++++++-- tests/unit_tests/test_path_traversal.py | 41 +++++++++++++++++++ verda/helpers.py | 16 +++++++- verda/http_client/_http_client.py | 13 +++++- 5 files changed, 106 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 803af4f..e2acc63 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,7 +101,9 @@ The client raises `ValueError` for values it cannot make safe — empty/`None`, Dot-segments are rejected rather than encoded because encoding does not hold end to end: `requests` decodes `%2E` back to `.` before sending, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. A slash on its own is fine and is encoded (`docker.io/myorg` → `docker.io%2Fmyorg`). -The one rule for "would a URL parser resolve this away?" lives in `verda.helpers.has_relative_path_segment`, which strips any query string and decodes the escapes `requests` decodes. Use it — do not re-implement the check. `InferenceClient` once carried a second copy that missed `%2E%2E`. +Checking a **finished path** for "would a URL parser resolve this away?" is `verda.helpers.has_relative_path_segment`. It strips any query string, decodes the escapes `requests` decodes, and decodes encoded separators (`%2F`, `%5C`) because an intermediary may unescape them before normalising. Use it — do not re-implement it. `InferenceClient` once carried a second copy that missed `%2E%2E`. + +`_encode_path_segment` checks the **raw value** instead, on purpose: that value still has `quote()` ahead of it, which escapes `%` and so makes `%2E%2E` and `..%2F..` inert as literal names. Using the finished-path helper there would reject them for no reason. Different question, different check — keep them apart. `tests/unit_tests/test_path_traversal.py` enforces its own completeness: it reads the source for methods passing `path_params` and fails if any is absent from its `_call_sites` table. diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index 590b22c..d37951a 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -91,11 +91,17 @@ def test_encode_path_segment_escapes_percent_so_encoded_dots_stay_literal(value) assert _encode_path_segment(value) == value.replace('%', '%25') -@pytest.mark.parametrize('value', ['', None]) -def test_encode_path_segment_rejects_empty_values(value): +def test_encode_path_segment_rejects_an_empty_value(): # An empty segment collapses the URL onto the collection endpoint, which turns # a delete-one call into a delete-all call. with pytest.raises(ValueError, match='must be a non-empty string'): + _encode_path_segment('') + + +@pytest.mark.parametrize('value', [None, 3.5, b'abc', True]) +def test_encode_path_segment_reports_a_wrong_type_as_a_type_problem(value): + # The message has to name the real cause: 3.5 and None are not empty strings. + with pytest.raises(ValueError, match='must be a str, int or UUID'): _encode_path_segment(value) @@ -244,6 +250,31 @@ def test_add_base_url_still_detects_traversal_beside_a_malformed_escape(self, ht with pytest.raises(ValueError, match='escape the API base path'): http_client._add_base_url('/x/%2E%2E/%zz') + @pytest.mark.parametrize( + 'path', + [ + '/container-deployments/..%2F..%2Fv1%2Finstances', + '/container-deployments/..%2f..%2fv1%2Finstances', + '/instances/%2E%2E%2Fssh-keys', + '/volumes/..%5C..%5Cadmin', + ], + ) + def test_add_base_url_rejects_traversal_hidden_behind_an_encoded_separator( + self, http_client, path + ): + # The backstop exists for a call site that forgot to encode. Percent-encoding + # the separator must not hide the dot-segments from it, because an + # intermediary that unescapes encoded slashes restores the traversal. + with pytest.raises(ValueError, match='escape the API base path'): + http_client._add_base_url(path) + + @pytest.mark.parametrize('path', ['/container-deployments/', '/volumes//x']) + def test_add_base_url_rejects_an_empty_path_segment(self, http_client, path): + # An empty segment collapses the request onto the collection endpoint, turning + # a delete-one into a delete-all. + with pytest.raises(ValueError, match='empty path segment'): + http_client._add_base_url(path) + @pytest.mark.parametrize( 'path', ['/secrets/x?force=true&', '/secrets/x#frag'], @@ -259,8 +290,9 @@ def test_add_base_url_rejects_a_query_string_in_the_path(self, http_client, path 'path', [ '/container-deployments/my-deployment', - '/container-deployments/..%2F..%2Fv1%2Finstances', '/container-deployments/%252E%252E', + # an encoded slash with no dot-segment around it is a legitimate name + '/container-registry-credentials/docker.io%2Fmyorg', '/volumes/name.with.dots', '/volumes/..leading', '/long-term/periods/clusters', diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py index c0b8bb4..f4888d9 100644 --- a/tests/unit_tests/test_path_traversal.py +++ b/tests/unit_tests/test_path_traversal.py @@ -125,6 +125,36 @@ def _service_methods_taking_a_path_param() -> set[tuple[str, str]]: return found +def _service_methods_interpolating_a_url() -> set[tuple[str, str]]: + """(class, method) for service methods that build a url with an f-string. + + The table below only knows about methods that already pass ``path_params``, so a + newly added method that interpolates instead would be invisible to it. This finds + those directly: it was an inline f-string url, not an endpoint constant, that hid + ``is_available`` from every earlier search. + """ + found = set() + for path in sorted(pathlib.Path(verda.__file__).parent.rglob('*.py')): + for node in ast.walk(ast.parse(path.read_text())): + if not isinstance(node, ast.ClassDef) or not node.name.endswith('Service'): + continue + for method in node.body: + if not isinstance(method, ast.FunctionDef): + continue + for expression in ast.walk(method): + if not isinstance(expression, ast.JoinedStr): + continue + literal = ''.join( + part.value for part in expression.values if isinstance(part, ast.Constant) + ) + interpolates = any( + isinstance(part, ast.FormattedValue) for part in expression.values + ) + if interpolates and '/' in literal: + found.add((node.name, method.name)) + return found + + def _call_sites(services, name): """Every single-resource call site that interpolates a caller-supplied value. @@ -280,6 +310,17 @@ def services(self, http_client): 'jobs': JobDeploymentsService(http_client), } + def test_no_service_builds_a_url_by_interpolation(self): + # CLAUDE.md: "Never interpolate a caller-supplied value into the request path." + # Without this, a new method that f-strings its url is caught by neither the + # coverage table below nor the runtime backstop. + interpolating = _service_methods_interpolating_a_url() + + assert not interpolating, ( + f'these methods build a url with an f-string instead of path_params, so a ' + f'caller-supplied value can escape its path segment: {sorted(interpolating)}' + ) + def test_every_method_taking_a_path_param_is_covered(self, services): # The table below is hand-maintained; this is what stops it drifting. Two # methods (instances/clusters.is_available) were once missed exactly this way. diff --git a/verda/helpers.py b/verda/helpers.py index 2935251..b4981db 100644 --- a/verda/helpers.py +++ b/verda/helpers.py @@ -26,6 +26,11 @@ _ESCAPED_UNRESERVED = re.compile(r'%([0-9A-Fa-f]{2})') _UNRESERVED = frozenset('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~') +# Percent-encoded path separators. `requests` leaves these encoded, but they are +# decoded by intermediaries that unescape before normalising, and a backslash is +# treated as a separator by some servers. +_ENCODED_SEPARATORS = ('%2F', '%2f', '%5C', '%5c', '\\') + def _decode_unreserved(path: str) -> str: """Decode the percent-escapes that ``requests`` decodes before sending a request. @@ -55,6 +60,12 @@ def has_relative_path_segment(path: str) -> bool: characters are then decoded, because ``requests`` decodes them before sending and a check against the raw string would miss ``%2E%2E``. + Encoded separators are decoded too. They survive to the wire, but an intermediary + that unescapes them before normalising the path (for example Envoy's + ``UNESCAPE_AND_FORWARD``) turns ``..%2F..%2Fx`` back into a working traversal, so + the check has to see what such a server would see. This does not reject an encoded + separator on its own: ``docker.io%2Fmyorg`` decodes to two ordinary segments. + Args: path: A url path to inspect. @@ -62,8 +73,11 @@ def has_relative_path_segment(path: str) -> bool: True if resolving the path would move it above one of its own segments. """ path = path.split('?', 1)[0].split('#', 1)[0] + decoded = _decode_unreserved(path) + for separator in _ENCODED_SEPARATORS: + decoded = decoded.replace(separator, '/') - return bool(_RELATIVE_SEGMENTS.intersection(_decode_unreserved(path).split('/'))) + return bool(_RELATIVE_SEGMENTS.intersection(decoded.split('/'))) def stringify_class_object_properties(class_object: type) -> str: diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index b4dba85..3687ea0 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -52,7 +52,7 @@ def _encode_path_segment(value: object) -> str: relative path segment that no amount of encoding can make safe. """ if not isinstance(value, str | int | UUID) or isinstance(value, bool): - raise ValueError(f'path segment must be a non-empty string, got {value!r}') + raise ValueError(f'path segment must be a str, int or UUID, got {type(value).__name__}') if not isinstance(value, str): value = str(value) if not value: @@ -63,6 +63,12 @@ def _encode_path_segment(value: object) -> str: # that unescapes encoded slashes before normalising the path (for example # Envoy's UNESCAPE_AND_FORWARD) would turn '..%2F..%2Fx' back into a traversal. # A resource name has no legitimate reason to contain a dot-segment. + # + # This deliberately checks the raw value rather than calling + # `has_relative_path_segment`, which answers a different question. That helper + # inspects a finished path that will not be encoded again, so it must decode + # escapes. Here the value still has `quote` ahead of it, which turns '%' into + # '%25' and so renders '%2E%2E' and '..%2F..' inert as literal names. if _RELATIVE_SEGMENTS.intersection(value.split('/')): raise ValueError(f'path segment must not contain a relative path segment, got {value!r}') @@ -431,6 +437,11 @@ def _add_base_url(self, url: str) -> str: if '?' in url or '#' in url: raise ValueError(f'request path must not contain a query string or fragment: {url!r}') + # An empty segment collapses the request onto the collection endpoint, which + # turns a delete-one call into a delete-all call. + if any(segment == '' for segment in url.split('/')[1:]): + raise ValueError(f'request path must not contain an empty path segment: {url!r}') + if has_relative_path_segment(url): raise ValueError( f'refusing to send a request whose path would escape the API base path: {url!r}' From 2894689b74dbd51a33c2a6b454371d1b73d0eb8e Mon Sep 17 00:00:00 2001 From: Alexey Shamrin Date: Mon, 10 Aug 2026 10:43:24 +0300 Subject: [PATCH 6/7] security: reject unsafe resource names instead of encoding them A resource name or id in a request path must now match `[A-Za-z0-9._~-]` (RFC 3986 unreserved). A value containing `/`, `\`, `%`, a space, `?` or `#` raises `ValueError` rather than being percent-encoded and sent. Every name the API takes in a path position is a slug, an id or a machine type (`my-deployment`, `1A100.22V`, a UUID), so ordinary calls are unaffected; pass a raw name rather than a pre-encoded one. `InferenceClient` now requires `endpoint_base_url` to include the deployment path. Without one, `rindex('/')` found the `//` of the scheme, making `base_domain` `https:/` and sending async status and result requests to a host named `status` while still carrying the inference key. Refusing `%` removes the ambiguity the previous approach had to resolve by predicting how `requests` and intermediaries decode a path. The http client no longer models either, so its raw-value and finished-path checks can no longer disagree: `..\..\x` previously passed the first and was caught only by the backstop, which is documented as redundant. `has_relative_path_segment` is now used by `InferenceClient` alone. **Breaking changes:** - A path value outside the unreserved set now raises `ValueError` instead of being percent-encoded. This includes `/`, which is no longer sent as `%2F`. - `InferenceClient` rejects an `endpoint_base_url` with no deployment path. --- CHANGELOG.md | 8 +- CLAUDE.md | 14 +- .../http_client/test_http_client.py | 124 +++++++++--------- .../inference_client/test_inference_client.py | 14 ++ tests/unit_tests/test_path_traversal.py | 67 ++++------ verda/http_client/_http_client.py | 83 ++++++------ verda/inference_client/_inference_client.py | 16 ++- 7 files changed, 160 insertions(+), 166 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8268cf8..cdd1a2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed a path traversal issue where a resource name or id containing `../` was resolved while the request was prepared, retargeting the call at a different API endpoint under the SDK's own credentials (for example `containers.delete_deployment('../../v1/instances')` issued `DELETE /v1/instances`). This also prevents a name from injecting query parameters, such as overriding the `force` flag of `containers.delete_secret`. - Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a keyword-only `path_params` mapping whose values are percent-encoded as a single path segment before substitution, and all service modules pass names and ids that way. This covers `instances.is_available()` and `clusters.is_available()`, where the affected value was the `instance_type`/`cluster_type`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. + Caller-supplied path values are no longer interpolated into the request path. `HTTPClient.get/post/put/patch/delete` now accept a keyword-only `path_params` mapping whose values are validated as a single path segment before substitution, and all service modules pass names and ids that way. This covers `instances.is_available()` and `clusters.is_available()`, where the affected value was the `instance_type`/`cluster_type`. As a backstop, `HTTPClient` refuses to send a request whose path would escape the API base path. `InferenceClient` paths are validated too: `path` may still span several segments, but it can no longer walk out of the deployment's base url. @@ -24,10 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **Breaking:** a resource name or id containing a relative path segment (`.` or `..` between slashes), an empty value, or `None` now raises `ValueError` instead of being sent. Encoding alone is not sufficient for these: `%2E` is decoded back to `.` before the request is sent, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. -- **Breaking:** resource names and ids are now percent-encoded, so a value that was already URL-encoded by the caller is encoded again — `get_deployment_by_name('my%20deployment')` now looks up a deployment literally named `my%20deployment` rather than `my deployment`. Pass the raw name instead. -- **Breaking:** a `/` inside a resource name is now encoded as `%2F` and stays one path segment, where it previously split the route — `delete_registry_credentials('docker.io/myorg')` sends `DELETE /v1/container-registry-credentials/docker.io%2Fmyorg`. Servers that reject or refuse to decode encoded slashes (nginx and Apache with `AllowEncodedSlashes off`) will not match such a name. +- **Breaking:** a resource name or id used in a request path must now match `[A-Za-z0-9._~-]+` (the RFC 3986 unreserved set). Anything else raises `ValueError` instead of being percent-encoded and sent — including `/`, `\`, `%`, spaces, `?`, `#` and non-ASCII characters. Every name the API takes in a path position is a slug, an id or a machine type (`my-deployment`, `1A100.22V`, a UUID), so ordinary calls are unaffected. If you have a name that was already URL-encoded, pass the raw name: `get_deployment_by_name('my%20deployment')` now raises rather than looking up a deployment literally named `my%20deployment`. +- **Breaking:** a relative path segment (`.` or `..`), an empty value, or `None` raises `ValueError`. Encoding is not sufficient for these: `%2E` is decoded back to `.` before the request is sent. - **Breaking:** a path value that is not a `str`, `int` or `UUID` now raises `ValueError` rather than being coerced with `str()` into a nonsense path segment. +- **Breaking:** `InferenceClient` now requires `endpoint_base_url` to include the deployment path. `InferenceClient(key, 'https://containers.example.com')` previously produced a `base_domain` of `https:/`, sending async status and result requests to a host named `status`/`result` while still carrying the inference key. - Refactored `Image` model to use `@dataclass` and `@dataclass_json` for consistency with `Instance` and `Volume` - License changed from MIT to Apache 2.0 diff --git a/CLAUDE.md b/CLAUDE.md index e2acc63..bbabe2d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,7 +77,7 @@ Service modules call the shared `HTTPClient` (`verda/http_client/`), which expos **Never interpolate a caller-supplied value into the request path.** Resource names and IDs arrive from application input. A value containing `../` is resolved while the request is prepared and retargets the call at a different API endpoint under the SDK's own credentials; a value containing `?` injects query parameters. -Pass such values as `path_params`. The client percent-encodes each one as exactly one path segment before substituting it: +Pass such values as `path_params`. The client validates each one as exactly one path segment before substituting it: ```python # correct @@ -97,19 +97,17 @@ response = self.client.get(f'{CONTAINER_DEPLOYMENTS_ENDPOINT}/{deployment_name}/ Endpoint paths belong in a module-level `_ENDPOINT` constant, never an inline string literal — a literal hides the call site from the greps and audits used to check this rule. -The client raises `ValueError` for values it cannot make safe — empty/`None`, and any value with a relative path segment (`.` or `..` between slashes) — and for any template/`path_params` mismatch. Non-string values (`int`, `UUID`) are coerced with `str()`. +A path value must match `[A-Za-z0-9._~-]+` (the RFC 3986 unreserved set). Anything else raises `ValueError`, as do `.`, `..`, empty/`None`, a non-`str`/`int`/`UUID` type, and any template/`path_params` mismatch. `int` and `UUID` are coerced with `str()`. -Dot-segments are rejected rather than encoded because encoding does not hold end to end: `requests` decodes `%2E` back to `.` before sending, and `%2F` is restored by any intermediary that unescapes encoded slashes before normalising the path. A slash on its own is fine and is encoded (`docker.io/myorg` → `docker.io%2Fmyorg`). - -Checking a **finished path** for "would a URL parser resolve this away?" is `verda.helpers.has_relative_path_segment`. It strips any query string, decodes the escapes `requests` decodes, and decodes encoded separators (`%2F`, `%5C`) because an intermediary may unescape them before normalising. Use it — do not re-implement it. `InferenceClient` once carried a second copy that missed `%2E%2E`. - -`_encode_path_segment` checks the **raw value** instead, on purpose: that value still has `quote()` ahead of it, which escapes `%` and so makes `%2E%2E` and `..%2F..` inert as literal names. Using the finished-path helper there would reject them for no reason. Different question, different check — keep them apart. +Reject, do not encode: `requests` decodes `%2E` back to `.` before sending, and an intermediary that unescapes `%2F` before normalising the path restores a traversal. Do not encode a rejected name at the call site — pass the raw name. `tests/unit_tests/test_path_traversal.py` enforces its own completeness: it reads the source for methods passing `path_params` and fails if any is absent from its `_call_sites` table. All five verbs delegate to a single private `_request`, which is where the url is built and validated. Add new verbs by delegating to it, never by calling `requests` directly. -Encoding lives in `_encode_path_segment` in `verda/http_client/_http_client.py`. It is private on purpose: `path_params` is the only supported way to put a caller-supplied value into a path, so service modules never hand-roll encoding. As a backstop, `_add_base_url` refuses to send a request whose path would escape the API base path. +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. ## Code style diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index d37951a..15608b5 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -38,40 +38,43 @@ def test_encode_path_segment_leaves_ordinary_names_unchanged(value): @pytest.mark.parametrize( - ('value', 'expected'), + 'value', [ - ('a/b', 'a%2Fb'), - ('docker.io/myorg', 'docker.io%2Fmyorg'), + 'a/b', + 'docker.io/myorg', + '../../v1/balance', + '../ssh-keys', + 'nested/../../escape', + 'a/./b', + 'a/..', + # some servers normalise '\' to a separator + '..\\..\\v1\\balance', + 'a\\b', ], ) -def test_encode_path_segment_encodes_path_separators(value, expected): - assert _encode_path_segment(value) == expected +def test_encode_path_segment_rejects_path_separators(value): + # '%2F' survives to the wire, but any intermediary that unescapes before + # normalising (Envoy's UNESCAPE_AND_FORWARD) turns '..%2F..%2Fv1%2Finstances' + # back into a working traversal. + with pytest.raises(ValueError, match='only letters, digits'): + _encode_path_segment(value) @pytest.mark.parametrize( 'value', - ['../../v1/balance', '../ssh-keys', 'nested/../../escape', 'a/./b', 'a/..'], + ['x?force=true&', 'name#fragment', 'a b', 'a;b', 'ünicode', 'name@host', 'a+b'], ) -def test_encode_path_segment_rejects_values_containing_dot_segments(value): - # Encoding '/' as '%2F' is not sufficient on its own: any intermediary that - # unescapes encoded slashes before normalising the path (Envoy's - # UNESCAPE_AND_FORWARD, some gateways and servlet containers) turns - # '..%2F..%2Fv1%2Finstances' back into a working traversal. A resource name has - # no legitimate reason to contain a dot-segment, so refuse it outright. - with pytest.raises(ValueError, match='relative path segment'): +def test_encode_path_segment_rejects_characters_outside_the_unreserved_set(value): + # These were previously percent-encoded and sent. + with pytest.raises(ValueError, match='only letters, digits'): _encode_path_segment(value) -@pytest.mark.parametrize( - ('value', 'expected'), - [ - ('x?force=true&', 'x%3Fforce%3Dtrue%26'), - ('name#fragment', 'name%23fragment'), - ('a b', 'a%20b'), - ], -) -def test_encode_path_segment_encodes_query_and_fragment_delimiters(value, expected): - assert _encode_path_segment(value) == expected +@pytest.mark.parametrize('value', ['%2E', '%2e%2E', '..%2F..', 'my%20deployment', '100%']) +def test_encode_path_segment_rejects_a_percent_sign(value): + # A pre-encoded name is caller error: pass the raw name. + with pytest.raises(ValueError, match='only letters, digits'): + _encode_path_segment(value) @pytest.mark.parametrize('value', ['.', '..']) @@ -84,13 +87,6 @@ def test_encode_path_segment_rejects_relative_segments(value): _encode_path_segment(value) -@pytest.mark.parametrize('value', ['%2E', '%2e%2E', '..%2F..']) -def test_encode_path_segment_escapes_percent_so_encoded_dots_stay_literal(value): - # A literal '%' is itself encoded to '%25', so these can never decode back into - # a dot-segment on the wire. - assert _encode_path_segment(value) == value.replace('%', '%25') - - def test_encode_path_segment_rejects_an_empty_value(): # An empty segment collapses the URL onto the collection endpoint, which turns # a delete-one call into a delete-all call. @@ -133,21 +129,16 @@ def test_multiple_params_are_substituted(self, http_client): assert path == '/container-deployments/dep/replicas/r-1' @pytest.mark.parametrize( - ('value', 'encoded'), - [ - ('a/b', 'a%2Fb'), - ('x?force=true&', 'x%3Fforce%3Dtrue%26'), - ('name#frag', 'name%23frag'), - ('a b', 'a%20b'), - ], + 'value', + ['a/b', 'x?force=true&', 'name#frag', 'a b', '..\\..\\x', '%2E%2E'], ) - def test_hostile_values_are_encoded_into_one_segment(self, http_client, value, encoded): - path = http_client._build_path('/secrets/{name}', {'name': value}) - assert path == f'/secrets/{encoded}' + def test_hostile_values_are_rejected(self, http_client, value): + with pytest.raises(ValueError, match='only letters, digits'): + http_client._build_path('/secrets/{name}', {'name': value}) @pytest.mark.parametrize('value', ['../../v1/instances', 'nested/../escape']) def test_values_containing_dot_segments_are_rejected(self, http_client, value): - with pytest.raises(ValueError, match='relative path segment'): + with pytest.raises(ValueError, match='path segment must'): http_client._build_path('/secrets/{name}', {'name': value}) @pytest.mark.parametrize('value', ['.', '..', '', None]) @@ -233,39 +224,30 @@ def test_add_base_url(self, http_client): '/instances/../ssh-keys', '/volumes/.', '/scripts/./x', - # requests decodes percent-encoded unreserved characters before sending, - # so these reach the server as real dot-segments. - '/container-deployments/%2E%2E', - '/volumes/%2e', ], ) def test_add_base_url_rejects_relative_path_segments(self, http_client, path): - # A call site that forgets to encode its path segment must not be able to - # retarget the request at a different endpoint. + # A call site that skips path_params must not be able to retarget the request. with pytest.raises(ValueError, match='escape the API base path'): http_client._add_base_url(path) - def test_add_base_url_still_detects_traversal_beside_a_malformed_escape(self, http_client): - # A malformed escape must not stop the dot-segment check from running. - with pytest.raises(ValueError, match='escape the API base path'): - http_client._add_base_url('/x/%2E%2E/%zz') - @pytest.mark.parametrize( 'path', [ + '/container-deployments/%2E%2E', + '/volumes/%2e', + '/x/%2E%2E/%zz', '/container-deployments/..%2F..%2Fv1%2Finstances', '/container-deployments/..%2f..%2fv1%2Finstances', '/instances/%2E%2E%2Fssh-keys', '/volumes/..%5C..%5Cadmin', + '/volumes/..\\..\\admin', ], ) - def test_add_base_url_rejects_traversal_hidden_behind_an_encoded_separator( - self, http_client, path - ): - # The backstop exists for a call site that forgot to encode. Percent-encoding - # the separator must not hide the dot-segments from it, because an - # intermediary that unescapes encoded slashes restores the traversal. - with pytest.raises(ValueError, match='escape the API base path'): + def test_add_base_url_rejects_an_escape_or_backslash_outright(self, http_client, path): + # Neither can appear in a path the client built: `_encode_path_segment` refuses + # both and endpoint constants are plain literals. + with pytest.raises(ValueError, match='unencoded value'): http_client._add_base_url(path) @pytest.mark.parametrize('path', ['/container-deployments/', '/volumes//x']) @@ -290,9 +272,8 @@ def test_add_base_url_rejects_a_query_string_in_the_path(self, http_client, path 'path', [ '/container-deployments/my-deployment', - '/container-deployments/%252E%252E', - # an encoded slash with no dot-segment around it is a legitimate name - '/container-registry-credentials/docker.io%2Fmyorg', + '/container-registry-credentials/my-dockerhub-creds', + '/instance-availability/1H100.80S.22V', '/volumes/name.with.dots', '/volumes/..leading', '/long-term/periods/clusters', @@ -337,20 +318,33 @@ def test_generate_headers(self, http_client): @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) @responses.activate - def test_request_methods_encode_path_params(self, http_client, method): + def test_request_methods_substitute_path_params(self, http_client, method): # arrange responses.add(getattr(responses, method.upper()), re.compile(r'.*'), json={}, status=200) # act getattr(http_client, method)( - '/container-deployments/{name}/status', path_params={'name': 'x?force=true&'} + '/container-deployments/{name}/status', path_params={'name': 'my-deployment'} ) # assert assert responses.calls[0].request.path_url == ( - '/v1/container-deployments/x%3Fforce%3Dtrue%26/status' + '/v1/container-deployments/my-deployment/status' ) + @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) + @responses.activate + def test_request_methods_reject_a_hostile_path_param_before_sending(self, http_client, method): + # arrange + responses.add(getattr(responses, method.upper()), re.compile(r'.*'), json={}, status=200) + + # act / assert + with pytest.raises(ValueError, match='path segment must'): + getattr(http_client, method)( + '/container-deployments/{name}/status', path_params={'name': 'x?force=true&'} + ) + assert not responses.calls + @pytest.mark.parametrize('method', ['get', 'post', 'put', 'patch', 'delete']) @responses.activate def test_request_methods_reject_unsafe_path_params_before_sending(self, http_client, method): diff --git a/tests/unit_tests/inference_client/test_inference_client.py b/tests/unit_tests/inference_client/test_inference_client.py index f53a3b6..dd24e13 100644 --- a/tests/unit_tests/inference_client/test_inference_client.py +++ b/tests/unit_tests/inference_client/test_inference_client.py @@ -88,3 +88,17 @@ def test_base_url_containing_a_dot_segment_is_rejected(self, base_url): # into the async status/result urls without further checks. with pytest.raises(InferenceClientError, match='relative path segment'): InferenceClient(inference_key='k', endpoint_base_url=base_url) + + @pytest.mark.parametrize( + 'base_url', + ['https://containers.example.com', 'https://containers.example.com/'], + ) + def test_base_url_without_a_deployment_path_is_rejected(self, base_url): + # rindex('/') would find the '//' of the scheme, making base_domain 'https:/' + # and the async status url 'https://status/containers.example.com'. + with pytest.raises(InferenceClientError, match='must include the deployment path'): + InferenceClient(inference_key='k', endpoint_base_url=base_url) + + def test_the_deployment_name_is_sliced_out_correctly(self, inference_client): + assert inference_client.deployment_name == 'my-deployment' + assert inference_client.base_domain == 'https://inference.example.com/v1' diff --git a/tests/unit_tests/test_path_traversal.py b/tests/unit_tests/test_path_traversal.py index f4888d9..cdc074c 100644 --- a/tests/unit_tests/test_path_traversal.py +++ b/tests/unit_tests/test_path_traversal.py @@ -49,28 +49,13 @@ BASE_PATH = '/v1' -# Hostile name -> the single path segment it must be reduced to on the wire. -# Expectations are written out literally rather than derived from the SDK helper, so -# the test fails if the encoding rule itself regresses. -HOSTILE_NAMES = [ - # query-string / fragment smuggling - ('x?force=true&', 'x%3Fforce%3Dtrue%26'), - ('name#frag', 'name%23frag'), - # a literal '%' must be escaped so it cannot decode into a dot-segment later - ('%2E%2E', '%252E%252E'), - ('..%2F..', '..%252F..'), - # a slash with no dot-segment is encoded, not refused: registry credential - # names legitimately look like 'docker.io/myorg' - ('a/b', 'a%2Fb'), -] - -# Names that cannot be made safe by encoding and must be refused outright: -# - '.' and '..' are RFC 3986 dot-segments, and '%2E' is decoded back to '.' by -# requests before the request is sent -# - a dot-segment anywhere in the value survives '%2F' encoding if any intermediary -# unescapes encoded slashes before normalising the path -# - an empty name collapses the URL onto the collection endpoint, turning a -# delete-one call into a delete-all call +# Names outside the RFC 3986 unreserved set, refused rather than encoded: +# - '.' and '..' are dot-segments, and requests decodes '%2E' back to '.' +# - a separator ('/', or '\' on servers that normalise it) addresses a different +# endpoint, and '%2F' holds only until an intermediary unescapes it +# - '?' and '#' start a query string or fragment +# - '%' makes a name ambiguous with the two cases above +# - an empty name turns a delete-one call into a delete-all call REJECTED_NAMES = [ '.', '..', @@ -81,6 +66,16 @@ '../ssh-keys', '../../v1/balance', 'nested/../../escape', + '..\\..\\v1\\instances', + 'nested\\..\\..\\escape', + 'a/b', + 'docker.io/myorg', + 'x?force=true&', + 'name#frag', + '%2E%2E', + '..%2F..', + 'my%20deployment', + 'a b', ] ANY_URL = re.compile(r'.*') @@ -339,24 +334,6 @@ def test_every_method_taking_a_path_param_is_covered(self, services): f'by this file; add them to _call_sites: {sorted(missing)}' ) - @pytest.mark.parametrize(('name', 'encoded'), HOSTILE_NAMES) - def test_hostile_name_stays_within_its_endpoint(self, services, name, encoded): - for label, call, expected_prefix in _call_sites(services, name): - with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: - for method in ('GET', 'POST', 'PUT', 'PATCH', 'DELETE'): - mock.add(method, ANY_URL, json={}, status=200) - try: - call() - except Exception: # response-shape errors are irrelevant here - pass - - sent = [c.request.path_url for c in mock.calls] - assert sent, f'{label}({name!r}) sent no request' - for path in sent: - assert path.startswith(expected_prefix + encoded), ( - f'{label}({name!r}) escaped its endpoint: {path}' - ) - @pytest.mark.parametrize('name', REJECTED_NAMES) def test_unsafe_name_is_rejected_before_a_request_is_sent(self, services, name): for label, call, _ in _call_sites(services, name): @@ -390,7 +367,15 @@ def test_query_string_injection_cannot_override_the_force_flag(self, services): # smuggle an earlier `force=true` into the query string. with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: mock.add('DELETE', ANY_URL, json={}, status=200) - services['containers'].delete_secret('x?force=true&', force=False) + with pytest.raises(ValueError, match='path segment must'): + services['containers'].delete_secret('x?force=true&', force=False) + assert not mock.calls, 'a name carrying a query string reached the wire' + + def test_the_force_flag_still_reaches_the_query_string(self, services): + # Without this the test above passes even if delete_secret stopped sending it. + with responses.RequestsMock(assert_all_requests_are_fired=False) as mock: + mock.add('DELETE', ANY_URL, json={}, status=200) + services['containers'].delete_secret('my-api-key', force=False) path = mock.calls[0].request.path_url assert path.count('force=') == 1, f'force flag was polluted: {path}' diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index 3687ea0..f6b798b 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -21,35 +21,38 @@ from verda._version import __version__ from verda.exceptions import APIException -from verda.helpers import _RELATIVE_SEGMENTS, has_relative_path_segment +from verda.helpers import _RELATIVE_SEGMENTS # A `{name}` placeholder in a relative url template. _PLACEHOLDER = re.compile(r'\{(\w+)\}') +# Anything outside the RFC 3986 unreserved set. Every value the API takes in a path +# 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: - """Encode a caller-supplied value for safe use as a single URL path segment. + """Check a caller-supplied value is safe as a single URL path segment. - Resource names and ids are interpolated into request paths. An unencoded value - containing ``/`` is resolved by ``requests`` while the request is prepared, which - silently retargets the call at a different API endpoint under the caller's own - credentials. Encoding with ``safe=''`` also neutralises ``?`` and ``#``, so a - name cannot inject query parameters either. + Only the unreserved set is allowed. Encoding a separator is not sufficient: + ``requests`` decodes ``%2E`` back to ``.`` before sending, and an intermediary + that unescapes ``%2F`` before normalising the path restores a traversal. - ``int`` and ``UUID`` are coerced with ``str()``, matching what the f-string - interpolation this replaced used to do, so ids passed as either keep working. Any - other type is refused: ``str()`` on it would yield a nonsense segment (``b'abc'`` - becomes ``"b'abc'"``) and a confusing 404 in place of an error at the call site. + ``int`` and ``UUID`` are coerced with ``str()``. Other types are refused, since + ``str()`` on them yields a nonsense segment (``b'abc'`` becomes ``"b'abc'"``). Args: value: A resource name or id supplied by the caller. Returns: - The value encoded as exactly one path segment. + The value as exactly one path segment. Raises: - ValueError: If the value is empty, is not a str, int or UUID, or contains a - relative path segment that no amount of encoding can make safe. + ValueError: If the value is empty, is not a str, int or UUID, contains a + character outside the unreserved set, or is a relative path segment. """ if not isinstance(value, str | int | UUID) or isinstance(value, bool): raise ValueError(f'path segment must be a str, int or UUID, got {type(value).__name__}') @@ -57,21 +60,15 @@ def _encode_path_segment(value: object) -> str: value = str(value) if not value: raise ValueError(f'path segment must be a non-empty string, got {value!r}') - # Encoding is not sufficient for dot-segments. '%2E' is decoded back to '.' by - # requests before the request is sent, and encoding '/' as '%2F' only holds as - # long as nothing between here and the server unescapes it -- an intermediary - # that unescapes encoded slashes before normalising the path (for example - # Envoy's UNESCAPE_AND_FORWARD) would turn '..%2F..%2Fx' back into a traversal. - # A resource name has no legitimate reason to contain a dot-segment. - # - # This deliberately checks the raw value rather than calling - # `has_relative_path_segment`, which answers a different question. That helper - # inspects a finished path that will not be encoded again, so it must decode - # escapes. Here the value still has `quote` ahead of it, which turns '%' into - # '%25' and so renders '%2E%2E' and '..%2F..' inert as literal names. - if _RELATIVE_SEGMENTS.intersection(value.split('/')): - raise ValueError(f'path segment must not contain a relative path segment, got {value!r}') + if _UNSAFE_IN_SEGMENT.search(value): + raise ValueError( + f'path segment must contain only letters, digits and "-._~", got {value!r}' + ) + # Only reachable as the whole value: a separator cannot pass the check above. + if value in _RELATIVE_SEGMENTS: + raise ValueError(f'path segment must not be a relative path segment, got {value!r}') + # A no-op for an allowlisted value, kept in case the allowlist widens. return quote(value, safe='') @@ -368,13 +365,12 @@ def _generate_user_agent(self) -> str: def _build_path(self, url: str, path_params: dict | None) -> str: """Substitutes caller-supplied values into a relative url template. - Each value is encoded as exactly one path segment, so a resource name or id + Each value is checked as exactly one path segment, so a resource name or id cannot introduce a path separator, walk out of its segment, or start a query - string. Call sites pass values as data rather than interpolating them, which - means the encoding cannot be forgotten. + string. Example: - ``_build_path('/instances/{id}', {'id': 'a/b'})`` returns ``'/instances/a%2Fb'`` + ``_build_path('/instances/{id}', {'id': 'abc'})`` returns ``'/instances/abc'`` Args: url: A relative url, optionally containing ``{name}`` placeholders. @@ -403,9 +399,8 @@ def _build_path(self, url: str, path_params: dict | None) -> str: except KeyError as error: raise ValueError(f'no value given for placeholder {error} in url {url!r}') from error - # An encoded value can never contain a separator, so substitution must not - # change the shape of the route. Belt and braces against a future change to - # the encoding rules. + # A checked value cannot contain a separator, so substitution must not change + # the shape of the route. if path.count('/') != url.count('/'): raise ValueError(f'path parameter added a path segment to {url!r}') @@ -419,9 +414,8 @@ def _add_base_url(self, url: str) -> str: and the base url is 'https://api.verda.com/v1' then this method will return 'https://api.verda.com/v1/balance' - Acts as a backstop for the per-segment encoding done by the service modules: - a relative path segment that reached this point would be resolved away by - ``requests`` (or by the server), pointing the request at a different endpoint. + Backstop for a call site that built a path without ``path_params``. Re-asserts + the same allowlist on the finished path. Args: url: A relative url path. @@ -432,17 +426,20 @@ 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`, so a '?' or '#' here means a - # caller-supplied value was interpolated into the path without encoding. + # 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}') - # An empty segment collapses the request onto the collection endpoint, which - # turns a delete-one call into a delete-all call. + # 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}') + + # 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 has_relative_path_segment(url): + if _RELATIVE_SEGMENTS.intersection(url.split('/')): raise ValueError( f'refusing to send a request whose path would escape the API base path: {url!r}' ) diff --git a/verda/inference_client/_inference_client.py b/verda/inference_client/_inference_client.py index 802fff0..c458b15 100644 --- a/verda/inference_client/_inference_client.py +++ b/verda/inference_client/_inference_client.py @@ -149,6 +149,13 @@ def __init__( f'endpoint_base_url must not contain a relative path segment, ' f'got {endpoint_base_url!r}' ) + # The slicing is `rindex('/')`, which without a path finds the '//' of the + # scheme: 'https://host' yields base_domain 'https:/', making the async status + # url 'https://status/host' -- a different host, still carrying the key. + if not parsed_url.path.strip('/'): + raise InferenceClientError( + f'endpoint_base_url must include the deployment path, got {endpoint_base_url!r}' + ) self.inference_key = inference_key self.endpoint_base_url = endpoint_base_url.rstrip('/') @@ -205,11 +212,10 @@ def remove_global_header(self, key: str) -> None: def _build_url(self, path: str) -> str: """Construct the full URL by joining the base URL with the path. - ``path`` is a caller-chosen API path and may span several segments, so it is - not encoded 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. + ``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. Args: path: API path relative to the deployment's endpoint. From c1e27fe15d10cb8c9769fcb26c686211aaeade2d Mon Sep 17 00:00:00 2001 From: Alexey Shamrin Date: Mon, 10 Aug 2026 10:50:20 +0300 Subject: [PATCH 7/7] refactor: drop the now-unreachable segment-count guard A checked path value cannot contain a separator, so substitution can no longer change the shape of the route. The guard was reachable only by monkeypatching the encoder, which is all its test did. --- tests/unit_tests/http_client/test_http_client.py | 12 ------------ verda/http_client/_http_client.py | 5 ----- 2 files changed, 17 deletions(-) diff --git a/tests/unit_tests/http_client/test_http_client.py b/tests/unit_tests/http_client/test_http_client.py index 15608b5..cb80681 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -190,18 +190,6 @@ def test_types_that_are_not_identifiers_are_rejected(self, http_client, value): with pytest.raises(ValueError, match='path segment must be'): http_client._build_path('/instances/{id}', {'id': value}) - def test_segment_count_guard_catches_an_encoder_that_leaks_a_separator( - self, http_client, monkeypatch - ): - # The guard is unreachable while the encoder is correct, so drive it with a - # deliberately broken encoder. This tests the guard itself rather than - # re-testing urllib's quote(). - monkeypatch.setattr( - 'verda.http_client._http_client._encode_path_segment', lambda value: value - ) - with pytest.raises(ValueError, match='added a path segment'): - http_client._build_path('/container-deployments/{name}/status', {'name': 'a/b'}) - class TestHttpClient: def test_add_base_url(self, http_client): diff --git a/verda/http_client/_http_client.py b/verda/http_client/_http_client.py index f6b798b..4c9c71e 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -399,11 +399,6 @@ def _build_path(self, url: str, path_params: dict | None) -> str: except KeyError as error: raise ValueError(f'no value given for placeholder {error} in url {url!r}') from error - # A checked value cannot contain a separator, so substitution must not change - # the shape of the route. - if path.count('/') != url.count('/'): - raise ValueError(f'path parameter added a path segment to {url!r}') - return path def _add_base_url(self, url: str) -> str: