dabla commented on code in PR #71842:
URL: https://github.com/apache/airflow/pull/71842#discussion_r3863441264
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -620,6 +624,27 @@ async def run(
return response
+ async def assert_allowed_host(self, url: str | None) -> None:
Review Comment:
I would prefer having an implementation looking like this. Instead of
adding `allowed_netlocs`, I would keep the existing `allowed_netloc` +
dynamically derive the `allowed_hosts` from the `RequestAdapter`. Advantage of
re-using this is that everything is already setup in the connection and we
don't have to do duplicate configuration.
So I was thinking something like this.
A method which resolves the `allowed_hosts` from the `RequestAdapter`:
```
async def get_allowed_hosts(self) -> set[str]:
request_adapter = await self.get_async_conn()
return
set(request_adapter._authentication_provider.access_token_provider.allowed_hosts)
```
Then we adapt the extracted `assert_allowed_host` method (I really like this
refactoring you did):
```
async def assert_allowed_host(self, url: str | None) -> None:
"""
Refuse an absolute ``url`` whose host the connection does not allow.
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
allowed_hosts = await self.get_allowed_hosts() | self.allowed_netloc
if urlparse(url).netloc.lower() not in allowed_hosts:
raise ValueError(
f"Refusing to follow pagination link {url!r}: its host is
not among the allowed "
f"Microsoft Graph endpoints {sorted(allowed_hosts)}."
)
```
Something like this, let me know what you think about it.
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -620,6 +624,27 @@ async def run(
return response
+ async def assert_allowed_host(self, url: str | None) -> None:
Review Comment:
I also opened an
[issue](https://github.com/microsoft/kiota-python/issues/700) on the Microsoft
Github repository for [kiota](https://github.com/microsoft/kiota-python)
regarding the `allowed_hosts` to add support for wildcards in the
`AllowedHostsValidator` with corresponding
[PR](https://github.com/microsoft/kiota-python/pull/701) which addresses it.
Let's see what comes out of it.
--
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]
