dabla commented on code in PR #72165:
URL: https://github.com/apache/airflow/pull/72165#discussion_r4307198291
##########
providers/http/src/airflow/providers/http/hooks/http.py:
##########
@@ -169,6 +169,35 @@ def _retryable_error_async(exception: BaseException) ->
bool:
return exception.status >= 500
+class _ConnectionHeaderSession(Session):
Review Comment:
**[warning]** The behaviour change for `HttpHook` is not documented in this
PR.
The changelog and connection docs were asked for on #70000 (@aaron-y-chen in
their review, and @Samin061 when closing it: "point the connection at the final
host, or set `allow_redirects=False` in `extra_options` and make the second
request yourself"). #72398 now adds a `.. warning::` to `changelog.rst` and a
paragraph to `connections/http.rst`, but it describes the async hook ("follow
aiohttp's origin rules"), and nothing in this PR covers the sync side. If the
two PRs merge in separate provider waves, or this one first, `HttpHook` users
get the change with no note.
The note should also say that **every** connection header is dropped, not
only credentials: `Accept` is dropped in
`test_connection_headers_dropped_on_cross_host_redirect`, and so would be a
`Content-Type` from the extra on a 307/308 that resends the body to the other
host. Someone who relied on a CDN hop getting the same `Accept` will see that
first.
Suggestion: one hook-neutral note (both hooks, the origin rule from
`should_strip_auth`, the two ways out quoted above), carried by whichever of
#72165 / #72398 merges first, or by both if they go together.
##########
providers/http/tests/unit/http/hooks/test_http.py:
##########
@@ -1385,3 +1386,87 @@ async def
test_resolve_srv_targets_async_missing_dependency_raises(self):
with mock.patch.dict(sys.modules, {"dns": None, "dns.asyncresolver":
None, "dns.exception": None}):
with pytest.raises(HttpSrvLookupException, match="dnspython"):
await hook._resolve_srv_targets_async("_http._tcp.example.com")
+
+
+class TestConnectionHeadersAcrossRedirects:
+ """Connection-supplied headers must not follow a redirect to a different
host.
+
+ ``requests`` strips only ``Authorization`` (``Session.rebuild_auth``), so
a credential
+ carried in a differently-named header — the documented ``extra``-field
pattern — would
+ otherwise be replayed to whatever host the redirect points at.
+ """
+
+ @staticmethod
+ def _redirect(session, old_url, new_url):
Review Comment:
**[nit]** All the redirect tests call `rebuild_auth` directly, so no test
goes through a real redirect.
`_redirect` builds a prepared request by hand and calls
`session.rebuild_auth`. The tests would still pass if `run()` /
`run_and_check()` stopped going through `Session.send` → `resolve_redirects`,
or if `requests` changed what it copies to the next hop. #72398 tests the async
side end to end with an aiohttp `TestServer`. One test like it for the sync
hook would cover the whole path. With `requests_mock`, the real redirect logic
of the session still runs:
```python
def test_run_drops_connection_header_on_cross_host_redirect(self,
requests_mock):
requests_mock.get("http://test:8080/v1/test";, status_code=302,
headers={"Location": "https://cdn.example.com/f"})
requests_mock.get("https://cdn.example.com/f";, text="ok")
with mock.patch(
"airflow.providers.http.hooks.http.HttpHook.get_connection",
side_effect=get_airflow_connection_with_extra({"X-API-Key":
"secret"}),
):
HttpHook(method="GET").run("v1/test")
assert requests_mock.request_history[0].headers["X-API-Key"] == "secret"
assert "X-API-Key" not in requests_mock.request_history[-1].headers
```
(I ran the equivalent locally against this head: the cross-host hop drops
`X-API-Key` and `Accept`, a same-host hop keeps them, a caller header survives,
and a port change strips. So this is about keeping it covered, not a bug.)
##########
providers/http/src/airflow/providers/http/hooks/http.py:
##########
@@ -265,10 +294,11 @@ def get_conn(
session = self._configure_session_from_extra(session, connection,
extra_options)
session = self._configure_session_from_mount_adapters(session)
- if self.default_headers:
- session.headers.update(self.default_headers)
- if headers:
- session.headers.update(headers)
+ for override in (self.default_headers, headers):
+ if override:
+ session.headers.update(override)
+ if isinstance(session, _ConnectionHeaderSession):
Review Comment:
**[nit]** The hook keeps the session's headers and `connection_header_keys`
in sync from outside, behind two `isinstance` checks.
`get_conn` (here) and `_configure_session_from_extra` (line 394) each ask
the session for its type and then update two structures that must stay
consistent: `session.headers` and `session.connection_header_keys`. If the
session took both updates as one call, e.g. `add_connection_headers(headers)`
and `add_caller_headers(headers)` on `_ConnectionHeaderSession`, the
lower-casing and the add/remove rule would live in one place next to
`rebuild_auth`, which reads them. The hook would then call a method instead of
checking the type ([Martin,
*OCP*](http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod)).
The trade-off: the `isinstance` guard also covers a plain `Session` (the
sensor tests' `FakeSession`, or a subclass passing its own session to
`_configure_session_from_extra`). With methods, `FakeSession` would need to
subclass `_ConnectionHeaderSession`. Fine to leave as is if you prefer that.
--
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]
