Skip to content

Commit d247683

Browse files
committed
fix(teams): harden graph URL routing per review — fail-closed parse, keep colon paths
Address PR review feedback on the SSRF routing fix: - Gemini (robustness): wrap urlparse in try/except and fail closed (treat an unparseable URL, e.g. a bad IPv6 literal, as absolute so the allowlist rejects it) instead of letting urlparse raise out of the function. - Codex (P2, colon paths): keep routing on `scheme or netloc`. A bare `root:/…` segment never joined via urljoin anyway (urljoin sees scheme `root` and returns it unchanged), so rejecting it is the safe behavior — routing it to the relative branch would attach the Graph token to a malformed `root:` URL. Realistic Graph colon paths put the colon AFTER a slash (`me/drive/root:/x:/content`), parse with no scheme, and still join onto the trusted base — added a test proving it. Tests: +malformed-URL-fails-closed, +colon-after-slash-relative-path-joins (19 total). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3dd16f5 commit d247683

2 files changed

Lines changed: 37 additions & 6 deletions

File tree

src/chat_sdk/adapters/teams/graph/__init__.py

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,17 @@ async def call_teams_graph_api(path_or_url: str, options: TeamsGraphOptions) ->
262262
# Route on the PARSED form, not a case-sensitive ``startswith("http")`` prefix.
263263
# ``HTTPS://evil`` (mixed-case scheme) and ``//evil`` (scheme-relative) both slip
264264
# past a lowercase-prefix test, yet ``urljoin`` still resolves them to an absolute
265-
# attacker URL — so the routing diverged from ``is_trusted_graph_url`` and the
266-
# Graph token could attach to an untrusted host. Anything ``urlparse`` reads as
267-
# having a scheme or a netloc is treated as absolute and forced through the host
268-
# allowlist (fail closed); only a truly relative path is joined onto the base.
269-
parsed = urlparse(path_or_url)
270-
if parsed.scheme or parsed.netloc:
265+
# attacker URL — so the routing diverged from ``is_trusted_graph_url`` and the Graph
266+
# token could attach to an untrusted host. Anything ``urlparse`` reads as having a
267+
# scheme or a netloc is treated as absolute and forced through the host allowlist;
268+
# a malformed URL (``urlparse`` raises, e.g. a bad IPv6 literal) fails closed the
269+
# same way. Only a truly relative path is joined onto the trusted base.
270+
try:
271+
parsed = urlparse(path_or_url)
272+
is_absolute = bool(parsed.scheme or parsed.netloc)
273+
except (ValueError, TypeError):
274+
is_absolute = True # unparseable -> fail closed; the allowlist below rejects it
275+
if is_absolute:
271276
if not is_trusted_graph_url(path_or_url):
272277
raise ValueError(f"Refusing to call Microsoft Graph API on untrusted host: {path_or_url}")
273278
url = path_or_url

tests/test_teams_graph_primitive.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,32 @@ async def test_rejects_mixed_case_scheme_and_scheme_relative_attacker_urls(self)
326326
await paginate_teams_graph(hostile, _opts("p", fetch=request))
327327
request.assert_not_awaited()
328328

329+
@pytest.mark.asyncio
330+
async def test_malformed_url_fails_closed_without_fetching_a_token(self) -> None:
331+
# urlparse raises on some inputs (e.g. a bad IPv6 literal). The router must
332+
# fail closed — treat it as absolute and reject via the allowlist — never join
333+
# it or fetch a token for it.
334+
request = _fetch(_json_response(_TOKEN_BODY))
335+
with pytest.raises(ValueError, match="untrusted host"):
336+
await paginate_teams_graph("https://[oops", _opts("p", fetch=request))
337+
request.assert_not_awaited()
338+
339+
@pytest.mark.asyncio
340+
async def test_colon_in_relative_graph_path_still_joins_onto_the_trusted_base(self) -> None:
341+
# A relative Graph segment whose colon falls after a slash (e.g. a OneDrive
342+
# ``…/root:/path:/content`` address) parses with no scheme/netloc, so the
343+
# parse-based routing still joins it onto the trusted base — it is not
344+
# over-rejected as an absolute URL.
345+
request = _fetch(_json_response(_TOKEN_BODY), _json_response({"id": "x"}))
346+
result = await paginate_teams_graph(
347+
"me/drive/root:/Reports/jan.csv:/content", _opts("p", fetch=request)
348+
)
349+
assert _url(request.await_args_list[1]) == (
350+
"https://graph.microsoft.com/v1.0/me/drive/root:/Reports/jan.csv:/content"
351+
)
352+
assert _headers(request.await_args_list[1])["authorization"] == "Bearer graph-token"
353+
assert result == {"id": "x"}
354+
329355
def test_is_trusted_graph_url_allowlist(self) -> None:
330356
assert is_trusted_graph_url("https://graph.microsoft.com/v1.0/next") is True
331357
# Wrong scheme, lookalike suffix, foreign host, and parse junk all fail.

0 commit comments

Comments
 (0)