Skip to content

Commit ff1b50b

Browse files
committed
Resolve dot-segments when deriving and matching OAuth resource URLs
resource_url_from_server_url() now applies RFC 3986 remove_dot_segments (including the %2E spellings WHATWG treats as dots) so the resource identifier names the location the HTTP client actually requests. check_resource_allowed() resolves both paths the same way before its prefix comparison, and parses with urlsplit so ";parameters" stay part of the last path segment instead of being dropped. Fixes #3303
1 parent 0cee624 commit ff1b50b

3 files changed

Lines changed: 138 additions & 9 deletions

File tree

src/mcp/shared/auth_utils.py

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

33
import time
4-
from urllib.parse import urlparse, urlsplit, urlunsplit
4+
from urllib.parse import urlsplit, urlunsplit
55

66
from pydantic import AnyUrl, HttpUrl
77

8+
# WHATWG URL treats these percent-encoded spellings as dot-segments too.
9+
_SINGLE_DOT_SEGMENTS = {".", "%2e"}
10+
_DOUBLE_DOT_SEGMENTS = {"..", ".%2e", "%2e.", "%2e%2e"}
11+
12+
13+
def _remove_dot_segments(path: str) -> str:
14+
"""Resolve "." and ".." segments in a URL path (RFC 3986 section 5.2.4)."""
15+
segments = path.split("/")
16+
output: list[str] = []
17+
for index, segment in enumerate(segments):
18+
is_last = index == len(segments) - 1
19+
kind = segment.lower()
20+
if kind in _DOUBLE_DOT_SEGMENTS:
21+
if len(output) > 1:
22+
output.pop()
23+
if is_last:
24+
output.append("")
25+
elif kind in _SINGLE_DOT_SEGMENTS:
26+
if is_last:
27+
output.append("")
28+
else:
29+
output.append(segment)
30+
return "/".join(output)
31+
832

933
def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str:
1034
"""Convert server URL to canonical resource URL per RFC 8707.
1135
1236
RFC 8707 section 2 states that resource URIs "MUST NOT include a fragment component".
13-
Returns absolute URI with lowercase scheme/host for canonical form.
37+
Returns absolute URI with lowercase scheme/host and dot-segments resolved, so the
38+
resource identifies the same location an HTTP client would actually request.
1439
1540
Args:
1641
url: Server URL to convert
@@ -23,7 +48,14 @@ def resource_url_from_server_url(url: str | HttpUrl | AnyUrl) -> str:
2348

2449
# Parse the URL and remove fragment, create canonical form
2550
parsed = urlsplit(url_str)
26-
canonical = urlunsplit(parsed._replace(scheme=parsed.scheme.lower(), netloc=parsed.netloc.lower(), fragment=""))
51+
canonical = urlunsplit(
52+
parsed._replace(
53+
scheme=parsed.scheme.lower(),
54+
netloc=parsed.netloc.lower(),
55+
path=_remove_dot_segments(parsed.path),
56+
fragment="",
57+
)
58+
)
2759

2860
return canonical
2961

@@ -34,7 +66,8 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) ->
3466
A requested resource matches if it has the same scheme, domain, port,
3567
and its path starts with the configured resource's path. This allows
3668
hierarchical matching where a token for a parent resource can be used
37-
for child resources.
69+
for child resources. Dot-segments in either path are resolved before
70+
comparing.
3871
3972
Args:
4073
requested_resource: The resource URL being requested
@@ -44,17 +77,17 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) ->
4477
True if the requested resource matches the configured resource
4578
"""
4679
# Parse both URLs
47-
requested = urlparse(requested_resource)
48-
configured = urlparse(configured_resource)
80+
requested = urlsplit(requested_resource)
81+
configured = urlsplit(configured_resource)
4982

5083
# Compare scheme, host, and port (origin)
5184
if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower():
5285
return False
5386

5487
# Normalize trailing slashes before comparison so that
5588
# "/foo" and "/foo/" are treated as equivalent.
56-
requested_path = requested.path
57-
configured_path = configured.path
89+
requested_path = _remove_dot_segments(requested.path)
90+
configured_path = _remove_dot_segments(configured.path)
5891
if not requested_path.endswith("/"):
5992
requested_path += "/"
6093
if not configured_path.endswith("/"):

