fetch: cap redirects at 20 and reject redirect failures with a TypeError - #33276
fetch: cap redirects at 20 and reject redirect failures with a TypeError#33276robobun wants to merge 3 commits into
Conversation
|
Updated 12:21 AM PT - Jul 29th, 2026
❌ @robobun, your commit 459b2e3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33276That installs a local version of the PR into your bun-33276 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRedirect handling now uses a shared default budget, maps redirect failures to ChangesRedirect limit and TypeError classification
Related PRs: None identified. Suggested labels: rust, javascript, http, fetch Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-facing defaults (redirect limit 127→20) and the error constructor for redirect failures, so it's worth a human sign-off on the compatibility trade-off.
Extended reasoning...
Overview
This PR aligns fetch() redirect handling with the WHATWG Fetch spec: it lowers the default redirect budget from ~127 to 20 and changes redirect-related failures (TooManyRedirects, UnexpectedRedirect, RedirectURLInvalid, etc.) to reject with a TypeError instead of a plain Error. It touches the HTTP client (src/http/lib.rs, src/http/AsyncHTTP.rs), adds a new C++ binding SystemError__toTypeErrorInstance in bindings.cpp/headers.h, adds a SystemTypeError variant to Body::ValueError, routes six redirect error codes through it in FetchTasklet::on_reject, updates the maxRedirects type docs, and adds seven tests.
Security risks
None identified. The change tightens a limit rather than loosening one, and the error-type change preserves the existing code/path/errno properties.
Level of scrutiny
Moderate. The implementation is clean and well-tested, but this is a user-visible behavioral change in two dimensions: (1) the default redirect cap drops from ~127 to 20, which will make previously-succeeding long redirect chains start rejecting, and (2) the rejection constructor changes from Error to TypeError. Both are spec-correct and match Node, and the PR deliberately scopes the error-type change narrowly (redirect failures only, not all network errors), but a maintainer should confirm the compat trade-off is acceptable — particularly whether the default-limit drop needs a changelog/release-note callout.
Other factors
The PR also introduces new FFI surface (a parameterized systemErrorToErrorInstance helper in bindings.cpp and a new ValueError enum variant with matching deinit/to_js/dupe arms). All arms look correctly wired and the new tests exercise the boundary at exactly 20/21 hops plus each redirect-error code, but the combination of a default change, an error-type change, and new cross-language plumbing is more than a mechanical fix.
|
Leaving #20486 open rather than adding On CI (updated, build #68045 is now final): the diff is green, the red is agent provisioning. Final job tally for The same error took out What did run on this commit:
So the full test suite has actually run against this change on three targets with no failures. Not pushing a retrigger: a new build lands in the same degraded agent pool. Happy to rebase or retrigger once the image is back, just say so. |
|
Both bot reviews came back with no findings, so picking up the one open question from the Claude review: whether the compatibility trade-off is acceptable, and whether it needs a release-note callout. Default limit, 127 to 20. Anything that needed more than 20 redirects now rejects. The spec and Node both stop at 20 (a 20-hop chain still resolves with a 200; the 21st redirect response is the network error, in both Node 26.3.0 and this branch, after exactly 21 requests). Error constructor, Happy to drop the three rarely-hit codes ( A release note for the default-limit change seems worthwhile. Say the word and I will add one. |
fa5b906 to
6215162
Compare
|
Rebased onto Worth flagging one interaction with #33072, which landed in the meantime: it added a Post-rebase, on the debug build: |
![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/http/lib.rs`:
- Around line 252-259: The redirect limit change in DEFAULT_REDIRECT_COUNT and
the redirect handling in HTTPClient::remaining_redirect_count/do_redirect is
correct, but this is a user-visible behavior change from the previous much
larger default. Update the release notes or changelog to explicitly call out
that fetch() now follows the spec’s 20-redirect budget by default so callers
depending on the old generous redirect behavior can adjust.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fa24466-642a-434b-a4f2-9dc1a13d325d
📒 Files selected for processing (7)
packages/bun-types/globals.d.tssrc/http/AsyncHTTP.rssrc/http/lib.rssrc/jsc/SystemError.rssrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers.hsrc/runtime/webcore/Body.rs
💤 Files with no reviewable changes (1)
- src/runtime/webcore/Body.rs
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-facing defaults (127→20 redirects, affecting bun install too) and error constructors, and you've raised open questions about the compat trade-off, narrowing the error-code set, and adding a release note — a maintainer should weigh in on those before merge.
Extended reasoning...
Overview
This PR caps fetch()'s default redirect limit at 20 (from 127) to match the WHATWG spec and Node.js, and makes redirect-family failures reject with a TypeError instead of a plain Error. It touches the HTTP client (src/http/lib.rs, src/http/AsyncHTTP.rs), adds a new C++/Rust binding for SystemError→TypeError (bindings.cpp, headers.h, SystemError.rs), adds a ValueError::SystemTypeError variant in Body.rs, routes six redirect error codes through it in FetchTasklet.rs, updates the maxRedirects JSDoc, and adds seven tests.
Security risks
None identified. The change tightens a limit rather than loosening one, and no auth/crypto/permission code is touched.
Level of scrutiny
This warrants maintainer review despite being mechanically clean, because it is a user-facing behavior change with non-local reach:
- The 127→20 default affects every consumer of
make_client, including — as the author notes —bun install(src/install/npm.rs,src/install/NetworkTask.rs). Registry redirect chains >20 hops are unlikely but not impossible with corporate proxies/mirrors. - The
Error→TypeErrorswap is spec-correct and mostly compatible (TypeError extends Error,codepreserved), but it is still an observable API change.
Other factors
The author has left explicit open questions for a maintainer in the thread: whether the compat trade-off is acceptable, whether to narrow the TypeError set to three codes instead of six, and whether to add a release note. Those are product/API decisions that should be answered by a human, not a bot. The implementation itself looks correct and is well-tested (7 new tests, existing suites re-run), and the bug-hunting system found no issues.
|
Release-note ask from both reviews is handled in On the On narrowing the
The sixth, So there is one call left for a maintainer, and it is the real one: accept the CI is clean on build #68324, no failures, now that the agent images are back. |
There was a problem hiding this comment.
No bugs found, but this changes a user-visible default (redirect cap 127→20, which the HTTP client shares with bun install) and swaps the rejection constructor to TypeError for six error codes — the compat/scope call you flagged is one a maintainer should make.
Extended reasoning...
Overview
This PR touches 10 files across the HTTP client (src/http/lib.rs, src/http/AsyncHTTP.rs), JSC bindings (src/jsc/bindings/bindings.cpp, headers.h, src/jsc/SystemError.rs), the fetch layer (src/runtime/webcore/Body.rs, FetchTasklet.rs), plus docs, type declarations, and tests. It (1) drops the default remaining_redirect_count from 127 to 21 (i.e. 20 followed redirects, matching the WHATWG spec and Node), and (2) routes six redirect-family failure codes through a new SystemError__toTypeErrorInstance binding so fetch() rejects with a TypeError instead of a plain Error. The bindings.cpp change is a clean refactor that parameterizes the existing helper on JSC::ErrorType; Body::ValueError gains a SystemTypeError variant handled in all three match sites (reset/to_js/dupe).
Security risks
None identified. The change tightens behavior (fewer redirects followed by default), which if anything reduces attack surface for redirect loops. No new untrusted-input parsing, no auth/crypto/permissions code touched.
Level of scrutiny
This warrants maintainer review, not because the implementation is questionable — it looks correct and is well-tested — but because it is a deliberate behavioral change on a hot, widely-used path:
- The 127→20 default affects every
fetch()caller and, as the author notes, the package manager (bun installuses the samemake_clientdefault). A registry chain longer than 20 hops would now fail. - Switching
Error→TypeErrorfor six codes is spec-correct but observable to any caller checkingerr.name === "Error"orerr.constructor === Error. - The author has an open question in the thread about whether to narrow the TypeError set to three codes instead of six; that's an API-surface decision a maintainer should answer.
Other factors
The implementation is clean: the C++ refactor preserves the existing SystemError__toErrorInstance symbol, the new Rust variant is handled everywhere the enum is matched, and the off-by-one budget math is documented and covered by tests that assert exact request counts (20 succeeds, 21 rejects, self-loop makes exactly 21 requests). Docs and .d.ts were updated. CI on the pre-rebase commit was green on the targets that ran; no bugs were surfaced by the multi-agent review. This is a good PR — it just isn't the kind of mechanical change that should merge without a human signing off on the compat trade-off.
3f6a4e7 to
b145491
Compare
|
Rebased onto #31859 changed The new Post-rebase, on the debug build:
CI for this head (build #70746, final): 281 passed, 3 failed, the diff is green.
The proxy-stress one superficially mentions "redirect", but Not pushing a retrigger: a fresh build hits the same unavailable Postgres and MySQL containers. This is ready for a maintainer. |
https://fetch.spec.whatwg.org/#http-redirect-fetch step 5 returns a network error once a request's redirect count reaches 20, and a network error rejects the fetch() promise with a TypeError. Bun's default budget was 127, so a 25-hop chain resolved with a 200 and a redirect loop made 127 round trips before failing, and the rejection was a plain Error. Replace the hardcoded 127 with DEFAULT_REDIRECT_COUNT (= 21: 20 followed redirects, the 21st redirect response rejects). The maxRedirects fetch option still overrides the default. Add SystemError__toTypeErrorInstance and ValueError::SystemTypeError so the redirect failure codes (TooManyRedirects, UnexpectedRedirect, RedirectURLInvalid, InvalidRedirectURL, RedirectURLTooLong, UnsupportedRedirectProtocol) reject with a TypeError that keeps its code, path, and message properties.
fetch.mdx did not mention redirects at all, and maxRedirects was only described in the .d.ts. Add a Redirects section covering the default 20-redirect budget, the TypeError rejection, and the bounds on maxRedirects (0 rejects the first redirect, 126 is the maximum).
b145491 to
1878855
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto Semantic updates to track main:
Git-level conflicts:
Post-rebase, debug build: |
…mments The fetch.mdx Redirects section claimed redirect: "manual" "works as the specification describes" and then showed status 302 and a readable Location header, which is the opposite of the spec's opaque-redirect filtered response. Reword to state Bun's actual behavior explicitly. Trim every new inline/doc comment to three lines or fewer per the comment-length check.
e98c2b9 to
459b2e3
Compare
1 participant
What does this PR do?
Related to #20486 (partially: that issue also covers DNS, connect, and URL-parse errors, which this PR leaves alone).
fetch()was not enforcing the WHATWG fetch specification's redirect limit, and redirect failures rejected with a plainErrorinstead of aTypeError.https://fetch.spec.whatwg.org/#http-redirect-fetch step 5: "If request's redirect count is 20, then return a network error." A network error rejects the
fetch()promise with aTypeError(https://fetch.spec.whatwg.org/#fetch-method).Reproduction
Bun 1.4.0 (and
main):200after 26 requestsError, which breakserr instanceof TypeErrorfeature detectionerror: The response redirected too many times. ... code: "TooManyRedirects"error: UnexpectedRedirect fetching "..." ... code: "UnexpectedRedirect"Node 26.3.0 rejects both after 21 requests with a
TypeError.Cause
make_clientinsrc/http/AsyncHTTP.rshardcodedremaining_redirect_count: 127.FetchTasklet::on_rejectconverts every HTTP client failure into aSystemError, whichSystemError__toErrorInstancematerializes withErrorType::Error.Fix
src/http/lib.rs: addDEFAULT_REDIRECT_COUNT = 20 + 1(do_redirectdecrements before checking for 0, so the stored budget is one larger than the number of redirects followed) and use it inmake_client. 20 redirects are followed and the 21st redirect response rejects, matching Node exactly. The existingmaxRedirectsfetch option still overrides the default, up to 126.src/jsc/bindings/bindings.cpp: factorSystemError__toErrorInstanceinto a helper parameterized onJSC::ErrorTypeand addSystemError__toTypeErrorInstance. Exposed to Rust asSystemError::to_type_error_instanceandBody::ValueError::SystemTypeError.FetchTasklet::on_reject: route the redirect failure codes (TooManyRedirects,UnexpectedRedirect,RedirectURLInvalid,InvalidRedirectURL,RedirectURLTooLong,UnsupportedRedirectProtocol) through the newTypeErrorpath. Every one of these is a network error under HTTP-redirect fetch. Messages and thecode,path, anderrnoproperties are unchanged, so existingerr.code === "UnexpectedRedirect"handling (and the tests that assert it) keeps working; only the error's constructor changes.Out of scope, deliberately: non-redirect network errors (
ConnectionRefused,ECONNRESET, DNS and TLS failures, ...) stay plainErrors. Those carrycodevalues users widely branch on, and widening further is a separate, larger compatibility decision.How did you verify your code works?
New tests in
test/js/web/fetch/fetch-redirect.test.ts, next to the existing redirect coverage. Onmain, 6 of the 7 fail (the 7th proves exactly 20 redirects is still allowed); all pass with the fix:Also ran
test/js/web/fetch/fetch.test.ts(maxRedirectsandredirect:blocks),test/js/web/fetch/client-fetch.test.ts,test/js/web/fetch/fetch-http2-client.test.ts,test/js/first_party/undici/undici.test.ts,test/js/bun/http/proxy.test.ts, andtest/integration/bun-types/bun-types.test.tsagainst the debug build; no new failures.[review] gate passed · iteration 1 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file