fetch: only reject WHATWG redirect statuses under redirect: 'error'

[robobun]

What does this PR do?

Per https://fetch.spec.whatwg.org/#redirect-status a redirect status is only 301, 302, 303, 307, or 308. fetch(url, {redirect: 'error'}) was gating on the full 300..=399 range, so 300 Multiple Choices, 304 Not Modified, 305 Use Proxy, and 306 all rejected with UnexpectedRedirect instead of being returned to the caller.

The follow and manual modes already handled these correctly (the follow arm matched only the five redirect statuses and fell through otherwise). Only the error-mode branch was too wide.

This breaks the standard conditional-request pattern: send If-None-Match, server replies 304 Not Modified, use the cached copy. With redirect: 'error' set defensively, every successful revalidation rejected.

Repro

import net from 'node:net';
const srv = net.createServer(sock => {
  sock.once('data', () =>
    sock.end('HTTP/1.1 304 Not Modified\r\nETag: "v1"\r\nContent-Length: 0\r\n\r\n'));
});
await new Promise(r => srv.listen(0, '127.0.0.1', r));
const res = await fetch('http://127.0.0.1:' + srv.address().port, {
  redirect: 'error',
  headers: { 'if-none-match': '"v1"' },
});
console.log(res.status); // should print 304; before: rejects UnexpectedRedirect
srv.close();

Node/undici returns 304 here; Bun rejected.

Fix

Narrow is_redirect in src/http/lib.rs to matches!(status_code, 301 | 302 | 303 | 307 | 308). The now-redundant inner match on the same set is dropped (the large diff is rustfmt re-indenting that block).

Verification

test/js/web/fetch/fetch-redirect.test.ts covers 300/304/304+Location/305/306 under both redirect: 'error' and redirect: 'follow' (returned with redirected: false), and 301/302/303/307/308 under redirect: 'error' (reject with code: 'UnexpectedRedirect').

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-redirect.test.ts

[robobun]

