diff --git a/src/chat_sdk/adapters/teams/graph/__init__.py b/src/chat_sdk/adapters/teams/graph/__init__.py index 75d6085..c850fa4 100644 --- a/src/chat_sdk/adapters/teams/graph/__init__.py +++ b/src/chat_sdk/adapters/teams/graph/__init__.py @@ -245,11 +245,11 @@ async def resolve_graph_access_token(options: TeamsGraphOptions) -> str: async def call_teams_graph_api(path_or_url: str, options: TeamsGraphOptions) -> Any: """Call a Microsoft Graph endpoint with a Graph-scoped bearer token. - Port of upstream ``callTeamsGraphApi``. An absolute ``path_or_url`` (one - starting with ``http``) is used as-is; otherwise it is joined onto the base - Graph URL (``options.graph_url`` or ``https://graph.microsoft.com/v1.0/``) - after stripping leading slashes. Raises :class:`TeamsApiError` for non-2xx - responses. + Port of upstream ``callTeamsGraphApi``. An absolute ``path_or_url`` (one that + :func:`urllib.parse.urlparse` reads as having a scheme or a host) is used + as-is; otherwise it is joined onto the base Graph URL (``options.graph_url`` + or ``https://graph.microsoft.com/v1.0/``) after stripping leading slashes. + Raises :class:`TeamsApiError` for non-2xx responses. Python-specific hardening: an absolute ``path_or_url`` is validated through :func:`is_trusted_graph_url` **before** the token is resolved or attached, @@ -259,7 +259,20 @@ async def call_teams_graph_api(path_or_url: str, options: TeamsGraphOptions) -> trusted base URL, so they need no host check. Upstream performs no such check. """ - if path_or_url.startswith("http"): + # Route on the PARSED form, not a case-sensitive ``startswith("http")`` prefix. + # ``HTTPS://evil`` (mixed-case scheme) and ``//evil`` (scheme-relative) both slip + # past a lowercase-prefix test, yet ``urljoin`` still resolves them to an absolute + # attacker URL — so the routing diverged from ``is_trusted_graph_url`` and the Graph + # token could attach to an untrusted host. Anything ``urlparse`` reads as having a + # scheme or a netloc is treated as absolute and forced through the host allowlist; + # a malformed URL (``urlparse`` raises, e.g. a bad IPv6 literal) fails closed the + # same way. Only a truly relative path is joined onto the trusted base. + try: + parsed = urlparse(path_or_url) + is_absolute = bool(parsed.scheme or parsed.netloc) + except (ValueError, TypeError): + is_absolute = True # unparseable -> fail closed; the allowlist below rejects it + if is_absolute: if not is_trusted_graph_url(path_or_url): raise ValueError(f"Refusing to call Microsoft Graph API on untrusted host: {path_or_url}") url = path_or_url diff --git a/tests/test_teams_graph_primitive.py b/tests/test_teams_graph_primitive.py index 7965d50..5ab0cab 100644 --- a/tests/test_teams_graph_primitive.py +++ b/tests/test_teams_graph_primitive.py @@ -309,6 +309,47 @@ async def test_call_with_an_absolute_attacker_url_is_rejected(self) -> None: await paginate_teams_graph("http://graph.microsoft.com/v1.0/next", _opts("p", fetch=request)) request.assert_not_awaited() + @pytest.mark.asyncio + async def test_rejects_mixed_case_scheme_and_scheme_relative_attacker_urls(self) -> None: + # Regression: a case-sensitive ``startswith("http")`` routing test let a + # mixed-case scheme (``HTTPS://``) or a scheme-relative (``//host``) URL + # skip the absolute branch; ``urljoin`` still resolved it to the attacker + # host and attached the Graph token. Routing now keys off the parsed + # scheme/netloc, so every absolute form is forced through the allowlist. + for hostile in ( + "HTTPS://evil.example/x", + "HtTpS://evil.example/x", + "//evil.example/x", + ): + request = _fetch(_json_response(_TOKEN_BODY)) + with pytest.raises(ValueError, match="untrusted host"): + await paginate_teams_graph(hostile, _opts("p", fetch=request)) + request.assert_not_awaited() + + @pytest.mark.asyncio + async def test_malformed_url_fails_closed_without_fetching_a_token(self) -> None: + # urlparse raises on some inputs (e.g. a bad IPv6 literal). The router must + # fail closed — treat it as absolute and reject via the allowlist — never join + # it or fetch a token for it. + request = _fetch(_json_response(_TOKEN_BODY)) + with pytest.raises(ValueError, match="untrusted host"): + await paginate_teams_graph("https://[oops", _opts("p", fetch=request)) + request.assert_not_awaited() + + @pytest.mark.asyncio + async def test_colon_in_relative_graph_path_still_joins_onto_the_trusted_base(self) -> None: + # A relative Graph segment whose colon falls after a slash (e.g. a OneDrive + # ``…/root:/path:/content`` address) parses with no scheme/netloc, so the + # parse-based routing still joins it onto the trusted base — it is not + # over-rejected as an absolute URL. + request = _fetch(_json_response(_TOKEN_BODY), _json_response({"id": "x"})) + result = await paginate_teams_graph("me/drive/root:/Reports/jan.csv:/content", _opts("p", fetch=request)) + assert _url(request.await_args_list[1]) == ( + "https://graph.microsoft.com/v1.0/me/drive/root:/Reports/jan.csv:/content" + ) + assert _headers(request.await_args_list[1])["authorization"] == "Bearer graph-token" + assert result == {"id": "x"} + def test_is_trusted_graph_url_allowlist(self) -> None: assert is_trusted_graph_url("https://graph.microsoft.com/v1.0/next") is True # Wrong scheme, lookalike suffix, foreign host, and parse junk all fail.