Skip to content

Restrict MSGraph deferrable pagination to the configured host - #71842

Merged
dabla merged 1 commit into
apache:mainfrom
FrankYang0529:airflow-msgraph-pagination-host-guard
Aug 28, 2026
Merged

Restrict MSGraph deferrable pagination to the configured host#71842
dabla merged 1 commit into
apache:mainfrom
FrankYang0529:airflow-msgraph-pagination-host-guard

Conversation

@FrankYang0529

@FrankYang0529 FrankYang0529 commented Aug 19, 2026

Copy link
Copy Markdown
Member

Why

  • When MSGraphAsyncOperator fetches 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

  • Extract the netloc comparison that paginated_run() already does (added in Restrict MSGraph pagination nextLink to the configured host #69742) into KiotaRequestAdapterHook.assert_allowed_host(). The allowed host comes from the request adapter's base_url.
  • Add pagination_link: bool = False to MSGraphTrigger, carry it through serialize(), and call the helper at the top of run() when it is set. MSGraphAsyncOperator.trigger_next_link() passes pagination_link=True, so the check applies only to urls that came back from a response.
  • Relative urls still pass the check. Airflow's own $top/$skip branch and a custom pagination_function can both produce one, so assert_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 msgraph

Was generative AI tooling used to co-author this PR?
  • Yes - Claude Code

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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.

@FrankYang0529
FrankYang0529 force-pushed the airflow-msgraph-pagination-host-guard branch 2 times, most recently from c8f00af to cdde34f Compare August 20, 2026 04:08
@FrankYang0529
FrankYang0529 marked this pull request as ready for review August 20, 2026 05:19
@FrankYang0529
FrankYang0529 requested a review from dabla as a code owner August 20, 2026 05:19

@dabla dabla left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Configurability gap (inline comment on hooks/msgraph.py): the single-endpoint check is correct for normal use (SharePoint/Power BI callers configure host to match, so pagination links pass). The gap is when a caller passes an absolute url that differs from the connection’s host, or uses a custom pagination_function returning absolute URLs for a different target. Making allowed_netlocs an 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.

  2. Test isolation (inline comment on test_msgraph.py): test_pagination_refuses_cross_host_next_link validates the full end-to-end path but hides the exception-wrapping mechanics. A direct unit test of assert_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

@FrankYang0529
FrankYang0529 force-pushed the airflow-msgraph-pagination-host-guard branch from cdde34f to db1a33a Compare August 24, 2026 10:43
@FrankYang0529
FrankYang0529 requested a review from dabla August 25, 2026 05:30
@FrankYang0529
FrankYang0529 force-pushed the airflow-msgraph-pagination-host-guard branch 2 times, most recently from 79408c3 to c9817ac Compare August 27, 2026 01:36
@dabla

dabla commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

@FrankYang0529 Wow nice refactorings, thank you! Will test it locally first, if it works I think we can merge.

@dabla dabla left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@FrankYang0529
FrankYang0529 force-pushed the airflow-msgraph-pagination-host-guard branch from c9817ac to 55cc2cc Compare August 28, 2026 14:18
@dabla
dabla merged commit 449e589 into apache:main Aug 28, 2026
83 checks passed
@FrankYang0529
FrankYang0529 deleted the airflow-msgraph-pagination-host-guard branch August 29, 2026 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants