fix(http_client): strip credentials on cross-origin redirect hops#2635
Conversation
Central fix for the PR #2604 review finding (qodo Option B): the manual redirect loops in fetch/afetch (httpx sync/async + aiohttp backends) reused the caller's headers and cookies across hops, so a cross-origin redirect resent Authorization / x-api-key / cookies to the redirect target. Every per-hop request (including the HEAD->GET Range fallbacks) now: - strips SENSITIVE_REDIRECT_HEADERS (authorization, proxy-authorization, cookie, x-api-key, api-key, x-auth-token; case-insensitive) when the hop's origin (scheme/host/port, default ports normalized) differs from the original request's origin — unparseable URLs fail closed - drops cookies on cross-origin hops - logs stripped header names (debug, bound target_host) Same-origin hops are unchanged. Supersedes the per-caller mitigation from PR #2634 (tokenizer_resolver allow_redirects=False stays — provider APIs never legitimately redirect). Tests: origin helpers incl. hypothesis self-consistency property, header stripping unit tests, and MockTransport end-to-end flows through the real sync+async redirect loops (cross-origin strips, same-origin keeps). 96/96 tests/http_client pass; 117/117 adapter/tokenizer consumers pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoHarden redirects: strip credentials on cross-origin redirect hops
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements credential hardening for cross-origin redirects in the HTTP client. It introduces helper functions to identify cross-origin hops and strip sensitive headers (such as Authorization, API keys, and cookies) or drop cookies entirely when a redirect leaves the original origin. These checks are integrated into both the synchronous and asynchronous request execution paths, and a new test suite is added to verify the behavior. The review feedback suggests catching ValueError in _url_origin to handle malformed ports gracefully, optimizing the header stripping logic to use a single pass, and adding a test case to verify that invalid ports fail closed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except _HTTPCLIENT_NONCRITICAL_EXCEPTIONS: | ||
| return None |
There was a problem hiding this comment.
Accessing parsed.port or parsing malformed URLs (e.g., with invalid port formats like :invalid-port or malformed IPv6 brackets) can raise a ValueError. Catching ValueError explicitly ensures that these cases are handled gracefully and fail closed instead of raising an unhandled exception.
| except _HTTPCLIENT_NONCRITICAL_EXCEPTIONS: | |
| return None | |
| except ValueError: | |
| return None | |
| except _HTTPCLIENT_NONCRITICAL_EXCEPTIONS: | |
| return None |
There was a problem hiding this comment.
Verified rather than changed: ValueError is already in _HTTPCLIENT_NONCRITICAL_EXCEPTIONS (http_client.py:76-92), so _url_origin returns None on malformed ports and _is_cross_origin fails closed. Added the explicit regression test (test_invalid_ports_and_malformed_hosts_fail_closed) proving it.
| kept = {k: v for k, v in headers.items() if k.lower() not in SENSITIVE_REDIRECT_HEADERS} | ||
| dropped = sorted(k for k in headers if k.lower() in SENSITIVE_REDIRECT_HEADERS) | ||
| if dropped: | ||
| logger.bind(target_host=_parse_host_from_url(target_url)).debug( | ||
| "Stripped sensitive headers on cross-origin redirect: {}", ", ".join(dropped) | ||
| ) | ||
| return kept |
There was a problem hiding this comment.
The headers dictionary is currently iterated over twice to separate kept and dropped headers. We can optimize this to a single pass for better efficiency and readability.
kept = {}
dropped = []
for k, v in headers.items():
if k.lower() in SENSITIVE_REDIRECT_HEADERS:
dropped.append(k)
else:
kept[k] = v
if dropped:
dropped.sort()
logger.bind(target_host=_parse_host_from_url(target_url)).debug(
\"Stripped sensitive headers on cross-origin redirect: {}\", \", \".join(dropped)
)
return keptThere was a problem hiding this comment.
Addressed in 3fb470c: kept/dropped are now split in a single pass over the headers.
| def test_unparseable_urls_fail_closed(self) -> None: | ||
| assert _is_cross_origin("https://a.example", "not a url") | ||
| assert _is_cross_origin("", "https://a.example") |
There was a problem hiding this comment.
Add a test case to verify that URLs with invalid ports are handled gracefully and treated as cross-origin (fail closed).
def test_unparseable_urls_fail_closed(self) -> None:
assert _is_cross_origin(\"https://a.example\", \"not a url\")
assert _is_cross_origin(\"\", \"https://a.example\")
assert _is_cross_origin(\"https://a.example\", \"https://a.example:invalid-port\")There was a problem hiding this comment.
Addressed in 3fb470c: test_invalid_ports_and_malformed_hosts_fail_closed covers invalid ports, out-of-range ports, and malformed IPv6 brackets — all fail closed and strip.
Code Review by Qodo
Context used✅ Compliance rules (platform):
74 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| async def _do_once(ac: httpx.AsyncClient, target_url: str) -> tuple[httpx.Response | None, str]: | ||
| req_headers = _inject_trace_headers(headers) | ||
| req_headers = _strip_sensitive_headers_for_cross_origin( | ||
| req_headers, original_url=url, target_url=target_url | ||
| ) | ||
| req_cookies = _cookies_for_hop(cookies, original_url=url, target_url=target_url) | ||
| # Parity with other paths: drop 'zstd' from Accept-Encoding for httpx | ||
| try: | ||
| with suppress(_HTTPCLIENT_NONCRITICAL_EXCEPTIONS): |
There was a problem hiding this comment.
6. Ambient redirect cookies unhandled 🐞 Bug ⛨ Security
Cross-origin redirect handling in fetch/afetch only drops the explicit per-call cookies argument via _cookies_for_hop; it does not clear or bypass any cookies already stored on the underlying httpx/aiohttp client/session, leaving a gap where ambient cookie-jar state is not prevented from being sent on cross-origin hops.
Agent Prompt
## Issue description
The redirect hardening now drops the per-call `cookies` parameter on cross-origin hops, but it does not address cookies stored on the underlying client/session cookie jar. Elsewhere in this module (the “simple fetch” redirect loop), redirect-boundary logic explicitly clears session cookie state, suggesting ambient cookie state is considered part of redirect-borne credentials.
## Issue Context
- `fetch/afetch` accept a caller-supplied `client` / `session`, which can accumulate cookie state.
- The new change only influences the explicit `cookies=` argument.
- The module already contains redirect-boundary code that clears cookie jars (`_clear_session_cookie_state`) for another redirect implementation.
## Fix Focus Areas
- tldw_Server_API/app/core/http_client.py[1186-1192]
- tldw_Server_API/app/core/http_client.py[2218-2353]
- tldw_Server_API/app/core/http_client.py[2550-2641]
- tldw_Server_API/app/core/http_client.py[3267-3440]
- tldw_Server_API/tests/Web_Scraping/test_http_client_fetch.py[444-505]
### Implementation options (pick one)
1) **Bypass the client cookie jar on cross-origin hops** (preferred if feasible): construct/send the hop request in a way that does not merge in client/session cookie-jar state.
2) **Clear cookie jar only for internally-managed clients/sessions** on cross-origin hops (avoid mutating caller-supplied shared clients), mirroring the existing `_clear_session_cookie_state(...)` behavior used in the other redirect loop.
3) **Add a guardrail**: if `allow_redirects=True` and a caller-supplied client/session has non-empty cookie state, either refuse redirects or require the caller to opt in explicitly.
### Add regression test
Add a unit/integration test that simulates a cookie being present in the client/session state and asserts it is not sent on a cross-origin redirect hop in `fetch/afetch`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Addressed in b4fc13d (option 2, mirroring the simple-fetch precedent): new _clear_client_cookie_jar clears client/session jar state at cross-origin redirect boundaries in all five redirect loops; regression test asserts the jar is emptied and no cookie reaches the redirect target.
…losed Address PR #2635 review: split kept/dropped headers in one iteration; add tests proving malformed ports and IPv6 brackets fail closed (ValueError from urlparse .port is in _HTTPCLIENT_NONCRITICAL_EXCEPTIONS, so _url_origin returns None -> treated as cross-origin -> stripped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address remaining PR #2635 review findings (qodo): - SSE streaming redirect loops (_astream_sse_httpx/_astream_sse_aiohttp) now strip sensitive headers per hop like fetch/afetch — previously an active cross-origin credential path for streaming providers - ambient client/session cookie-jar state is cleared at cross-origin redirect boundaries in all five redirect loops via new _clear_client_cookie_jar (mirrors the simple-fetch loop's _clear_session_cookie_state precedent) - tests: drop unnecessary requires_httpx skip guard (httpx is a hard dep), remove redundant @pytest.mark.asyncio markers, annotate _handler return type, add docstrings throughout, plus regression tests for SSE redirect stripping and jar clearing 100/100 tests/http_client; 131/131 Web_Scraping fetch + adapter + tokenizer consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Central redirect-credential hardening — the tracked follow-up from PR #2604's review (qodo "Option B") and the
Docs/superpowers/plans/2026-07-04-test-suite-improvement-implementation-plan.mdTracked Follow-Ups section.Problem: the manual redirect loops in
fetch/afetch(httpx sync/async and aiohttp backends) rebuilt each hop's request from the caller's originalheaders/cookies, so a cross-origin redirect resentAuthorization,x-api-key, cookies, etc. to the redirect target.Fix: every per-hop request (including the HEAD→GET Range fallbacks) now strips
SENSITIVE_REDIRECT_HEADERSand drops cookies when the hop's origin (scheme/host/port, default ports normalized) differs from the original request's origin. Unparseable URLs fail closed. Same-origin hops are byte-for-byte unchanged. Stripped header names are logged at debug with the target host bound.The per-caller mitigation from PR #2634 (
tokenizer_resolverallow_redirects=False) stays — provider tokenizer APIs never legitimately redirect.Verification (local — CI currently broken per maintainer)
tests/http_client/test_redirect_header_hardening.py: 13 tests — origin-helper units + hypothesis self-consistency property +httpx.MockTransportend-to-end flows through the real sync and async redirect loops (cross-origin strips credentials and keeps non-sensitive headers; same-origin keeps credentials)tests/http_client/: 96/96 passtest_research_adapters.py,test_tokenizer_resolver_unit.py): 117/117 pass🤖 Generated with Claude Code
Summary by cubic
Hardened redirect handling in the HTTP client to prevent credential leaks. On cross-origin hops we now strip credential headers, drop cookies, clear cookie jars, and do the same for SSE streaming redirects; same-origin is unchanged and invalid ports or malformed IPv6 targets fail closed.
Authorization,Proxy-Authorization,Cookie,X-API-Key,API-Key,X-Auth-Tokenon cross-origin hops (case-insensitive).httpx(sync/async) andaiohttp.Written for commit b4fc13d. Summary will update on new commits.