Skip to content

Strip connection headers on cross-host redirect in HttpHook#70000

Open
Samin061 wants to merge 2 commits into
apache:mainfrom
Samin061:http-conn-header-redirect
Open

Strip connection headers on cross-host redirect in HttpHook#70000
Samin061 wants to merge 2 commits into
apache:mainfrom
Samin061:http-conn-header-redirect

Conversation

@Samin061

Copy link
Copy Markdown
Contributor

An HTTP connection's extra field is documented as a place to put credentials ("Login and Password authentication can be used along with any authentication method using headers"), and those end up in session.headers, while run_and_check follows redirects by default. requests strips only Authorization once should_strip_auth sees the host change, so a secret under any other name (X-API-Key, Private-Token) is replayed to the redirect target — whether that is an attacker's host reached through an open redirect on the API, or simply the CDN the API bounces downloads to. This gives the connection's own headers the same treatment as Authorization; same-host redirects still carry them.

HttpAsyncHook has the same gap and aiohttp behaves identically, but it offers no equivalent redirect hook, so covering it means reworking the async redirect handling rather than extending an existing one. I left it out to keep this reviewable, and I'm happy to follow up if you'd like it in scope.

The FakeSession patch targets in the sensor tests move to the new class because the hook now instantiates it.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Opus 4.8) following the guidelines

An HTTP connection's extra field is documented as a place to carry credentials, and requests only protects the Authorization name, so a secret under any other header name reached whatever host a redirect named.
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 18, 2026
@eladkal
eladkal requested review from Lee-W, dabla and jason810496 July 19, 2026 13:01

@nailo2c nailo2c 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 looks like the intended security behavior, I think we might need to write down a change log in somewhere to let users know about it.

For example, users who intentionally relied on connection extra headers being replayed to a different redirect host, such as a CDN, may need to update their flow or handle the redirect explicitly.

@nailo2c nailo2c 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.

I'm not a committer, so I'd defer to others on whether this change and its scope look right. If so, I think we should open an issue to track the async part so it doesn't get lost.

@Samin061

Copy link
Copy Markdown
Contributor Author

Good call on both.

Added a breaking-changes note at the top of providers/http/docs/changelog.rst covering exactly that case: if you were relying on the connection's headers reaching the redirect target (a CDN or object store that needs the same key), the options are to point the connection at a host that matches the final destination, or set allow_redirects=False in extra_options and issue the second request yourself.

Opened #70164 for the async side, and left a pointer to it next to the header handling in HttpAsyncHook.config so it survives after this merges. aiohttp has no per-redirect callback equivalent to requests.Session.rebuild_auth, so that fix means walking the redirect chain manually rather than subclassing, which is why it isn't in here.

@jroachgolf84

Copy link
Copy Markdown
Collaborator

Linking to #70164

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

uv.lock on main just moved via #69526 ("Make psycopg (v3) the default synchronous Postgres driver"), commit 7c272f1 and this PR currently conflicts.

Quickest fix:

git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-lease

Automated nudge — ignore if you're not ready to rebase. This comment is updated in place on future uv.lock bumps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:http ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants