fix(event): return 400 for malformed percent-encoded request URLs#1424
Conversation
📝 WalkthroughWalkthroughAdds malformed-percent URL detection, rejects those requests with HTTP 400 before routing unless opted in, falls back to raw pathnames in base URL resolution, and updates tests, docs, and bundle-size thresholds. ChangesMalformed URL Detection and Rejection
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The H3Event constructor normalizes the pathname via decodePathname (decodeURI), which throws a URIError on malformed percent-encoding such as `/foo%`, `/%ZZ` or `/bar%2`. Because the event is constructed before the try/catch in `~request`, the error escaped h3's error handling entirely: `app.fetch()`/`app.request()` rejected instead of returning a Response, bypassing onError/onResponse hooks and surfacing as an unhandled 500/crash depending on the runtime adapter — a cheap robustness/DoS vector for unauthenticated clients. Catch the decode failure, keep the raw pathname (so route matching and middleware guards still see one consistent value — no bypass), flag the event, and reject it with a clean 400 before any routing or app logic runs. `requestWithBaseURL` (mount path) now falls back to the raw pathname instead of throwing as well. Co-Authored-By: Claude Fable 5 <[email protected]>
aaf9ba0 to
f0af5d6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/event.ts (1)
71-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard correctly prevents URIError from escaping the constructor.
Catching the decode failure and preserving the raw pathname keeps route matching and middleware guards consistent, addressing the DoS vector described in the PR. One minor note: this duplicates the same try/catch fallback pattern used in
requestWithBaseURL(src/utils/request.ts). Consider extracting a shared helper (e.g.safeDecodePathname) to avoid maintaining two copies of this logic.♻️ Example shared helper
// src/utils/internal/path.ts export function tryDecodePathname(pathname: string): { pathname: string; malformed: boolean } { try { return { pathname: decodePathname(pathname), malformed: false }; } catch { return { pathname, malformed: true }; } }🤖 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 `@src/event.ts` around lines 71 - 78, The pathname decode fallback is duplicated between the constructor logic in Event and the equivalent handling in requestWithBaseURL, so extract the shared try/catch behavior into a helper such as safeDecodePathname or tryDecodePathname and reuse it from both places. Keep the helper responsible for returning the decoded pathname plus the malformed flag, then update the Event constructor and the request utility to call that helper instead of maintaining separate copies of the decodePathname fallback.
🤖 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.
Nitpick comments:
In `@src/event.ts`:
- Around line 71-78: The pathname decode fallback is duplicated between the
constructor logic in Event and the equivalent handling in requestWithBaseURL, so
extract the shared try/catch behavior into a helper such as safeDecodePathname
or tryDecodePathname and reuse it from both places. Keep the helper responsible
for returning the decoded pathname plus the malformed flag, then update the
Event constructor and the request utility to call that helper instead of
maintaining separate copies of the decodePathname fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5641ac98-2d12-44e7-970f-b16b70634b19
📒 Files selected for processing (5)
src/event.tssrc/h3.tssrc/utils/request.tstest/bench/bundle.test.tstest/security.test.ts
…med URLs By default H3 rejects requests with a malformed percent-encoded URL path (e.g. `/foo%`, `/%ZZ`) with a 400 before routing. Setting `allowMalformedURL: true` lets such requests through with the raw, undecoded pathname instead, leaving handling to the app. Co-Authored-By: Claude Fable 5 <[email protected]>
Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/security.test.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew tests bypass the established
describeMatrix/ctx.fetchpattern.Every other block in this file (and the repo's documented test convention) uses
describeMatrixwithctx.app/ctx.fetchfor a fresh, cross-runtimeH3instance per test. The newsecurity: allowMalformedURL opt-inblock instead uses plaindescribe/itfrom vitest and manually instantiatesnew H3()/app.request(...), skipping web/node matrix coverage and thectx.errorsunhandled-error assertions the other suites get for free.♻️ Suggested refactor to align with existing test conventions
-describe("security: allowMalformedURL opt-in", () => { - it("rejects malformed URLs with 400 by default", async () => { - const app = new H3(); - app.get("/**", () => "ok"); - const res = await app.request("/foo%"); - expect(res.status).toBe(400); - }); - - it("passes malformed URLs through with the raw pathname when enabled", async () => { - const app = new H3({ allowMalformedURL: true }); - app.get("/**", (event) => event.url.pathname); - const res = await app.request("/foo%"); - expect(res.status).toBe(200); - expect(await res.text()).toBe("/foo%"); - }); -}); +describeMatrix("security: allowMalformedURL opt-in", (ctx, { it, expect }) => { + it("rejects malformed URLs with 400 by default", async () => { + ctx.app.get("/**", () => "ok"); + const res = await ctx.fetch("/foo%"); + expect(res.status).toBe(400); + }); +}); + +describeMatrix( + "security: allowMalformedURL opt-in (enabled)", + (ctx, { it, expect }) => { + it("passes malformed URLs through with the raw pathname when enabled", async () => { + ctx.app.config.allowMalformedURL = true; + ctx.app.get("/**", (event) => event.url.pathname); + const res = await ctx.fetch("/foo%"); + expect(res.status).toBe(200); + expect(await res.text()).toBe("/foo%"); + }); + }, +);Note:
ctx.appfrom_setup.tsmay not currently support constructing withallowMalformedURLdirectly — check whether the config can be mutated post-construction or if_setup.tsneeds a variant that accepts customH3Config.As per coding guidelines, "Use
describeMatrixfor writing cross-runtime tests that execute in bothwebandnodemodes" and "Ensure each test has a freshH3instance viabeforeEachsetup".Also applies to: 125-140
🤖 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 `@test/security.test.ts` around lines 1 - 3, The new security tests are bypassing the repo’s standard cross-runtime setup, so refactor the `security: allowMalformedURL opt-in` block to use `describeMatrix` instead of plain `describe`/`it`. Build the test around the existing `_setup.ts` pattern with `ctx.app` and `ctx.fetch` (or extend `_setup.ts` to accept the needed `H3Config`) so each case gets a fresh `H3` instance and the usual `ctx.errors` assertions. Keep the `allowMalformedURL` coverage, but wire it through the matrix harness rather than calling `new H3()` and `app.request(...)` directly.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.
Nitpick comments:
In `@test/security.test.ts`:
- Around line 1-3: The new security tests are bypassing the repo’s standard
cross-runtime setup, so refactor the `security: allowMalformedURL opt-in` block
to use `describeMatrix` instead of plain `describe`/`it`. Build the test around
the existing `_setup.ts` pattern with `ctx.app` and `ctx.fetch` (or extend
`_setup.ts` to accept the needed `H3Config`) so each case gets a fresh `H3`
instance and the usual `ctx.errors` assertions. Keep the `allowMalformedURL`
coverage, but wire it through the matrix harness rather than calling `new H3()`
and `app.request(...)` directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d093be3d-d8aa-4339-9ffa-b3246e13e769
📒 Files selected for processing (4)
docs/1.guide/900.api/1.h3.mdsrc/h3.tssrc/types/h3.tstest/security.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/1.guide/900.api/1.h3.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/h3.ts
2 participants
Problem
The
H3Eventconstructor normalizes the request pathname viadecodePathname(which usesdecodeURI) to keep route matching and middleware guards consistent.decodeURIthrows aURIErroron any malformed percent sequence —/foo%,/%ZZ,/bar%2,/%.The event is constructed on
h3.tsbefore thetry/catchin~request, so the error escaped h3's error handling entirely:app.fetch()/app.request()rejected instead of returning aResponse, violating the handler contract.onError/onResponseand h3's error formatting, surfacing as an unhandled 500 / connection crash depending on the runtime adapter.Native
URLdoes not throw on these inputs, so this exposure was introduced by the percent-encoding normalization added for the middleware-bypass fix; it did not exist before.Reproduced against the actual app:
Fix
H3Eventconstructor, keep the raw pathname (route matching and middleware guards read the same singleevent.url.pathname, so there is no matcher disagreement / auth bypass), and flag the event.~request, reject a flagged event with a clean 400 (HTTPError({ status: 400, message: "Bad Request" })) before any routing or app logic runs. Response body:{"status":400,"message":"Bad Request"}.requestWithBaseURL(themountpath) now falls back to the raw pathname instead of throwing.Opt out:
allowMalformedURLFor apps that want the old lenient behavior (e.g. proxies that forward the raw path untouched), a new config flag disables the 400 and lets malformed URLs through with the raw, undecoded pathname:
Default is
false(reject with 400). The middleware-bypass invariant still holds either way, since routing and guards read the same singleevent.url.pathname.Why raw-fallback is not a bypass
The original middleware-bypass class came from route matching and guards seeing different path representations. Here both read the single
event.url.pathname; on decode failure that value is simply the raw string, which both consumers still read identically — they cannot disagree. Verified: no malformed probe reaches the guarded admin handler, and the full existingsecurity.test.tssuite passes.Cross-runtime behavior comparison
To confirm 400 is the right default, I compared against nginx and the bare Bun/Deno runtimes. Probed with
curl --path-as-is(raw path sent unchanged); table showsstatus · resulting-path:/foo%(malformed)/foo%/foo%/%ZZ(malformed)/%ZZ/%ZZ/bar%2(truncated)/bar%2/bar%2/%2e%2e/x(encoded..)/x/x/x/%61dmin(encodeda)/admin/%61dmin/%61dmin/admin/a%2fb(encoded/)/a/b/a%2fb/a%2fb/a%2fbTakeaways:
URIErroron these inputs — strictly worse than either nginx (clean 400) or Bun/Deno (lenient 200).allowMalformedURL: truereproduces the lenient Bun/Deno behavior for anyone who wants it.new URL(req.url)never throws (WHATWG parser tolerates stray%); Node's URL is the same. The throw only appeared in h3 becausedecodePathnamedeliberately uses the stricterdecodeURIto normalize./%61dminis the security-relevant divergence: nginx and h3 both normalize to/admin(an encoded path and its plain form are the same route → a guard can't be dodged), whereas bare Bun/Deno keep/%61dminas a distinct string — the middleware-bypass footgun. h3 normalizes identically across runtimes: verified h3-on-Bun also yields/adminand 400s, so it does not inherit the underlying runtime's leniency.%2fencoded (safer than nginx decoding it), and h3 routes a null byte (%00) where nginx 400s (could tighten later if desired, but out of scope).Net: this PR removes the crash and lands h3 on nginx's proven 400 behavior for un-decodable URLs by default, while bare Bun/Deno would silently pass those requests through — and
allowMalformedURLis available for apps that need the lenient path.Tests
New matrix tests in
test/security.test.ts(web + real Node server):/foo%,/%ZZ,/bar%2,/%→ 400, no throw./api/admin%ZZ/usersdoes not reach the guarded handler.allowMalformedURL: true→ malformed URL passes through with the raw pathname (200); default → 400.Bundle-size guard rails in
test/bench/bundle.test.tsbumped modestly to account for the added code (H3Core gzip 2600→2650, defineHandler bytes 6100→6200).Full suite: 1191 passed / 15 skipped. Lint + typecheck clean.
🤖 Generated with Claude Code
Summary by CodeRabbit