Skip to content

Response.redirect: parse and serialize the url into the Location header - #33126

Merged
Jarred-Sumner merged 5 commits into
mainfrom
farm/64a7d49a/response-redirect-url-parse
Jun 30, 2026
Merged

Jarred-Sumner merged 5 commits into
mainfrom
farm/64a7d49a/response-redirect-url-parse

Conversation

@robobun

@robobun robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator

Response.redirect(url) placed its argument into the Location header verbatim instead of parsing and serializing it, as https://fetch.spec.whatwg.org/#dom-response-redirect steps 1 and 6 require.

Reproduction

Response.redirect("http://example.com/a b").headers.get("location");
// expected (Node, spec): "http://example.com/a%20b"     bun: "http://example.com/a b"

Response.redirect("http://x/é").headers.get("location");
// expected: "http://x/%C3%A9"                           bun: "http://x/é"

Response.redirect("HTTP://U:P@EX.COM:80/p/../q").headers.get("location");
// expected: "http://U:P@ex.com/q"                       bun: the raw string

Response.redirect("http://x/a\nb").headers.get("location");
// expected: "http://x/ab" (the URL parser strips ASCII tab and newline)
// bun: TypeError: Header '51' has invalid value: 'http://x/a\nb'

Cause

Response::construct_redirect_impl never ran its argument through a URL parser. It converted the JS string to UTF-8 bytes and put those bytes into Location as Latin-1, so

  • the spec's parse + serialize step (percent-encoding, tab/newline stripping, scheme and host lowercasing, default-port removal, dot-segment resolution, punycode) never happened, and
  • any non-ASCII input came back corrupted, because FetchHeaders::put wrapped its &[u8] argument in a Latin-1-tagged ZigString.

Separately, canWriteHeader's HTTPHeaderName overload in FetchHeaders.cpp formatted the enum's numeric value into the error message (Location is 51). That is reachable from any well-known header name, for example new Headers({ location: "a\nb" }).

Fix

  • construct_redirect_impl now runs the argument through bun_url::href_from_string (the same WHATWG parser new Request(url) already uses) and puts the serialized href into Location.

    A non-absolute input keeps the raw string. The spec would throw a TypeError there, but Response.redirect("/login") is documented Bun behavior (docs/runtime/http/server.mdx, docs/guides/http/server.mdx) and widely used, so it is preserved; this is called out in a code comment.

  • FetchHeaders::put takes a &BunString instead of &[u8]. A WTFStringImpl-tagged value carries its encoding and gets ref'd by the C++ side (BunString::toWTFString) instead of having its characters copied, exactly as Headers.prototype.set does. Response.redirect("/café") previously produced Location: /café because that path went through a Latin-1 read of the JS string's UTF-8 bytes; Response.render(path) (the bake dev server) had the same round-trip and now hands the JS string straight to put, and construct_redirect_impl hands it the parsed href (already a BunString) with no re-encode. The remaining byte-slice callers (the content-type sites, the presigned S3 url, the JSON mime) wrap in BunString::ascii, which is the same ZigString::init that put built internally before, so they are unchanged.

    One deliberate consequence: a relative target containing a code point above U+00FF, which cannot be a header value, now throws TypeError: Header 'Location' has invalid value. That is exactly what headers.set("location", "/€"), new Headers({ location: "/€" }), and new Response(null, { headers: { location: "/€" } }) already do for the same input; Response.redirect was the one path that instead silently wrote a Latin-1-corrupted value (Response.redirect("/€") produced Location: /â¬). A test covers both sides of the boundary (/café passes, /€ throws).

  • canWriteHeader(HTTPHeaderName, ...) uses httpHeaderNameString(name) in its error message, so the example above now reports Header 'Location' has invalid value.

Two existing tests asserted the unserialized Location value and were updated: http://example.com serializes as http://example.com/, matching Node and the spec.

