fix(auth): normalize URL paths before hierarchical resource check

src/mcp/shared/auth_utils.py

@@ -1,11 +1,38 @@
 """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
 
 
+def _normalize_url_path(path: str) -> str:
+    """Decode percent-encoding and resolve dot-segments in a URL path.
+
+    Used by check_resource_allowed to ensure hierarchical matching operates on
+    normalized paths. Without normalization, a path like "/api/../admin" would
+    satisfy startswith("/api") even though the resolved path is "/admin",
+    enabling a confused-deputy attack at downstream resource servers that DO
+    normalize paths.
+
+    - unquote() decodes percent-encoded segments so "%2e%2e" -> ".." cannot
+      bypass the normalization step.
+    - posixpath.normpath() resolves "." and ".." segments to canonical form.
+
+    Returns a path that ends with "/" if the original did, so trailing-slash
+    semantics are preserved.
+    """
+    decoded = unquote(path)
+    normalized = posixpath.normpath(decoded) if decoded else decoded
+    # posixpath.normpath collapses "//" to "/" and strips trailing slashes.
+    # Restore trailing slash only if the original path ended with one AND the
+    # normalized form is non-empty (avoid producing "//" for root paths).
+    if path.endswith("/") and normalized != "/" and not normalized.endswith("/"):
+        normalized += "/"
+    return normalized
+
+
 def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str:
     """Convert server URL to canonical resource URL per RFC 8707.
 
@@ -51,10 +78,17 @@ 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
 
+    # Normalize paths: decode percent-encoding and resolve dot-segments before
+    # comparison. This prevents bypass via "/api/../admin" (resolves to
+    # "/admin", not a child of "/api") and "/api/%2e%2e/admin" (URL-encoded
+    # equivalent). Without this, downstream resource servers that DO normalize
+    # paths would interpret the access as "/admin" while the auth check passed
+    # against the literal "/api/.." prefix.
+    requested_path = _normalize_url_path(requested.path)
+    configured_path = _normalize_url_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("/"):

tests/shared/test_auth_utils.py

@@ -121,3 +121,48 @@ 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_dot_segment_escape():
+    """Dot-segment traversal must not bypass hierarchical scope.
+
+    Without path normalization, "/api/../admin" satisfies startswith("/api/")
+    even though the resolved path is "/admin" — a confused-deputy risk at
+    downstream resource servers that DO normalize paths. After normalization,
+    "/api/../admin" resolves to "/admin" and correctly fails the check.
+    """
+    assert check_resource_allowed("https://example.com/api/../admin", "https://example.com/api") is False
+    assert check_resource_allowed("https://example.com/api/v1/../v2", "https://example.com/api/v1") is False
+    assert check_resource_allowed("https://example.com/api/v1/../../etc", "https://example.com/api/v1") is False
+
+
+def test_check_resource_allowed_percent_encoded_escape():
+    """URL-encoded dot-segments must not bypass scope check.
+
+    "%2e%2e" decodes to ".." — without unquote+normpath, the literal string
+    "/api/%2e%2e/admin" passes startswith("/api") while resolving to "/admin"
+    on any downstream server that URL-decodes (i.e., all of them).
+    """
+    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/admin", "https://example.com/api") is False
+    # Mixed-case + nested
+    assert check_resource_allowed("https://example.com/api/v1/%2e%2e/v2", "https://example.com/api/v1") is False
+
+
+def test_check_resource_allowed_double_slash_collapses():
+    """Double-slash sequences in paths must collapse before comparison.
+
+    Otherwise "/api//victim" could naively match "/api/victim2" due to literal
+    string-prefix semantics. Normalization collapses "//" to "/" so the
+    comparison happens on canonical paths.
+    """
+    # "/api//victim" normalizes to "/api/victim" — must NOT match "/api/victim2"
+    assert check_resource_allowed("https://example.com/api//victim", "https://example.com/api/victim2") is False
+
+
+def test_check_resource_allowed_legitimate_paths_still_match():
+    """Normalization must not break legitimate hierarchical matches."""
+    # Self-reference dot-segment normalizes to identity
+    assert check_resource_allowed("https://example.com/api/./v1", "https://example.com/api") is True
+    # Hierarchical descent still works
+    assert check_resource_allowed("https://example.com/api/v1/users", "https://example.com/api") is True