fix(serve-static): decode the resolved id before lookup

src/utils/static.ts

@@ -84,14 +84,32 @@ export async function serveStatic(
     throw new HTTPError({ status: 405 });
   }
 
+  // Resolve `.`/`..` traversal FIRST, then decode, so the on-disk id matches
+  // what `sirv`/`serve-static` serve (a filesystem-backed `getContents` no
+  // longer needs self-decoding logic — e.g. `/50%25.png` finds `50%.png`).
+  //
   // `event.url.pathname` is already decoded once by the event layer
-  // (`decodePathname`, a single `decodeURI` that preserves `%25`) — the same
-  // value the rest of h3 routes on. Decoding again here would peel a second
-  // `%25` level, so serveStatic would resolve/serve a different id than was
-  // dispatched (e.g. a double-encoded separator collapsing to `%2f`).
-  // `resolveDotSegments` decodes nested-encoded dot segments itself, so no
-  // extra decode is needed for traversal safety.
-  const originalId = resolveDotSegments(withoutTrailingSlash(event.url.pathname));
+  // (`decodePathname`, a single `decodeURI` that preserves `%25`), and
+  // `resolveDotSegments` neutralizes every traversal escape (literal `../`,
+  // `..\`, and `%2e`-encoded dot segments at any `%25`-nesting depth). Only
+  // then do we `decodeURI` to peel one `%25` level (`%25` → `%`) for the
+  // lookup. This never reintroduces a separator: `decodeURI` preserves `%2f`
+  // (reserved), and a single-encoded `%5c` can't reach here — the event layer
+  // already decoded it to `\` and `resolveDotSegments` normalized that away, so
+  // only a double-encoded `%255c` survives and `decodeURI` collapses it to a
+  // literal `%5c`, not a raw `\`.
+  //
+  // The final decode is guarded: with `allowMalformedURL`, a raw malformed `%`
+  // (e.g. `/foo%`, `/%ZZ`) reaches here and `decodeURI` throws — fall back to
+  // the traversal-resolved (still-safe) value so `fallthrough`/404 handling is
+  // reached instead of a 500.
+  const resolvedId = resolveDotSegments(withoutTrailingSlash(event.url.pathname));
+  let originalId: string;
+  try {
+    originalId = decodeURI(resolvedId);
+  } catch {
+    originalId = resolvedId;
+  }
 
   const acceptEncodings = parseAcceptEncoding(
     event.req.headers.get("accept-encoding") || "",

test/static.test.ts

@@ -1,5 +1,5 @@
-import { beforeEach, vi } from "vitest";
-import { serveStatic, type ServeStaticOptions } from "../src/index.ts";
+import { beforeEach, describe, it, expect, vi } from "vitest";
+import { H3, serveStatic, type ServeStaticOptions } from "../src/index.ts";
 import { describeMatrix } from "./_setup.ts";
 
 describeMatrix("serve static", (t, { it, expect }) => {
@@ -163,32 +163,43 @@ describeMatrix("serve static", (t, { it, expect }) => {
     expect(text).not.toContain("%2E%2E");
   });
 
-  it("does not decode the pathname a second time", async () => {
-    // The event layer already applied the one canonical decode (preserving
-    // `%25`), so a double-encoded separator stays opaque and must reach the
-    // backend as the same id h3 routes on — not the twice-decoded `%2f`.
+  it("does not reintroduce a backslash separator when decoding the id", async () => {
+    // A double-encoded backslash traversal exercises the final `decodeURI`:
+    // `%255c` survives `resolveDotSegments` (a single `%5c` is decoded to `\`
+    // and normalized away one layer earlier), and the decode must collapse it
+    // to a *literal* `%5c` — never a raw `\`. So the whole thing stays one
+    // opaque segment: the `..` never becomes a bare path segment, and no new
+    // separator boundary is introduced.
+    const res = await t.fetch("/..%255c..%255cetc%255cpasswd");
+    const text = await res.text();
+    expect(text).not.toContain("\\"); // no raw backslash separator
+    expect(text).toContain("%5c"); // stayed literal, not decoded to `\`
+    expect(text).not.toMatch(/(^|\/)\.\.(\/|$)/); // no bare `..` path segment
+  });
+
+  it("decodes an encoded separator for the on-disk lookup", async () => {
+    // serveStatic resolves `.`/`..` traversal first, then decodes once more, so
+    // the id reaches the backend fully decoded — matching `sirv`/`serve-static`
+    // and letting a filesystem-backed `getContents` skip self-decoding.
+    // `/files/a%252fb` → id `a%2fb` (the encoded slash stays a literal `%2f`
+    // segment, so no traversal is reintroduced).
     const res = await t.fetch("/files/a%252fb");
     expect(res.status).toEqual(200);
-    // A trailing encoding suffix (e.g. `.gz`) may be appended by the
-    // accept-encoding search, so match the distinguishing part loosely:
-    // the id keeps the single-decoded `%252f`, not the twice-decoded `%2f`.
     const text = await res.text();
-    expect(text).toContain("a%252fb");
-    expect(text).not.toContain("a%2fb");
+    expect(text).toContain("a%2fb");
+    expect(text).not.toContain("a%252fb");
   });
 
-  it("keeps a literal `%` in a filename encoded in the served id", async () => {
-    // Regression for the dropped second decode: a file whose name contains a
-    // literal `%` arrives as `%25` and the event layer preserves it, so the id
-    // reaches the backend as `/50%25.png` (the same value h3 routes on), not
-    // the old twice-decoded `/50%.png`. A filesystem-backed `getContents` must
-    // therefore decode `%25` itself. (See followup issue on aligning this with
-    // `sirv`/`serve-static`, which decode the id for the on-disk lookup.)
+  it("decodes a literal `%` in a filename for the on-disk lookup", async () => {
+    // A file whose name contains a literal `%` (`50%.png`) is requested as
+    // `/50%25.png`. The event layer preserves `%25`, then serveStatic decodes it
+    // once more so the id reaches the backend as `/50%.png` — the on-disk name —
+    // aligning with `sirv`/`serve-static` instead of forcing self-decoding.
     const res = await t.fetch("/50%25.png");
     expect(res.status).toEqual(200);
     const text = await res.text();
-    expect(text).toContain("/50%25.png");
-    expect(text).not.toContain("/50%.png");
+    expect(text).toContain("/50%.png");
+    expect(text).not.toContain("/50%25.png");
   });
 
   it("allows legitimate paths with dots", async () => {
@@ -374,3 +385,29 @@ describeMatrix("serve static MIME types", (t, { it, expect }) => {
     expect(res.headers.get("content-length")).toBe("999");
   });
 });
+
+describe("serve static (malformed url)", () => {
+  it("falls through on a malformed `%` instead of throwing", async () => {
+    // With `allowMalformedURL`, a raw malformed `%` (e.g. `/foo%`) reaches
+    // serveStatic and the final `decodeURI` would throw a URIError — a 500 that
+    // bypasses `fallthrough`. The guarded decode falls back to the resolved id
+    // so `fallthrough` (and 404 when off) are reached as normal.
+    const options: ServeStaticOptions = {
+      getContents: () => undefined,
+      getMeta: () => undefined,
+    };
+
+    const fallthroughApp = new H3({ allowMalformedURL: true })
+      .use((event) => serveStatic(event, { ...options, fallthrough: true }))
+      .use(() => "next");
+    const res = await fallthroughApp.request("/foo%");
+    expect(res.status).toBe(200);
+    expect(await res.text()).toBe("next");
+
+    const notFoundApp = new H3({ allowMalformedURL: true }).all("/**", (event) =>
+      serveStatic(event, options),
+    );
+    const res404 = await notFoundApp.request("/foo%");
+    expect(res404.status).toBe(404);
+  });
+});