Skip to content

Bun.serve: register HTML import outputs under their percent-encoded path - #41615

Closed
robobun wants to merge 1 commit into
mainfrom
robobun/067045aa/html-serve-encoded-routes
Closed

robobun wants to merge 1 commit into
mainfrom
robobun/067045aa/html-serve-encoded-routes

Conversation

@robobun

@robobun robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • A production Bun.serve HTML import (and a compiled executable) serves an asset whose name has a space or a non-ASCII character as a 404. The page says src="/my img-24bdys0j.png". The browser requests /my%20img-24bdys0j.png. Only the raw bytes /my img-24bdys0j.png match. The dev server is not affected, it serves /_bun/asset/<hash>.png.
  • The cause: HTMLBundle.rs:613 and server_body.rs:620 key the static route on the raw dest_path. The router compares request-target bytes as-is.

Fix

  • percent_encode_route_path (src/runtime/server/mod.rs) encodes a path the way a browser does before a request: the WHATWG URL path percent-encode set (C0 controls, space, ", #, <, >, ?, `, {, }, DEL and non-ASCII), upper-case hex. Both registration sites use it.
  • Correct because the page keeps the raw name, which is valid HTML, and every browser encodes the same way. The route key now equals the request target. It does not change the router or the matcher. A name with no byte in the set is registered unchanged (Cow::Borrowed).
  • Verified: test/js/bun/http/bun-serve-html.test.ts (production build, space and ünï) and test/bundler/bundler_html_server.test.ts (HTMLServerEncodedAssetRoute, compiled, both backends). Both fail on 1.4.3 with 404. Also ran bun-serve-routes.test.ts, bun-serve-static.test.ts, bundler_html.test.ts.

Background

  • A production HTML import bundles once, then registers one static route per output file (JS, CSS, copied assets, the page) on the server. The page is served under the route it was mounted on.
  • A compiled executable (bun build --compile) embeds the same outputs and registers them from the HTML import manifest at server start. That is the second site.
  • The router matches the request target against the route key byte for byte. It does not percent-decode. This PR does not change that. It makes the keys for bun's own outputs match what browsers send.
Notes

Repro on 1.4.3:

D=$(mktemp -d); cd $D
printf '<!doctype html><html><body><img src="./my img.png"><img src="./ünï.png"><script type="module" src="./app.ts"></script></body></html>' > index.html
printf 'PNG' > "my img.png"; cp "my img.png" "ünï.png"; echo 'export{}' > app.ts
cat > serve.mjs <<'JS'
import p from "./index.html";
const s = Bun.serve({ port: 0, development: false, routes: { "/": p }, fetch: () => new Response("no route", { status: 404 }) });
const h = await (await fetch(s.url)).text();
for (const m of h.matchAll(/src="(\/[^"]+\.png)"/g)) console.log(m[1], "->", encodeURI(m[1]), (await fetch(s.url.origin + encodeURI(m[1]))).status);
s.stop(true);
JS
bun serve.mjs
# /my img-24bdys0j.png -> /my%20img-24bdys0j.png 404
# /ünï-24bdys0j.png -> /%C3%BCn%C3%AF-24bdys0j.png 404

With this change both return 200. bun build --compile serve.mjs had the same two 404s and returns 200 with this change.

bun_core::percent_encode_write was not reused on purpose. Its escape set is the one for source map URLs: it does not escape a space and it escapes %, ~, [, ], |, ^, \. A browser leaves those as they are in a path.

# and ? in an output name are escaped like the rest of the set, but a browser parses them as the fragment and the query, so such a name stays unreachable either way. That is a naming problem, not a route problem.

The PRs for the router side (#35249, #33096) normalize every request target. This change is independent of them: it only changes the keys bun registers for its own outputs.

Found by a fuzz pass over HTML import reference kinds. No user report.

The page carries the raw output name, for example /my img-24bdys0j.png.
A browser percent-encodes the path before the request, so it asks for
/my%20img-24bdys0j.png. The static route was keyed on the raw bytes and
the request got a 404. The production build of an HTML import and the
manifest of a compiled executable now register each output under the
WHATWG path percent-encoded form of its path.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.

Or wait 3 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 174737bf-7ae0-4f7b-9ca7-3b64784d42fb

📥 Commits

Reviewing files that changed from the base of the PR and between d316760 and e69e210.

📒 Files selected for processing (5)
  • src/runtime/server/HTMLBundle.rs
  • src/runtime/server/mod.rs
  • src/runtime/server/server_body.rs
  • test/bundler/bundler_html_server.test.ts
  • test/js/bun/http/bun-serve-html.test.ts

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

@github-actions github-actions Bot added the claude label Sep 6, 2026
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:21 AM PT - Sep 6th, 2026

❌ @robobun, your commit e69e210 has 2 failures in Build #111246 (All Failures):

  • test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan
  • ❌ CPU instruction violation on Linux x64 — 1 check(s) failed
  • The baseline build contains instructions not available on Nehalem (SSE4.2, no AVX/AVX2/AVX512).

    • Static instruction scan

    Static scan violations

    llint_op_wide16  [INVLPGB]  (1 insns)
    

    If these are runtime-dispatched behind a CPUID gate: add each symbol to scripts/verify-baseline-static/allowlist-x64.txt with a comment pointing at the gate.

    If there's no gate: this is a real bug — a -march leaked into a subbuild.

  • ❌ CPU instruction violation on Linux x64 — 1 check(s) failed
  • The baseline build contains instructions not available on Nehalem (SSE4.2, no AVX/AVX2/AVX512).

    • Static instruction scan

    Static scan violations

    llint_op_wide16  [INVLPGB]  (1 insns)
    

    If these are runtime-dispatched behind a CPUID gate: add each symbol to scripts/verify-baseline-static/allowlist-x64.txt with a comment pointing at the gate.

    If there's no gate: this is a real bug — a -march leaked into a subbuild.


🧪   To try this PR locally:

bunx bun-pr 41615

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

bun-41615 --bun

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on bun 1.4.3 with the commands in the Notes block. Fixed and tested on this branch. Waiting for CI.

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I checked needs_escape against the WHATWG URL path percent-encode set — it matches exactly (C0 controls via < 0x20, >U+007E via >= 0x7F, plus space " # < > ? ` { }), and both registration sites (HTMLBundle.rs and server_body.rs) are covered.

Extended reasoning...

The core correctness question is whether the escape set matches what browsers actually send, and it does — the WHATWG path percent-encode set is the C0-control set (U+0000–U+001F and >U+007E) plus space, ", #, <, >, ?, `, {, }; the byte < 0x20 || byte >= 0x7F || matches!(...) predicate is byte-for-byte that set. Both call sites that register bundler outputs as static routes now go through the helper, and the Cow::Borrowed fast path means ASCII-clean names (the overwhelming common case) allocate nothing. The tests cover both the runtime Bun.serve production path and the compiled-executable path across the existing backend loop, and assert the encoded pathname the browser would produce.

Comment thread src/runtime/server/mod.rs
let mut out = Vec::with_capacity(path.len() + 16);
for &byte in path {
if needs_escape(byte) {
let hex = bun_core::fmt::hex2_upper(byte);

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.

🟡 nit (optional): bun_core::fmt::hex2_upper is a #[doc(hidden)] compat alias (fmt.rs:2858 "compat aliases (pre-dedup names)"); the canonical helper is hex_byte_upper. Fix: call bun_core::fmt::hex_byte_upper(byte) so new code doesn't add a caller to the alias being phased out.

Extended reasoning...

src/bun_core/fmt.rs:2858-2863 marks hex2_upper as #[doc(hidden)] under a "compat aliases (pre-dedup names)" banner and delegates to hex_byte_upper at line 2851 (whose doc even says "Used by percent-encoders"). New percent-encoders in the tree (src/md/html_renderer.rs:500, src/resolver/data_url.rs:274) already use hex_byte_upper; the remaining hex2_upper sites carry comments like js_printer/lib.rs:877 "remaining \xHH site below" indicating they are leftovers, not the name to reach for. No behaviour difference — purely which name new code should use.

Verification: nit — The new code at src/runtime/server/mod.rs:184 calls bun_core::fmt::hex2_upper(byte). In src/bun_core/fmt.rs:2858-2863, hex2_upper sits under the banner // ── compat aliases (pre-dedup names) ──, is #[doc(hidden)], and just delegates to hex_byte_upper(b) (line 2862). The canonical helper is hex_byte_upper at fmt.rs:2851, whose doc comment explicitly says "Used by… | nit — the…

@robobun

robobun commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up for whoever lands second: #41793 makes the bundler percent-encode the output paths it writes into an HTML entry point's src/href (a name like shot #1.png is cut at the # by the browser otherwise). It adds bun_core::strings::percent_encode_url_path, which escapes the WHATWG path percent-encode set plus %, \ and ^.

For the route key registered here to equal the URL the page then carries for every name, percent_encode_route_path needs the same byte set. Today it leaves %, \ and ^ raw, so an output named 100%-<hash>.png would be requested as /100%25-<hash>.png and miss. Either add those three bytes here or switch to the shared helper once #41793 is in. The test expectations on the raw src value (/my img-…png) become /my%20img-…png after #41793.

@robobun

robobun commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #41793. That PR now registers the Bun.serve HTML-import routes (HTMLBundle.rs) and the compiled-manifest routes (server_body.rs) under bun_core::strings::percent_encode_url_path(dest_path), the same helper the bundler uses for the URL it writes into the page, and keeps the raw name as a second key. One helper for both sides means the route key and the emitted URL cannot disagree on %, \ or ^. The cases tested here (space and non-ASCII names, HTML import and --compile) are covered there, with shot #1.png and 100%.png added.

@robobun robobun closed this Sep 8, 2026
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