fix(serve-static): decode the resolved id before lookup#1431
Conversation
serveStatic stopped decoding percent-encoded characters for filesystem lookups (regression from #1428), so requests for files whose names contain reserved/encoded characters 404'd, diverging from sirv and serve-static: - `/50%25.png` no longer served `50%.png` - `/files/a%252fb` no longer served `a%2fb` Resolve `.`/`..` traversal first, then `decodeURI` the result, so the id reaches the backend fully decoded while staying traversal-safe: the decode preserves `%2f` (reserved) and collapses a double-encoded `%255c` to a literal `%5c` (never a raw `\`), so no separator or `..` segment is reintroduced. The decode is guarded so a malformed `%` under `allowMalformedURL` falls through instead of throwing a 500. Closes #1429 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesserveStatic decoding fix
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/static.test.ts (1)
389-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
describeMatrix+ctx.fetchfor the malformed-URL suite.This new block uses plain
describe/itwith inlinenew H3(...).request(...), so the malformed-%fallback is only exercised in a single (web) mode. The decode-guard fallback is exactly the kind of path that benefits from cross-runtime coverage. Consider porting it todescribeMatrixwith a freshH3per test andctx.fetchfor URL resolution so it runs in bothwebandnode.As per coding guidelines:
test/**/*.test.tsshould "UsedescribeMatrixfor writing cross-runtime tests that execute in bothwebandnodemodes", "Ensure each test has a freshH3instance viabeforeEachsetup", and "Usectx.fetchfor URL resolution that works in both web and node test contexts".🤖 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/static.test.ts` around lines 389 - 413, The malformed-URL static-serving test is currently only covered in one runtime and uses direct H3.request calls, so it should be converted to cross-runtime coverage. Refactor the new malformed-% suite to use describeMatrix with a fresh H3 instance created in beforeEach, and replace inline request usage with ctx.fetch so the fallback behavior is exercised in both web and node modes. Keep the existing serveStatic assertions for fallthrough and 404, but structure them through the matrix test helpers.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/static.test.ts`:
- Around line 389-413: The malformed-URL static-serving test is currently only
covered in one runtime and uses direct H3.request calls, so it should be
converted to cross-runtime coverage. Refactor the new malformed-% suite to use
describeMatrix with a fresh H3 instance created in beforeEach, and replace
inline request usage with ctx.fetch so the fallback behavior is exercised in
both web and node modes. Keep the existing serveStatic assertions for
fallthrough and 404, but structure them through the matrix test helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 578b1973-a335-4ab0-97a6-17f3d0503811
📒 Files selected for processing (2)
src/utils/static.tstest/static.test.ts
Skip decodeURI for `%`-free ids (fast-path matching the event layer and resolveDotSegments), and lock in the invariant that serveStatic never receives a raw `\` or bare `..` even when a malformed `%` keeps the raw pathname and single-encoded `%5c` separators survive — safety that rests on the event layer's URL normalization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pi0
changed the title
pi0
changed the title
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 `@test/static.test.ts`:
- Around line 416-443: This test bypasses the file’s cross-runtime conventions
by creating `H3` inline and calling `app.request(...)` directly, so it should be
moved into the existing `describeMatrix`/`beforeEach` setup and use `ctx.fetch`
like the other cases. Keep a fresh `H3` instance per test, assert any failures
through `ctx.errors`, and preserve the `serveStatic`/`allowMalformedURL`
scenario so it runs consistently in both `web` and `node` targets.
- Around line 416-443: The malformed-URL test still allows single-encoded `%5c`
to reappear as raw backslashes after the final decode, so the assertion does not
actually prove separator neutralization. Update the input in the static test
case to use the double-encoded `%255c` form (or normalize separators before the
final `decodeURI()` step) so the `serveStatic`/`H3` path handling is exercised
without introducing raw backslashes or bare `..` segments.
🪄 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: 58b06df0-35e8-428c-8911-61c100103584
📒 Files selected for processing (2)
src/utils/static.tstest/static.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/static.ts
| it("never hands a raw backslash or bare `..` to the backend under allowMalformedURL", async () => { | ||
| // Defense-in-depth guard for the trickiest input we could construct: a | ||
| // malformed `%` (forcing the raw pathname) whose segment is popped by a | ||
| // `..`, leaving single-encoded `%5c` separators. If the on-disk decode | ||
| // (`%5c` → `\`) were not neutralized, the id would become | ||
| // `/..\..\windows\win.ini` — a traversal above the root on backslash-aware | ||
| // (e.g. Windows) filesystem backends. It is neutralized because the event | ||
| // layer's URL normalization collapses `\` → `/` and resolves `..` when the | ||
| // decoded pathname is assigned back, *before* serveStatic runs. This asserts | ||
| // that invariant end-to-end so a future event-layer change can't silently | ||
| // regress it. | ||
| let servedId: string | undefined; | ||
| const app = new H3({ allowMalformedURL: true }).all("/**", (event) => | ||
| serveStatic(event, { | ||
| getContents: (id) => { | ||
| servedId = id; | ||
| return `asset:${id}`; | ||
| }, | ||
| getMeta: () => ({ size: 1 }), | ||
| }), | ||
| ); | ||
|
|
||
| const res = await app.request("/a%ZZ/../..%5c..%5cwindows%5cwin.ini"); | ||
| expect(res.status).toBe(200); | ||
| expect(servedId).toBeDefined(); | ||
| expect(servedId).not.toContain("\\"); // no raw backslash separator | ||
| expect(servedId).not.toMatch(/(^|[\\/])\.\.([\\/]|$)/); // no bare `..` segment | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New test bypasses established cross-runtime test conventions.
This test constructs its own H3 instance inline and calls app.request(...) directly, instead of using the file's describeMatrix/beforeEach/ctx.fetch pattern. It also never touches ctx.errors, so unhandled errors from this handler aren't tracked/asserted like the rest of the suite. Since this exercises a security-sensitive decoding path, it's exactly the kind of test that should run across both web and node targets to catch runtime-specific divergence (e.g. Node's FastURL raw-slicing pathname parsing vs. the standard URL parser used in web/Workers runtimes).
♻️ Suggested restructure
- it("never hands a raw backslash or bare `..` to the backend under allowMalformedURL", async () => {
- let servedId: string | undefined;
- const app = new H3({ allowMalformedURL: true }).all("/**", (event) =>
- serveStatic(event, {
- getContents: (id) => {
- servedId = id;
- return `asset:${id}`;
- },
- getMeta: () => ({ size: 1 }),
- }),
- );
-
- const res = await app.request("/a%ZZ/../..%5c..%5cwindows%5cwin.ini");
- expect(res.status).toBe(200);
- expect(servedId).toBeDefined();
- expect(servedId).not.toContain("\\");
- expect(servedId).not.toMatch(/(^|[\\/])\.\.([\\/]|$)/);
- });
+ describeMatrix(
+ "malformed url edge cases",
+ (t) => {
+ let app: H3;
+ beforeEach(() => {
+ app = new H3({ allowMalformedURL: true });
+ });
+
+ it("never hands a raw backslash or bare `..` to the backend", async () => {
+ let servedId: string | undefined;
+ app.all("/**", (event) =>
+ serveStatic(event, {
+ getContents: (id) => {
+ servedId = id;
+ return `asset:${id}`;
+ },
+ getMeta: () => ({ size: 1 }),
+ }),
+ );
+ const res = await t.fetch("/a%ZZ/../..%5c..%5cwindows%5cwin.ini");
+ expect(res.status).toBe(200);
+ expect(servedId).toBeDefined();
+ expect(servedId).not.toContain("\\");
+ expect(servedId).not.toMatch(/(^|[\\/])\.\.([\\/]|$)/);
+ });
+ },
+ { app: () => app },
+ );(Adapt to the actual describeMatrix/ctx API used elsewhere in this file.)
As per coding guidelines, "Use describeMatrix for writing cross-runtime tests that execute in both web and node modes" and "Ensure each test has a fresh H3 instance via beforeEach setup" and "Use ctx.fetch for URL resolution that works in both web and node test contexts".
🤖 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/static.test.ts` around lines 416 - 443, This test bypasses the file’s
cross-runtime conventions by creating `H3` inline and calling `app.request(...)`
directly, so it should be moved into the existing `describeMatrix`/`beforeEach`
setup and use `ctx.fetch` like the other cases. Keep a fresh `H3` instance per
test, assert any failures through `ctx.errors`, and preserve the
`serveStatic`/`allowMalformedURL` scenario so it runs consistently in both `web`
and `node` targets.
Source: Coding guidelines
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect resolveDotSegments implementation for handling of encoded separators (%5c, %2f) vs only encoded dots (%2e)
fd -t f path.ts src/utils/internal
rg -n -B2 -A40 'function resolveDotSegments' src/utils/internal/path.tsRepository: h3js/h3
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline: src/event.ts ==\n'
ast-grep outline src/event.ts --view expanded || true
printf '\n== outline: src/utils/internal/path.ts ==\n'
ast-grep outline src/utils/internal/path.ts --view expanded || true
printf '\n== outline: src/utils/static.ts ==\n'
ast-grep outline src/utils/static.ts --view expanded || trueRepository: h3js/h3
Length of output: 2843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/utils/internal/path.ts ==\n'
cat -n src/utils/internal/path.ts | sed -n '1,140p'
printf '\n== src/event.ts ==\n'
cat -n src/event.ts | sed -n '120,170p'
printf '\n== src/utils/static.ts ==\n'
cat -n src/utils/static.ts | sed -n '60,180p'Repository: h3js/h3
Length of output: 8593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/event.ts | sed -n '60,120p'Repository: h3js/h3
Length of output: 2313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "resolveDotSegments" src testRepository: h3js/h3
Length of output: 5556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/utils/path.ts | sed -n '1,220p'Repository: h3js/h3
Length of output: 7017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const input = "/a%ZZ/../..%5c..%5cwindows%5cwin.ini";
const DOT_SEGMENT_RE = /(?:^|\/)(?:\.|%(?:25)*2e){1,2}(?:\/|$)/i;
const ENCODED_SEP_RE = /%(?:25)*(?:2f|5c)/i;
const ENCODED_SEP_RE_G = new RegExp(ENCODED_SEP_RE.source, "gi");
const ENCODED_DOT_RE_G = /%(?:25)*2e/gi;
function resolveDotSegments(path, opts) {
if (path[0] !== "/" || path[1] === "/" || path[1] === "\\") {
path = "/" + path.replace(/^[/\\]+/, "");
}
const decodeSlashes = opts?.decodeSlashes;
const hasBackslash = path.includes("\\");
const hasEncodedSep = decodeSlashes && ENCODED_SEP_RE.test(path);
if (!hasBackslash && !hasEncodedSep && !DOT_SEGMENT_RE.test(path)) {
return path;
}
let normalized = hasBackslash ? path.replaceAll("\\", "/") : path;
if (hasEncodedSep) {
normalized = normalized.replace(ENCODED_SEP_RE_G, "/");
}
const segments = normalized.split("/");
const resolved = [];
for (const segment of segments) {
const normalizedSegment = segment.includes("%")
? segment.replace(ENCODED_DOT_RE_G, ".")
: segment;
if (normalizedSegment === "..") {
if (resolved.length > 1) resolved.pop();
} else if (normalizedSegment !== ".") {
resolved.push(segment);
}
}
const result = resolved.join("/") || "/";
return result.replace(/^\/+/, "/");
}
const resolvedId = resolveDotSegments(input);
let originalId = resolvedId;
if (resolvedId.includes("%")) {
try {
originalId = decodeURI(resolvedId);
} catch {
originalId = resolvedId;
}
}
console.log({ input, resolvedId, originalId });
console.log("containsBackslash", originalId.includes("\\"));
console.log("containsBareDotDot", /(^|[\\/])\.\.([\\/]|$)/.test(originalId));
JSRepository: h3js/h3
Length of output: 333
%ZZ still lets %5c reappear as raw backslashes. This path turns into /..\\..\\windows\\win.ini after the final decodeURI(), so the malformed-URL case does not prove the separators are neutralized. Use the double-encoded %255c form here, or normalize separators before the final decode.
🤖 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/static.test.ts` around lines 416 - 443, The malformed-URL test still
allows single-encoded `%5c` to reappear as raw backslashes after the final
decode, so the assertion does not actually prove separator neutralization.
Update the input in the static test case to use the double-encoded `%255c` form
(or normalize separators before the final `decodeURI()` step) so the
`serveStatic`/`H3` path handling is exercised without introducing raw
backslashes or bare `..` segments.
2 participants
Summary
serveStaticstopped decoding percent-encoded characters when looking up files on disk (regression from #1428), diverging fromsirv/serve-static. Files whose names contain reserved/encoded characters became unreachable:/50%25.png50%.png/files/a%252fba%2fbFix
Resolve
./..traversal first, thendecodeURIthe result — so the id reaches the backend fully decoded while staying traversal-safe:The trailing decode cannot reintroduce a traversal escape:
resolveDotSegmentsalready neutralizes literal../,..\, and%2e-encoded dot segments at any%25-nesting depth.decodeURIpreserves%2f(reserved) — no raw/appears.%5ccan't reach the decode (the event layer already turned it into\, whichresolveDotSegmentsnormalized away); only a double-encoded%255csurvives, anddecodeURIcollapses it to a literal%5c, never a raw\.%underallowMalformedURL(e.g./foo%) falls through / 404s instead of throwing a 500.This lets filesystem-backed
getContentsimplementations drop their own self-decoding logic, at the cost of exact byte-for-byte id↔URL matching — an intentional trade for ecosystem consistency.Tests
/50%.png,/files/a%2fb)./..%255c..%255cetc%255cpasswd) proving%5cstays literal — no raw\, no bare..segment.%+allowMalformedURLtest provingfallthrough/404 are reached instead of a 500.Change was peer-reviewed by an independent adversarial pass (path-traversal fuzzing across nested
%25depths + mixed dot/slash/backslash encodings, zero escapes found); theallowMalformedURLregression it surfaced is fixed here.Closes #1429
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
%characters are preserved correctly during static file lookup.Tests