feat(teams): graph primitive subpath + SSRF allowlist divergence (chat@4.31 8c71411)#160
Conversation
…t@4.31 8c71411) Port the Microsoft Graph primitives subpath (NEW in chat@4.31.0) for the Teams adapter as a runtime-free, SDK-free package mirroring slack/api and teams/api: call_teams_graph_api (Graph .default scope), paginate_teams_graph (follows @odata.nextLink as-is), get_teams_channel, list chat/channel messages + replies, get_teams_channel_message, and the ordered extract_text_from_graph_message HTML->text pass (entity-decode & last). Reuses the Round-1 api subpath (resolve_teams_access_token, read_response_body, TeamsApiError) via the allowed cross-subpath import. Injectable async fetch (AsyncMock in tests) defaults to the api subpath's lazy-httpx transport. Python-first divergence: gate an absolute path_or_url and every followed @odata.nextLink to the graph.microsoft.com host BEFORE attaching the Bearer token (SSRF / token-leak guard), raising ValueError for any other host. Upstream attaches the token to whatever URL it is handed. Ports all 7 graph/index.test.ts it-blocks + the boundary source-scan, plus direct text-extraction coverage, the nextLink-host SSRF rejection test, and a no-eager-httpx import check.
|
Warning Review limit reached
More reviews will be available in 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Microsoft Graph primitives for Teams as a runtime-free subpath, providing fetch-based utilities for calling the Graph REST API, handling pagination, and extracting plain text from HTML messages, along with comprehensive unit tests. The review feedback highlights a critical security vulnerability in call_teams_graph_api where a case-sensitive scheme check could allow SSRF and token leakage via uppercase schemes (e.g., HTTPS://). Additionally, a defensive programming improvement is suggested in to_graph_message to explicitly check if the user object is a Mapping to prevent potential runtime crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if path_or_url.startswith("http"): | ||
| 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 | ||
| else: | ||
| url = urljoin( | ||
| options.graph_url if options.graph_url is not None else _DEFAULT_GRAPH_URL, | ||
| _LEADING_SLASH_PATTERN.sub("", path_or_url), | ||
| ) |
There was a problem hiding this comment.
Security Vulnerability: SSRF and Token Leakage via Case-Sensitivity Bypass
The current implementation of call_teams_graph_api checks if path_or_url starts with "http" using a case-sensitive check. If an attacker passes an absolute URL with an uppercase scheme (e.g., "HTTPS://evil.com/v1.0/next"), the check path_or_url.startswith("http") evaluates to False.
As a result, the code falls into the else block and resolves the URL using urljoin. Since urljoin handles uppercase schemes correctly, it returns the absolute URL "HTTPS://evil.com/v1.0/next" unchanged. Because the validation is_trusted_graph_url is only performed inside the if block, the untrusted URL is never validated, and the Graph-scoped bearer token is attached and sent to the untrusted host.
To fix this, normalize the scheme check to be case-insensitive, and always validate the final resolved url using is_trusted_graph_url before proceeding.
| if path_or_url.startswith("http"): | |
| 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 | |
| else: | |
| url = urljoin( | |
| options.graph_url if options.graph_url is not None else _DEFAULT_GRAPH_URL, | |
| _LEADING_SLASH_PATTERN.sub("", path_or_url), | |
| ) | |
| if path_or_url.lower().startswith(("http://", "https://")): | |
| url = path_or_url | |
| else: | |
| url = urljoin( | |
| options.graph_url if options.graph_url is not None else _DEFAULT_GRAPH_URL, | |
| _LEADING_SLASH_PATTERN.sub("", path_or_url), | |
| ) | |
| if not is_trusted_graph_url(url): | |
| raise ValueError(f"Refusing to call Microsoft Graph API on untrusted host: {url}") |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
| text=extract_text_from_graph_message(message), | ||
| raw=raw, | ||
| created_at=created_at if created_at else None, | ||
| from_=_to_graph_user(user) if user else None, |
There was a problem hiding this comment.
Defensive Programming: Type Guard for user Mapping
In to_graph_message, user is extracted from from_block.get("user"). While it is expected to be a dictionary/mapping, if the API returns a non-mapping truthy value (e.g., a string or list), the truthiness check if user will pass, but _to_graph_user(user) will raise an AttributeError when attempting to call .get() on it.
To prevent potential runtime crashes, use an explicit isinstance(user, Mapping) check instead of a simple truthiness check.
| from_=_to_graph_user(user) if user else None, | |
| from_=_to_graph_user(user) if isinstance(user, Mapping) else None, |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| """ | ||
|
|
||
| def test_graph_source_does_not_import_the_adapter_sdk_or_runtime(self) -> None: | ||
| import chat_sdk.adapters.teams.graph as graph_pkg |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0c26e97e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # which permits the ``.graph.microsoft.com`` suffix and the exact | ||
| # ``graph.microsoft.com`` host); a Graph *API* call only ever targets the bare | ||
| # ``graph.microsoft.com`` host, so we pin to it exactly. | ||
| _TRUSTED_GRAPH_HOSTS = frozenset({"graph.microsoft.com"}) |
There was a problem hiding this comment.
When a caller uses TeamsGraphOptions.graph_url for a Microsoft Graph national cloud such as https://graph.microsoft.us/v1.0/, the list helpers can fetch the first page because they join relative paths to that base, but Graph returns an absolute @odata.nextLink on the same host and paginate_teams_graph rejects it here. This makes paged results unusable in supported sovereign Teams/Graph deployments; the trust check should accept the configured Graph host or the supported Microsoft Graph cloud endpoints rather than only graph.microsoft.com.
Useful? React with 👍 / 👎.
| url = path_or_url | ||
| else: | ||
| url = urljoin( | ||
| options.graph_url if options.graph_url is not None else _DEFAULT_GRAPH_URL, |
There was a problem hiding this comment.
If options.graph_url is set, this branch joins every relative helper path onto that URL without running is_trusted_graph_url, then resolves and attaches the bearer token. In deployments where the base URL is tenant/config driven, a typo or malicious value like https://evil.example/v1.0/ makes list_teams_chat_messages send Authorization: Bearer ... to that host, bypassing the token-leak guard that only covers absolute path_or_url/nextLink. Validate the resolved URL or configured base before resolving the token.
Useful? React with 👍 / 👎.
1 participant
Summary
Ports the Microsoft Graph primitives subpath for Teams — NEW in chat@4.31.0 (
packages/adapter-teams/src/graph/, commit8c71411), exposed upstream as@chat-adapter/teams/graph. This is Wave-B Round-2 of the 4.31 Teams sync.New
src/chat_sdk/adapters/teams/graph/subpackage (single-file__init__.py, mirroring theslack/apiand Round-1teams/apilayout template). SDK-free: plain dicts + lazyhttpx/stdlib, nomicrosoft_teamsimports.Reuses the Round-1 api primitive (now on main) via the allowed cross-subpath import:
resolve_teams_access_token,read_response_body,TeamsApiError(and the lazy-httpx default fetch + response-protocol helpers).Ported (all 7
graph/index.test.tsitblocks + boundary)call_teams_graph_api— Microsoft Graph.defaultscope; absolute URL used as-is, relative path joined onto the base Graph URLpaginate_teams_graph— follows@odata.nextLinkverbatim (never re-encoded)get_teams_channel— channel metadata;idfalls back tochannel_id,display_nameomitted when absentlist_teams_chat_messages/list_teams_channel_messages/list_teams_message_replies—$topquery-param joinget_teams_channel_message,to_graph_messageextract_text_from_graph_message— ordered regex HTML→text pass; entity-decode&lastSSRF divergence (Python-first, no upstream counterpart)
call_teams_graph_apigates an absolutepath_or_urland every followed@odata.nextLinkto thegraph.microsoft.comhost before resolving/attaching the Bearer token — raisingValueErrorfor any other host. Upstream attaches the token to whatever URL the server hands back. A dedicated test asserts a hostile@odata.nextLinkis rejected before the injected fetch is ever awaited (the token never leaves the process). Divergence-doc rows are consolidated in T7 (this lane does not touchdocs/UPSTREAM_SYNC.mdorCHANGELOG.md).Tests —
tests/test_teams_graph_primitive.py(16, all green)7 faithful index ports + boundary source-scan + no-eager-httpx import check + direct text-extraction coverage (incl. the
&-last ordering) + the nextLink-host SSRF rejection. Injectable AsyncMock fetch (not MagicMock). Mutation-checked: disabling the SSRF gate and reordering the entity decode both fail the relevant tests.Gauntlet (from worktree)
ruff check— All checks passedruff format --check— 270 files already formattedaudit_test_quality.py— 0 hard failurespyrefly check— 0 errorspytest tests/— 4978 passed, 4 skippedLane scope
Only two new files added;
teams/__init__.py(lazy subpath registration deferred to T7),adapter.py,CHANGELOG.md,docs/UPSTREAM_SYNC.md, and other primitives are untouched.