diff --git a/src/mcp/shared/auth_utils.py b/src/mcp/shared/auth_utils.py index 3ba880f40d..bfaa0ae1f0 100644 --- a/src/mcp/shared/auth_utils.py +++ b/src/mcp/shared/auth_utils.py @@ -1,11 +1,47 @@ """Utilities for OAuth 2.0 Resource Indicators (RFC 8707) and PKCE (RFC 7636).""" +import re import time from urllib.parse import urlparse, urlsplit, urlunsplit from pydantic import AnyUrl, HttpUrl +def _normalize_resource_path(path: str) -> str: + """Resolve dot-segments without decoding encoded path separators.""" + # RFC 3986 treats percent-encoded unreserved characters as equivalent. Decode + # only encoded dots here: unquoting the whole path would turn %2F into a + # separator and change the resource hierarchy being authorized. + path = re.sub(r"%2e", ".", path, flags=re.IGNORECASE) + + # Remove RFC 3986 dot-segments without collapsing empty segments. Using + # posixpath.normpath() here would turn /api//v1 into /api/v1 and could + # widen a resource boundary that intentionally contains a repeated slash. + output: list[str] = [] + while path: + if path.startswith("/./"): + path = "/" + path[3:] + elif path == "/.": + path = "/" + elif path.startswith("/../"): + path = "/" + path[4:] + if output: + output.pop() + elif path == "/..": + path = "/" + if output: + output.pop() + else: + prefix = "/" if path.startswith("/") else "" + segment = path[len(prefix) :] + segment, separator, remainder = segment.partition("/") + output.append(prefix + segment) + path = ("/" if separator else "") + remainder + + path = "".join(output) + return path + + def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str: """Convert server URL to canonical resource URL per RFC 8707. @@ -51,10 +87,14 @@ 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 before normalizing trailing slashes so that a + # resource cannot escape its configured path through ../ or its encoded + # equivalent. + requested_path = _normalize_resource_path(requested.path) + configured_path = _normalize_resource_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..1a0f85d556 100644 --- a/tests/shared/test_auth_utils.py +++ b/tests/shared/test_auth_utils.py @@ -121,3 +121,29 @@ 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_resolves_dot_segments(): + """Dot-segments should be resolved before checking the resource hierarchy.""" + assert check_resource_allowed("https://example.com/api/./v1", "https://example.com/api") is True + assert check_resource_allowed("https://example.com/api/.", "https://example.com/api") is True + assert check_resource_allowed("https://example.com/api/../admin", "https://example.com/api") is False + assert check_resource_allowed("https://example.com/../admin", "https://example.com/api") is False + assert check_resource_allowed("https://example.com/api/..", "https://example.com") is True + assert check_resource_allowed("https://example.com/..", "https://example.com") is True + + +def test_check_resource_allowed_resolves_percent_encoded_dot_segments(): + """Percent-encoded dot-segments should not bypass the resource boundary.""" + assert check_resource_allowed("https://example.com/api/%2e/v1", "https://example.com/api") is True + assert check_resource_allowed("https://example.com/api/%2e%2e/admin", "https://example.com/api") is False + + +def test_check_resource_allowed_preserves_encoded_path_separators(): + """Encoded separators should not be decoded into path hierarchy.""" + assert check_resource_allowed("https://example.com/api/a%2Fb", "https://example.com/api/a/b") is False + + +def test_check_resource_allowed_preserves_repeated_path_separators(): + """Repeated separators should remain significant for resource boundaries.""" + assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api//") is False