fix(event): don't crash the process on a malformed request URI#1406
fix(event): don't crash the process on a malformed request URI#1406davidz627 wants to merge 1 commit into
Conversation
decodePathname calls decodeURI, which throws a URIError on invalid percent-encoding (a bare "%", or an invalid UTF-8 byte sequence such as "%C0"). This runs in the H3Event constructor, before the per-request try/catch in H3Core["~request"], so the throw escapes onError/middleware and surfaces as an uncaughtException — any scanner hitting a junk URL (GET /%) crashes the server. Guard the decode and fall back to the raw, undecoded pathname when decodeURI throws; routing then simply won't match it (404) instead of crashing. Valid encoded paths still decode normally, and the existing %25-preservation behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds defensive error handling to the ChangesURI Decoding Safety
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/path.test.ts (1)
4-22: ⚡ Quick winUse
describeMatrixfor this test file.This suite currently runs only as a plain Vitest unit suite, so it misses the repository’s required web/node matrix coverage for
test/**/*.test.ts.♻️ Minimal change
-import { describe, it, expect } from "vitest"; +import { describeMatrix } from "../_setup.ts"; import { decodePathname } from "../../src/utils/internal/path.ts"; -describe("decodePathname", () => { +describeMatrix("decodePathname", (_ctx, { it, expect }) => { it("decodes valid percent-encoding", () => { expect(decodePathname("/caf%C3%A9")).toBe("/café"); }); @@ -}); +});As per coding guidelines, "Use
describeMatrixfor writing cross-runtime tests that execute in bothwebandnodemodes".🤖 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/unit/path.test.ts` around lines 4 - 22, Replace the plain Vitest suite with a cross-runtime suite by swapping describe("decodePathname", ...) for describeMatrix(...) so the tests run in both web and node; update the top-level test declaration to use describeMatrix with a runtimes option (e.g., { runtimes: ["node", "web"] }) and keep the existing it(...) blocks and assertions unchanged, targeting the decodePathname function in this file.
🤖 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 `@test/security.test.ts`:
- Around line 103-107: The test currently only checks for a 200 status when
fetching malformed paths; update the spec in the loop that calls ctx.fetch(...)
to also assert that the response body (which your handler returns via
event.url.pathname) exactly equals the original raw request path variable (path)
for each case; locate the test using ctx.fetch and the loop over ["/%",
"/%C0%AF", "/foo%", "/%E0%A4%A"] and add an expectation comparing the response
text or returned pathname to the original path to ensure it isn’t rewritten.
---
Nitpick comments:
In `@test/unit/path.test.ts`:
- Around line 4-22: Replace the plain Vitest suite with a cross-runtime suite by
swapping describe("decodePathname", ...) for describeMatrix(...) so the tests
run in both web and node; update the top-level test declaration to use
describeMatrix with a runtimes option (e.g., { runtimes: ["node", "web"] }) and
keep the existing it(...) blocks and assertions unchanged, targeting the
decodePathname function in this file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60c50d7f-8445-4e1a-b3ac-0d47a651d9d8
📒 Files selected for processing (3)
src/utils/internal/path.tstest/security.test.tstest/unit/path.test.ts
| for (const path of ["/%", "/%C0%AF", "/foo%", "/%E0%A4%A"]) { | ||
| it(`handles ${path} without throwing`, async () => { | ||
| const res = await ctx.fetch(path); | ||
| expect(res.status).toBe(200); | ||
| }); |
There was a problem hiding this comment.
Assert the raw pathname too, not just the 200.
Right now this regression passes even if malformed input gets rewritten to some other safe path. Since Line 96 already returns event.url.pathname, assert that it stays equal to the original malformed request path.
💡 Suggested assertion
for (const path of ["/%", "/%C0%AF", "/foo%", "/%E0%A4%A"]) {
it(`handles ${path} without throwing`, async () => {
const res = await ctx.fetch(path);
expect(res.status).toBe(200);
+ expect(await res.json()).toEqual({ path });
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const path of ["/%", "/%C0%AF", "/foo%", "/%E0%A4%A"]) { | |
| it(`handles ${path} without throwing`, async () => { | |
| const res = await ctx.fetch(path); | |
| expect(res.status).toBe(200); | |
| }); | |
| for (const path of ["/%", "/%C0%AF", "/foo%", "/%E0%A4%A"]) { | |
| it(`handles ${path} without throwing`, async () => { | |
| const res = await ctx.fetch(path); | |
| expect(res.status).toBe(200); | |
| expect(await res.json()).toEqual({ path }); | |
| }); |
🤖 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 103 - 107, The test currently only checks
for a 200 status when fetching malformed paths; update the spec in the loop that
calls ctx.fetch(...) to also assert that the response body (which your handler
returns via event.url.pathname) exactly equals the original raw request path
variable (path) for each case; locate the test using ctx.fetch and the loop over
["/%", "/%C0%AF", "/foo%", "/%E0%A4%A"] and add an expectation comparing the
response text or returned pathname to the original path to ensure it isn’t
rewritten.
|
hi @pi0! Hope you're doing well! What do you think of this fix? It's something that has trigged in production for my system a few times so I've already monkey-patched but would love to contribute the fix upstream |
| // the H3Event constructor, before the per-request try/catch, so an | ||
| // unguarded throw escapes as an uncaughtException and crashes the process. | ||
| // Fall back to the raw, undecoded pathname; routing then won't match it. | ||
| return pathname; |
There was a problem hiding this comment.
It is unsafe to ignore parse errors. (routing mismatches can cause some middleware like auth to be bypassed in certain conditions for example
2 participants
🔗 Linked issue
Refs #1361 (and the previously-closed #1363).
📝 Description
decodePathnamecallsdecodeURI, which throws aURIError: "URI malformed"on invalid percent-encoding — a bare%, or an invalid UTF-8 byte sequence such as%C0:It's called from the
H3Eventconstructor:The
new H3Event(...)call inH3Core["~request"]runs before the per-requesttry/catch, so the throw can't be caught byonError, Nitro, or consumer middleware. With thesrvxNode adapter,serve()invokes the app'sfetchdirectly inside the HTTP request listener, and h3'sfetchthrows synchronously (the event is constructed outside thetry) — so it escapes as an uncaughtException and crashes the process. Any scanner/bot hitting a junk URL (GET /%) takes down a pod.🔄 Changes
Guard the decode and fall back to the raw, undecoded pathname when
decodeURIthrows. Routing then simply won't match the un-decodable path (404) instead of crashing. Valid encoded paths still decode normally, and the existing%25-preservation behaviour is unchanged.This is the same guard the issue reporter proposed in #1361. It was previously included in #1363, which was closed in favour of landing the path-traversal hardening (
withoutBase/resolveDotSegments) separately — those landed onmain, but this crash-guard did not. This PR is the minimal, guard-only remainder.✅ Tests
test/unit/path.test.ts—decodePathnamedecodes valid input, preserves%25(no double-decode), and returns malformed input unchanged without throwing.test/security.test.ts— matrix (web + node) regression asserting/%,/%C0%AF,/foo%,/%E0%A4%Aroute normally instead of crashing.Both fail against the current code (the node-mode cases time out — the crash signature) and pass with the fix.
pnpm lintis clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests