fix(security): do not allow to read files above

Author: alexander-akait

.cspell.json

@@ -18,7 +18,8 @@
     "configurated",
     "mycustom",
     "commitlint",
-    "nosniff"
+    "nosniff",
+    "deoptimize"
   ],
   "ignorePaths": [
     "CHANGELOG.md",

src/middleware.js

@@ -80,6 +80,18 @@ function wrapper(context) {
         extra,
       );
 
+      if (extra.errorCode) {
+        if (extra.errorCode === 403) {
+          context.logger.error(`Malicious path "${filename}".`);
+        }
+
+        sendError(req, res, extra.errorCode, {
+          modifyResponseData: context.options.modifyResponseData,
+        });
+
+        return;
+      }
+
       if (!filename) {
         await goNext();
 
@@ -164,6 +176,7 @@ function wrapper(context) {
             headers: {
               "Content-Range": res.getHeader("Content-Range"),
             },
+            modifyResponseData: context.options.modifyResponseData,
           });
 
           return;

src/utils/compatibleAPI.js

@@ -177,6 +177,8 @@ function destroyStream(stream, suppress) {
 
 /** @type {Record<number, string>} */
 const statuses = {
+  400: "Bad Request",
+  403: "Forbidden",
   404: "Not Found",
   416: "Range Not Satisfiable",
   500: "Internal Server Error",

src/utils/getFilenameFromUrl.js

@@ -1,6 +1,5 @@
 const path = require("path");
 const { parse } = require("url");
-const querystring = require("querystring");
 
 const getPaths = require("./getPaths");
 
@@ -43,11 +42,32 @@ const mem = (fn, { cache = new Map() } = {}) => {
 };
 const memoizedParse = mem(parse);
 
+const UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/;
+
 /**
  * @typedef {Object} Extra
  * @property {import("fs").Stats=} stats
+ * @property {number=} errorCode
  */
 
+/**
+ * decodeURIComponent.
+ *
+ * Allows V8 to only deoptimize this fn instead of all of send().
+ *
+ * @param {string} path
+ * @returns {string | -1}
+ */
+
+function decode(path) {
+  try {
+    return decodeURIComponent(path);
+  } catch (err) {
+    return -1;
+  }
+}
+
+// TODO refactor me in the next major release, this function should return `{ filename, stats, error }`
 /**
  * @template {IncomingMessage} Request
  * @template {ServerResponse} Response
@@ -85,21 +105,37 @@ function getFilenameFromUrl(context, url, extra = {}) {
       continue;
     }
 
-    if (
-      urlObject.pathname &&
-      urlObject.pathname.startsWith(publicPathObject.pathname)
-    ) {
-      filename = outputPath;
+    let pathname = decode(urlObject.pathname);
+
+    // Can't be decoded
+    if (pathname === -1) {
+      // eslint-disable-next-line no-param-reassign
+      extra.errorCode = 400;
+
+      return;
+    }
+
+    // Null byte(s)
+    if (pathname.includes("\0")) {
+      // eslint-disable-next-line no-param-reassign
+      extra.errorCode = 400;
+
+      return;
+    }
+
+    // ".." is malicious
+    if (UP_PATH_REGEXP.test(pathname)) {
+      // eslint-disable-next-line no-param-reassign
+      extra.errorCode = 403;
 
+      return;
+    }
+
+    if (pathname && pathname.startsWith(publicPathObject.pathname)) {
       // Strip the `pathname` property from the `publicPath` option from the start of requested url
       // `/complex/foo.js` => `foo.js`
-      const pathname = urlObject.pathname.slice(
-        publicPathObject.pathname.length,
-      );
-
-      if (pathname) {
-        filename = path.join(outputPath, querystring.unescape(pathname));
-      }
+      pathname = pathname.slice(publicPathObject.pathname.length);
+      filename = path.join(outputPath, pathname);
 
       try {
         // eslint-disable-next-line no-param-reassign

test/middleware.test.js

@@ -99,6 +99,10 @@ describe.each([
             path.resolve(outputPath, "image.svg"),
             "svg image",
           );
+          instance.context.outputFileSystem.writeFileSync(
+            path.resolve(outputPath, "image image.svg"),
+            "svg image",
+          );
           instance.context.outputFileSystem.writeFileSync(
             path.resolve(outputPath, "byte-length.html"),
             "\u00bd + \u00bc = \u00be",
@@ -447,6 +451,29 @@ describe.each([
             false,
           );
         });
+
+        it('should return the "200" code for the "GET" request to the "image image.svg" file', async () => {
+          const fileData = instance.context.outputFileSystem.readFileSync(
+            path.resolve(outputPath, "image image.svg"),
+          );
+
+          const response = await req.get("/image image.svg");
+
+          expect(response.statusCode).toEqual(200);
+          expect(response.headers["content-length"]).toEqual(
+            fileData.byteLength.toString(),
+          );
+          expect(response.headers["content-type"]).toEqual("image/svg+xml");
+        });
+
+        it('should return the "400" code for the "GET" request to the "%FF" file', async () => {
+          const response = await req.get("/%FF");
+
+          expect(response.statusCode).toEqual(400);
+          expect(response.headers["content-type"]).toEqual(
+            "text/html; charset=UTF-8",
+          );
+        });
       });
 
       describe('should not work with the broken "publicPath" option', () => {
@@ -2575,6 +2602,7 @@ describe.each([
             output: {
               filename: "bundle.js",
               path: path.resolve(__dirname, "./outputs/write-to-disk-true"),
+              publicPath: "/public/",
             },
           });
 
@@ -2598,7 +2626,7 @@ describe.each([
 
         it("should find the bundle file on disk", (done) => {
           request(app)
-            .get("/bundle.js")
+            .get("/public/bundle.js")
             .expect(200, (error) => {
               if (error) {
                 return done(error);
@@ -2632,6 +2660,25 @@ describe.each([
               );
             });
         });
+
+        it("should not allow to get files above root", async () => {
+          const response = await req.get("/public/..%2f../middleware.test.js");
+
+          expect(response.statusCode).toEqual(403);
+          expect(response.headers["content-type"]).toEqual(
+            "text/html; charset=UTF-8",
+          );
+          expect(response.text).toEqual(`<!DOCTYPE html>
+<html lang="en">
+<head>
+<meta charset="utf-8">
+<title>Error</title>
+</head>
+<body>
+<pre>Forbidden</pre>
+</body>
+</html>`);
+        });
       });
 
       describe('should work with "true" value when the `output.clean` is `true`', () => {

types/utils/getFilenameFromUrl.d.ts

@@ -1,9 +1,5 @@
 /// <reference types="node" />
 export = getFilenameFromUrl;
-/**
- * @typedef {Object} Extra
- * @property {import("fs").Stats=} stats
- */
 /**
  * @template {IncomingMessage} Request
  * @template {ServerResponse} Response
@@ -25,6 +21,7 @@ declare namespace getFilenameFromUrl {
 }
 type Extra = {
   stats?: import("fs").Stats | undefined;
+  errorCode?: number | undefined;
 };
 type IncomingMessage = import("../index.js").IncomingMessage;
 type ServerResponse = import("../index.js").ServerResponse;