Reproduced on main (ff512ea): fetch(url, {redirect:'error'}) rejects 300/304/305/306 with UnexpectedRedirect. Fixed by narrowing the redirect-status check to the WHATWG set. Tests pass locally; the diff is just src/http/lib.rs + tests (the boringssl build-unblock landed separately as #36540).

CI on 0ce78df (build #86243): cargo clippy passes, fetch-redirect.test.ts passes on every lane. Remaining red is the darwin-14-aarch64 lane expiring without an agent, plus unrelated parallel-batch flakes (watch-many-dirs, napi GC timing, webview, etc.) that all passed on retry. Ready for review.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 703328d1-fa90-494f-aa6b-098777bfe807

📥 Commits

Reviewing files that changed from the base of the PR and between 03be6b0 and 0ce78df.

📒 Files selected for processing (1)

• test/js/web/fetch/fetch-redirect.test.ts

Walkthrough

The certificate alternate-name formatter now uses local safety checks. HTTP redirect handling now recognizes only standard Fetch redirect statuses. Tests cover rejection of those statuses and passthrough of other 3xx responses.

Changes

Alternate-name validation

Layer / File(s)	Summary
Alternate-name validation src/boringssl/lib.rs	NameBytes uses a local check for printable alternate-name bytes and excludes ", \, ,, and ' from unquoted output.

Fetch redirects

Layer / File(s)	Summary
Fetch redirect status handling src/http/lib.rs, test/js/web/fetch/fetch-redirect.test.ts	Automatic redirects now process only 301, 302, 303, 307, and 308. Existing URL, protocol, length, stream-body, method, header, and redirect-state handling remains within that status set. Tests verify rejection of these statuses and unchanged responses for other 3xx statuses.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change to redirect:error handling.
Description check	✅ Passed	The description explains the issue, fix, reproduction, and verification, including relevant test coverage.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 5041-5043: Add automated redirect: "follow" tests covering 300 and
305 responses with Location headers, using the existing redirect test helpers or
suite near the is_redirect gate. Assert these newly excluded statuses are
returned to the caller rather than followed, while preserving existing follow
behavior for recognized redirect statuses.

🪄 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: 1e4c0389-b760-45cd-906b-32f9b3ee6bfd

📥 Commits

Reviewing files that changed from the base of the PR and between ff512ea and 03be6b0.

📒 Files selected for processing (3)

• src/boringssl/lib.rs
• src/http/lib.rs
• test/js/web/fetch/fetch-redirect.test.ts

[robobun]

^{Updated 6:05 AM PT - Jul 31st, 2026}

@robobun, your commit 0ce78df is building: #86243

[claude]

LGTM — the redundant inner match was dropped as requested, so the redirect-status set is now spelled once.

What was reviewed:

• is_redirect narrowed to the WHATWG set; traced that follow/manual modes are unchanged (previously fell through _ => {}), only redirect: 'error' for 300/304/305/306 now returns instead of rejecting.
• is_redirect is used at exactly one site, so no other consumers are affected by the narrowing.
• Restored is_safe_alt_name matches ncrypto.cpp's non-UTF8 path (' '..='~' minus " \\ , '); the sole caller passed utf8=false.
• Tests cover both error and follow modes across all ten 3xx statuses with local servers and port: 0.

Extended reasoning...

Overview

Three files: src/http/lib.rs narrows is_redirect from 300..=399 to matches!(status_code, 301 | 302 | 303 | 307 | 308) and drops the now-redundant inner match (pure de-indent of ~250 lines); test/js/web/fetch/fetch-redirect.test.ts adds a describe covering 300/304/304+Location/305/306 under both error and follow plus 301–308 rejection under error; src/boringssl/lib.rs restores is_safe_alt_name as a local helper because #36165 re-added a call to X509::is_safe_alt_name after #36252 removed it, breaking main.

Security risks

None. Narrowing which statuses count as redirects to the exact WHATWG set cannot cause a redirect to be followed that wasn't before — follow mode already gated on the same five statuses via the inner match. The only behavioral delta is that redirect: 'error' stops rejecting non-redirect 3xx responses (304 etc.), which is a compliance fix matching Node/undici. The is_safe_alt_name restore is byte-equivalent to the C++ IsSafeAltName non-UTF8 path in ncrypto.cpp:753 and only affects error-message quoting, not certificate validation.

Level of scrutiny

Low-to-moderate. The diff looks large but is almost entirely rustfmt re-indentation after removing a match wrapper. The actual semantic change is one line, spec-cited, and matches the reference implementation. I confirmed is_redirect has no other consumers (rg shows two hits: the definition and the immediately-following if). The de-indented follow-mode body is unchanged token-for-token.

Other factors

My prior review's nit (drop the dead inner match) was addressed in 03be6b0. The comment-cop hits on the re-indented block were correctly identified as pre-existing spec citations, and the CodeRabbit request for follow-mode coverage was addressed in 229d65f (author correctly noted follow-mode behavior didn't change, but added the tests as regression guards). All threads resolved. Tests follow harness conventions: port: 0, local net.createServer, it.concurrent.each, try/finally cleanup, exact-value assertions on {status, redirected, body} and {rejected, code}.

[claude]

LGTM — the one-line is_redirect narrowing is spec-correct and the rest of the src/http/lib.rs diff is pure rustfmt re-indentation after dropping the now-dead inner match. The remaining inline note about the two 304 rows sharing a test title is cosmetic and doesn't need to block.

Extended reasoning...

Overview