Two elysia vendor tests (adapter/web-standard/map-response.test.ts and map-early-response.test.ts) pin the unserialized Location and are skipped in test/vendor.json with a TEMPORARY note, following the existing ws*connection.test.ts entry; they are the only Response.redirect assertions in elysia 1.4.28's test tree.

Not in this PR

The same statics also have a non-null .body where the spec says null. #33125 already fixes that, so this PR does not touch those lines.

Verification

bun bd test test/js/web/fetch/response.test.ts \
            test/js/web/fetch/fetch_headers.test.js

Both fail without the src/ change and pass with it. Also ran the headers, serve-static, serve-if-none-match, cookie, proxy, client-fetch, and bake response regression suites against the fixed build with no new failures.

Response.redirect(url) placed the raw argument string into Location:
spaces were not percent-encoded, non-ASCII came back as a Latin-1 read
of the UTF-8 bytes, no normalization ran, and an embedded tab or
newline threw "Header '51' has invalid value" instead of being
stripped by the URL parser.

Run the argument through the WHATWG URL parser and put the serialized
href into Location. A non-absolute input keeps the raw string, since
relative redirect targets are documented Bun behavior. FetchHeaders::put
now takes a ZigString so that fallback passes the JS string in its
native encoding, and the HTTPHeaderName header-validation error names
the header instead of its enum index.
@robobun

robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:51 AM PT - Jun 30th, 2026

❌ @robobun, your commit 14ea8ce has 2 failures in Build #67150 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33126

That installs a local version of the PR into your bun-33126 executable, so you can run:

bun-33126 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Malformed Response.redirect should throw error #24002 - Response.redirect with invalid URL arguments now goes through the WHATWG URL parser, which may address the missing validation reported here

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #24002

🤖 Generated with Claude Code

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

This PR does not fix #24002, so I am not adding Fixes #24002.

That issue asks Response.redirect to throw a TypeError when the url argument does not parse, which is what the spec and Node do. This PR intentionally keeps Bun's non-throwing behavior for unparseable input: if the argument does not parse as an absolute URL, the raw string still goes into Location. Two reasons:

  • Bun documents relative redirect targets (Response.redirect("/source", 301) in docs/guides/http/server.mdx, Response.redirect("/blog/hello/world") in docs/runtime/http/server.mdx), and a relative reference is exactly what the WHATWG parser rejects when there is no base URL. Making parse failure throw would break that documented API.
  • There is already a test asserting the opposite of Malformed Response.redirect should throw error #24002: test/js/web/fetch/response.test.ts has expect(() => Response.redirect(400, "a")).not.toThrow(), added for Malformed Response.redirect causes crash #18414.

Concretely, Response.redirect(420, "blaze it") on this branch still does not throw; it produces Location: 420, same as before. Deciding to start throwing there has to be reconciled with the relative-URL support and with #18414 first, so it belongs in its own change.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

FetchHeaders header-value APIs now take &ZigString. Request, Response, and BakeResponse pass ZigString values to headers.put. Redirect Location handling normalizes absolute URLs through bun_url::href_from_string. Tests were updated for serialization, error text, snapshot output, and related skips.

Changes

FetchHeaders ZigString encoding and redirect normalization

