diff --git a/src/mcp/shared/auth_utils.py b/src/mcp/shared/auth_utils.py index 3ba880f40d..80f6e252fc 100644 --- a/src/mcp/shared/auth_utils.py +++ b/src/mcp/shared/auth_utils.py @@ -1,7 +1,8 @@ """Utilities for OAuth 2.0 Resource Indicators (RFC 8707) and PKCE (RFC 7636).""" +import posixpath import time -from urllib.parse import urlparse, urlsplit, urlunsplit +from urllib.parse import unquote, urlparse, urlsplit, urlunsplit from pydantic import AnyUrl, HttpUrl @@ -28,6 +29,18 @@ def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str: return canonical +def _normalize_path(path: str) -> str: + """Percent-decode (single pass, per RFC 3986) and resolve "."/".." segments. + + Anchoring at "/" keeps ".." from escaping above root. "%2f" decodes to "/" + and is treated as a separator (conservative for an authorization check). + """ + decoded = unquote(path) + if not decoded: + return "/" + return posixpath.normpath("/" + decoded.lstrip("/")) + + def check_resource_allowed(requested_resource: str, configured_resource: str) -> bool: """Check if a requested resource URL matches a configured resource URL. @@ -51,10 +64,12 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) -> if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower(): return False + # Resolve dot-segments/encoding so "/api/../admin" can't pass as "/api". + requested_path = _normalize_path(requested.path) + configured_path = _normalize_path(configured.path) + # Normalize trailing slashes before comparison so that # "/foo" and "/foo/" are treated as equivalent. - requested_path = requested.path - configured_path = configured.path if not requested_path.endswith("/"): requested_path += "/" if not configured_path.endswith("/"): diff --git a/tests/shared/test_auth_utils.py b/tests/shared/test_auth_utils.py index 5ae0e22b0c..a478d6293f 100644 --- a/tests/shared/test_auth_utils.py +++ b/tests/shared/test_auth_utils.py @@ -121,3 +121,33 @@ def test_check_resource_allowed_empty_paths(): assert check_resource_allowed("https://example.com", "https://example.com") is True assert check_resource_allowed("https://example.com/", "https://example.com") is True assert check_resource_allowed("https://example.com/api", "https://example.com") is True + + +def test_check_resource_allowed_rejects_dot_segment_traversal(): + """Traversal like /api/../admin resolves to /admin and must not match.""" + assert check_resource_allowed("https://example.com/api/../admin", "https://example.com/api") is False + assert check_resource_allowed("https://example.com/api/../../etc", "https://example.com/api") is False + assert check_resource_allowed("https://example.com/api/./../admin", "https://example.com/api") is False + + +def test_check_resource_allowed_rejects_percent_encoded_traversal(): + """Percent-encoded dot-segments must be decoded before matching.""" + assert check_resource_allowed("https://example.com/api/%2e%2e/admin", "https://example.com/api") is False + assert check_resource_allowed("https://example.com/api/%2e%2e%2fadmin", "https://example.com/api") is False + + +def test_check_resource_allowed_allows_harmless_dot_segments(): + """Dot-segments that resolve back within the configured resource still match.""" + assert check_resource_allowed("https://example.com/api/v1/../v1/users", "https://example.com/api") is True + assert check_resource_allowed("https://example.com/api/./v1", "https://example.com/api") is True + + +def test_check_resource_allowed_decodes_a_single_pass(): + """Single-pass decode (RFC 3986): double-encoded "%252e%252e" stays literal.""" + assert check_resource_allowed("https://example.com/api/%252e%252e/x", "https://example.com/api") is True + + +def test_check_resource_allowed_normalizes_configured_path_encoding(): + """Both sides are normalized, so encoded and decoded paths compare equal.""" + assert check_resource_allowed("https://example.com/a b/x", "https://example.com/a%20b") is True + assert check_resource_allowed("https://example.com/a%20b/x", "https://example.com/a b") is True