Repository navigation
Python: normalize OpenAPI allowlist paths before prefix match - #14668
Mangesh Raut (mangeshraut712) wants to merge 6 commits into
Conversation
GitHub Actions
Active
semantic-kernel-automation
Bot
added
the
python
There was a problem hiding this comment.
🟡 Changes recommended
Path-scope bypasses remain, and terminal dot segments lose trailing slashes required for correct allowlist matching.
3 open issues
What changed in this PR
This PR hardens Python OpenAPI URL allowlist validation against same-host path escapes, addressing issue #14662.
Changes:
- Normalizes request and allowlist paths before prefix matching.
- Decodes percent-encoded dots and resolves dot segments while preserving empty segments.
- Adds tests for traversal attempts, valid paths, and prefix boundaries.
| File | Description |
|---|---|
| python/tests/unit/connectors/openapi_plugin/test_server_url_validator.py | Adds rejection and acceptance regression cases. |
| python/semantic_kernel/connectors/openapi_plugin/server_url_validator.py | Adds path normalization to allowlist prefix matching. |
Review effort: Balanced
| if segment == ".": | ||
| continue | ||
| if segment == "..": | ||
| if resolved and resolved[-1] != "": |
| url_path = _normalize_path(url_path or "/") | ||
| base_path = _normalize_path(base_path or "/") |
| # Absolute paths keep a leading empty segment from the initial "/". | ||
| if not resolved: | ||
| return "/" | ||
| normalized = "/".join(resolved) |
GitHub Actions
Active
Decode %2e/%2E and apply RFC 3986 remove_dot_segments so allowed_base_urls cannot be escaped with .. or encoded dots. Matches httpx path normalization; empty segments preserved. Fixes microsoft#14662
7b423e9 to
42e3f4b
Compare
GitHub Actions
Active
|
Rebased onto latest main (CI re-running). Ready for review/merge when you have a chance — thanks. |
GitHub Actions
Active
|
Thanks for picking this up — confirming the core bug from #14662: a dot-segment path can escape an /api allowlist while the HTTP client sends a different path. Two gaps worth closing before merge: (1) Both the wire path and the decoded path should satisfy the allowlist. E.g. /admin/%2e%2e/api normalizes to /api and would be accepted, but on a server that does not decode, the wire path stays under /admin — checking the decoded path alone is fail-open in that direction, even though the original issue's fails-closed claim held in the decode direction. (2) Trailing-slash handling: /api/. and /api/users/.. normalize to bare /api, so an /api/ allowlist wrongly rejects them and an /api/. allowlist wrongly permits bare /api. Either preserve the trailing slash through normalization or define explicitly that /api and /api/ are treated the same. |
GitHub Actions
Active
…om/mangeshraut712/semantic-kernel into tip/sk-14662-review2
GitHub Actions
Active
Summary
%2e/%2Eand apply RFC 3986 remove_dot_segments beforeallowed_base_urlspath-prefix matching/api/../adminand/api/%2e%2e/adminmatched an/apiallowlist while httpx would request/admin//) preserved to match httpx; other percent-encodings left intactTests
..and%2e%2eescapes under an/apiallowlist/api/./users,/api/users,/api,/api//apiusers(shared prefix chars only)Fixes #14662