feat!: update getFilenameFromUrl to throw errors and return undefined when not found

Author: bjohansebas

.cspell.json

@@ -23,7 +23,8 @@
     "cachable",
     "finalhandler",
     "hono",
-    "rspack"
+    "rspack",
+    "malformed"
   ],
   "ignorePaths": [
     "CHANGELOG.md",

README.md

@@ -459,7 +459,18 @@ const app = new express();
 app.use(instance);
 
 instance.waitUntilValid(() => {
-  const filename = instance.getFilenameFromUrl("/bundle.js");
+  let resolver;
+
+  try {
+    resolved = instance.getFilenameFromUrl("/bundle.js");
+  } catch (err) {
+    console.log(`Error: ${err}`);
+  }
+
+  if (!resolved) {
+    console.log("Not found");
+    return;
+  }
 
   console.log(`Filename is ${filename}`);
 });

src/index.js

@@ -131,19 +131,18 @@ const noop = () => {};
  * @template {IncomingMessage} [RequestInternal=IncomingMessage]
  * @template {ServerResponse} [ResponseInternal=ServerResponse]
  * @callback Middleware
- * @param {RequestInternal} req
- * @param {ResponseInternal} res
- * @param {NextFunction} next
+ * @param {RequestInternal} req request
+ * @param {ResponseInternal} res response
+ * @param {NextFunction} next next function
  * @returns {Promise<void>}
  */
 
 /** @typedef {import("./utils/getFilenameFromUrl").Extra} Extra */
 
 /**
  * @callback GetFilenameFromUrl
- * @param {string} url
- * @param {Extra=} extra
- * @returns {string | undefined}
+ * @param {string} url request URL
+ * @returns {{ filename: string, extra: Extra } | undefined} a filename with additional information, or `undefined` if nothing is found
  */
 
 /**
@@ -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

@@ -31,6 +31,8 @@ const ready = require("./utils/ready");
 /** @typedef {import("./index.js").IncomingMessage} IncomingMessage */
 /** @typedef {import("./index.js").ServerResponse} ServerResponse */
 /** @typedef {import("./index.js").NormalizedHeaders} NormalizedHeaders */
+/** @typedef {import("./utils/getFilenameFromUrl.js").FilenameError} FilenameError */
+/** @typedef {import("./utils/getFilenameFromUrl.js").Extra} Extra */
 /** @typedef {import("fs").ReadStream} ReadStream */
 
 const BYTES_RANGE_REGEXP = /^ *bytes/i;
@@ -498,30 +500,37 @@ function wrapper(context) {
      */
     async function processRequest() {
       // Pipe and SendFile
-      /** @type {import("./utils/getFilenameFromUrl").Extra} */
-      const extra = {};
-      const filename = getFilenameFromUrl(
-        context,
-        /** @type {string} */ (getRequestURL(req)),
-        extra,
-      );
-
-      if (extra.errorCode) {
-        if (extra.errorCode === 403) {
-          context.logger.error(`Malicious path "${filename}".`);
+      /** @type {{ filename: string, extra: Extra } | undefined} */
+      let resolved;
+
+      const requestUrl = /** @type {string} */ (getRequestURL(req));
+
+      try {
+        resolved = getFilenameFromUrl(context, requestUrl);
+      } catch (err) {
+        // Fallback to 403 for unknown errors
+        const errorCode =
+          typeof err === "object" &&
+          err !== null &&
+          typeof (/** @type {FilenameError} */ (err).code) !== "undefined"
+            ? /** @type {FilenameError} */ (err).code
+            : 403;
+
+        if (errorCode === 403) {
+          context.logger.error(`Malicious path "${requestUrl}".`);
         }
 
         await sendError(
-          extra.errorCode === 400 ? "Bad Request" : "Forbidden",
-          extra.errorCode,
+          errorCode === 400 ? "Bad Request" : "Forbidden",
+          errorCode,
           {
             modifyResponseData: context.options.modifyResponseData,
           },
         );
         return;
       }
 
-      if (!filename) {
+      if (!resolved) {
         await goNext();
         return;
       }
@@ -531,7 +540,8 @@ function wrapper(context) {
         return;
       }
 
-      const { size } = /** @type {import("fs").Stats} */ (extra.stats);
+      const { extra, filename } = resolved;
+      const { size } = extra.stats;
 
       let len = size;
       let offset = 0;
@@ -609,9 +619,7 @@ function wrapper(context) {
         context.options.lastModified &&
         !getResponseHeader(res, "Last-Modified")
       ) {
-        const modified =
-          /** @type {import("fs").Stats} */
-          (extra.stats).mtime.toUTCString();
+        const modified = extra.stats.mtime.toUTCString();
 
         setResponseHeader(res, "Last-Modified", modified);
       }
@@ -667,7 +675,7 @@ function wrapper(context) {
         const result = await getETag()(
           isStrongETag
             ? /** @type {Buffer | ReadStream} */ (bufferOrStream)
-            : /** @type {import("fs").Stats} */ (extra.stats),
+            : extra.stats,
         );
 
         // Because we already read stream, we can cache buffer to avoid extra read from fs

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,18 @@ function decode(input) {
   return querystring.unescape(input);
 }
 
-const memoizedParse = memorize(parse, undefined, (value) => {
-  if (value.pathname) {
-    value.pathname = decode(value.pathname);
-  }
+const memoizedParse = memorize((url) => {
+  const urlObject = new URL(url, "http://localhost");
 
-  return value;
-});
+  // We can'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 {import("fs").Stats} stats stats
  * @property {boolean=} immutable true when immutable, otherwise false
  */
 
@@ -42,43 +38,55 @@ const UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/;
  * @returns {string}
  */
 
-// TODO refactor me in the next major release, this function should return `{ filename, stats, error }`
+class FilenameError extends Error {
+  /**
+   * @param {string} message message
+   * @param {number=} code error code
+   */
+  constructor(message, code) {
+    super(message);
+    this.name = "FilenameError";
+    this.code = code;
+  }
+}
+
 // 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 } | undefined} result of get filename from url
  */
-function getFilenameFromUrl(context, url, extra = {}) {
-  const { options } = context;
-  const paths = getPaths(context);
+function getFilenameFromUrl(context, url) {
+  /** @type {URL} */
+  let urlObject;
 
   /** @type {string | undefined} */
   let foundFilename;
-  /** @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;
   }
 
+  const { options } = context;
+  const paths = getPaths(context);
+
+  /** @type {Extra} */
+  const extra = {};
+
   for (const { publicPath, outputPath, assetsInfo } of paths) {
     /** @type {string | undefined} */
     let filename;
-    /** @type {import("node:url").Url} */
+    /** @type {URL} */
     let publicPathObject;
 
     try {
       publicPathObject = memoizedParse(
         publicPath !== "auto" && publicPath ? publicPath : "/",
-        false,
-        true,
       );
     } catch {
       continue;
@@ -94,16 +102,12 @@ function getFilenameFromUrl(context, url, extra = {}) {
     ) {
       // Null byte(s)
       if (pathname.includes("\0")) {
-        extra.errorCode = 400;
-
-        return;
+        throw new FilenameError("Bad Request", 400);
       }
 
       // ".." is malicious
       if (UP_PATH_REGEXP.test(path.normalize(`./${pathname}`))) {
-        extra.errorCode = 403;
-
-        return;
+        throw new FilenameError("Forbidden", 403);
       }
 
       // Strip the `pathname` property from the `publicPath` option from the start of requested url
@@ -161,7 +165,12 @@ function getFilenameFromUrl(context, url, extra = {}) {
     }
   }
 
-  return foundFilename;
+  if (!foundFilename) {
+    return;
+  }
+
+  return { filename: foundFilename, extra };
 }
 
 module.exports = getFilenameFromUrl;
+module.exports.FilenameError = FilenameError;