Conversation
The route table matched the raw request-target while request.url is built with the URL parser, which resolves dot-segments (including their %2e spellings), treats a backslash as a slash, and ends the path at #. A raw target like "GET /w/../admin/x" was dispatched to the "/w/*" route (or to the fetch fallback) while the handler observed request.url ending in "/admin/x", so per-route handlers and guards could be bypassed by the spelling of the request-target. Normalize the path inside uWS::HttpRouter::route() the same way the URL parser does, so routing and request.url agree. Percent-decoding is not applied. node:http is unaffected: it only registers "/*" and keeps exposing the raw request-target on IncomingMessage.url.
|
Updated 5:23 PM PT - Jun 29th, 2026
❌ @robobun, your commit bfe2f3a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33096That installs a local version of the PR into your bun-33096 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Walkthrough
ChangesURL Normalization in HttpRouter
Documentation updates
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/bun-uws/src/HttpRouter.h`:
- Around line 174-177: The comment on the request-path normalization contract is
too long and needs to be reduced to three lines while preserving the key
behavior. Update the documentation near the routing logic in HttpRouter.h so it
still states that handlers see the URL-parser-normalized path, that routes must
match that normalized path rather than the raw request-target, and that
percent-decoding is not performed; keep the wording tighter without changing the
meaning.
In `@test/js/bun/http/bun-serve-routes.test.ts`:
- Around line 1037-1065: The route-normalization table in
bun-serve-routes.test.ts covers fragment-delimited paths but is missing the
query-delimiter variant. Add a case in the existing cases array near the
fragment example in the route-matching test so the same behavior is verified for
a target containing ? and confirms matching stops before the query string. Keep
it aligned with the existing matrix-driven style used in the test.
🪄 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: 923c0a74-033b-41b5-981a-c81bb27d6579
📒 Files selected for processing (2)
packages/bun-uws/src/HttpRouter.htest/js/bun/http/bun-serve-routes.test.ts
…rgets in the test
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-uws/src/HttpRouter.h (1)
345-368: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
route()against re-entrant use on the same router.normalizeUrl()writes into sharednormalizedUrlBuffer, andcurrentUrl/routeParametersare mutable router state, so a handler that callsroute()again can overwrite the outer request’s segments and params mid-dispatch.🤖 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 `@packages/bun-uws/src/HttpRouter.h` around lines 345 - 368, `HttpRouter::route` is not safe for re-entrant use because `normalizeUrl()`, `setUrl()`, and `routeParameters.reset()` mutate shared router state that can be clobbered if a handler calls `route()` again. Add a guard in `route()` to detect and block nested dispatch on the same router instance (for example with a re-entrancy flag scoped to the router), and make sure the guard is cleared on every exit path before returning from `route()` or `executeHandlers()`.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.
Outside diff comments:
In `@packages/bun-uws/src/HttpRouter.h`:
- Around line 345-368: `HttpRouter::route` is not safe for re-entrant use
because `normalizeUrl()`, `setUrl()`, and `routeParameters.reset()` mutate
shared router state that can be clobbered if a handler calls `route()` again.
Add a guard in `route()` to detect and block nested dispatch on the same router
instance (for example with a re-entrancy flag scoped to the router), and make
sure the guard is cleared on every exit path before returning from `route()` or
`executeHandlers()`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb9ca620-8f27-49ac-b1e4-4cd280bca2dd
📒 Files selected for processing (4)
docs/guides/util/base64.mdxdocs/runtime/web-apis.mdxpackages/bun-uws/src/HttpRouter.htest/js/bun/http/bun-serve-routes.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Dotfile segments like /.well-known/x start with a dot but are not dot-segments, so urlNeedsNormalization now classifies the whole segment instead of only its first byte, keeping those paths on the zero-copy path.
|
On the re-entrancy note from the automated review: route() is only entered from the HTTP parser callbacks (HttpContext.h onHttpRequest and Http3Context on_stream_headers), which the event loop invokes one at a time on the loop thread, and a handler cannot cause another request on the same router to be dispatched before it returns. currentUrl, urlSegmentVector, and routeParameters are the same per-router state already written by setUrl() on every route() call, so normalizedUrlBuffer does not add a new re-entrancy hazard; a guard flag would be unreachable code. |
std::string::rfind and std::string_view::find_first_of pull MSVC's vectorized find helpers (__std_find_last_trivial_1, __std_find_end_2) into the Windows x64 baseline binary, which the static instruction scan rejects for containing AVX instructions. Plain loops over these short paths avoid the dependency.
There was a problem hiding this comment.
Thanks for addressing the .well-known predicate — that's now exact. I'm deferring to a human reviewer since this adds ~100 lines of path-normalization logic to the per-request hot path in HttpRouter::route() and changes routing semantics in a security-relevant way (route confusion / traversal-style spellings).
Extended reasoning...
Overview
This PR normalizes the HTTP request-target before Bun.serve route matching so that routing agrees with what handlers observe via request.url (which goes through the WHATWG URL parser). The substantive change is in packages/bun-uws/src/HttpRouter.h: a new normalizeUrl() helper plus urlNeedsNormalization(), isSingleDotSegment(), isDoubleDotSegment(), and isEncodedDotAt(), with a per-router std::string normalizedUrlBuffer for stable backing storage, wired into route() via setUrl(normalizeUrl(url)). A new 21-case test matrix in bun-serve-routes.test.ts writes raw request-targets over a TCP socket and asserts both the dispatched route and the handler-observed req.url pathname. The two docs files are autofix whitespace/table-padding reflows with no content change.
Security risks
The change is itself a security hardening — before it, GET /w/../admin/x would be dispatched to the /w/* handler while request.url reports /admin/x, letting a raw-socket client skip route-attached guards or land in a handler whose prefix it has escaped. However, path-normalization code in an HTTP router is a classic high-risk surface: off-by-one errors, mishandling of %2e spellings, backslash-as-separator, or buffer-lifetime bugs here would directly affect every request. The implementation looks careful (length-gated segment classifiers, never walks above root, returns the input view unchanged when nothing needs normalizing, buffer is per-router and route() is not re-entered on the loop thread), and the test matrix is thorough, but this is exactly the kind of change that deserves a second pair of human eyes on the C++.
Level of scrutiny
High. This is new logic in the per-request hot path of HttpRouter for both HTTP/1.x and HTTP/3, and it changes user-visible routing semantics — paths that previously fell through to fetch() or matched a wildcard by raw spelling will now match exact/parameterized routes. The PR description deliberately scopes out percent-decoding and node:http, which is reasonable, but the semantics shift and the performance profile (zero-copy fast path vs. buffer copy) both warrant maintainer review.
Other factors
My earlier inline note about urlNeedsNormalization() over-triggering on dotfile segments (/.well-known/...) was addressed in 3ccbf27 — the predicate now classifies the whole segment via isSingleDotSegment/isDoubleDotSegment, and corresponding test rows were added. CodeRabbit's two style nits (comment length, query-delimiter test case) were also addressed and resolved. The bug-hunting system found no issues. The one CI failure (test-net-connect-memleak.js) is unrelated to this change. Given all that, the code looks correct to me, but the combination of hot-path C++, security relevance, and routing-semantics change puts it outside what I'm comfortable approving without a human.
|
CI status: every lane that exercises this change is green, and the remaining failures are unrelated and differ between runs. Build 66918: test/js/node/test/parallel/test-net-connect-memleak.js on alpine x64 and x64-baseline (GC finalization timing; passes 3/3 locally against this branch), plus the known-flaky Windows update_interactive_install.test.ts (retried). The only CI failure this PR did cause (the Windows x64 verify-baseline AVX scan) was fixed in 6c00ed4 by not pulling in the MSVC vectorized STL helpers. Locally against this branch: the raw request-target matrix in bun-serve-routes.test.ts (25 spellings), plus serve.test.ts, bun-serve-static.test.ts, bun-serve-html.test.ts, and bun-server.test.ts show no failures beyond ones reproducible on main, and node:http still exposes the raw request-target. |
…segments Both route() call sites hand the router a target already cut at the first question mark (HttpParser querySeparator, Http3Request), so the query branches were dead; the comment now states that precondition. New test rows cover empty segments: a dot-segment pops the empty segment produced by an adjacent slash or backslash rather than the one before it.
|
Follow-up commit bfe2f3a from a self-review pass on the diff:
Also checked the normalization differentially against the URL parser (new URL(target, base).pathname) over 37,449 generated request-targets made of /, , ., a, %2e, %2E, %2f, and # with no disagreement. |
There was a problem hiding this comment.
No issues found and the earlier nit is addressed, but this rewrites the per-request path normalization in the core HTTP router — security-sensitive enough that a human should sign off on the semantics.
Extended reasoning...
Overview
This PR adds WHATWG-style request-target normalization inside HttpRouter::route() (packages/bun-uws/src/HttpRouter.h) so that Bun.serve route matching uses the same path that handlers observe via request.url. About 100 lines of new C++ implement dot-segment resolution (including %2e spellings), \\ → /, and # termination, with a fast-path predicate that skips the copy when no normalization is needed. A 25-case raw-socket test matrix is added in bun-serve-routes.test.ts. The two doc changes are pure formatting/whitespace reflow with no content change.
Security risks
This is squarely security-sensitive: it changes which handler receives a given raw request-target. The fix closes a routing/request.url mismatch (e.g. GET /w/../admin/x previously hit the /w/* handler while req.url showed /admin/x), but any bug in the new normalizer could itself create a mismatch the other way, or diverge from WTF::URL in an edge case (empty segments, trailing dot-segments, mixed encoded/literal dots). The implementation deliberately does not percent-decode and never walks above root, both of which look correct, and the test table covers the tricky cases I'd worry about — but this is hand-rolled URL normalization on the hot path of every HTTP request.
Level of scrutiny
High. HttpRouter::route() runs for every request to every Bun.serve instance, and the change reimplements a subset of WHATWG URL path semantics rather than calling into the existing parser. The normalizedUrlBuffer member also adds per-router mutable state whose lifetime backs the string_views handed to route parameters; the author's re-entrancy analysis in the thread looks right, but it's the kind of invariant a maintainer should confirm.
Other factors
My one prior inline comment (the conservative /.well-known fast-path predicate) was addressed in 3ccbf27 with added test coverage. CodeRabbit's two style nits are also resolved. The author's CI summary attributes remaining failures to known flakes. No correctness bugs surfaced from the multi-agent review of the latest revision. Given all that, this looks ready for a human reviewer to evaluate the normalization semantics and the buffer-lifetime story rather than for further automated iteration.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
|
Replaced by #42724, which carries this change rebased onto main, with a setup-time gate, an HTTP/3 check and more tests. |
1 participant
What
Bun.servedispatched routes by matching the raw request-target, whilerequest.urlis built with the URL parser, which resolves dot-segments (including their%2espellings), treats\as/, and ends the path at#. A raw request-target can therefore reach a different handler than the onerequest.urlnames. Browsers andfetch()normalize before sending, so only a raw socket client can produce these spellings.Handlers attached to a route (and anything keyed on which route matched) can be skipped by the raw spelling, while the handler that does run, and everything it logs, observes the normalized path. The
%2e/%2Espellings of.and.., backslashes (which the URL parser turns into/), and#behave the same way.Cause
HttpRouter::route()receives the raw target (HttpContext.hfor HTTP/1.x,Http3Context.hfor HTTP/3), whilerequest.urlgoes throughWTF::URL, which performs WHATWG normalization.Fix
Normalize the path inside
HttpRouter::route()(packages/bun-uws/src/HttpRouter.h) the same way the URL parser does: resolve.and..segments including their%2espellings, treat\as/, and end the path at#(both callers already strip the query before routing). Paths that need no normalization, which is the common case, are matched exactly as before with no copy. Registered route patterns are untouched, empty segments are preserved, and normalization never walks above the root.Deliberately unchanged, both in the percent-encoding family rather than the path-structure family this PR fixes:
/p/%73ecretstill matches/p/:vrather than a literal/p/secretroute, the same semantics as Express.{, or a non-ASCII byte) is still matched as sent whilerequest.urlshows it percent-encoded. Neither direction can change path structure (no new separators, no dot-segments), and both deserve one consistent policy in a follow-up rather than being half-changed here.node:httpservers register only/*, so handler selection cannot change, andIncomingMessage.urlstill exposes the raw request-target.Verification
The new test in
test/js/bun/http/bun-serve-routes.test.tswrites raw request-targets over a plain socket and asserts, for 25 spellings, that the handler that runs is the onerequest.urlimplies, that:paramsstill percent-decode, that an encoded/(%2f) still does not split a segment, and that empty segments round-trip (..pops an empty segment produced by//or\\rather than the segment before it).On
main, 11 of the 25 spellings are dispatched to a handler other than the onerequest.urlnames (for exampleGET /w/../admin/xis served by the/w/*handler withrequest.urlending in/admin/x); with this change the test passes. The normalization was also checked differentially against the URL parser (new URL(target, base).pathname) over 37,449 generated request-targets built from/,\,.,a,%2e,%2E,%2f, and#with no disagreement.serve.test.ts,bun-serve-static.test.ts,bun-serve-html.test.ts, andbun-server.test.tsshow no new failures, andnode:http'sIncomingMessage.urlstill returns the raw request-target.