fix: replace portfinder with custom implementation and fix security problem (#4384)

Author: ludofischer

lib/Server.js

@@ -393,9 +393,8 @@ class Server {
     }
 
     const pRetry = require("p-retry");
-    const portfinder = require("portfinder");
-
-    portfinder.basePort =
+    const getPort = require("./getPort");
+    const basePort =
       typeof process.env.WEBPACK_DEV_SERVER_BASE_PORT !== "undefined"
         ? parseInt(process.env.WEBPACK_DEV_SERVER_BASE_PORT, 10)
         : 8080;
@@ -407,7 +406,7 @@ class Server {
         ? parseInt(process.env.WEBPACK_DEV_SERVER_PORT_RETRY, 10)
         : 3;
 
-    return pRetry(() => portfinder.getPortPromise(), {
+    return pRetry(() => getPort(basePort), {
       retries: defaultPortRetry,
     });
   }

lib/getPort.js

@@ -0,0 +1,122 @@
+"use strict";
+
+/*
+ * Based on the packages get-port https://www.npmjs.com/package/get-port
+ * and portfinder https://www.npmjs.com/package/portfinder
+ * The code structure is similar to get-port, but it searches
+ * ports deterministically like portfinder
+ */
+const net = require("net");
+const os = require("os");
+
+const minPort = 1024;
+const maxPort = 65_535;
+
+/**
+ * @return {Set<string|undefined>}
+ */
+const getLocalHosts = () => {
+  const interfaces = os.networkInterfaces();
+
+  // Add undefined value for createServer function to use default host,
+  // and default IPv4 host in case createServer defaults to IPv6.
+  // eslint-disable-next-line no-undefined
+  const results = new Set([undefined, "0.0.0.0"]);
+
+  for (const _interface of Object.values(interfaces)) {
+    if (_interface) {
+      for (const config of _interface) {
+        results.add(config.address);
+      }
+    }
+  }
+
+  return results;
+};
+
+/**
+ * @param {number} basePort
+ * @param {string | undefined} host
+ * @return {Promise<number>}
+ */
+const checkAvailablePort = (basePort, host) =>
+  new Promise((resolve, reject) => {
+    const server = net.createServer();
+    server.unref();
+    server.on("error", reject);
+
+    server.listen(basePort, host, () => {
+      // Next line should return AdressInfo because we're calling it after listen() and before close()
+      const { port } = /** @type {import("net").AddressInfo} */ (
+        server.address()
+      );
+      server.close(() => {
+        resolve(port);
+      });
+    });
+  });
+
+/**
+ * @param {number} port
+ * @param {Set<string|undefined>} hosts
+ * @return {Promise<number>}
+ */
+const getAvailablePort = async (port, hosts) => {
+  /**
+   * Errors that mean that host is not available.
+   * @type {Set<string | undefined>}
+   */
+  const nonExistentInterfaceErrors = new Set(["EADDRNOTAVAIL", "EINVAL"]);
+  /* Check if the post is available on every local host name */
+  for (const host of hosts) {
+    try {
+      await checkAvailablePort(port, host); // eslint-disable-line no-await-in-loop
+    } catch (error) {
+      /* We throw an error only if the interface exists */
+      if (
+        !nonExistentInterfaceErrors.has(
+          /** @type {NodeJS.ErrnoException} */ (error).code
+        )
+      ) {
+        throw error;
+      }
+    }
+  }
+
+  return port;
+};
+
+/**
+ * @param {number} basePort
+ * @return {Promise<number>}
+ */
+async function getPorts(basePort) {
+  if (basePort < minPort || basePort > maxPort) {
+    throw new Error(`Port number must lie between ${minPort} and ${maxPort}`);
+  }
+
+  let port = basePort;
+  const hosts = getLocalHosts();
+  /** @type {Set<string | undefined>} */
+  const portUnavailableErrors = new Set(["EADDRINUSE", "EACCES"]);
+  while (port <= maxPort) {
+    try {
+      const availablePort = await getAvailablePort(port, hosts); // eslint-disable-line no-await-in-loop
+      return availablePort;
+    } catch (error) {
+      /* Try next port if port is busy; throw for any other error */
+      if (
+        !portUnavailableErrors.has(
+          /** @type {NodeJS.ErrnoException} */ (error).code
+        )
+      ) {
+        throw error;
+      }
+      port += 1;
+    }
+  }
+
+  throw new Error("No available ports found");
+}
+
+module.exports = getPorts;

package-lock.json

@@ -55,7 +55,6 @@
     "ipaddr.js": "^2.0.1",
     "open": "^8.0.9",
     "p-retry": "^4.5.0",
-    "portfinder": "^1.0.28",
     "rimraf": "^3.0.2",
     "schema-utils": "^4.0.0",
     "selfsigned": "^2.0.1",

package.json

@@ -803,11 +803,10 @@ describe("API", () => {
     it("should throw the error when the port isn't found", async () => {
       expect.assertions(1);
 
-      jest.mock("portfinder", () => {
-        return {
-          getPortPromise: () => Promise.reject(new Error("busy")),
-        };
-      });
+      jest.mock(
+        "../../lib/getPort",
+        () => () => Promise.reject(new Error("busy"))
+      );
 
       process.env.WEBPACK_DEV_SERVER_PORT_RETRY = 1;
 

test/e2e/api.test.js

@@ -0,0 +1,36 @@
+"use strict";
+
+const net = require("net");
+const util = require("util");
+const getPort = require("../../lib/getPort");
+
+it("it should bind to the preferred port", async () => {
+  const preferredPort = 8080;
+  const port = await getPort(8080);
+  expect(port).toBe(preferredPort);
+});
+
+it("should pick the next port if the preferred port is unavailable", async () => {
+  const preferredPort = 8345;
+  const server = net.createServer();
+  server.unref();
+  await util.promisify(server.listen.bind(server))(preferredPort);
+  const port = await getPort(preferredPort);
+  expect(port).toBe(preferredPort + 1);
+});
+
+it("should reject privileged ports", async () => {
+  try {
+    await getPort(80);
+  } catch (e) {
+    expect(e.message).toBeDefined();
+  }
+});
+
+it("should reject too high port numbers", async () => {
+  try {
+    await getPort(65536);
+  } catch (e) {
+    expect(e.message).toBeDefined();
+  }
+});