Repository navigation
Conversation
Reported-by: notlakshitha Github-Issue: tadata-org#354
There was a problem hiding this comment.
🟡 Changes recommended
Encoding / with safe="" silently breaks FastAPI {param:path} converter endpoints, a behavioral tradeoff that should be consciously confirmed before approval.
1 open finding
What changed in this PR
This PR fixes a security/correctness bug in FastApiMCP._execute_api_tool, where path parameter values were substituted into the operation URL as raw strings. Because tool arguments originate from an LLM, reserved characters like /, ?, #, and .. could truncate the path, inject query/fragment data, or normalize to a different route — including routes deliberately excluded via include_operations/exclude_operations (issue #354). The fix percent-encodes each path value before substitution, additionally escaping the exact ./.. dot-segments that quote() leaves untouched.
Changes:
- Percent-encode each path parameter value with
quote(..., safe="")before inserting it into the path template, and escape literal./..values to%2E/%2E%2Eto prevent HTTP-client dot-segment normalization. - Add a dedicated test module covering literal-data round-trips, excluded-route traversal attempts, and placeholder-collision protection.
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
I checked this through the default ASGI transport and added test_path_converter_preserves_nested_names with three cases. The escaped slash does not cause a 404 there: ASGI provides a percent-decoded scope['path'] for FastAPI to match, while scope['raw_path'] still contains %2F.
The test asserts both the encoded raw URL and the exact name received by /nested/{name:path}, including docs/readme.txt, a nested name with spaces/Unicode/query-fragment characters, and a literal %2F inside a nested name. The explicit query parameter and original arguments remain unchanged.
All 17 path-parameter cases pass, including the existing ordinary-segment traversal/excluded-route controls, and the complete Linux suite passes with 134 tests. The runtime code and escaping remain unchanged. A custom HTTP client or reverse proxy can impose its own encoded-slash policy; these tests establish the library's default in-process ASGI behavior.
Reference: ASGI HTTP connection scope.
Test commit: 573eeb5.
Describe your changes
Path parameters were substituted into operation URLs as raw strings. Values such as
x?q=injected,a#band../admincould be truncated or change the request's route;{version}inside a value could also be overwritten while substituting another parameter.Percent-encode each value before inserting it into the path. Escape the exact
.and..values too, sincequote(..., safe="")leaves dots unescaped and HTTPX otherwise normalizes those segments. There are no API, dependency, query/header/body handling or permission changes.The follow-up commit adds three compatibility cases for
/nested/{name:path}after the automated review raised a concern about escaped slashes. They verify both%2Fin the raw request path and the original nested name received by FastAPI. The default ASGI transport decodes the path before routing, so these cases work without changing the runtime fix. Reverse proxies and custom HTTP clients may impose their own encoded-slash policies.Issue ticket number and link
Fixes #354.
Testing
/adminendpoint. The three added catch-all cases preserve nested names, including literal%2Ftext.The existing Windows real-transport fixtures request unsupported
fork, causing 40 baseline errors; those were reproduced on unchanged main. Windows was not rerun for this test-only follow-up. Existing Linux test-harness warnings remain.Tests use synthetic data and local/in-memory transports, without credentials or paid API calls. URL escaping and the existing traversal controls remain intact.
Checklist before requesting a review
Prepared with AI assistance.
Upstream CI awaits maintainer workflow approval; no upstream jobs have executed. The complete Linux suite and quality checks above were run locally.