Layer / File(s) Summary
FetchHeaders API and error text
src/jsc/FetchHeaders.rs, src/jsc/bindings/webcore/FetchHeaders.cpp
put_default and put accept &ZigString values and forward them directly; invalid-header TypeError text formats the header name with httpHeaderNameString(name).
Header value call-site migration
src/runtime/webcore/Request.rs, src/runtime/webcore/Response.rs, src/runtime/webcore/BakeResponse.rs
Content-Type and Location header insertions now pass &ZigString::init(...) or get_zig_string(...) values into headers.put.
Redirect Location normalization
src/runtime/webcore/Response.rs
construct_redirect_impl builds url_string, resolves href with bun_url::href_from_string, and chooses either the original string or the normalized UTF-8 href for Location.
Updated fetch and response tests
test/js/web/fetch/fetch.test.ts, test/js/web/fetch/fetch_headers.test.js, test/js/web/fetch/response.test.ts, test/vendor.json
Tests now expect redirect URL normalization, preserved relative redirects, header-name error messages, the updated response snapshot size, and related temporary skips.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: parsing and serializing the redirect URL before putting it into Location.
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.
Description check ✅ Passed The PR description covers the change and verification details, even though it uses custom section names instead of the template headings.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/runtime/webcore/BakeResponse.rs`:
- Around line 203-207: The `headers.put` call in `BakeResponse` is re-wrapping
the path through `path_utf8.slice()` and `ZigString::init(...)`, which drops the
original JS encoding metadata. Update the `Location` header write to pass the
original `ZigString` source for the path, using the same
`path_arg`/`global_this` retrieval path already available in this scope (for
example via `path_arg.get_zig_string(global_this)?`), and avoid the UTF-8
round-trip so non-ASCII values stay intact.
🪄 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: 75a7fafd-9c2b-44d3-9ba9-1972a956d41c

📥 Commits

Reviewing files that changed from the base of the PR and between 459c33f and 921f656.

📒 Files selected for processing (8)
  • src/jsc/FetchHeaders.rs
  • src/jsc/bindings/webcore/FetchHeaders.cpp
  • src/runtime/webcore/BakeResponse.rs
  • src/runtime/webcore/Request.rs
  • src/runtime/webcore/Response.rs
  • test/js/web/fetch/fetch.test.ts
  • test/js/web/fetch/fetch_headers.test.js
  • test/js/web/fetch/response.test.ts

Comment thread src/runtime/webcore/BakeResponse.rs Outdated
… encoding

Response.render(path) had the same UTF-8-bytes-read-as-Latin-1 round
trip that corrupted non-ASCII Response.redirect(url) values. Use the JS
string's ZigString view directly, as the Response.redirect fallback now
does, instead of converting to UTF-8 bytes first. The now-unused
path_str/path_utf8 bindings are removed.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/runtime/webcore/BakeResponse.rs:203-207 — The PR description states "the other put callers all pass ASCII", but construct_render does not: path_utf8.slice() is UTF-8 bytes from a user-supplied JS string, and ZigString::init tags them Latin-1, so Response.render("/café") still produces Location: /café. This is the same bug class the PR fixes for Response.redirect("/café") on a line the PR touched; init → from_bytes (or pass path_arg.get_zig_string(...) directly, as the redirect path now does) would close it. Pre-existing behavior, not a regression.

    Extended reasoning...

    What

    construct_render in src/runtime/webcore/BakeResponse.rs puts the user-supplied path into the Location header via:

    let path_utf8 = path_str.to_utf8();
    headers.put(
        HTTPHeaderName::Location,
        &ZigString::init(path_utf8.slice()),
        global_this,
    )?;

    path_utf8.slice() is the UTF-8 encoding of an arbitrary JS string (Response.render(path)). ZigString::init (src/bun_core/string/mod.rs:1437) stores the pointer with no encoding tag, which means Latin-1; only init_utf8/from_bytes set the UTF-8 tag. On the C++ side, WebCore__FetchHeaders__put → Zig::toStringCopy (helpers.h:187) checks isTaggedUTF8Ptr; when not tagged, the bytes go through WTF::StringImpl::create as Latin-1 code points.

    Step-by-step

    1. JS calls Response.render("/café") inside the Bake dev server.
    2. path_str.to_utf8() yields the UTF-8 bytes 2F 63 61 66 C3 A9 (é → 0xC3 0xA9).
    3. ZigString::init(path_utf8.slice()) wraps those bytes with no UTF-8 ptr tag → treated as Latin-1.
    4. headers.put(...) → Zig::toStringCopy sees !isTaggedUTF8Ptr, calls StringImpl::create on the raw byte span, producing the WTF string "/café" (six Latin-1 code points).
    5. headers.get("location") → "/café" instead of "/café".

    Why nothing prevents it

    The PR explicitly changed FetchHeaders::put from &[u8] to &ZigString so callers can express the correct encoding, and the PR description justifies wrapping the remaining call sites in ZigString::init with "the other put callers all pass ASCII". That is true for the content-type / S3-URL / JSON-mime callers, but not for construct_render, whose argument is arbitrary user input. The new put doc comment even says "a raw &[u8] parameter would force every caller into a Latin-1 read" — ZigString::init on UTF-8 bytes does exactly that.

    Impact

    Low. Response.render is a Bake/dev-server-only API, and route paths are almost always ASCII, so this rarely bites in practice. It is also pre-existing: before this PR, put took &[u8] and did ZigString::init internally, so behavior is byte-identical to before. But it is the same bug class the PR fixes for Response.redirect("/café"), on a line the PR explicitly rewrote, and it contradicts the PR's stated rationale.

    Fix

    One token: ZigString::init(path_utf8.slice()) → ZigString::from_bytes(path_utf8.slice()) (auto-tags UTF-8 when non-ASCII). Alternatively, skip the UTF-8 round-trip entirely and pass the JS string's native ZigString (path_arg.get_zig_string(global_this)?), exactly as the relative-redirect fallback now does.

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

The BakeResponse finding is already addressed in 4d8b19e, which this review raced with: construct_render now passes the original JS string via path_arg.get_zig_string(global_this)? directly into FetchHeaders::put, and the now-unused path_str / path_utf8 bindings were removed.

CodeRabbit raised the same thing against the first commit; its incremental re-review of 4d8b19e reported no remaining findings. The existing Response.render() devTests in test/bake/dev/react-response.test.ts still pass against the change.

Comment thread src/runtime/webcore/Response.rs Outdated
…tion

Response.redirect now serializes its url into Location, so
Response.redirect('https://cunny.school') yields 'https://cunny.school/',
matching Node and the fetch spec. elysia 1.4.28's 'map redirect' tests in
adapter/web-standard/map-response.test.ts and map-early-response.test.ts
assert the unserialized string and fail deterministically on every
platform. Skip them until the upstream assertion expects the trailing
slash.
Comment thread src/runtime/webcore/Response.rs Outdated
…rgets

A relative redirect target containing a code point above U+00FF cannot be
a header value. Response.redirect now throws the same TypeError that
headers.set('location', s) and new Headers({ location: s }) already
throw for that input; it previously wrote a Latin-1-corrupted Location
without throwing. Test both sides of the boundary: '/café' (U+00E9)
round-trips, '/€' (U+20AC) and '/搜索' throw.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing both findings (vendor skips in 58a5acb, the U+00FF boundary test in aebb8f1) — I have nothing further, but since this is a user-facing Response.redirect behavior change with a deliberate compat tradeoff on the relative-URL fallback, I'd like a human to sign off.

Extended reasoning...

Overview

This PR makes Response.redirect(url) parse and serialize its argument through the WHATWG URL parser before placing it into the Location header (fetch spec steps 1 & 6), instead of passing the raw string. It also widens FetchHeaders::put from &[u8] to &ZigString so callers preserve the JS string's native encoding, updates ~7 call sites in Request.rs/Response.rs/BakeResponse.rs, fixes the canWriteHeader(HTTPHeaderName, ...) error message in FetchHeaders.cpp to print the header name instead of its enum index, adds tests, and skips two elysia vendor tests pinned to the old unserialized output.

Security risks

None identified. The change tightens header-value handling (URL serialization percent-encodes/strips control chars; the relative fallback now goes through the same isValidHTTPHeaderValue gate as Headers.set). No auth, crypto, or permission surfaces touched.

Level of scrutiny

Medium-high. This is a user-facing web API behavior change with two deliberate compat decisions a maintainer should ratify: (1) Bun continues to accept relative redirect targets (diverging from the spec's TypeError, preserving documented Bun behavior and #18414's test), and (2) relative targets containing code points > U+00FF now throw TypeError instead of silently writing mojibake — the author chose option (a) over percent-encoding the fallback, with a well-argued rationale, but it is a new throw on a path that previously returned a Response. The &[u8] → &ZigString signature change also crosses the Rust↔C++ FFI encoding boundary across several call sites.

Other factors

Both of my prior inline findings are resolved: 58a5acb added the elysia skipTests entries with verified globs, and aebb8f1 added the /café ↔ /€//搜索 boundary test plus an updated PR description. The bug-hunting system found nothing on this revision. CI on 58a5acb shows only two failures (test/package.json timeout on macOS aarch64, test-net-connect-memleak.js on Linux x64) that are unrelated to this change. Test coverage for the new behavior is thorough (serialization table, relative fallback, header-value boundary, error-message fix). The remaining reason not to auto-approve is purely that this is an intentional behavior change to a public API, not a mechanical fix.

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

All review findings are resolved (3 of 3 threads, plus CodeRabbit's incremental re-review reporting no actionable comments), and this is ready for a maintainer.

Why buildkite/bun is red

It is not this diff. Build 67118 ran this PR's exact src/ change to near completion: 278 of its 287 jobs passed, and every one of its 4 failed jobs falls into two buckets with no connection to the change:

  • darwin 26 aarch64 - test-bun (2 jobs). The agent's buildkite-agent artifact download and bun install both time out before a single test runs. This is a fleet-wide CI issue right now, not this branch: the last 5 builds on the pipeline, across 5 different branches, all have a failed darwin 26 aarch64 shard with the same timeout, and there is already a runner fix in flight on farm/eb157405/retry-artifact-download-timeout. Every build of this PR (67101, 67109, 67118, 67129) hits it, which is also why I am not pushing an empty retrigger; it would land on the same agents.
  • test/js/node/test/parallel/test-net-connect-memleak.js on Alpine (2 jobs). A conservative-GC liveness assertion (assert.strictEqual(collected, true) after globalThis.gc()) on a net.connect socket. It also fails on unrelated branches (3 of the last 12 non-main builds), and it has no overlap with Response.redirect or FetchHeaders.

There were no other failures. In particular, the two elysia vendor tests that were the one real piece of CI fallout from the serialization change stopped failing once they were skipped in test/vendor.json (58a5acb): build 67118 has zero test-failure annotations beyond the Alpine one above.

What needs a human call

Two deliberate decisions, both described in the PR body and in code comments:

  1. Relative redirect targets keep the raw string instead of throwing the spec's TypeError. Response.redirect("/login") is documented Bun behavior (docs/runtime/http/server.mdx, docs/guides/http/server.mdx), and test/js/web/fetch/response.test.ts already asserts Response.redirect(400, "a") does not throw (Malformed Response.redirect causes crash #18414). Malformed Response.redirect should throw error #24002 asks for the spec's throw and is left as a separate decision.
  2. A relative target containing a code point above U+00FF now throws the same header-validation TypeError that headers.set("location", s), new Headers({ location: s }), and new Response(null, { headers: { location: s } }) already throw for that input. Response.redirect was the one path that instead silently wrote a Latin-1-corrupted Location. Both sides of the boundary are tested.

Comment thread src/runtime/webcore/BakeResponse.rs Outdated

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use BunString instead.

A WTFStringImpl-tagged BunString lets the C++ side ref the existing
string (BunString::toWTFString) instead of copying its characters.
Response.redirect now hands the parsed href, or the JS string's own WTF
string on the relative fallback, straight to the header map, and
Response.render does the same, which also removes their ZigString/UTF-8
round-trips. The ASCII byte-slice callers wrap in BunString::ascii, the
exact ZigString the previous &[u8] signature built internally.
@robobun

robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI for 14ea8ce (the BunString change): build 67150 finished at 283 passed / 3 failed, and none of the 3 is this PR.

I am not pushing a retrigger: the Alpine test fails on essentially every build until #33045 lands, so a re-roll would come back red on it. The darwin-26 artifact-download infra issue from my earlier comment has cleared (all darwin 26 jobs passed on this build). Once #33045 is in, this PR's CI should be fully green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants