Repository navigation
Conversation
Path parameters declared with a non-default style were substituted as plain path segments, so style=label and style=matrix produced `abc` instead of `.abc` and `;id=abc`. Serialize label and matrix path parameters per the OpenAPI style table, including explode variants for arrays and objects, while keeping the default simple style unchanged. Values are still percent-encoded and the dot-segment traversal guard is applied per component.
Label-style structural dots can combine with empty components or template dots to form "." or ".." segments. httpx normalizes those away, so the request could silently escape the route or the base URL's path. Validate the fully substituted template after substitution and raise the existing dot-segment error instead of sending the request. Also serialize styled (label/matrix) booleans using the OpenAPI true/false convention, matching query parameter handling; simple style is unchanged.
|
This pull request is awaiting maintainer assignment. Linked PRs stay open while maintainers triage the issue; this check remains failing until assignment. Per CONTRIBUTING.md, an external PR must reference an issue that's assigned to its author. To get there:
Once you're assigned and the link is present, this check is re-run automatically. Previously gate-closed PRs also reopen — no further action needed. Maintainers: reopen this PR or remove the |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marvin-context-protocol
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edfaf67db6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # each other (or next to template dots) produce "." or "..". httpx | ||
| # normalizes those away, silently retargeting the request. Validate the | ||
| # fully substituted template to catch such compositions. | ||
| if any(part in {".", ".."} for part in url_path.split("/")): |
There was a problem hiding this comment.
When an empty label parameter is adjacent to a percent-encoded template dot (for example /api/{id}%2e/admin), substitution produces /api/.%2e/admin, which this raw split("/") comparison accepts. A backend that decodes before normalizing—such as SimpleHTTPRequestHandler, already used in the path-safety tests—interprets that segment as .., so one or more such segments can leave the declared route or base path despite the traversal guard; apply the same decoding/backslash-aware validation used for individual components to the completed path.
AGENTS.md reference: AGENTS.md:L186-L188
Useful? React with 👍 / 👎.
| Path parameters support ``simple`` (the default), ``label`` and | ||
| ``matrix`` styles. ``simple`` keeps its historical behavior: values are | ||
| stringified and encoded as a single segment. ``label`` and ``matrix`` | ||
| follow the OAS serialization table for scalars, arrays and objects, | ||
| including explode variants. |
There was a problem hiding this comment.
When specs declare a label or matrix array, this new branch can emit .a.b or ;id=a;id=b, but docs/integrations/openapi.mdx:469 still describes path arrays as comma-separated simple-style values and lines 478-479 call them “always comma-separated.” The newly supported public behavior therefore contradicts the live user contract; revise the Path/Array Parameters documentation to describe the style/explode outputs and the empty-label restrictions.
AGENTS.md reference: AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
|
Closing this one to reduce review volume, per maintainer feedback. The underlying issue remains open for triage; the branch and tests stay on the fork if it becomes useful later. |
Closes #5856
Description
OpenAPI path parameters declared with
style: labelorstyle: matrixwere substituted as plain path segments (/label/abc,/matrix/abcinstead of/label/.abc,/matrix/;id=abc), so endpoints relying on those serialization styles were unusable through the provider.simple(the default) was and remains correct.What changed
RequestDirector.build()now passes the route into_build_url(), which resolves each path parameter'sstyle/explodeand delegates to a new_serialize_path_parameter().labelandmatrix: scalars, arrays and objects, explode false/true.simpleoutput is byte-for-byte unchanged.quote(..., safe=""), with.kept as%2Ewhere it was before); structural delimiters (.,;,=) are emitted literally. Query, header, and cookie serialization are untouched.Contribution type
Bug fix verification
Test path:
tests/server/providers/openapi/test_path_parameter_styles.py(18 end-to-end tests viaOpenAPIProvider+httpx2.MockTransport).Red at
adf90b66(implementation reverted, final tests applied): 23 failed, 4 passed; the passes are the simple-style controls and traversal pins; all failures are assertion mismatches; no collection errors.Green at the fix commit: 1730 passed, 1 skipped across
tests/server/providers/openapi/ tests/utilities/.Validation
uv run --isolated pytest tests/server/providers/openapi/ tests/utilities/ -q-> 1730 passed, 1 skippeduv run --isolated ruff checkandruff format --checkon the changed files -> cleanuv run --isolated ty check-> All checks passedAI assistance
Tool(s) used: Claude Code
How you used it: investigated from a verified reproduction, implemented the style serialization and the regression tests.
Checklist
uv run prek run --all-filesand all checks pass (focused checks passed)