Skip to content

fix(mount): restore pathname on error with try/finally #1319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
productdevbook wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix(mount): restore pathname on error with try/finally #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a47900
fix(mount): restore pathname on error with try/finally
productdevbook Mar 14, 2026
ce83021
test(mount): use async middleware in pathname restore test
productdevbook Mar 14, 2026
a41716e
fix(mount): restore pathname on error without async/await
productdevbook Mar 14, 2026
46c3ff8
fix: restore pathname on sync success path in mount middleware
productdevbook Mar 15, 2026
c15b5fe
test(mount): add regression test for sync success pathname restore
productdevbook Mar 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/h3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,23 @@ export const H3 = /* @__PURE__ */ (() => {
return next();
}
event.url.pathname = event.url.pathname.slice(base.length) || "/";
return callMiddleware(event, input["~middleware"], () => {
let result: unknown;
try {
result = callMiddleware(event, input["~middleware"], () => {
event.url.pathname = originalPathname;
return next();
});
} catch (err) {
event.url.pathname = originalPathname;
return next();
});
throw err;
}
if (typeof (result as PromiseLike<unknown>)?.then === "function") {
return (result as Promise<unknown>).finally(() => {
event.url.pathname = originalPathname;
});
Comment on lines +140 to +143
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify where thenable checks are followed by direct .finally assumptions.
rg -n --type=ts -C3 'typeof\s*\(.+\)\?\.\s*then\s*===\s*"function"|\.finally\s*\(' src

# Inspect middleware return behavior and promise-like contracts.
sed -n '60,130p' src/middleware.ts
rg -n --type=ts -C3 'type MaybePromise|interface Middleware|EventHandlerResponse' src/types

Repository: h3js/h3

Length of output: 5810

Normalize thenables before calling .finally().

At Line 140 you check for .then, but Line 141 calls .finally which is not part of the PromiseLike interface. A custom thenable without .finally will throw TypeError and skip the pathname restoration. Use Promise.resolve() to normalize the thenable to a proper Promise first.

💡 Proposed fix 
-            if (typeof (result as PromiseLike<unknown>)?.then === "function") {
-              return (result as Promise<unknown>).finally(() => {
+            if (typeof (result as PromiseLike<unknown>)?.then === "function") {
+              return Promise.resolve(result as PromiseLike<unknown>).finally(() => {
                 event.url.pathname = originalPathname;
               });
             }
📝 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.

Suggested change
if (typeof (result as PromiseLike<unknown>)?.then === "function") {
return (result as Promise<unknown>).finally(() => {
event.url.pathname = originalPathname;
});
if (typeof (result as PromiseLike<unknown>)?.then === "function") {
return Promise.resolve(result as PromiseLike<unknown>).finally(() => {
event.url.pathname = originalPathname;
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/h3.ts` around lines 140 - 143, The code checks for a thenable by testing
(result as PromiseLike<unknown>)?.then but calls .finally directly, which can
throw for thenables without finally; normalize the thenable by wrapping with
Promise.resolve(result) and call .finally on that normalized Promise so
event.url.pathname is always restored (update the branch handling result to
return Promise.resolve(result).finally(() => { event.url.pathname =
originalPathname; }) and keep existing return behavior).

}
event.url.pathname = originalPathname;
return result;
});
}
for (const r of input["~routes"]) {
Expand Down
60 changes: 60 additions & 0 deletions test/mount.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { H3 } from "../src/h3.ts";
import { HTTPError } from "../src/error.ts";
import { describeMatrix } from "./_setup.ts";

describeMatrix("mount", (t, { it, expect, describe }) => {
Expand Down Expand Up @@ -155,6 +156,65 @@ describeMatrix("mount", (t, { it, expect, describe }) => {
expect(logs).toContain("admin: /admin/users"); // Adjusted path
});

it("restores pathname when mounted middleware returns without calling next", async () => {
let pathInResponse = "";
t.app.config.onResponse = (_res, event) => {
pathInResponse = event.url.pathname;
};

const subApp = new H3();
subApp.use((_event) => {
return "intercepted";
});
subApp.get("/test", () => new Response("ok"));

t.app.mount("/api", subApp);

const res = await t.fetch("/api/test");
expect(await res.text()).toBe("intercepted");
// onResponse must see the original pathname, not the stripped one
expect(pathInResponse).toBe("/api/test");
});

it("restores pathname when mounted middleware throws synchronously", async () => {
const subApp = new H3();
subApp.use((_event) => {
throw new HTTPError({ status: 500, statusText: "Sync Error" });
});
subApp.get("/test", () => new Response("ok"));

t.app.mount("/api", subApp);

t.app.config.onError = (error, event) => {
return Response.json({ path: event.url.pathname }, { status: 500 });
};

const res = await t.fetch("/api/test");
const body = await res.json();
expect(body.path).toBe("/api/test");
t.errors = [];
});

it("restores pathname when mounted middleware throws asynchronously", async () => {
const subApp = new H3();
subApp.use(async (_event) => {
await Promise.resolve();
throw new HTTPError({ status: 500, statusText: "Async Error" });
});
subApp.get("/test", () => new Response("ok"));

t.app.mount("/api", subApp);

t.app.config.onError = (error, event) => {
return Response.json({ path: event.url.pathname }, { status: 500 });
};

const res = await t.fetch("/api/test");
const body = await res.json();
expect(body.path).toBe("/api/test");
t.errors = [];
});

it("middleware should not execute for non-matching paths", async () => {
const logs: string[] = [];

Expand Down
Loading