🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport#12850
Conversation
Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route doc-scoped tool calls to a different endpoint path. The fetch wrapper used `redirect: 'manual'` for SSRF protection, which silently dropped these redirects and caused tool calls to fail with empty error bodies. Follow 307/308 redirects (method-preserving per RFC 7538) up to a depth of 5. SSRF safety is preserved because the same undici Agent with its SSRF-safe connect function validates redirect targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the PR! Will review soon. @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7ca04872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const baseUrl = typeof url === 'string' ? url : url.toString(); | ||
| url = new URL(location, baseUrl).href; |
There was a problem hiding this comment.
This follows Location targets without validating the new origin, while subsequent hops reuse the original request init (including Authorization/user headers), so a redirecting MCP endpoint can exfiltrate credentials to another host. This is a real security regression from the previous manual-redirect behavior, and it is especially risky in allowedDomains deployments where SSRF connect-time checks are disabled (shouldEnableSSRFProtection() returns false when an allowlist is configured).
Useful? React with 👍 / 👎.
| const response = await undiciFetch(url, mergedInit); | ||
| const isMethodPreservingRedirect = response.status === 307 || response.status === 308; | ||
|
|
||
| let initHeaders: Record<string, string> = {}; | ||
| if (init?.headers) { | ||
| if (init.headers instanceof Headers) { | ||
| initHeaders = Object.fromEntries(init.headers.entries()); | ||
| } else if (Array.isArray(init.headers)) { | ||
| initHeaders = Object.fromEntries(init.headers); | ||
| } else { | ||
| initHeaders = init.headers as Record<string, string>; | ||
| if (!isMethodPreservingRedirect || redirects >= MAX_REDIRECTS) { | ||
| return response; |
There was a problem hiding this comment.
When a 307/308 is followed, the intermediate response body is never consumed or canceled before issuing the next undiciFetch. Undici warns that leaving bodies unreleased can hold connection resources until GC, which can reduce reuse and eventually stall under repeated redirects or redirect responses with bodies. Cancel/drain each intermediate redirect response before continuing the loop.
Useful? React with 👍 / 👎.
…ial leaks - Validate every redirect target against `resolveHostnameSSRF` so allowlist deployments (which disable connect-time SSRF protection) still block hops to private/reserved IPs. - Strip `Authorization`, `Cookie`, `mcp-session-id`, and any user-injected headers when a 307/308 crosses an origin boundary, mirroring browser/Fetch behavior so a redirecting MCP server can't exfiltrate credentials. - Cancel the intermediate response body before each next hop so undici can reuse pooled sockets rather than holding them until GC. - Restructure redirect test helpers to be same-origin (matching real-world Coda-style routing), drop dead setup code, fix the misleading "5 hops successfully" test, and add coverage for SSRF-blocked redirects, cross- origin credential stripping, and same-origin credential preservation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f72a9373a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const userInjectedKeys: ReadonlySet<string> = new Set( | ||
| Object.keys(requestHeaders ?? {}).map((key) => key.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
userInjectedKeys is built only from getHeaders() (runtime headers set via setRequestHeaders), so secret headers that come from serverConfig.headers/requestInit.headers are not considered user-injected and survive stripCrossOriginHeaders on 307/308 cross-origin hops. With this commit now following those redirects, a malicious MCP endpoint can return a cross-origin Location during connect and receive configured API-key-style headers (anything except the hardcoded authorization/cookie/mcp-session-id set), which is a credential leak introduced by the redirect-follow logic.
Useful? React with 👍 / 👎.
Previously only runtime `setRequestHeaders` keys were treated as secret on a 307/308 cross-origin hop. API keys baked into `serverConfig.headers` (passed through `requestInit.headers` at transport construction time) survived stripping, so a malicious MCP endpoint could exfiltrate them by returning a cross-origin `Location`. Pass the configured header keys through to `createFetchFunction` so both runtime and config secrets are stripped. The cross-origin credential test now also configures `serverConfig.headers` to lock in this behavior.
|
@codex review |
- Add `proxy-authorization` to the cross-origin forbidden header set so a forward-proxy credential header would also be stripped on a cross-origin hop, matching the Fetch-spec list. - Strengthen the cross-origin credential test with positive assertions that benign protocol headers (`accept`, `content-type`) survive the hop, so a regression that strips everything indiscriminately would now fail. - Extract the duplicated MCP request handler / session-teardown logic from three test helpers into shared `createMCPRequestHandler` and `closeMCPSessions` utilities.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 451ca905b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| initHeaders = init.headers as Record<string, string>; | ||
| let currentInit = buildFetchInit(init, dispatcher, requestHeaders); | ||
| let url = input; | ||
| const originalOrigin = new URL(typeof url === 'string' ? url : url.toString()).origin; |
There was a problem hiding this comment.
customFetch is typed to accept UndiciRequestInfo, which includes Request, but this change computes originalOrigin via url.toString(). For Request objects, toString() yields "[object Request]", so new URL(...) throws before any network call. This is a regression from the previous implementation (which passed input through directly) and will break callers that pass a Request (for example, middleware/wrappers that normalize fetch calls to Request instances) even when no redirect is involved.
Useful? React with 👍 / 👎.
`customFetch` is typed to accept `UndiciRequestInfo` (`string | URL | Request`), but `Request.prototype.toString()` returns `"[object Request]"`. The previous implementation derived `originalOrigin` and the redirect base via `url.toString()`, so a `Request` input would throw inside `new URL(...)` before any network call — a regression even when no redirect was involved. Add a `getRequestUrlString` helper that extracts the URL string for all three shapes, track the URL string alongside the fetch input through the redirect loop, and add parameterized tests that exercise `customFetch` with each shape.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04373a5171
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const headers = requestHeaders ? { ...initHeaders, ...requestHeaders } : initHeaders; | ||
| return { | ||
| ...init, | ||
| redirect: 'manual', | ||
| headers, |
There was a problem hiding this comment.
Do not unconditionally set headers in buildFetchInit: when customFetch is called with a Request input and no init.headers, normalizeInitHeaders returns {} and this code passes headers: {} to fetch, which replaces the original Request headers. That drops auth/session/protocol headers carried on the Request itself, so Request-based callers can fail authentication or protocol negotiation even without redirects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 914ad71. buildFetchInit now skips setting headers entirely when neither init.headers nor runtime getHeaders() contribute anything, so the headers carried on a Request input survive intact. Added a regression test that puts Authorization and a custom header on a Request and asserts both reach the target server.
Previously `buildFetchInit` always set `headers` on the returned init —
even when neither `init.headers` nor runtime headers contributed anything.
Passing `headers: {}` to `undiciFetch` overrides the headers carried on a
`Request` input (auth tokens, MCP session, protocol negotiation), so
Request-based wrappers could fail authentication even without a redirect
in play. Skip the `headers` override entirely when there is nothing to
merge.
Adds a regression test that supplies `Authorization` and a custom header
on the `Request` itself and asserts both reach the target server.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 914ad714c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url = targetUrl.href; | ||
| currentUrlString = targetUrl.href; |
There was a problem hiding this comment.
When customFetch receives a Request input (which this commit now explicitly supports), redirect retries switch url to targetUrl.href and reuse currentInit; if init.method/init.body were not set, subsequent hops default to GET with no body. That breaks 307/308 method-preserving semantics and can turn redirected POST MCP calls into GETs (or drop payloads), causing failures on servers that require POST semantics after redirect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 123718e. Added a resolveFetchInput helper that runs once at the top of customFetch: when the input is a Request, its method/headers/body are extracted into the shared init (body buffered via arrayBuffer() so 307/308 retries can replay it), and the loop then operates uniformly on a (string, init) pair. Regression test posts a Request with a JSON-RPC payload at a 308-redirecting endpoint and asserts both POST and the original body land at the redirect target.
| const headers = currentInit.headers as Record<string, string>; | ||
| currentInit = { | ||
| ...currentInit, | ||
| headers: stripCrossOriginHeaders(headers, secretHeaderKeys), |
There was a problem hiding this comment.
This branch assumes currentInit.headers is always an object, but buildFetchInit intentionally omits headers when neither runtime nor init headers exist. In that case, a cross-origin 307/308 redirect path passes undefined into stripCrossOriginHeaders, which calls Object.entries and throws at runtime; redirected requests from Request inputs without explicit init headers will fail with a TypeError instead of returning a response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 123718e. Guarded the cross-origin branch on currentInit.headers != null so it skips entirely when there are no headers to strip (which is the legitimate path after the prior fix that stopped buildFetchInit from emitting headers: {}). Regression test does a no-headers cross-origin redirect and asserts the response comes back cleanly without a TypeError.
…ross-origin strip
Two regressions surfaced by extending `customFetch` to accept `Request` inputs:
1. **307/308 method/body loss.** The redirect loop switches `url` to the
new `Location` string, but the original `Request`'s method and body
stayed bound to the (now-discarded) `Request` object. A redirected
POST silently became a GET with no payload — the exact behavior the
method-preserving codes are designed to prevent. Added a
`resolveFetchInput` helper that runs once at the top of `customFetch`,
extracts a `Request`'s method/body/headers into the shared init, and
buffers the body via `arrayBuffer()` so 307/308 retries can replay it.
2. **Cross-origin strip crashed on absent headers.** After the previous
fix that stopped `buildFetchInit` from setting `headers: {}`,
`currentInit.headers` could legitimately be `undefined`. The
cross-origin branch read it as a `Record` and called `Object.entries`
on `undefined`, throwing `TypeError`. Guard the branch on
`currentInit.headers != null` — when there are no headers there is
nothing to strip.
Adds two regression tests: a POST-with-body `Request` that 308-redirects
cross-origin (asserts both method and body survive) and a no-headers
cross-origin redirect (asserts the strip path no longer crashes).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 123718e334
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolvedInit: { | ||
| ...init, | ||
| method: init?.method ?? req.method, | ||
| body: init?.body ?? (reqBody as unknown as UndiciRequestInit['body']), | ||
| headers: mergedHeaders, |
There was a problem hiding this comment.
resolveFetchInput rewrites a Request input into { urlString, resolvedInit } but only copies method/body/headers, so the original Request.signal is dropped. Any caller that passes an already-wired Request (with an AbortController used for timeouts/cancellation) will lose abort behavior and the fetch can continue running after cancellation, which is a regression from the previous implementation that forwarded input directly to undiciFetch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in dc3da4d. resolveFetchInput now forwards Request.signal alongside method/body/headers (explicit init.signal still wins). Regression test aborts a controller before calling customFetch with a Request wired to that controller and asserts the call rejects.
`resolveFetchInput` was copying method/body/headers off a `Request` input but dropping `Request.signal` on the floor, so a caller that wired an `AbortController` onto the `Request` for cancellation/timeouts lost that wiring as soon as we re-shaped the input into the `(string, init)` pair used by the redirect loop. Subsequent aborts no longer reached the in-flight fetch — a regression from the pre-PR code, which forwarded the original `Request` directly to undici. Forward the signal alongside method/body/headers, with explicit `init.signal` still winning per Fetch-spec semantics. Regression test aborts a controller before calling \`customFetch\` with the wired `Request` and asserts the call rejects.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Audit follow-up. The cross-origin strip path keys off `targetUrl.origin !== originalOrigin`, and `URL.origin` is defined as `scheme + "://" + host + ":" + port`, so a same-host `https → http` redirect produces a different origin and trips the strip path through the existing logic — no separate code path needed. Pin that contract with a small unit test so a future change to URL semantics (or a refactor that swaps in a different comparison) doesn't silently regress protocol-downgrade stripping. Standing up a TLS fixture (self-signed cert, undici skip-verify, etc.) just to re-prove the URL spec is wasted complexity.
…ny-avila#12850) * 🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route doc-scoped tool calls to a different endpoint path. The fetch wrapper used `redirect: 'manual'` for SSRF protection, which silently dropped these redirects and caused tool calls to fail with empty error bodies. Follow 307/308 redirects (method-preserving per RFC 7538) up to a depth of 5. SSRF safety is preserved because the same undici Agent with its SSRF-safe connect function validates redirect targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 🛡️ fix: Harden MCP 307/308 redirect handling against SSRF and credential leaks - Validate every redirect target against `resolveHostnameSSRF` so allowlist deployments (which disable connect-time SSRF protection) still block hops to private/reserved IPs. - Strip `Authorization`, `Cookie`, `mcp-session-id`, and any user-injected headers when a 307/308 crosses an origin boundary, mirroring browser/Fetch behavior so a redirecting MCP server can't exfiltrate credentials. - Cancel the intermediate response body before each next hop so undici can reuse pooled sockets rather than holding them until GC. - Restructure redirect test helpers to be same-origin (matching real-world Coda-style routing), drop dead setup code, fix the misleading "5 hops successfully" test, and add coverage for SSRF-blocked redirects, cross- origin credential stripping, and same-origin credential preservation. * 🛡️ fix: Also strip `serverConfig.headers` on cross-origin MCP redirects Previously only runtime `setRequestHeaders` keys were treated as secret on a 307/308 cross-origin hop. API keys baked into `serverConfig.headers` (passed through `requestInit.headers` at transport construction time) survived stripping, so a malicious MCP endpoint could exfiltrate them by returning a cross-origin `Location`. Pass the configured header keys through to `createFetchFunction` so both runtime and config secrets are stripped. The cross-origin credential test now also configures `serverConfig.headers` to lock in this behavior. * 🧹 chore: Tighten MCP redirect-stripping coverage and helper duplication - Add `proxy-authorization` to the cross-origin forbidden header set so a forward-proxy credential header would also be stripped on a cross-origin hop, matching the Fetch-spec list. - Strengthen the cross-origin credential test with positive assertions that benign protocol headers (`accept`, `content-type`) survive the hop, so a regression that strips everything indiscriminately would now fail. - Extract the duplicated MCP request handler / session-teardown logic from three test helpers into shared `createMCPRequestHandler` and `closeMCPSessions` utilities. * 🛠️ fix: Handle `Request` inputs in MCP `customFetch` URL derivation `customFetch` is typed to accept `UndiciRequestInfo` (`string | URL | Request`), but `Request.prototype.toString()` returns `"[object Request]"`. The previous implementation derived `originalOrigin` and the redirect base via `url.toString()`, so a `Request` input would throw inside `new URL(...)` before any network call — a regression even when no redirect was involved. Add a `getRequestUrlString` helper that extracts the URL string for all three shapes, track the URL string alongside the fetch input through the redirect loop, and add parameterized tests that exercise `customFetch` with each shape. * 🛠️ fix: Don't override `Request` input headers in MCP `buildFetchInit` Previously `buildFetchInit` always set `headers` on the returned init — even when neither `init.headers` nor runtime headers contributed anything. Passing `headers: {}` to `undiciFetch` overrides the headers carried on a `Request` input (auth tokens, MCP session, protocol negotiation), so Request-based wrappers could fail authentication even without a redirect in play. Skip the `headers` override entirely when there is nothing to merge. Adds a regression test that supplies `Authorization` and a custom header on the `Request` itself and asserts both reach the target server. * 🛠️ fix: Preserve `Request` method/body across MCP redirects + guard cross-origin strip Two regressions surfaced by extending `customFetch` to accept `Request` inputs: 1. **307/308 method/body loss.** The redirect loop switches `url` to the new `Location` string, but the original `Request`'s method and body stayed bound to the (now-discarded) `Request` object. A redirected POST silently became a GET with no payload — the exact behavior the method-preserving codes are designed to prevent. Added a `resolveFetchInput` helper that runs once at the top of `customFetch`, extracts a `Request`'s method/body/headers into the shared init, and buffers the body via `arrayBuffer()` so 307/308 retries can replay it. 2. **Cross-origin strip crashed on absent headers.** After the previous fix that stopped `buildFetchInit` from setting `headers: {}`, `currentInit.headers` could legitimately be `undefined`. The cross-origin branch read it as a `Record` and called `Object.entries` on `undefined`, throwing `TypeError`. Guard the branch on `currentInit.headers != null` — when there are no headers there is nothing to strip. Adds two regression tests: a POST-with-body `Request` that 308-redirects cross-origin (asserts both method and body survive) and a no-headers cross-origin redirect (asserts the strip path no longer crashes). * 🛠️ fix: Forward `Request.signal` through MCP `customFetch` normalization `resolveFetchInput` was copying method/body/headers off a `Request` input but dropping `Request.signal` on the floor, so a caller that wired an `AbortController` onto the `Request` for cancellation/timeouts lost that wiring as soon as we re-shaped the input into the `(string, init)` pair used by the redirect loop. Subsequent aborts no longer reached the in-flight fetch — a regression from the pre-PR code, which forwarded the original `Request` directly to undici. Forward the signal alongside method/body/headers, with explicit `init.signal` still winning per Fetch-spec semantics. Regression test aborts a controller before calling \`customFetch\` with the wired `Request` and asserts the call rejects. * 🧪 test: Pin URL.origin contract for protocol-downgrade redirect handling Audit follow-up. The cross-origin strip path keys off `targetUrl.origin !== originalOrigin`, and `URL.origin` is defined as `scheme + "://" + host + ":" + port`, so a same-host `https → http` redirect produces a different origin and trips the strip path through the existing logic — no separate code path needed. Pin that contract with a small unit test so a future change to URL semantics (or a refactor that swaps in a different comparison) doesn't silently regress protocol-downgrade stripping. Standing up a TLS fixture (self-signed cert, undici skip-verify, etc.) just to re-prove the URL spec is wasted complexity. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>
Adds an "Upstream context and related work" section pointing at relevant work on danny-avila/LibreChat: - PR danny-avila#11799 + Issue danny-avila#10641: direct overlap with this plan's MCP Apps work; lists the 10 axes on which v8 intentionally diverges (self-contained HTML, no direct browser networking, one proxy per instance, hop-specific relay validation, manifest-hash approval, etc.). - Issue danny-avila#11997: the only upstream artifact for MCP Tasks (no PR yet); plan's Tasks work is greenfield. - PR danny-avila#12850: 307/308 redirect handling and credential stripping is merged into dev but NOT in HEAD 738003b. Earlier revisions of this plan claimed main already had it; corrected. Phase 0 now tracks the upstream merge or ports the work if it slips. - PR danny-avila#12535, danny-avila#12853, danny-avila#12910, Issue danny-avila#12802: adjacent transport reliability and OAuth hygiene worth tracking. - PRs already in HEAD listed for context (danny-avila#12782, danny-avila#12763, danny-avila#12755, danny-avila#12745, danny-avila#12812). - Notably absent upstream: session-id reuse correctness, header consistency, 404 → re-init, per-user token scoping, outstanding-task revalidation, legacy renderer retirement. References section reorganized into Specs / Upstream LibreChat / MDN subsections. https://claude.ai/code/session_011NZqb4xN9QcXpdY2LCtnuH
…s Hardened split) Restructures the implementation strategy after the eighth review: - Stop parallel-building. Phase 1 inherits upstream PR danny-avila#11799 as the Apps Preview substrate (per-instance outer iframes, same-server binding, /api/mcp/sandbox auth, ui:// URI validation, capability gating, mcp_app artifact, test server, stableMCPAppRef) and verifies these with regression tests rather than re-implementing them. - Split Apps phasing into: * Phase 2P (Apps Preview, ~1-2w): adopt danny-avila#11799 substantially as-is on chat surface only; consolidate to single ACL; truthful HostContext; payload truncation; rate limits; fullscreen behind appSettings.allowFullscreen; capability advertise behind MCP_APPS_PREVIEW_ENABLED. * Phase 2H (Apps Hardened GA, +2-3w later release): layer the v8 security delta - dedicated MCP_SANDBOX_ORIGIN, explicit-target-origin transport, hop-specific relay + proxy-stamped nonce, folded-in ui/initialize probe, connect-src 'none', self-contained HTML, MCPAppLaunchManifest review, MCPAppInstance write path, full UIResourceRenderer retirement across all surfaces - behind MCP_APPS_HARDENED_ENABLED. - Narrow first Apps release to live chat only. Share, search, and plugin-rendered surfaces fall back to text in both Preview and Hardened GA. Cross-surface support is post-Hardened-GA. - Defer MCPAppInstance full-conversation remount to Hardened GA. Preview relies on upstream stableMCPAppRef for parent- re-render survival. - Accept upstream fullscreen support behind appSettings.allowFullscreen (default OFF per server) instead of stripping in delta. - Trim Tasks v1 to status-first jobs panel. Progress bars deferred until upstream PR danny-avila#12535 lands; reuse rather than rebuild. Tasks v1 is independent of Apps tracks. - Phase 0 starts from dev (inheriting PRs danny-avila#12850, danny-avila#12853, danny-avila#12910) rather than older main HEAD; remaining work is session reuse, header consistency, basic 404 -> re-init, per-user token scoping, authContextHash. - Three independent feature flags: MCP_APPS_PREVIEW_ENABLED, MCP_APPS_HARDENED_ENABLED, MCP_TASKS_ENABLED. Old MCP_APPS_ENABLED becomes a deprecated alias for Preview with a startup warning. - Default-deny sandboxPermissions on the legacy renderer applies even when both Apps flags are off. Updated changelog header, Closed go/no-go decisions (added 19/20/21), Carried-forward list with track tags, current-state table with track markers, phases (Phase 0/1/2P/2H/4), test matrix (P/H/A tags), risks, and effort summary (~3-4w for Preview+Tasks; full v1 ~5-7w). References section unchanged. https://claude.ai/code/session_011NZqb4xN9QcXpdY2LCtnuH
…ny-avila#12850) * 🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route doc-scoped tool calls to a different endpoint path. The fetch wrapper used `redirect: 'manual'` for SSRF protection, which silently dropped these redirects and caused tool calls to fail with empty error bodies. Follow 307/308 redirects (method-preserving per RFC 7538) up to a depth of 5. SSRF safety is preserved because the same undici Agent with its SSRF-safe connect function validates redirect targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 🛡️ fix: Harden MCP 307/308 redirect handling against SSRF and credential leaks - Validate every redirect target against `resolveHostnameSSRF` so allowlist deployments (which disable connect-time SSRF protection) still block hops to private/reserved IPs. - Strip `Authorization`, `Cookie`, `mcp-session-id`, and any user-injected headers when a 307/308 crosses an origin boundary, mirroring browser/Fetch behavior so a redirecting MCP server can't exfiltrate credentials. - Cancel the intermediate response body before each next hop so undici can reuse pooled sockets rather than holding them until GC. - Restructure redirect test helpers to be same-origin (matching real-world Coda-style routing), drop dead setup code, fix the misleading "5 hops successfully" test, and add coverage for SSRF-blocked redirects, cross- origin credential stripping, and same-origin credential preservation. * 🛡️ fix: Also strip `serverConfig.headers` on cross-origin MCP redirects Previously only runtime `setRequestHeaders` keys were treated as secret on a 307/308 cross-origin hop. API keys baked into `serverConfig.headers` (passed through `requestInit.headers` at transport construction time) survived stripping, so a malicious MCP endpoint could exfiltrate them by returning a cross-origin `Location`. Pass the configured header keys through to `createFetchFunction` so both runtime and config secrets are stripped. The cross-origin credential test now also configures `serverConfig.headers` to lock in this behavior. * 🧹 chore: Tighten MCP redirect-stripping coverage and helper duplication - Add `proxy-authorization` to the cross-origin forbidden header set so a forward-proxy credential header would also be stripped on a cross-origin hop, matching the Fetch-spec list. - Strengthen the cross-origin credential test with positive assertions that benign protocol headers (`accept`, `content-type`) survive the hop, so a regression that strips everything indiscriminately would now fail. - Extract the duplicated MCP request handler / session-teardown logic from three test helpers into shared `createMCPRequestHandler` and `closeMCPSessions` utilities. * 🛠️ fix: Handle `Request` inputs in MCP `customFetch` URL derivation `customFetch` is typed to accept `UndiciRequestInfo` (`string | URL | Request`), but `Request.prototype.toString()` returns `"[object Request]"`. The previous implementation derived `originalOrigin` and the redirect base via `url.toString()`, so a `Request` input would throw inside `new URL(...)` before any network call — a regression even when no redirect was involved. Add a `getRequestUrlString` helper that extracts the URL string for all three shapes, track the URL string alongside the fetch input through the redirect loop, and add parameterized tests that exercise `customFetch` with each shape. * 🛠️ fix: Don't override `Request` input headers in MCP `buildFetchInit` Previously `buildFetchInit` always set `headers` on the returned init — even when neither `init.headers` nor runtime headers contributed anything. Passing `headers: {}` to `undiciFetch` overrides the headers carried on a `Request` input (auth tokens, MCP session, protocol negotiation), so Request-based wrappers could fail authentication even without a redirect in play. Skip the `headers` override entirely when there is nothing to merge. Adds a regression test that supplies `Authorization` and a custom header on the `Request` itself and asserts both reach the target server. * 🛠️ fix: Preserve `Request` method/body across MCP redirects + guard cross-origin strip Two regressions surfaced by extending `customFetch` to accept `Request` inputs: 1. **307/308 method/body loss.** The redirect loop switches `url` to the new `Location` string, but the original `Request`'s method and body stayed bound to the (now-discarded) `Request` object. A redirected POST silently became a GET with no payload — the exact behavior the method-preserving codes are designed to prevent. Added a `resolveFetchInput` helper that runs once at the top of `customFetch`, extracts a `Request`'s method/body/headers into the shared init, and buffers the body via `arrayBuffer()` so 307/308 retries can replay it. 2. **Cross-origin strip crashed on absent headers.** After the previous fix that stopped `buildFetchInit` from setting `headers: {}`, `currentInit.headers` could legitimately be `undefined`. The cross-origin branch read it as a `Record` and called `Object.entries` on `undefined`, throwing `TypeError`. Guard the branch on `currentInit.headers != null` — when there are no headers there is nothing to strip. Adds two regression tests: a POST-with-body `Request` that 308-redirects cross-origin (asserts both method and body survive) and a no-headers cross-origin redirect (asserts the strip path no longer crashes). * 🛠️ fix: Forward `Request.signal` through MCP `customFetch` normalization `resolveFetchInput` was copying method/body/headers off a `Request` input but dropping `Request.signal` on the floor, so a caller that wired an `AbortController` onto the `Request` for cancellation/timeouts lost that wiring as soon as we re-shaped the input into the `(string, init)` pair used by the redirect loop. Subsequent aborts no longer reached the in-flight fetch — a regression from the pre-PR code, which forwarded the original `Request` directly to undici. Forward the signal alongside method/body/headers, with explicit `init.signal` still winning per Fetch-spec semantics. Regression test aborts a controller before calling \`customFetch\` with the wired `Request` and asserts the call rejects. * 🧪 test: Pin URL.origin contract for protocol-downgrade redirect handling Audit follow-up. The cross-origin strip path keys off `targetUrl.origin !== originalOrigin`, and `URL.origin` is defined as `scheme + "://" + host + ":" + port`, so a same-host `https → http` redirect produces a different origin and trips the strip path through the existing logic — no separate code path needed. Pin that contract with a small unit test so a future change to URL semantics (or a refactor that swaps in a different comparison) doesn't silently regress protocol-downgrade stripping. Standing up a TLS fixture (self-signed cert, undici skip-verify, etc.) just to re-prove the URL spec is wasted complexity. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>
2 participants
Fixes #12849
Summary
Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route doc-scoped tool calls to a different endpoint path (e.g.
/apis/mcp→/apis/mcp/docs/{docId}). The fetch wrapper increateFetchFunctionusedredirect: 'manual'for SSRF protection, which silently dropped these method-preserving redirects. Tool calls failed with empty error bodies while stateless tools (search,whoami) that don't trigger redirects worked fine.This PR follows 307/308 redirects (method-preserving per RFC 7538) up to a depth limit of 5. SSRF safety is preserved because the same undici
Agentwith its SSRF-safeconnectfunction validates redirect destination IPs. Header-merging logic is extracted to abuildFetchInithelper for clarity.Change Type
Testing
Verified end-to-end with Coda's MCP server in a Docker deployment —
table_rows_read,document_read,page_read, and other doc-scoped tools that previously failed with empty 308 responses now work correctly after following the redirect.4 new integration tests added in
MCPConnectionSSRF.test.tsusing real in-process HTTP servers (no mocks, per project testing philosophy):Test Configuration:
tsc --noEmit)Checklist
🤖 Generated with Claude Code