fix: update getFilenameFromUrl to return errorCode and filename

README.md

@@ -459,7 +459,7 @@ const app = new express();
 app.use(instance);
 
 instance.waitUntilValid(() => {
-  const filename = instance.getFilenameFromUrl("/bundle.js");
+  const { filename } = instance.getFilenameFromUrl("/bundle.js");
 
   console.log(`Filename is ${filename}`);
 });

src/index.js

@@ -142,8 +142,7 @@ const noop = () => {};
 /**
  * @callback GetFilenameFromUrl
  * @param {string} url
- * @param {Extra=} extra
- * @returns {string | undefined}
+ * @returns {{ filename?: string, extra: Extra, errorCode?: number }}
  */
 
 /**
@@ -278,8 +277,7 @@ function wdm(compiler, options = {}) {
     (middleware(filledContext));
 
   // API
-  instance.getFilenameFromUrl = (url, extra) =>
-    getFilenameFromUrl(filledContext, url, extra);
+  instance.getFilenameFromUrl = (url) => getFilenameFromUrl(filledContext, url);
 
   instance.waitUntilValid = (callback = noop) => {
     ready(filledContext, callback);

src/middleware.js

@@ -498,22 +498,19 @@ function wrapper(context) {
      */
     async function processRequest() {
       // Pipe and SendFile
-      /** @type {import("./utils/getFilenameFromUrl").Extra} */
-      const extra = {};
-      const filename = getFilenameFromUrl(
+      const { filename, extra, errorCode } = getFilenameFromUrl(
         context,
         /** @type {string} */ (getRequestURL(req)),
-        extra,
       );
 
-      if (extra.errorCode) {
-        if (extra.errorCode === 403) {
+      if (errorCode) {
+        if (errorCode === 403) {
           context.logger.error(`Malicious path "${filename}".`);
         }
 
         await sendError(
-          extra.errorCode === 400 ? "Bad Request" : "Forbidden",
-          extra.errorCode,
+          errorCode === 400 ? "Bad Request" : "Forbidden",
+          errorCode,
           {
             modifyResponseData: context.options.modifyResponseData,
           },

src/utils/getFilenameFromUrl.js

@@ -1,7 +1,5 @@
 const path = require("node:path");
 const querystring = require("node:querystring");
-// eslint-disable-next-line n/no-deprecated-api
-const { parse } = require("node:url");
 
 const getPaths = require("./getPaths");
 const memorize = require("./memorize");
@@ -17,20 +15,31 @@ function decode(input) {
   return querystring.unescape(input);
 }
 
-const memoizedParse = memorize(parse, undefined, (value) => {
-  if (value.pathname) {
-    value.pathname = decode(value.pathname);
-  }
+// const memoizedParse = memorize(parse,
+//   undefined,
+//   (value) => {
+//     console.log({encode: value.pathname});
+//     if (value.pathname) {
+//       value.pathname = decode(value.pathname);
+//     }
+//     console.log({decode: value.pathname});
+
+//     return value;
+//   },
+// );
+
+const memoizedParse = memorize((url) => {
+  const urlObject = new URL(url, "http://localhost");
 
-  return value;
-});
+  // We cann't change pathname in URL object directly because don't decode correctly
+  return { ...urlObject, pathname: decode(urlObject.pathname) };
+}, undefined);
 
 const UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/;
 
 /**
  * @typedef {object} Extra
  * @property {import("fs").Stats=} stats stats
- * @property {number=} errorCode error code
  * @property {boolean=} immutable true when immutable, otherwise false
  */
 
@@ -42,43 +51,43 @@ const UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/;
  * @returns {string}
  */
 
-// TODO refactor me in the next major release, this function should return `{ filename, stats, error }`
 // TODO fix redirect logic when `/` at the end, like https://github.com/pillarjs/send/blob/master/index.js#L586
 /**
  * @template {IncomingMessage} Request
  * @template {ServerResponse} Response
  * @param {import("../index.js").FilledContext<Request, Response>} context context
  * @param {string} url url
- * @param {Extra=} extra extra
- * @returns {string | undefined} filename
+ * @returns {{ filename?: string, extra: Extra, errorCode?: number }} result of get filename from url
  */
-function getFilenameFromUrl(context, url, extra = {}) {
+function getFilenameFromUrl(context, url) {
+  /** @type {Extra} */
+  const extra = {};
   const { options } = context;
   const paths = getPaths(context);
 
   /** @type {string | undefined} */
   let foundFilename;
-  /** @type {import("node:url").Url} */
+  /** @type {number | undefined} */
+  let errorCode;
+  /** @type {import("node:url").URL} */
   let urlObject;
 
   try {
     // The `url` property of the `request` is contains only  `pathname`, `search` and `hash`
-    urlObject = memoizedParse(url, false, true);
+    urlObject = memoizedParse(url);
   } catch {
-    return;
+    return { errorCode, filename: foundFilename, extra };
   }
 
   for (const { publicPath, outputPath, assetsInfo } of paths) {
     /** @type {string | undefined} */
     let filename;
-    /** @type {import("node:url").Url} */
+    /** @type {import("node:url").URL} */
     let publicPathObject;
 
     try {
       publicPathObject = memoizedParse(
         publicPath !== "auto" && publicPath ? publicPath : "/",
-        false,
-        true,
       );
     } catch {
       continue;
@@ -94,16 +103,16 @@ function getFilenameFromUrl(context, url, extra = {}) {
     ) {
       // Null byte(s)
       if (pathname.includes("\0")) {
-        extra.errorCode = 400;
+        errorCode = 400;
 
-        return;
+        return { errorCode, filename: foundFilename, extra };
       }
 
       // ".." is malicious
       if (UP_PATH_REGEXP.test(path.normalize(`./${pathname}`))) {
-        extra.errorCode = 403;
+        errorCode = 403;
 
-        return;
+        return { errorCode, filename: foundFilename, extra };
       }
 
       // Strip the `pathname` property from the `publicPath` option from the start of requested url
@@ -161,7 +170,7 @@ function getFilenameFromUrl(context, url, extra = {}) {
     }
   }
 
-  return foundFilename;
+  return { filename: foundFilename, extra, errorCode };
 }
 
 module.exports = getFilenameFromUrl;

test/middleware.test.js

@@ -573,23 +573,24 @@ describe.each([
 
           it("should work", (done) => {
             instance.waitUntilValid(() => {
-              expect(instance.getFilenameFromUrl("/bundle.js")).toBe(
+              expect(instance.getFilenameFromUrl("/bundle.js").filename).toBe(
                 path.join(webpackConfig.output.path, "/bundle.js"),
               );
-              expect(instance.getFilenameFromUrl("/")).toBe(
+              expect(instance.getFilenameFromUrl("/").filename).toBe(
                 path.join(webpackConfig.output.path, "/index.html"),
               );
-              expect(instance.getFilenameFromUrl("/index.html")).toBe(
+              expect(instance.getFilenameFromUrl("/index.html").filename).toBe(
                 path.join(webpackConfig.output.path, "/index.html"),
               );
-              expect(instance.getFilenameFromUrl("/svg.svg")).toBe(
+              expect(instance.getFilenameFromUrl("/svg.svg").filename).toBe(
                 path.join(webpackConfig.output.path, "/svg.svg"),
               );
               expect(
-                instance.getFilenameFromUrl("/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown.unknown").filename,
               ).toBeUndefined();
               expect(
-                instance.getFilenameFromUrl("/unknown/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
 
               done();
@@ -617,22 +618,23 @@ describe.each([
 
           it("should work", (done) => {
             instance.waitUntilValid(() => {
-              expect(instance.getFilenameFromUrl("/bundle.js")).toBe(
+              expect(instance.getFilenameFromUrl("/bundle.js").filename).toBe(
                 path.join(webpackConfig.output.path, "/bundle.js"),
               );
 
-              expect(instance.getFilenameFromUrl("/")).toBeUndefined();
-              expect(instance.getFilenameFromUrl("/index.html")).toBe(
+              expect(instance.getFilenameFromUrl("/").filename).toBeUndefined();
+              expect(instance.getFilenameFromUrl("/index.html").filename).toBe(
                 path.join(webpackConfig.output.path, "/index.html"),
               );
-              expect(instance.getFilenameFromUrl("/svg.svg")).toBe(
+              expect(instance.getFilenameFromUrl("/svg.svg").filename).toBe(
                 path.join(webpackConfig.output.path, "/svg.svg"),
               );
               expect(
-                instance.getFilenameFromUrl("/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown.unknown").filename,
               ).toBeUndefined();
               expect(
-                instance.getFilenameFromUrl("/unknown/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
 
               done();
@@ -658,28 +660,33 @@ describe.each([
           it("should work", (done) => {
             instance.waitUntilValid(() => {
               expect(
-                instance.getFilenameFromUrl("/public/path/bundle.js"),
+                instance.getFilenameFromUrl("/public/path/bundle.js").filename,
               ).toBe(
                 path.join(webpackPublicPathConfig.output.path, "/bundle.js"),
               );
-              expect(instance.getFilenameFromUrl("/public/path/")).toBe(
+              expect(
+                instance.getFilenameFromUrl("/public/path/").filename,
+              ).toBe(
                 path.join(webpackPublicPathConfig.output.path, "/index.html"),
               );
               expect(
-                instance.getFilenameFromUrl("/public/path/index.html"),
+                instance.getFilenameFromUrl("/public/path/index.html").filename,
               ).toBe(
                 path.join(webpackPublicPathConfig.output.path, "/index.html"),
               );
-              expect(instance.getFilenameFromUrl("/public/path/svg.svg")).toBe(
+              expect(
+                instance.getFilenameFromUrl("/public/path/svg.svg").filename,
+              ).toBe(
                 path.join(webpackPublicPathConfig.output.path, "/svg.svg"),
               );
 
-              expect(instance.getFilenameFromUrl("/")).toBeUndefined();
+              expect(instance.getFilenameFromUrl("/").filename).toBeUndefined();
               expect(
-                instance.getFilenameFromUrl("/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown.unknown").filename,
               ).toBeUndefined();
               expect(
-                instance.getFilenameFromUrl("/unknown/unknown.unknown"),
+                instance.getFilenameFromUrl("/unknown/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
 
               done();
@@ -704,49 +711,56 @@ describe.each([
 
           it("should work", (done) => {
             instance.waitUntilValid(() => {
-              expect(instance.getFilenameFromUrl("/static-one/bundle.js")).toBe(
+              expect(
+                instance.getFilenameFromUrl("/static-one/bundle.js").filename,
+              ).toBe(
                 path.join(webpackMultiConfig[0].output.path, "/bundle.js"),
               );
-              expect(instance.getFilenameFromUrl("/static-one/")).toBe(
+              expect(instance.getFilenameFromUrl("/static-one/").filename).toBe(
                 path.join(webpackMultiConfig[0].output.path, "/index.html"),
               );
               expect(
-                instance.getFilenameFromUrl("/static-one/index.html"),
+                instance.getFilenameFromUrl("/static-one/index.html").filename,
               ).toBe(
                 path.join(webpackMultiConfig[0].output.path, "/index.html"),
               );
-              expect(instance.getFilenameFromUrl("/static-one/svg.svg")).toBe(
-                path.join(webpackMultiConfig[0].output.path, "/svg.svg"),
-              );
               expect(
-                instance.getFilenameFromUrl("/static-one/unknown.unknown"),
+                instance.getFilenameFromUrl("/static-one/svg.svg").filename,
+              ).toBe(path.join(webpackMultiConfig[0].output.path, "/svg.svg"));
+              expect(
+                instance.getFilenameFromUrl("/static-one/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
               expect(
                 instance.getFilenameFromUrl(
                   "/static-one/unknown/unknown.unknown",
-                ),
+                ).filename,
               ).toBeUndefined();
 
-              expect(instance.getFilenameFromUrl("/static-two/bundle.js")).toBe(
+              expect(
+                instance.getFilenameFromUrl("/static-two/bundle.js").filename,
+              ).toBe(
                 path.join(webpackMultiConfig[1].output.path, "/bundle.js"),
               );
               expect(
-                instance.getFilenameFromUrl("/static-two/unknown.unknown"),
+                instance.getFilenameFromUrl("/static-two/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
               expect(
                 instance.getFilenameFromUrl(
                   "/static-two/unknown/unknown.unknown",
-                ),
+                ).filename,
               ).toBeUndefined();
 
-              expect(instance.getFilenameFromUrl("/")).toBeUndefined();
+              expect(instance.getFilenameFromUrl("/").filename).toBeUndefined();
               expect(
-                instance.getFilenameFromUrl("/static-one/unknown.unknown"),
+                instance.getFilenameFromUrl("/static-one/unknown.unknown")
+                  .filename,
               ).toBeUndefined();
               expect(
                 instance.getFilenameFromUrl(
                   "/static-one/unknown/unknown.unknown",
-                ),
+                ).filename,
               ).toBeUndefined();
 
               done();

types/index.d.ts

@@ -108,8 +108,7 @@ export = wdm;
 /**
  * @callback GetFilenameFromUrl
  * @param {string} url
- * @param {Extra=} extra
- * @returns {string | undefined}
+ * @returns {{ filename?: string, extra: Extra, errorCode?: number }}
  */
 /**
  * @callback WaitUntilValid
@@ -460,10 +459,11 @@ type Middleware<
   next: NextFunction,
 ) => Promise<void>;
 type Extra = import("./utils/getFilenameFromUrl").Extra;
-type GetFilenameFromUrl = (
-  url: string,
-  extra?: Extra | undefined,
-) => string | undefined;
+type GetFilenameFromUrl = (url: string) => {
+  filename?: string;
+  extra: Extra;
+  errorCode?: number;
+};
 type WaitUntilValid = (callback: Callback) => any;
 type Invalidate = (callback: Callback) => any;
 type Close = (callback: (err: Error | null | undefined) => void) => any;