Two files: src/http/lib.rs narrows is_redirect from 300..=399 to the WHATWG set {301, 302, 303, 307, 308}, and drops the inner match status_code { 302|301|307|308|303 => … _ => {} } wrapper that this narrowing made redundant (per my earlier comment, now addressed). Everything else in that file is rustfmt de-indenting the follow-mode body one level — I compared each hunk and the logic (stream-body guard, three Location-parsing branches, method→GET rewrite, cross-origin header strip, is_redirect_pending/resend_request_body_on_redirect flags) is byte-for-byte preserved modulo whitespace and line-wrapping. test/js/web/fetch/fetch-redirect.test.ts adds a table-driven suite covering 300/304/304+Location/305/306 under both error and follow, plus 301–303/307/308 under error. The unrelated src/boringssl/lib.rs build-fix hunk from earlier revisions was dropped after merging main (#36540 landed it), so the diff is now clean.

Behavioral analysis

The only observable change is in the FetchRedirect::Error arm: previously any 3xx entered if is_redirect and hit the else if … Error branch → UnexpectedRedirect; now only the five WHATWG redirect statuses do. Follow-mode is provably unchanged: the old inner match already fell through _ => {} for 300/304/305/306, landing at the same normal-response path that is_redirect = false now reaches directly. Manual mode never entered this block. I also checked that the else if structure means narrowing the outer gate can't newly admit any status into a branch it didn't reach before.

Security risks

None. This loosens an error rejection to match spec, but the responses in question (300/304/305/306) were never followed before and still aren't — they're just returned to the caller instead of thrown. No new URL parsing, credential handling, or origin logic is reachable that wasn't already.

Level of scrutiny

Medium — HTTP client redirect handling is correctness-sensitive, but the actual change is a single predicate matched against a well-defined spec list, and the large diff footprint is mechanical re-indentation. The prior review round already caught and fixed the dead-code concern; the CodeRabbit follow-mode question was answered (and withdrawn) with a correct explanation and extra regression coverage was added anyway.

Other factors

Tests are hermetic (local net.createServer on port: 0, try/finally cleanup, it.concurrent.each), assert the full {status, redirected, body} object, and cover both flag states of redirect. The one open finding — two 304 table rows producing identical test titles — is a diagnostic-only nit; both cases still register and run independently. All prior inline threads on the PR are resolved.

[claude]

LGTM — both earlier nits are addressed and the diff is now the one-line is_redirect narrowing plus rustfmt de-indent from dropping the redundant inner match.

What was reviewed:

• Confirmed the de-indented Follow block is byte-identical to the original modulo whitespace; is_redirect is only read at line 5043, so no other consumer is affected.
• Checked follow-mode for 300/304/305/306 is unchanged (previously fell through _ => {}, now skips the block — same net effect); only error-mode behavior changes.
• Looked for sibling redirect-status checks — Response::validate_redirect_status_code already uses the same WHATWG set, nothing else to sync.
• Tests cover both flag states, all five non-redirect 3xx (incl. 304+Location), and all five reject cases with exact code assertion; titles are now distinct.

Extended reasoning...

Overview

Narrows is_redirect in HTTPClient::handle_response_metadata (src/http/lib.rs:5042) from 300..=399 to the WHATWG redirect-status set {301, 302, 303, 307, 308}, and drops the inner match status_code { 302|301|307|308|303 => {...} _ => {} } that the outer gate now makes redundant. The ~250-line diff in lib.rs is entirely rustfmt re-indentation of that block after the wrapper was removed — I diffed each hunk and found no logic drift. Adds a describe block to test/js/web/fetch/fetch-redirect.test.ts covering 300/304/304+Location/305/306 under both redirect: 'error' and redirect: 'follow', plus 301/302/303/307/308 under redirect: 'error' asserting code: 'UnexpectedRedirect'.

Security risks

None. This loosens rejection (304 etc. now resolve instead of throwing), which is the spec-mandated behavior and matches Node/undici. No new URL parsing, no new allocation, no trust-boundary change. The Location header on non-redirect 3xx is now ignored rather than triggering an error — which is correct; it was never followed under follow mode either (the old inner match already excluded these statuses).

Level of scrutiny

Medium. The semantic change is one line and spec-cited (https://fetch.spec.whatwg.org/#redirect-status). The rest is a mechanical de-indent I verified is content-preserving. I checked that is_redirect is a local used only at line 5043 (all other is_redirect* grep hits are the unrelated is_redirect_pending state flag), and that the only sibling redirect-status set in the tree (Response::validate_redirect_status_code in src/runtime/webcore/Response.rs) already matches.

Other factors

Both prior review nits from me (redundant inner match; duplicate 304 test titles) were addressed in 03be6b0 and 0ce78df. CodeRabbit's follow-mode-coverage suggestion was addressed and its incorrect premise about a follow-mode behavior change was withdrawn. All inline threads are resolved. The comment-cop flags were false positives on pre-existing spec-citation comments that were only re-indented. Tests use port: 0, local net.createServer, try/finally cleanup, it.concurrent.each, and exact-value assertions — consistent with the file's existing patterns and REVIEW.md test guidance.