Skip to content

Commit b01895a

Browse files
committed
fix: resolve dot-segments and percent-encoding in check_resource_allowed
check_resource_allowed compared raw URL paths with str.startswith, so a requested resource such as https://host/api/../admin (which resolves to /admin) satisfied the check for a token scoped to /api, bypassing the resource-authorization boundary. Percent-encoded variants (%2e%2e) slipped through the same way. Normalize both the requested and configured paths (single-pass percent-decode per RFC 3986, then posixpath.normpath anchored at /) before the existing origin and hierarchical-prefix comparison. Legitimate child resources still match; traversal that escapes the configured path no longer does. Fixes #3303
1 parent 6affe5c commit b01895a

2 files changed

Lines changed: 48 additions & 3 deletions

File tree

src/mcp/shared/auth_utils.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
"""Utilities for OAuth 2.0 Resource Indicators (RFC 8707) and PKCE (RFC 7636)."""
22

3+
import posixpath
34
import time
4-
from urllib.parse import urlparse, urlsplit, urlunsplit
5+
from urllib.parse import unquote, urlparse, urlsplit, urlunsplit
56

67
from pydantic import AnyUrl, HttpUrl
78

@@ -28,6 +29,18 @@ def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str:
2829
return canonical
2930

3031

32+
def _normalize_path(path: str) -> str:
33+
"""Percent-decode (single pass, per RFC 3986) and resolve "."/".." segments.
34+
35+
Anchoring at "/" keeps ".." from escaping above root. "%2f" decodes to "/"
36+
and is treated as a separator (conservative for an authorization check).
37+
"""
38+
decoded = unquote(path)
39+
if not decoded:
40+
return "/"
41+
return posixpath.normpath("/" + decoded.lstrip("/"))
42+
43+
3144
def check_resource_allowed(requested_resource: str, configured_resource: str) -> bool:
3245
"""Check if a requested resource URL matches a configured resource URL.
3346
@@ -51,10 +64,12 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) ->
5164
if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower():
5265
return False
5366

67+
# Resolve dot-segments/encoding so "/api/../admin" can't pass as "/api".
68+
requested_path = _normalize_path(requested.path)
69+
configured_path = _normalize_path(configured.path)
70+
5471
# Normalize trailing slashes before comparison so that
5572
# "/foo" and "/foo/" are treated as equivalent.
56-
requested_path = requested.path
57-
configured_path = configured.path
5873
if not requested_path.endswith("/"):
5974
requested_path += "/"
6075
if not configured_path.endswith("/"):

tests/shared/test_auth_utils.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,3 +121,33 @@ def test_check_resource_allowed_empty_paths():
121121
assert check_resource_allowed("https://example.com", "https://example.com") is True
122122
assert check_resource_allowed("https://example.com/", "https://example.com") is True
123123
assert check_resource_allowed("https://example.com/api", "https://example.com") is True
124+
125+
126+
def test_check_resource_allowed_rejects_dot_segment_traversal():
127+
"""Traversal like /api/../admin resolves to /admin and must not match."""
128+
assert check_resource_allowed("https://example.com/api/../admin", "https://example.com/api") is False
129+
assert check_resource_allowed("https://example.com/api/../../etc", "https://example.com/api") is False
130+
assert check_resource_allowed("https://example.com/api/./../admin", "https://example.com/api") is False
131+
132+
133+
def test_check_resource_allowed_rejects_percent_encoded_traversal():
134+
"""Percent-encoded dot-segments must be decoded before matching."""
135+
assert check_resource_allowed("https://example.com/api/%2e%2e/admin", "https://example.com/api") is False
136+
assert check_resource_allowed("https://example.com/api/%2e%2e%2fadmin", "https://example.com/api") is False
137+
138+
139+
def test_check_resource_allowed_allows_harmless_dot_segments():
140+
"""Dot-segments that resolve back within the configured resource still match."""
141+
assert check_resource_allowed("https://example.com/api/v1/../v1/users", "https://example.com/api") is True
142+
assert check_resource_allowed("https://example.com/api/./v1", "https://example.com/api") is True
143+
144+
145+
def test_check_resource_allowed_decodes_a_single_pass():
146+
"""Single-pass decode (RFC 3986): double-encoded "%252e%252e" stays literal."""
147+
assert check_resource_allowed("https://example.com/api/%252e%252e/x", "https://example.com/api") is True
148+
149+
150+
def test_check_resource_allowed_normalizes_configured_path_encoding():
151+
"""Both sides are normalized, so encoded and decoded paths compare equal."""
152+
assert check_resource_allowed("https://example.com/a b/x", "https://example.com/a%20b") is True
153+
assert check_resource_allowed("https://example.com/a%20b/x", "https://example.com/a b") is True

0 commit comments

Comments
 (0)