fix: preserve request headers on HTTP 3xx redirects - #13652
fix: preserve request headers on HTTP 3xx redirects#13652Protocol-zero-0 wants to merge 4 commits into
Conversation
Chromium drops Authorization and other headers when following redirects that change origin (e.g. http to https). Manually follow 3xx responses so provider checks and API calls keep custom headers. Fixes CherryHQ#13236 Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Fixes provider authentication failures caused by Chromium stripping sensitive/custom headers on cross-origin HTTP redirects (notably http → https) by introducing a manual-redirect-following fetch wrapper and applying it to modern AI SDK provider requests.
Changes:
- Added
createFetchPreservingHeadersOnRedirectwrapper to manually follow 3xx redirects while reapplying method/body/headers (with special handling for 303). - Composed the redirect-preserving fetch wrapper with the existing developer→system role conversion fetch wrapper in
providerToAiSdkConfig. - Added Vitest unit coverage for 307 (preserve headers + POST body) and 303 (switch to GET, drop body).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/renderer/src/aiCore/provider/providerConfig.ts |
Wraps the provider fetch used by modern SDK configs with the new redirect-preserving fetch implementation. |
src/renderer/src/aiCore/provider/preserveHeadersOnRedirectFetch.ts |
Implements manual redirect following logic intended to preserve Authorization/custom headers across redirects. |
src/renderer/src/aiCore/provider/__tests__/providerConfig.redirectFetch.test.ts |
Adds unit tests validating redirect behavior for 307 and 303 scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const location = response.headers.get('Location') ?? response.headers.get('location') | ||
| if (!location) { | ||
| return response | ||
| } | ||
|
|
||
| url = new URL(location, url).toString() | ||
|
|
There was a problem hiding this comment.
Security: this redirect-follow implementation will re-send Authorization and any custom headers to whatever host appears in Location. That enables credential exfiltration via open redirects or MITM when the user config is http:// (the exact scenario in #13236). Please restrict header preservation to safe redirects (e.g., only allow http -> https upgrade with the same hostname+port, or same-origin), and for other redirects either drop sensitive headers (Authorization/X-Api-Key/etc.) or stop following and surface a clear error.
| @@ -0,0 +1,91 @@ | |||
| const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]) | |||
There was a problem hiding this comment.
For 301/302, the current logic preserves method+body across redirects. That diverges from standard fetch redirect behavior (many clients switch POST to GET for 301/302, while 307/308 are the ones that guarantee method preservation). To avoid surprising compatibility issues, consider either handling 301/302 like fetch (switch to GET and drop body/content headers similarly to 303 for non-GET/HEAD), or remove 301/302 from REDIRECT_STATUSES so you don’t change semantics for those cases.
| const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]) | |
| const REDIRECT_STATUSES = new Set([303, 307, 308]) |
| h.delete('content-type') | ||
| h.delete('Content-Type') | ||
| h.delete('content-length') | ||
| h.delete('Content-Length') |
There was a problem hiding this comment.
When switching to GET for a 303 you only delete Content-Type/Content-Length. Other Content-* headers (e.g., content-encoding) could also be left behind and produce invalid requests. Consider removing all headers whose name starts with content- when dropping the body, rather than hard-coding a small list.
| h.delete('content-type') | |
| h.delete('Content-Type') | |
| h.delete('content-length') | |
| h.delete('Content-Length') | |
| h.forEach((_, key) => { | |
| if (key.toLowerCase().startsWith('content-')) { | |
| h.delete(key) | |
| } | |
| }) |
Made-with: Cursor
|
Updated the redirect wrapper to address the security concerns. Sensitive headers are now only preserved for same-origin redirects or same-host http→https upgrades. For other redirects, the wrapper follows safely but strips sensitive auth-style headers. I also aligned 301/302 handling with standard fetch semantics for POST redirects and broadened content-header cleanup, with focused tests covering safe redirect preservation, cross-origin auth stripping, and 302/303 behavior. |
DeJeune
left a comment
There was a problem hiding this comment.
LGTM. The security handling for cross-origin vs same-host redirects is solid, and the test coverage is good.
…s-on-redirect Made-with: Cursor # Conflicts: # src/renderer/src/aiCore/provider/providerConfig.ts
|
Synced this branch with latest upstream/main and resolved merge conflicts in provider config. The redirect header-preservation logic is still intact after the sync. Please take another look when convenient. |
|
Quick follow-up: this branch is now synced with current main and conflict-free. Could a maintainer help with a re-review when available? |
|
Synced this branch to latest main again today and resolved the new merge drift. No failing checks on the updated head. If maintainers are okay with current scope, this should be ready for final merge/review action. |
|
error when my cherrystudio v2.0.3 fetch https://officecli.ai/SKILL.md |
…18380) > ### Branch strategy > > - Active development targets `main`. > - v1 maintenance targets `v1`; forward-port fixes to `main` separately when needed. ### What this PR does Before this PR: - `fetchRemoteText()` defaults `maxRedirects` to `0`, and neither the built-in `@cherry/fetch` MCP server nor the web-search content fetcher passed the option. - Any `301`/`302`/`303`/`307`/`308` therefore surfaced to the user as `Failed to fetch <url>: HTTP error: 301`, so a large class of ordinary URLs (vanity domains pointing at a CDN, `http` → `https` upgrades, docs shortlinks) could never be read. - `CitationPreviewService` was the only caller that opted in (`maxRedirects: 5`), so the same URL worked in a citation preview and failed in the fetch tool. After this PR: - `@cherry/fetch` and `fetchWebSearchContent()` opt into the helper's strict hop limit (`maxRedirects: 5`), matching `CitationPreviewService`. - Verified against the URL from the user report on #13652: ``` maxRedirects=0 -> HTTP error: 301 maxRedirects=5 -> OK 25870 bytes | --- name: officecli description: Create, ``` Fixes #N/A — no issue was filed for this; the user report is on #13652 (comment). ### Why we need it and why it was done in this way `remoteFetch.ts` already implements safe redirect following: every hop repeats literal-URL validation, DNS resolution, private-address rejection, and pinned connection setup before opening the next request, and sensitive headers (`Authorization`, `Cookie`, `Proxy-Authorization`) are dropped on cross-origin hops. `docs/references/security/remote-fetch.md` documents this as an explicit opt-in — "Redirects are rejected by default. Callers may opt into a strict hop limit". This PR is only the missing opt-in for two callers; no new fetch or safety code. The following tradeoffs were made: - `5` hops (matching `CitationPreviewService`) rather than an unbounded or configurable limit — redirect chains longer than that are indistinguishable from loops for these bounded-text callers, and the limit is not user-facing. - The added tests assert the outcome (a redirecting URL yields content instead of an error) rather than the option value, so they fail if the opt-in is removed but do not pin the call shape. One pre-existing `toHaveBeenCalledWith` assertion in `fetch.test.ts` that pinned the whole options object was narrowed to the URL it actually cares about; the header assertions immediately below it are unchanged. The following alternatives were considered: - Making `maxRedirects` default to a non-zero value in `fetchRemoteText()` was rejected: the default-deny stance is deliberate (PR #16966), and flipping it would silently change every present and future caller. - Handling redirects inside `@cherry/fetch` was rejected: it would duplicate the per-hop revalidation and pinning that the shared helper already owns, which is exactly the SSRF gap #16964 closed. Links to places where the discussion took place: #13652 (comment), #16964, #16835 ### Breaking changes None. No IPC, persisted-data, or public API contract changes. Redirects remain rejected by default for every other `fetchRemoteText()` caller. ### Special notes for your reviewer This is **not** the redirect problem PR #13652 is about, despite the user report that landed there. Two separate things: - **#13652 / #13236** — request headers stripped across a 3xx redirect. That PR targets `src/renderer/src/aiCore/provider/providerConfig.ts`, which no longer exists: the v2 refactor (#14911) moved the AI runtime into the main process, so the branch merges with a `modify/delete` conflict and is 2370 commits behind. Its mechanism also does not survive the move — provider traffic now goes through Electron `net.fetch` (`src/main/ai/utils/customFetch.ts`), and `net.fetch(url, { redirect: 'manual' })` throws `Redirect was cancelled` because `lib/browser/api/net-fetch.ts` registers no `redirect` listener. Measured on Electron 41.8.0, `Authorization` already survives an `http` → `https` same-host redirect through `net.fetch`, so #13236 does not appear to reproduce on the current architecture. #13236 was auto-closed as `NOT_PLANNED` in April. - **This PR** — the built-in fetch tool refusing to follow a redirect at all, which is what the reporter on #13652 actually hit. Not addressed here: #18344 (`@cherry/fetch` rejecting Surge/Clash Enhanced Mode fake-IP addresses in `198.18.0.0/15`). That is the SSRF DNS guard doing what it was designed to do, and relaxing it is a security tradeoff for maintainers to decide, not a missing option. ### Checklist This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR. Approvers are expected to review this list. - [x] Branch: This PR targets the correct branch — `main` for active development, `v1` for v1 maintenance fixes - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: [Write code that humans can understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans) and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle) - [x] Refactor: You have [left the code cleaner than you found it (Boy Scout Rule)](https://learning.oreilly.com/library/view/97-things-every/9780596809515/ch08.html) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A [user-guide update](https://docs.cherry-ai.com) was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior. - [x] Self-review: I have reviewed my own code (e.g., via [`/gh-pr-review`](/.claude/skills/gh-pr-review/SKILL.md), `gh pr diff`, or GitHub UI) before requesting review from others ### Release note ```release-note Fix the built-in @cherry/fetch tool and web search page fetching failing with "HTTP error: 301" on URLs that redirect. ``` Signed-off-by: suyao <sy20010504@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thank you very much for your contribution and for your patience while the v2 work was landing. We are now reviewing pull requests opened before the v2 transition. This PR has not received a recent code update and no longer applies cleanly to the latest We are leaving the PR open for now. If you would like to continue the work, please rebase or recreate the change from the latest Thank you again for the work and for your understanding. |
|
Reworked this fix against the current v2 main-process networking architecture in #19614. The new PR moves safe redirect handling to the shared provider customFetch, retains the earlier security constraints, and includes updated regression coverage. Closing this superseded PR in favor of the clean v2 implementation. |
<!-- Template from https://github.com/kubevirt/kubevirt/blob/main/.github/PULL_REQUEST_TEMPLATE.md?--> <!-- Thanks for sending a pull request! Here are some tips for you: 1. Consider creating this PR as draft: https://github.com/CherryHQ/cherry-studio/blob/main/CONTRIBUTING.md --> > ### Branch strategy > > - Active development targets `main`. ### What this PR does Before this PR: Provider requests that followed an HTTP redirect through Chromium's network stack could lose authentication headers, causing otherwise valid provider checks and API calls to fail. After this PR: The shared provider `customFetch` follows redirects manually. It preserves sensitive headers for same-origin redirects and same-host HTTP-to-HTTPS upgrades, strips authentication and API-key headers for other cross-origin redirects, and retains standard 301/302/303 method and body semantics. <!-- (optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged)*: --> Fixes #13236 ### Why we need it and why it was done in this way The v2 architecture routes provider traffic through Electron `net.fetch` in the main process so requests use the proxy-aware Chromium network stack. Chromium intentionally strips sensitive headers on cross-origin redirects, including the common case where a configured HTTP endpoint upgrades to HTTPS on the same host. The following tradeoffs were made: Manual redirect handling is limited to the string/URL request shape used by the AI SDK and only when redirect mode is omitted or set to `follow`. Explicit `manual`/`error` modes and `Request` inputs retain their native behavior. Redirects are capped at 20 hops. The following alternatives were considered: Reusing the renderer implementation from #13652 was rejected because v2 moved provider networking into the main process. Rewriting configured HTTP URLs to HTTPS was also rejected because it would break intentional HTTP and local endpoints. Links to places where the discussion took place: #13652 ### Breaking changes <!-- optional --> None. ### Special notes for your reviewer The regression tests cover safe same-host HTTP-to-HTTPS authentication preservation, cross-host credential stripping, and POST/PUT conversion for 302/303 redirects with all `Content-*` headers removed when the body is dropped. ### Checklist This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR. Approvers are expected to review this list. - [x] Branch: This PR targets `main` - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: [Write code that humans can understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans) and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle) - [x] Refactor: You have [left the code cleaner than you found it (Boy Scout Rule)](https://learning.oreilly.com/library/view/97-things/every/9780596809515/ch08.html) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A [user-guide update](https://docs.cherry-ai.com) was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior. - [x] Self-review: I have reviewed my own code (e.g., via [`/gh-pr-review`](/.claude/skills/gh-pr-review/SKILL.md), `gh pr diff`, or GitHub UI) before requesting review from others ### Release note <!-- Write your release note: 1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required". 2. If no release note is required, just write "NONE". 3. Only include user-facing changes (new features, bug fixes visible to users, UI changes, behavior changes). For CI, maintenance, internal refactoring, build tooling, or other non-user-facing work, write "NONE". --> ```release-note Provider requests now retain authentication headers across safe same-host HTTP-to-HTTPS redirects. ``` --------- Signed-off-by: Protocol Zero <257158451+Protocol-zero-0@users.noreply.github.com> Signed-off-by: suyao <sy20010504@gmail.com> Co-authored-by: suyao <sy20010504@gmail.com>
Problem
When a model provider base URL uses
httpand the server responds with a redirect (e.g. 307) tohttps, Chromium treats this as a cross-origin redirect and strips sensitive headers such asAuthorizationand custom headers. Users see failed provider checks / auth errors even though the endpoint works.Closes #13236
Solution
createFetchPreservingHeadersOnRedirectthat follows redirects withredirect: 'manual'and reapplies the same method, body, and headers (RFC 303: switch to GET and drop content headers).providerToAiSdkConfigso all modern SDK provider requests benefit.Tests
Made with Cursor