Restrict MSGraph deferrable pagination to the configured host - #71842
Conversation
c8f00af to
cdde34f
Compare
dabla
left a comment
There was a problem hiding this comment.
This PR addresses a genuine CWE-918 (SSRF) risk: when MSGraphAsyncOperator.trigger_next_link() fires a new MSGraphTrigger for the next page, the url comes directly from the previous API response's @odata.nextLink. Without a host check the Kiota adapter would attach the bearer token to whatever host the response contained.
The approach — adding pagination_link: bool to the trigger, propagating it through serialize(), and calling a centralised assert_allowed_host() before the request — is clean and correct for the trigger path.
Two points worth addressing before merge:
-
Configurability gap (inline comment on hooks/msgraph.py): the single-endpoint check is correct for normal use (SharePoint/Power BI callers configure
hostto match, so pagination links pass). The gap is when a caller passes an absoluteurlthat differs from the connection’shost, or uses a custompagination_functionreturning absolute URLs for a different target. Makingallowed_netlocsan explicit opt-in set fixes this at no security cost. The reviewer has working local code; a ready-to-use snippet is in the inline comment. -
Test isolation (inline comment on test_msgraph.py):
test_pagination_refuses_cross_host_next_linkvalidates the full end-to-end path but hides the exception-wrapping mechanics. A direct unit test ofassert_allowed_host()on the hook would make the contract explicit and survive any future changes to the trigger’s exception handler.
The nit on hooks/msgraph.py is a minor refactoring suggestion (making the check synchronous) that follows naturally if point 1 is adopted.
Drafted-by: Claude Sonnet 4.6 (claude-sonnet-4.6); reviewed by @dabla before posting
cdde34f to
db1a33a
Compare
79408c3 to
c9817ac
Compare
|
@FrankYang0529 Wow nice refactorings, thank you! Will test it locally first, if it works I think we can merge. |
dabla
left a comment
There was a problem hiding this comment.
This PR extends the CWE-918 (SSRF) guard introduced in #69742 to the deferred pagination path. When MSGraphAsyncOperator.trigger_next_link() fires a new MSGraphTrigger for the next page, the url originates from the previous response's @odata.nextLink. Without a host check the Kiota adapter would attach the bearer token to whatever host the response contained; with pagination_link=True the trigger now calls assert_allowed_host() before the request goes out.
The main previous concern — that the allowlist was hardcoded to only the connection's base_url host — has been addressed. get_allowed_netlocs() now dynamically combines self.allowed_netloc with the full allowed_hosts set read from the RequestAdapter's AzureIdentityAccessTokenProvider, so callers that configure allowed_hosts on the Airflow connection (e.g., SharePoint alongside Graph) do not hit false positives. The .lower() normalisation is a correct addition that prevents case-sensitivity bypasses. The three new hook-level unit tests make assert_allowed_host()'s contract explicit and survive changes to the trigger's exception-handling.
The one remaining gap is a direct trigger-level unit test for the pagination_link=True guard (see inline comment). The fix is straightforward and the rest of the implementation is sound.
Drafted-by: Claude Sonnet 4.6 (claude-sonnet-4.6); reviewed by @dabla before posting
Signed-off-by: PoAn Yang <payang@apache.org>
c9817ac to
55cc2cc
Compare
Why
MSGraphAsyncOperatorfetches the next page, the url comes from the previous response. The Kiota adapter attaches the connection's access token to every request it sends. Nothing checks that url before the request goes out.How
paginated_run()already does (added in Restrict MSGraph pagination nextLink to the configured host #69742) intoKiotaRequestAdapterHook.assert_allowed_host(). The allowed host comes from the request adapter'sbase_url.pagination_link: bool = FalsetoMSGraphTrigger, carry it throughserialize(), and call the helper at the top ofrun()when it is set.MSGraphAsyncOperator.trigger_next_link()passespagination_link=True, so the check applies only to urls that came back from a response.$top/$skipbranch and a custompagination_functioncan both produce one, soassert_allowed_host()returns early for any url that is not absolute.Verification
uv run --project providers/microsoft/azure pytest providers/microsoft/azure/tests/unit/microsoft/azure -k msgraphWas generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.