security: fix path traversal and query injection in resource names - #96
Merged
Conversation
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.
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.
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.
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.
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.
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.
shamrin
requested changes
Aug 10, 2026
shamrin
left a comment
shamrin
left a comment
Contributor
There was a problem hiding this comment.
The current design for the fix seems to be: "accept almost any character, encode it, then model what downstream systems will decode." I think the PR is trying to do too much. In fact I've pushed the change to fix it. Please review.
shamrin
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resource names and IDs are no longer interpolated into request paths.
containers.delete_deployment('../../v1/instances')issuedDELETE /v1/instancesunder the SDK's own credentials; a?in a name could inject query parameters, e.g. overriding theforceflag ofdelete_secret.HTTPClient.get/post/put/patch/deletenow takepath_paramsmapping, and every service module passes names and IDs that way. Values are validated as a single path segment.Breaking changes
[A-Za-z0-9._~-];/,\,%, spaces,?and#now raiseValueError..,.., empty,None, and any type other than str/int/UUID raise.InferenceClientrequiresendpoint_base_urlto include the deployment path. Without one,base_domainbecamehttps:/and async status/result requests went to a host named status, still carrying the inference key..