Conversation
`proxy()` and `proxyRequest()` called `fetch(target, ...)` without a `redirect` option, so undici's default `redirect: "follow"` silently followed up to 20 upstream 3xx hops. A reverse proxy should instead pass 3xx responses through to the client. Additionally, `proxyRequest()` streams the request body (`duplex: "half"`), which fetch cannot replay on a redirect — so proxying a POST to an upstream returning 3xx errored and surfaced as a 502. Default `fetchOptions.redirect` to `"manual"` (placed before the spread so an explicit caller value still wins), and document the pass-through behavior on `proxy`, `proxyRequest`, and `ProxyOptions`. Internal sub-requests via `event.app.fetch()` are unaffected — they never follow redirects. Co-Authored-By: Claude Fable 5 <[email protected]>
Add a `timeout` option to `ProxyOptions` that composes an `AbortSignal.timeout(timeout)` into the request's abort signal so a slow upstream is aborted instead of hanging. A fired timeout aborts with a `TimeoutError`, which is now mapped to `504 Gateway Timeout` in the catch block, before the existing `AbortError`/`499` client-disconnect handling (which is left unchanged). A caller-supplied `AbortSignal.timeout` also maps to 504. Co-Authored-By: Claude Fable 5 <[email protected]>
… headers, case-insensitive filters) - Stop stripping the client's `accept` request header so upstream content negotiation (JSON vs HTML) keeps working behind the proxy. `accept-encoding` stays stripped since fetch auto-decompresses. - Strip hop-by-hop headers from the upstream response via a new exported `ignoredResponseHeaders` set (connection, keep-alive, proxy-authenticate, proxy-connection, upgrade, trailer, te + the existing content-length/ content-encoding/transfer-encoding) instead of chained comparisons. - Match `forwardHeaders`/`filterHeaders` case-insensitively by lowercasing the option arrays once, so `["X-Custom"]` matches the lowercased header names from `Headers.entries()`. Documented in the ProxyOptions JSDoc. Co-Authored-By: Claude Fable 5 <[email protected]>
…ling Previously `proxyRequest()` only forwarded a request body when the method was in a hardcoded allowlist (PATCH/POST/PUT/DELETE/QUERY). Bodies on WebDAV-style methods (PROPFIND/REPORT/SEARCH) and custom methods were silently dropped. - Fix A: gate body forwarding on body presence instead of a method allowlist — forward `event.req.body` whenever it is non-null and the method is not GET/HEAD (fetch rejects bodies on those). Remove the now-unused `PayloadMethods` set from `src/utils/internal/proxy.ts`. - Fix B: derive `duplex` from the final effective body (`opts.fetchOptions.body ?? requestBody`) after the spread, so a streamed body supplied via `fetchOptions` on a body-less (GET) event still sets `duplex` and fetch does not throw. An explicit `fetchOptions.duplex` wins. - Fix C: in `createSubRequest`, default `duplex: "half"` when a body is present and no duplex is set, so streamed bodies via `fetchWithEvent` don't throw in the Request constructor on Node. Adds regression tests covering a custom-method (REPORT) body, a stream body supplied via fetchOptions on a GET event, and fetchWithEvent with a stream body. Co-Authored-By: Claude Fable 5 <[email protected]>
Add an `xfwd` option to `ProxyOptions` (default `false`). When enabled, `proxyRequest()` adds standard `x-forwarded-*` request headers derived from the incoming request so the upstream learns the real client and origin: - `x-forwarded-for`: appends `event.req.ip` to any existing value - `x-forwarded-proto`: appends the incoming request protocol - `x-forwarded-host`: original host (incl. port), set only when absent - `x-forwarded-port`: original port or protocol default, set only when absent Logic lives in a small internal `applyXForwardedHeaders` helper that normalizes the merged headers (plain object or `Headers`) to `Headers`. Co-Authored-By: Claude Fable 5 <[email protected]>
# Conflicts: # src/utils/proxy.ts
# Conflicts: # src/utils/internal/proxy.ts # src/utils/proxy.ts # test/proxy.test.ts
Note forced response decompression, host header rewrite portability, and the undici dispatcher escape hatch for sockets/TLS/agents. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProxy forwarding now preserves ChangesProxy behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Never modify values already present on the incoming request (or set via header options) — no append semantics. Co-Authored-By: Claude Fable 5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/proxy.ts`:
- Around line 121-125: Update the requestBody guard near the derived method
declaration to use method instead of event.req.method when excluding GET and
HEAD bodies. Preserve the existing body presence check and ensure overridden
outgoing GET/HEAD requests remain bodyless.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c8ea06d-0231-42b9-9085-5421a0f4abbc
📒 Files selected for processing (5)
docs/2.utils/5.proxy.mdsrc/utils/internal/proxy.tssrc/utils/proxy.tstest/proxy.test.tstest/unit/proxy.test.ts
Upstream location and refresh headers whose origin matches the proxy target are rewritten to the incoming request's origin, so client-side redirects keep flowing through the proxy instead of exposing (and sending the browser to) the upstream host. Relative and third-party URLs are untouched. Opt out with `locationRewrite: false`. Co-Authored-By: Claude Fable 5 <[email protected]>
Like nginx `proxy_redirect <from> <to>`, a record maps location/refresh URL prefixes to replacements; the first matching prefix wins and only explicit mappings apply (including for internal targets). Co-Authored-By: Claude Fable 5 <[email protected]>
pi0
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/proxy.test.ts (1)
729-729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
it.skipIfinstead ofit.runIffor runtime-specific test skips.The coding guidelines for test files specify using
it.skipIf(ctx.target === "node")for runtime-specific skips. This test usesit.runIf(t.target === "web"), which is functionally equivalent but deviates from the prescribed API.♻️ Proposed refactor
- it.runIf(t.target === "web")( + it.skipIf(t.target === "node")( "replaces the first matching prefix with a locationRewrite record",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/proxy.test.ts` at line 729, Update the runtime-specific test declaration using it.runIf in the affected test block to use it.skipIf with the equivalent node-target condition, preserving execution on web and skipping on node according to the test-file convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/proxy.test.ts`:
- Line 729: Update the runtime-specific test declaration using it.runIf in the
affected test block to use it.skipIf with the equivalent node-target condition,
preserving execution on web and skipping on node according to the test-file
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42c0a60d-6e6e-4e85-b618-22bae2def949
📒 Files selected for processing (3)
src/utils/internal/proxy.tssrc/utils/proxy.tstest/proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/internal/proxy.ts
- src/utils/proxy.ts
- timeout: use a cancelable timer cleared once the upstream responds, so it can no longer truncate long-running body streams; sanitize invalid values instead of throwing RangeError - timeout/disconnect: race internal (/) sub-requests against the request signal (H3.fetch does not observe it) so 504/499 work there too; a pre-aborted client no longer dispatches the sub-request - redirect: an explicitly undefined fetchOptions.redirect can no longer silently restore fetch's follow default; opaque-redirect responses (browser/service-worker manual mode) now fail loudly with 502 - body: gate forwarding on the outgoing (possibly overridden) method, and drop the stale incoming content-length when the forwarded body differs - headers: strip response headers nominated by the upstream connection header (RFC 7230 6.1), collect set-cookie via getSetCookie(), stop forwarding request te/trailer - locationRewrite: rewrite protocol-relative locations pointing at the target (upstream host leak), widen refresh matching to optional-delay/ padded/quoted shapes, iterate prefix maps with Object.keys - docs: xfwd spoofing warning + proxyRequest-only note, fix stale internal xfwd comment, forwardHeaders denylist-bypass caveat, propagateAbortError/timeout consistency, browser/SW redirect caveat Co-Authored-By: Claude Fable 5 <[email protected]>
…logic
- proxy: clamp a positive `timeout` up to at least 1ms so a sub-millisecond
value no longer `Math.trunc`s to 0, firing `setTimeout(..., 0)` and 504-ing
every request before the upstream can respond
- proxy: document that an already-disconnected client skips dispatch of an
internal ("/") sub-request handler (side effects do not run)
- cache/static: extract the RFC 9110 §13.1.3 conditional-request precedence
into a shared `isCacheMatch` helper so serveStatic and handleCacheHeaders
can't diverge
Co-Authored-By: Claude Fable 5 <[email protected]>
…esponses Address a second-round review (ISSUES.md): - Strip incoming request headers nominated by the client's `Connection` header (RFC 9110 §7.6.1), and add `proxy-authorization`/`proxy-connection` to the default request denylist so proxy credentials aren't leaked upstream. Extract a shared `connectionTokens` helper for both directions. - Classify aborts by the composed signal's reason instead of only `error.name === "AbortError"`, so a client disconnect that aborts with a raw socket error (e.g. srvx-on-Node `ECONNRESET`) still yields a quiet 499 rather than a 502. - Reject unrelayable opaque/error/status-0 fetch responses with a 502 instead of surfacing an empty `200 OK` via `HTTPResponse`'s `status || 200`. - Use own-property checks in `rewriteCookieProperty` so a polluted prototype can't inject phantom domain/path rewrite rules (matches `rewritePrefix`). - `fetchWithEvent`: accept a `duplex` init, default it for streamed external bodies, and clarify that external URLs use native fetch without inheriting event headers/context. - Document that `forwardHeaders` is a denylist-bypass (not an exclusive allowlist) and that `x-forwarded-proto`/`-host` derive from `event.url`. Co-Authored-By: Claude Fable 5 <[email protected]>
srvx now gates `X-Forwarded-*` behind an opt-in `trustProxy` (srvx#223), so by default `event.url` reflects the real transport and on-the-wire `Host` — a client can no longer spoof `x-forwarded-proto`/`-host` into it. Reword the `xfwd` security note accordingly: the values follow inbound forwarded headers only when the server is configured to trust an upstream proxy. Co-Authored-By: Claude Fable 5 <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
pi0
approved these changes
Jul 11, 2026
This was referenced Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improvements to the
proxy/proxyRequest/fetchWithEventutils, based on a comparison with httpxy'sproxyFetch.Fixes
proxy()now defaultsredirect: "manual", so upstream 3xx responses are passed through to the client instead of undici silently following up to 20 hops. This also fixes a502when proxying a streamed request body to an upstream that redirects (the body cannot be replayed on redirect). Opt back in withfetchOptions: { redirect: "follow" }.acceptheader is forwarded again (it was inignoredHeaders, breaking upstream content negotiation).connection,keep-alive,proxy-authenticate,proxy-connection,upgrade,trailer,te) are now stripped via a newignoredResponseHeadersset.forwardHeaders/filterHeadersnow match header names case-insensitively.PATCH/POST/PUT/DELETE/QUERYallowlist, so WebDAV-style (PROPFIND,REPORT,SEARCH) and custom methods keep their bodies (PayloadMethodsremoved as now-unused).duplexis derived from the final body, so a stream supplied viafetchOptions.bodyon a body-less event no longer throws.createSubRequestdefaultsduplex: "half"for stream bodies (fetchWithEventpath).Features
timeoutoption (7fe8fcc): milliseconds to wait for the upstream response; composed into the existing signal handling viaAbortSignal.timeout. Timeouts map to504 Gateway Timeoutinstead of a generic502. Client-disconnect (499) behavior is unchanged.xfwdoption (b9f83aa, 56a87a3): addsx-forwarded-for/x-forwarded-proto/x-forwarded-host/x-forwarded-portderived fromevent.req.ipandevent.url. Each header is only set when absent — values already present on the incoming request are never modified. Defaultfalse.locationrewriting, like nginxproxy_redirect(874c5fb, 0ce0f9f): upstreamlocation/refreshheaders whose origin matches the proxytargetare rewritten to the incoming request's origin, so client-side redirects keep flowing through the proxy instead of exposing the upstream host. Relative and third-party URLs (and internal/targets) are untouched. On by default; opt out withlocationRewrite: false, or pass a record of prefix mappings (nginxproxy_redirect <from> <to>, e.g.{ "https://upstream.example/two/": "/one/" }) to replace explicit URL prefixes instead — first match wins, and mappings also apply to internal targets.Docs
fetch-inherited limitations (db3ab5c): forced response decompression,hostrewrite portability across runtimes, and the undicidispatcherescape hatch for unix sockets / TLS options / agents.Review hardening (e2b0040)
A parallel security/correctness review of each changed area surfaced and fixed:
timeoutnow uses a cancelable timer cleared once the upstream responds — it can no longer truncate long-running body streams (SSE, large downloads); invalid values are sanitized instead of throwingRangeError.timeout/ client disconnects now also apply to internal (/) targets by racingevent.app.fetch()against the request signal (H3.fetchdoes not observe it); a pre-aborted client short-circuits to499without dispatching the sub-request.undefinedfetchOptions.redirectcan no longer silently restore fetch's follow default; browser/service-worker opaque-redirect responses fail loudly with502instead of an empty200.content-lengthis dropped when the forwarded body differs (fetch trusts a suppliedcontent-lengthverbatim — framing desync).Connectionheader are stripped (RFC 7230 §6.1);set-cookieis collected viagetSetCookie(); requestte/trailerare no longer forwarded.locationRewritealso rewrites protocol-relative locations pointing at the target (upstream host leak) and handles non-canonicalrefreshshapes (optional delay, padded=, quoted URLs); prefix maps iterate own keys only.xfwdspoofing warning (client-suppliedx-forwarded-forwins; strip viafilterHeaderson internet-facing edges) +proxyRequest-only note,forwardHeadersdenylist-bypass caveat,propagateAbortError/timeoutconsistency, browser/SW redirect caveat.The review confirmed the rest sound: no open-redirect in the location rewrite (origin re-rooting verified against
//, userinfo, and backslash tricks), correct 499-vs-504 signal classification, no smuggling surface (undici re-frames; hop-by-hop stripped), andAbortSignal.any/URL.canParse/getSetCookieavailability onengines.node >= 20.11.1and other supported runtimes.Notes
maincommits since the work was based onmain's tip.🤖 Generated with Claude Code
Summary by CodeRabbit
X-Forwarded-*handling when enabled (adds missing values; does not override an existingx-forwarded-for).accept/accept-encodingforwarding.location/refreshrewrite behavior (including limitations with streamed bodies).xfwd, timeout-to-504, andlocation/refreshrewriting.