Skip to content

fix: improve URL handling in getRequestURL function #2304

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

Merged
alexander-akait merged 3 commits into from
Apr 7, 2026
Merged

fix: improve URL handling in getRequestURL function #2304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58ed112
fix: improve URL handling in getRequestURL function
bjohansebas Apr 5, 2026
7476ce4
docs: add changelog
bjohansebas Apr 5, 2026
911c292
refactor: logic
alexander-akait Apr 7, 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
5 changes: 5 additions & 0 deletions .changeset/stale-geese-open.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"webpack-dev-middleware": patch
---

Respect `req.url` when modified by middleware such as `connect-history-api-fallback`.
15 changes: 12 additions & 3 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ function parseTokenList(str) {
* @property {(() => string | undefined)=} getMethod get method extra method
* @property {(() => string | undefined)=} getURL get URL extra method
* @property {string=} originalUrl an extra option for `fastify` (and `@fastify/express`) to get original URL
* @property {string=} id an extra option for `fastify` (and `@fastify/express`) to get ID of request
*/

/**
Expand Down Expand Up @@ -312,9 +313,17 @@ function getRequestURL(req) {
if (typeof req.getURL === "function") {
return req.getURL();
}
// Fastify decodes URI by default, Our logic is based on encoded URI
else if (typeof req.originalUrl !== "undefined") {
return req.originalUrl;
// Fastify decodes URI by default, our logic is based on encoded URI.
// `req.url` may be modified by middleware (e.g. connect-history-api-fallback), in which case we use req.url instead.
// `req.id` is a special property of `fastify`
else if (req.id && req.originalUrl) {
try {
if (req.url === decodeURI(req.originalUrl)) {
return req.originalUrl;
}
} catch {
// decodeURI can throw on malformed sequences, fall through
}
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Apr 6, 2026

Choose a reason for hiding this comment

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

It this problem only for fastlify? decodeURI is not fast, so it is bad for performance

Copy link
Copy Markdown
Member Author

@bjohansebas bjohansebas Apr 6, 2026

Choose a reason for hiding this comment

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

Yep, it's unique to Fastify, or at least I don't remember Express having the same issue. Fastify uses its own logic instead of using decodeURI fastify/middie#245

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Apr 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

in this case let's add a pseudo API for fastify here and implement this logic only for fastify, don't want to lose perfomance for other frameworks

Copy link
Copy Markdown
Member Author

@bjohansebas bjohansebas Apr 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I’m not sure what steps to follow. It would be great if Fastify exposed something like req.raw on the request (since it currently doesn’t, which is a bit odd—but I’m not a Fastify expert). That way we could quickly detect it, since neither Express nor Connect have that property. However, Hono also has a getURL method, and since our logic checks getURL, it would never reach the fallback logic using req.raw.

Now the real issue is that Fastify does decode req.url, while Express and others don’t.

We could add logic like this:

if (typeof req.originalUrl !== "undefined") {
  if (req.url === req.originalUrl) {
    return req.originalUrl;
  }

  // Some frameworks can expose decoded req.url while keeping
  // encoded req.originalUrl.
  if (typeof req.url !== "undefined" && req.originalUrl.includes("%")) {
    try {
      if (decodeURI(req.originalUrl) === req.url) {
        return req.originalUrl;
      }
    } catch {
      // Ignore malformed URI sequences and fall back to req.url.
    }
  }
}

Or we could create a wrapper for Fastify, but I’m not sure how reliable it would be to correctly detect that Fastify is running underneath.

Another option would be to open an issue in @fastify/express to have them expose raw, so we can better detect Fastify internally.

What would you recommend without hurting performance too much?

I’m thinking about other ways to detect it—I’ll see what I come up with by tomorrow night

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Apr 7, 2026

Choose a reason for hiding this comment

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

I see the problem... Made refactor to use req.id to apply this logic only for fastify

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Apr 7, 2026

Choose a reason for hiding this comment

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

Or we could create a wrapper for Fastify, but I’m not sure how reliable it would be to correctly detect that Fastify is running underneath.

I think in future we can do it and implement built-in fastify support without the fastify-express layer, but to be honestly fastify is not very popular, so I think we will use fastify-express until it was supported, when it will be deprecated we will implement own wrapper

Another option would be to open an issue in @fastify/express to have them expose raw, so we can better detect Fastify internally.

It will be fine, let's try it, if it will be fixed we will revert this code and will use raw, so feel free to open an issue

Copy link
Copy Markdown
Member Author

@bjohansebas bjohansebas Apr 7, 2026

Choose a reason for hiding this comment

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

I didn’t know req.id existed, that’s new to me.

}

return req.url;
Expand Down
71 changes: 71 additions & 0 deletions test/middleware.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3473,6 +3473,77 @@ describe.each([
});
});

if (!["hapi", "hono"].includes(name)) {
describe("should work when req.url is modified by middleware to a file with encoded characters", () => {
let compiler;

const outputPath = path.resolve(
__dirname,
"./outputs/basic-test-modified-url-encoded",
);

beforeAll(async () => {
compiler = getCompiler({
...webpackConfig,
output: {
filename: "bundle.js",
path: outputPath,
},
});

[server, req, instance] = await frameworkFactory(
name,
framework,
compiler,
{},
{
setupMiddlewares: (middlewares) => {
if (name === "koa") {
middlewares.unshift(async (ctx, next) => {
ctx.url = "/file with spaces.html";

await next();
});
} else {
middlewares.unshift({
route: "/",
fn: (oldReq, res, next) => {
oldReq.url = "/file with spaces.html";
next();
},
});
}

return middlewares;
},
},
);

instance.context.outputFileSystem.mkdirSync(outputPath, {
recursive: true,
});
instance.context.outputFileSystem.writeFileSync(
path.resolve(outputPath, "file with spaces.html"),
"HTML with spaces",
);
});

afterAll(async () => {
await close(server, instance);
});

it('should return the "200" code for the "GET" request when req.url is rewritten to a path with spaces', async () => {
const response = await req.get("/any-path");

expect(response.statusCode).toBe(200);
expect(response.headers["content-type"]).toBe(
"text/html; charset=utf-8",
);
expect(response.text).toBe("HTML with spaces");
});
});
}

describe("should work and don't call the next middleware for finished or errored requests by default", () => {
let compiler;

Expand Down
5 changes: 5 additions & 0 deletions types/utils.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ export type ExpectedIncomingMessage = {
* an extra option for `fastify` (and `@fastify/express`) to get original URL
*/
originalUrl?: string | undefined;
/**
* an extra option for `fastify` (and `@fastify/express`) to get ID of request
*/
id?: string | undefined;
};
export type ExpectedServerResponse = {
/**
Expand Down Expand Up @@ -147,6 +151,7 @@ export function getOutgoing<
* @property {(() => string | undefined)=} getMethod get method extra method
* @property {(() => string | undefined)=} getURL get URL extra method
* @property {string=} originalUrl an extra option for `fastify` (and `@fastify/express`) to get original URL
* @property {string=} id an extra option for `fastify` (and `@fastify/express`) to get ID of request
*/
/**
* @typedef {object} ExpectedServerResponse
Expand Down
Loading