diff --git a/CHANGELOG.md b/CHANGELOG.md index 4561af6..cdd1a2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ 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 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. + ### Added - `LongTermService` with `get_cluster_periods()` and `get_instance_periods()` methods @@ -16,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **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 525f30d..bbabe2d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,6 +71,44 @@ 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 validates 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)`). + +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. + +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()`. + +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. + +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 ### Formatting and linting @@ -143,6 +181,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..cb80681 100644 --- a/tests/unit_tests/http_client/test_http_client.py +++ b/tests/unit_tests/http_client/test_http_client.py @@ -12,12 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re +import uuid 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 +29,168 @@ 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', + [ + '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_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', + ['x?force=true&', 'name#fragment', 'a b', 'a;b', 'ünicode', 'name@host', 'a+b'], +) +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', ['%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', ['.', '..']) +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) + + +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) + + +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', + ['a/b', 'x?force=true&', 'name#frag', 'a b', '..\\..\\x', '%2E%2E'], + ) + 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='path segment must'): + 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'): + 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_placeholder_missing_from_path_params_raises(self, http_client): + # '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_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'), + '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. + 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}) + + class TestHttpClient: def test_add_base_url(self, http_client): # arrange @@ -39,6 +204,72 @@ 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', + ], + ) + def test_add_base_url_rejects_relative_path_segments(self, http_client, path): + # 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) + + @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_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']) + 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'], + ) + 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', + [ + '/container-deployments/my-deployment', + '/container-registry-credentials/my-dockerhub-creds', + '/instance-availability/1H100.80S.22V', + '/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 +304,60 @@ 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_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': 'my-deployment'} + ) + + # assert + assert responses.calls[0].request.path_url == ( + '/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): + # 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 + + @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/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..dd24e13 --- /dev/null +++ b/tests/unit_tests/inference_client/test_inference_client.py @@ -0,0 +1,104 @@ +# 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', + '..', + # requests decodes percent-encoded unreserved characters before sending, + # so these reach the wire as real dot-segments. + '%2e%2e/%2e%2e/v1/victim', + '%2E%2E/other-deployment', + ], + ) + def test_paths_that_escape_the_deployment_are_rejected(self, inference_client, path): + # endpoint_base_url ends with this deployment's name, so a dot-segment walks + # the request onto a different deployment while still carrying the caller's + # inference key. + with pytest.raises(InferenceClientError, match='relative path segment'): + 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) + + @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 new file mode 100644 index 0000000..cdc074c --- /dev/null +++ b/tests/unit_tests/test_path_traversal.py @@ -0,0 +1,382 @@ +# 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 ast +import pathlib +import re + +import pytest +import responses + +import verda +from verda.clusters import ClustersService +from verda.containers import ( + ComputeResource, + Container, + ContainersService, + Deployment, + EnvVar, + EnvVarType, + QueueLoadScalingTrigger, + ScalingOptions, + ScalingPolicy, + ScalingTriggers, +) +from verda.instances import InstancesService +from verda.job_deployments import JobDeployment, JobDeploymentsService +from verda.ssh_keys import SSHKeysService +from verda.startup_scripts import StartupScriptsService +from verda.volumes import VolumesService + +BASE_PATH = '/v1' + +# 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 = [ + '.', + '..', + '', + None, + '../../v1/instances', + '../../../v1/instances', + '../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'.*') + +# 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 _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 _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. + + 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/', + ), + ( + '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), + ('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), + } + + 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. + # 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', 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 + + # 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}' + ) + + 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) + 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}' + assert path.endswith('force=false'), path diff --git a/verda/clusters/_clusters.py b/verda/clusters/_clusters.py index 26cb5e1..702a9de 100644 --- a/verda/clusters/_clusters.py +++ b/verda/clusters/_clusters.py @@ -23,6 +23,8 @@ from verda.http_client import HTTPClient CLUSTERS_ENDPOINT = '/clusters' +CLUSTER_AVAILABILITY_ENDPOINT = '/cluster-availability' +CLUSTER_IMAGES_ENDPOINT = '/images/cluster' # Default shared volume size is 30TB DEFAULT_SHARED_VOLUME_SIZE = 30000 @@ -141,7 +143,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( @@ -276,8 +280,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]: @@ -290,7 +297,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 @@ -307,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/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/helpers.py b/verda/helpers.py index a0ae6d7..b4981db 100644 --- a/verda/helpers.py +++ b/verda/helpers.py @@ -13,8 +13,72 @@ # 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-._~') + +# 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. + + 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``. + + 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. + + Returns: + 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(decoded.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 dabedc9..4c9c71e 100644 --- a/verda/http_client/_http_client.py +++ b/verda/http_client/_http_client.py @@ -13,18 +13,73 @@ # limitations under the License. import json +import re +from urllib.parse import quote +from uuid import UUID import requests from verda._version import __version__ from verda.exceptions import APIException +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: + """Check a caller-supplied value is safe as a single URL path segment. + + 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()``. 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 as exactly one path segment. + + Raises: + 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__}') + 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 _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='') 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) @@ -48,7 +103,13 @@ 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. @@ -56,30 +117,34 @@ 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 - - :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. """ - self._refresh_token_if_expired() - - url = self._add_base_url(url) - 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, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + *, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a PUT request. @@ -87,57 +152,65 @@ 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 - - :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. """ - self._refresh_token_if_expired() - - url = self._add_base_url(url) - headers = self._generate_headers() - - response = requests.put(url, json=json, headers=headers, params=params, **kwargs) - handle_error(response) - - return response - - def get(self, url: str, params: dict | None = None, **kwargs) -> requests.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: """Sends a GET request. A wrapper for the requests.get method. 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 + 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 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. """ - self._refresh_token_if_expired() - - url = self._add_base_url(url) - 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, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + *, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a PATCH request. @@ -145,30 +218,34 @@ 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 - - :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. """ - self._refresh_token_if_expired() - - url = self._add_base_url(url) - 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, **kwargs + self, + url: str, + json: dict | None = None, + params: dict | None = None, + *, + path_params: dict | None = None, + **kwargs, ) -> requests.Response: """Sends a DELETE request. @@ -176,24 +253,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 + 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. - :raises APIException: an api exception with message and error type code + 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``. - :return: Response object - :rtype: requests.Response + 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. """ - 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)) - url = self._add_base_url(url) - headers = self._generate_headers() + self._refresh_token_if_expired() - 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 @@ -201,9 +317,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 @@ -215,8 +333,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(), @@ -228,22 +346,61 @@ 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] 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 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. + + Example: + ``_build_path('/instances/{id}', {'id': 'abc'})`` returns ``'/instances/abc'`` + + 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: + 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()} + try: + path = url.format(**encoded) + except KeyError as error: + raise ValueError(f'no value given for placeholder {error} in url {url!r}') from error + + return path + def _add_base_url(self, url: str) -> str: """Adds the base url to the relative url. @@ -252,9 +409,34 @@ 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' - :param url: a relative url path - :type url: str - :return: the full url path - :rtype: str + 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. + + Returns: + The full url path. + + Raises: + ValueError: If the path could escape the API base path. """ + # Query data is passed separately as `params`. + if '?' in url or '#' in url: + raise ValueError(f'request path must not contain a query string or fragment: {url!r}') + + # Catches '%', which requests may decode into a separator or a dot, and '\', + # which some servers normalise to '/'. + if not _ALLOWED_IN_PATH.match(url): + raise ValueError(f'request path must not contain an unencoded value: {url!r}') + + # An empty segment collapses a delete-one call onto the collection endpoint. + if any(segment == '' for segment in url.split('/')[1:]): + raise ValueError(f'request path must not contain an empty path segment: {url!r}') + + if _RELATIVE_SEGMENTS.intersection(url.split('/')): + raise ValueError( + f'refusing to send a request whose path would escape the API base path: {url!r}' + ) + return self._base_url + url diff --git a/verda/inference_client/_inference_client.py b/verda/inference_client/_inference_client.py index c341ecd..c458b15 100644 --- a/verda/inference_client/_inference_client.py +++ b/verda/inference_client/_inference_client.py @@ -22,6 +22,8 @@ from dataclasses_json import Undefined, dataclass_json # type: ignore from requests.structures import CaseInsensitiveDict +from verda.helpers import has_relative_path_segment + class InferenceClientError(Exception): """Base exception for InferenceClient errors.""" @@ -139,6 +141,21 @@ 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}' + ) + # 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('/') @@ -193,8 +210,29 @@ 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`` 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. + + Returns: + The full request URL. + + Raises: + InferenceClientError: If the path contains a relative path segment. + """ + relative = path.lstrip('/') + if has_relative_path_segment(relative): + raise InferenceClientError( + f'path must not contain a relative path segment, got {path!r}' + ) + + return f'{self.endpoint_base_url}/{relative}' def _build_request_headers( self, request_headers: dict[str, str] | None = None diff --git a/verda/instances/_instances.py b/verda/instances/_instances.py index 6460a7d..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'] @@ -145,7 +146,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( @@ -296,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 @@ -313,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() 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: