FrankYang0529 commented on code in PR #71842:
URL: https://github.com/apache/airflow/pull/71842#discussion_r3842715127
##########
providers/microsoft/azure/tests/unit/microsoft/azure/operators/test_msgraph.py:
##########
@@ -369,6 +369,47 @@ def
test_pagination_issues_every_page_with_the_configured_request(self):
assert request.headers.try_get("ConsistencyLevel") == {"eventual"}
assert request.content == json.dumps(data).encode("utf-8")
+ def test_pagination_refuses_cross_host_next_link(self):
+ first_page = {
+ "@odata.nextLink":
"https://attacker.example/v1.0/users?$skiptoken=steal";,
+ "value": [{"id": "1"}],
+ }
+ second_page = {"value": [{"id": "2"}]}
+ response = mock_json_response(200, first_page, second_page)
+
+ with patch_hook_and_request_adapter(response) as (*_,
mock_get_http_response):
+ operator = MSGraphAsyncOperator(
+ task_id="users_delta",
+ conn_id="msgraph_api",
+ url="users",
+ )
+
+ with pytest.raises(AirflowException, match="attacker.example"):
+ execute_operator(operator)
Review Comment:
Add three new test cases:
- `test_assert_allowed_host_refuses_another_host`
- `test_assert_allowed_host_accepts_a_relative_url`
- `test_assert_allowed_host_accepts_a_host_listed_in_the_connection`
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -620,6 +615,27 @@ async def run(
return response
+ async def assert_allowed_host(self, url: str | None) -> None:
+ """
+ Refuse an absolute ``url`` whose host differs from the configured
Microsoft Graph endpoint.
+
+ A pagination link (e.g. ``@odata.nextLink``) is echoed from the API
response and is re-fetched
+ with the connection's bearer token attached. That token is withheld
only from hosts outside
+ ``allowed_hosts``, which defaults to empty (any host) unless
configured, so a tampered response
+ could send it to an arbitrary host (CWE-918).
+ """
+ if not url or not url.startswith("http"):
+ return
Review Comment:
The guard inside `assert_allowed_host()` is what makes the method correct no
matter who calls it. Moving `await self.hook.get_async_conn()` into
`MSGraphTrigger.run()` relocates the await rather than removing it, and turns
the precondition into something every future caller has to remember. I prefer
to keep it in `assert_allowed_host`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