tests/client/test_auth.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,36 @@ async def test_validate_resource_rejects_mismatched_resource(
862862
await provider._validate_resource_match(prm)
863863

864864

865+
@pytest.mark.anyio
866+
async def test_validate_resource_rejects_sibling_path_reached_via_dot_segments(
867+
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage
868+
) -> None:
869+
"""A `server_url` whose dot-segments resolve to `/m/mcp` rejects a PRM `resource` of `/victim/mcp`.
870+
871+
SDK-defined: the resource identifier is derived from the location the HTTP client actually
872+
requests (RFC 3986 section 5.2.4), so a same-origin sibling path is neither accepted during
873+
discovery nor adopted as the RFC 8707 `resource` parameter.
874+
"""
875+
provider = OAuthClientProvider(
876+
server_url="https://shared.example.com/victim/mcp/../../m/mcp",
877+
client_metadata=client_metadata,
878+
storage=mock_storage,
879+
)
880+
provider._initialized = True
881+
882+
prm = ProtectedResourceMetadata(
883+
resource=AnyHttpUrl("https://shared.example.com/victim/mcp"),
884+
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
885+
)
886+
with pytest.raises(OAuthFlowError) as exc_info:
887+
await provider._validate_resource_match(prm)
888+
assert str(exc_info.value) == snapshot(
889+
"Protected resource https://shared.example.com/victim/mcp does not match expected https://shared.example.com/m/mcp"
890+
)
891+
provider.context.protected_resource_metadata = prm
892+
assert provider.context.get_resource_url() == snapshot("https://shared.example.com/m/mcp")
893+
894+
865895
@pytest.mark.anyio
866896
async def test_validate_resource_accepts_matching_resource(
867897
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage

tests/shared/test_auth_utils.py

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
"""Tests for OAuth 2.0 Resource Indicators utilities."""
22

3-
from pydantic import HttpUrl
3+
import itertools
4+
5+
import pytest
6+
from pydantic import AnyHttpUrl, HttpUrl
47

58
from mcp.shared.auth_utils import check_resource_allowed, resource_url_from_server_url
69

@@ -46,6 +49,42 @@ def test_resource_url_from_server_url_handles_pydantic_urls():
4649
assert resource_url_from_server_url(url) == "https://example.com/path"
4750

4851

52+
@pytest.mark.parametrize(
53+
("server_url", "expected"),
54+
[
55+
("https://example.com/api/../admin", "https://example.com/admin"),
56+
("https://example.com/api/%2E%2e/admin", "https://example.com/admin"),
57+
("https://example.com/api/.%2e/admin", "https://example.com/admin"),
58+
("https://example.com/api/./v1", "https://example.com/api/v1"),
59+
("https://example.com/api/v1/..", "https://example.com/api/"),
60+
("https://example.com/api/v1/.", "https://example.com/api/v1/"),
61+
("https://example.com/../admin", "https://example.com/admin"),
62+
("https://example.com/a//b", "https://example.com/a//b"),
63+
("https://example.com/a%2Fb/c", "https://example.com/a%2Fb/c"),
64+
],
65+
)
66+
def test_resource_url_from_server_url_resolves_dot_segments(server_url: str, expected: str):
67+
"""Dot-segments (including `%2E` spellings) are resolved per RFC 3986 section 5.2.4.
68+
69+
Empty segments and encoded slashes are not path separators and stay as written.
70+
"""
71+
assert resource_url_from_server_url(server_url) == expected
72+
73+
74+
def test_resource_url_from_server_url_path_matches_whatwg_resolution_for_literal_dot_segments():
75+
"""Every combination of literal `.`, `..`, empty and plain segments resolves as pydantic's WHATWG parser does.
76+
77+
The PRM `resource` side is parsed by `AnyHttpUrl`, so both operands of `check_resource_allowed`
78+
must agree on dot-segment resolution for the comparison to be meaningful.
79+
"""
80+
atoms = ["", ".", "..", "a", "b.", "..."]
81+
for count in range(1, 5):
82+
for segments in itertools.product(atoms, repeat=count):
83+
path = "/" + "/".join(segments)
84+
expected = AnyHttpUrl(f"https://example.com{path}").path
85+
assert resource_url_from_server_url(f"https://example.com{path}") == f"https://example.com{expected}"
86+
87+
4988
# Tests for check_resource_allowed function
5089

5190

@@ -121,3 +160,30 @@ def test_check_resource_allowed_empty_paths():
121160
assert check_resource_allowed("https://example.com", "https://example.com") is True
122161
assert check_resource_allowed("https://example.com/", "https://example.com") is True
123162
assert check_resource_allowed("https://example.com/api", "https://example.com") is True
163+
164+
165+
@pytest.mark.parametrize(
166+
"requested",
167+
[
168+
"https://example.com/api/../admin",
169+
"https://example.com/api/%2e%2e/admin",
170+
"https://example.com/api/v1/../../admin",
171+
"https://example.com/api/..",
172+
],
173+
)
174+
def test_check_resource_allowed_rejects_dot_segments_escaping_configured_path(requested: str):
175+
"""A requested path that resolves outside the configured path is not a hierarchical match."""
176+
assert check_resource_allowed(requested, "https://example.com/api") is False
177+
178+
179+
def test_check_resource_allowed_resolves_dot_segments_on_both_sides():
180+
"""Both URLs are compared in resolved form, so equivalent spellings agree (SDK-defined matching)."""
181+
assert check_resource_allowed("https://example.com/api/./v1", "https://example.com/api") is True
182+
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/other/../api") is True
183+
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api/v1/../v2") is False
184+
185+
186+
def test_check_resource_allowed_keeps_encoded_slash_and_params_in_segment():
187+
"""`%2F` and `;params` are part of a segment (RFC 3986 sections 2.2, 3.3), not a boundary."""
188+
assert check_resource_allowed("https://example.com/api%2Fv1", "https://example.com/api") is False
189+
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api;x") is False

0 commit comments

Comments
 (0)