fix: Replace express-static-gzip and finalhandler with srvx (#32685)

Co-authored-by: Koen Kanters <koenkanters94@gmail.com>

test/extensions/frontend.test.ts

@@ -13,7 +13,12 @@ import ws from "ws";
 import {Controller} from "../../lib/controller";
 import * as settings from "../../lib/util/settings";
 
-let mockHTTPOnRequest: (request: {url: string}, response: number) => void;
+const mockRedirectResponse = {
+    writeHead: vi.fn<(statusCode: number, headers: Record<string, string>) => void>(),
+    end: vi.fn<() => void>(),
+};
+
+let mockHTTPOnRequest: (request: {url: string}, response: number | typeof mockRedirectResponse) => void;
 const mockHTTPEvents: Record<string, EventHandler> = {};
 const mockHTTP = {
     listen: vi.fn(),
@@ -64,7 +69,7 @@ const frontendPath = "frontend-path";
 const deviceIconsPath = path.join(data.mockDir, "device_icons");
 let mockNodeStatic: {[s: string]: Mock} = {};
 
-const mockFinalHandler = vi.fn();
+const mockSendNotFound = vi.fn();
 
 vi.mock("node:http", () => ({
     createServer: vi.fn().mockImplementation((onRequest) => {
@@ -79,11 +84,12 @@ vi.mock("node:https", () => ({
     Agent: vi.fn(),
 }));
 
-vi.mock("express-static-gzip", () => ({
-    default: vi.fn().mockImplementation((path: string) => {
+vi.mock("../../lib/util/staticFileServer", () => ({
+    createStaticFileServer: vi.fn().mockImplementation((path: string) => {
         mockNodeStatic[path] = vi.fn();
         return mockNodeStatic[path];
     }),
+    sendNotFound: vi.fn().mockImplementation((...args: unknown[]) => mockSendNotFound(...args)),
 }));
 
 vi.mock("zigbee2mqtt-windfront", () => ({
@@ -101,12 +107,6 @@ vi.mock("ws", () => ({
     },
 }));
 
-vi.mock("finalhandler", () => ({
-    default: vi.fn().mockImplementation(() => {
-        return mockFinalHandler;
-    }),
-}));
-
 const mocksClear = [
     mockHTTP.close,
     mockHTTP.listen,
@@ -118,7 +118,9 @@ const mocksClear = [
     mockWS.emit,
     mockWSClient.send,
     mockWSClient.terminate,
-    mockFinalHandler,
+    mockSendNotFound,
+    mockRedirectResponse.writeHead,
+    mockRedirectResponse.end,
     mockMQTTPublishAsync,
     mockLogger.error,
 ];
@@ -340,11 +342,7 @@ describe("Extension: Frontend", () => {
         mockHTTPOnRequest({url: "/file.txt"}, 2);
         expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledTimes(0);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith(
-            {originalUrl: "/file.txt", path: "/file.txt", url: "/file.txt"},
-            2,
-            expect.any(Function),
-        );
+        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({url: "/file.txt"}, 2);
     });
 
     it("Should serve device icons", async () => {
@@ -354,11 +352,7 @@ describe("Extension: Frontend", () => {
         mockHTTPOnRequest({url: "/device_icons/my_device.png"}, 2);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(0);
         expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledWith(
-            {originalUrl: "/device_icons/my_device.png", path: "/my_device.png", url: "/my_device.png"},
-            2,
-            expect.any(Function),
-        );
+        expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledWith({url: "/my_device.png"}, 2);
     });
 
     it("Static server", async () => {
@@ -402,34 +396,33 @@ describe("Extension: Frontend", () => {
 
         expect(ws.Server).toHaveBeenCalledWith({noServer: true, path: "/z2m/api"});
 
-        mockHTTPOnRequest({url: "/z2m"}, 2);
+        // the base url without trailing slash points at a directory, redirect so relative asset paths resolve against it
+        mockHTTPOnRequest({url: "/z2m"}, mockRedirectResponse);
+        expect(mockNodeStatic[frontendPath]).not.toHaveBeenCalled();
+        expect(mockRedirectResponse.writeHead).toHaveBeenCalledWith(301, {Location: "/z2m/"});
+        expect(mockRedirectResponse.end).toHaveBeenCalledTimes(1);
+        expect(mockSendNotFound).not.toHaveBeenCalled();
+
+        mockHTTPOnRequest({url: "/z2m/"}, 2);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({originalUrl: "/z2m", path: "/", url: "/"}, 2, expect.any(Function));
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({url: "/"}, 2);
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
 
         mockNodeStatic[frontendPath].mockReset();
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
         mockHTTPOnRequest({url: "/z2m/file.txt"}, 2);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith(
-            {originalUrl: "/z2m/file.txt", path: "/file.txt", url: "/file.txt"},
-            2,
-            expect.any(Function),
-        );
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({url: "/file.txt"}, 2);
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
 
         mockNodeStatic[frontendPath].mockReset();
         mockHTTPOnRequest({url: "/z/file.txt"}, 2);
         expect(mockNodeStatic[frontendPath]).not.toHaveBeenCalled();
-        expect(mockFinalHandler).toHaveBeenCalled();
+        expect(mockSendNotFound).toHaveBeenCalled();
 
         mockHTTPOnRequest({url: "/z2m/device_icons/my-device.png"}, 2);
         expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledWith(
-            {originalUrl: "/z2m/device_icons/my-device.png", path: "/my-device.png", url: "/my-device.png"},
-            2,
-            expect.any(Function),
-        );
+        expect(mockNodeStatic[deviceIconsPath]).toHaveBeenCalledWith({url: "/my-device.png"}, 2);
     });
 
     it("Works with non-default complex base url", async () => {
@@ -440,30 +433,28 @@ describe("Extension: Frontend", () => {
 
         expect(ws.Server).toHaveBeenCalledWith({noServer: true, path: "/z2m-more++/c0mplex.url/api"});
 
-        mockHTTPOnRequest({url: "/z2m-more++/c0mplex.url"}, 2);
+        mockHTTPOnRequest({url: "/z2m-more++/c0mplex.url"}, mockRedirectResponse);
+        expect(mockNodeStatic[frontendPath]).not.toHaveBeenCalled();
+        expect(mockRedirectResponse.writeHead).toHaveBeenCalledWith(301, {Location: "/z2m-more++/c0mplex.url/"});
+        expect(mockRedirectResponse.end).toHaveBeenCalledTimes(1);
+        expect(mockSendNotFound).not.toHaveBeenCalled();
+
+        mockHTTPOnRequest({url: "/z2m-more++/c0mplex.url/"}, 2);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith(
-            {originalUrl: "/z2m-more++/c0mplex.url", path: "/", url: "/"},
-            2,
-            expect.any(Function),
-        );
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({url: "/"}, 2);
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
 
         mockNodeStatic[frontendPath].mockReset();
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
         mockHTTPOnRequest({url: "/z2m-more++/c0mplex.url/file.txt"}, 2);
         expect(mockNodeStatic[frontendPath]).toHaveBeenCalledTimes(1);
-        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith(
-            {originalUrl: "/z2m-more++/c0mplex.url/file.txt", path: "/file.txt", url: "/file.txt"},
-            2,
-            expect.any(Function),
-        );
-        expect(mockFinalHandler).not.toHaveBeenCalledWith();
+        expect(mockNodeStatic[frontendPath]).toHaveBeenCalledWith({url: "/file.txt"}, 2);
+        expect(mockSendNotFound).not.toHaveBeenCalledWith();
 
         mockNodeStatic[frontendPath].mockReset();
         mockHTTPOnRequest({url: "/z/file.txt"}, 2);
         expect(mockNodeStatic[frontendPath]).not.toHaveBeenCalled();
-        expect(mockFinalHandler).toHaveBeenCalled();
+        expect(mockSendNotFound).toHaveBeenCalled();
     });
 
     it("prevents mismatching setting/extension state", async () => {

test/onboarding.test.ts

@@ -31,16 +31,10 @@ const mockHttpClose = vi.fn<Server["close"]>(
     },
 );
 const mockFindAllDevices = vi.fn<typeof findAllDevices>(async () => []);
-const mockStaticFileServer = vi.fn((_req, res, next) => {
-    if (typeof next === "function") {
-        next();
-    }
-
+const mockStaticFileServer = vi.fn((_req, res) => {
     res.end();
 });
-const mockExpressStaticGzip = vi.fn((_path: unknown, _options: unknown) => mockStaticFileServer);
-const mockFinalHandlerNext = vi.fn();
-const mockFinalhandler = vi.fn((_req: unknown, _res: unknown) => mockFinalHandlerNext);
+const mockCreateStaticFileServer = vi.fn((_dir: unknown, _logError: unknown) => mockStaticFileServer);
 
 vi.mock("node:fs", {spy: true});
 vi.mock("node:http", () => ({
@@ -62,11 +56,8 @@ vi.mock("node:http", () => ({
         };
     }),
 }));
-vi.mock("express-static-gzip", () => ({
-    default: vi.fn((path, options) => mockExpressStaticGzip(path, options)),
-}));
-vi.mock("finalhandler", () => ({
-    default: vi.fn((req, res) => mockFinalhandler(req, res)),
+vi.mock("../lib/util/staticFileServer", () => ({
+    createStaticFileServer: vi.fn((dir, logError) => mockCreateStaticFileServer(dir, logError)),
 }));
 vi.mock("zigbee-herdsman/dist/adapter/adapterDiscovery", () => ({
     findAllDevices: vi.fn(() => mockFindAllDevices()),
@@ -194,10 +185,7 @@ describe("Onboarding", () => {
         mockFindAllDevices.mockClear();
         mockHttpErrorListener = undefined;
         mockStaticFileServer.mockClear();
-        mockExpressStaticGzip.mockClear();
-        mockFinalHandlerNext.mockClear();
-        mockFinalhandler.mockClear();
-        mockStaticFileServer.mockClear();
+        mockCreateStaticFileServer.mockClear();
         settings.reRead();
     });
 
@@ -735,7 +723,6 @@ describe("Onboarding", () => {
         });
 
         await expect(p).resolves.toStrictEqual(true);
-        expect(mockFinalhandler).toHaveBeenCalled();
         expect(mockStaticFileServer).toHaveBeenCalled();
     });
 
@@ -757,7 +744,6 @@ describe("Onboarding", () => {
         });
 
         await expect(p).resolves.toStrictEqual(false);
-        expect(mockFinalhandler).toHaveBeenCalled();
         expect(mockStaticFileServer).toHaveBeenCalled();
     });
 

test/staticFileServer.test.ts

@@ -0,0 +1,185 @@
+import {mkdirSync, writeFileSync} from "node:fs";
+import {createServer, type Server} from "node:http";
+import {type AddressInfo, connect} from "node:net";
+import {join} from "node:path";
+import {brotliCompressSync, gzipSync} from "node:zlib";
+import tmp from "tmp";
+import {afterAll, beforeAll, describe, expect, it, vi} from "vitest";
+import {createStaticFileServer, type sendNotFound} from "../lib/util/staticFileServer";
+
+const INDEX_HTML = "<!DOCTYPE html><html lang='en'><body>index</body></html>";
+const APP_JS = `console.log("${"x".repeat(2048)}");`;
+/** Written next to the served directory, never inside it, so a traversal that succeeds is actually observable. */
+const SECRET = "topsecret-must-never-be-served";
+
+const mockLogError = vi.fn<(message: string) => void>();
+
+let dir: string;
+let server: Server;
+let baseUrl: string;
+
+/** Starts a `node:http` server serving `dir`, mirroring how the frontend/onboarding extensions wire it up. */
+function listen(handler: (request: Parameters<typeof sendNotFound>[0], response: Parameters<typeof sendNotFound>[1]) => void): Promise<void> {
+    server = createServer(handler);
+
+    return new Promise((resolve) => {
+        server.listen(0, "127.0.0.1", () => {
+            baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`;
+
+            resolve();
+        });
+    });
+}
+
+/** Writes a request line verbatim, bypassing the path normalization `fetch` applies before sending. */
+function rawRequest(target: string): Promise<string> {
+    return new Promise((resolve, reject) => {
+        const socket = connect((server.address() as AddressInfo).port, "127.0.0.1", () => {
+            socket.write(`GET ${target} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n`);
+        });
+        let received = "";
+
+        socket.setEncoding("utf8");
+        socket.on("data", (chunk) => {
+            received += chunk;
+        });
+        socket.on("end", () => resolve(received));
+        socket.on("error", reject);
+    });
+}
+
+describe("StaticFileServer", () => {
+    beforeAll(async () => {
+        const root = tmp.dirSync().name;
+
+        dir = join(root, "public");
+
+        // outside the served directory: only a working traversal could reach it
+        writeFileSync(join(root, "secret.txt"), SECRET);
+        mkdirSync(join(dir, "sub"), {recursive: true});
+        writeFileSync(join(dir, "index.html"), INDEX_HTML);
+        writeFileSync(join(dir, "app.js"), APP_JS);
+        // precompressed variants, as shipped by the frontend packages
+        writeFileSync(join(dir, "app.js.gz"), gzipSync(APP_JS));
+        writeFileSync(join(dir, "app.js.br"), brotliCompressSync(APP_JS));
+        writeFileSync(join(dir, "sub", "icon.png"), Buffer.from([0x89, 0x50, 0x4e, 0x47]));
+
+        await listen(createStaticFileServer(dir, mockLogError));
+    });
+
+    afterAll(async () => {
+        await new Promise((resolve) => server.close(resolve));
+    });
+
+    it("serves a file with its content type", async () => {
+        const response = await fetch(`${baseUrl}/sub/icon.png`);
+
+        expect(response.status).toStrictEqual(200);
+        expect(response.headers.get("content-type")).toStrictEqual("image/png");
+        expect(response.headers.get("content-encoding")).toBeNull();
+    });
+
+    it("serves index.html for the root, never cached", async () => {
+        const response = await fetch(`${baseUrl}/`);
+
+        expect(response.status).toStrictEqual(200);
+        expect(response.headers.get("content-type")).toStrictEqual("text/html; charset=utf-8");
+        expect(response.headers.get("cache-control")).toStrictEqual("no-store");
+        await expect(response.text()).resolves.toStrictEqual(INDEX_HTML);
+    });
+
+    it("serves the precompressed brotli variant", async () => {
+        const response = await fetch(`${baseUrl}/app.js`, {headers: {"Accept-Encoding": "br"}});
+
+        expect(response.status).toStrictEqual(200);
+        expect(response.headers.get("content-encoding")).toStrictEqual("br");
+        expect(response.headers.get("content-type")).toStrictEqual("text/javascript; charset=utf-8");
+        expect(response.headers.get("vary")).toStrictEqual("Accept-Encoding");
+        // decoded by fetch, so the served bytes must be the brotli variant of the original file
+        await expect(response.text()).resolves.toStrictEqual(APP_JS);
+    });
+
+    it("serves the precompressed gzip variant", async () => {
+        const response = await fetch(`${baseUrl}/app.js`, {headers: {"Accept-Encoding": "gzip"}});
+
+        expect(response.status).toStrictEqual(200);
+        expect(response.headers.get("content-encoding")).toStrictEqual("gzip");
+        await expect(response.text()).resolves.toStrictEqual(APP_JS);
+    });
+
+    it("serves the identity file when no encoding is accepted", async () => {
+        const response = await fetch(`${baseUrl}/app.js`, {headers: {"Accept-Encoding": "identity"}});
+
+        expect(response.status).toStrictEqual(200);
+        expect(response.headers.get("content-encoding")).toBeNull();
+        expect(response.headers.get("content-length")).toStrictEqual(String(Buffer.byteLength(APP_JS)));
+        await expect(response.text()).resolves.toStrictEqual(APP_JS);
+    });
+
+    it("revalidates with an etag", async () => {
+        const response = await fetch(`${baseUrl}/app.js`, {headers: {"Accept-Encoding": "identity"}});
+        const etag = response.headers.get("etag");
+
+        expect(etag).toBeTruthy();
+
+        const revalidated = await fetch(`${baseUrl}/app.js`, {headers: {"Accept-Encoding": "identity", "If-None-Match": etag as string}});
+
+        expect(revalidated.status).toStrictEqual(304);
+    });
+
+    it("returns 404 for an unknown file", async () => {
+        const response = await fetch(`${baseUrl}/nope.js`);
+
+        expect(response.status).toStrictEqual(404);
+        expect(response.headers.get("content-type")).toStrictEqual("text/html; charset=utf-8");
+        expect(response.headers.get("content-security-policy")).toStrictEqual("default-src 'none'");
+        expect(response.headers.get("x-content-type-options")).toStrictEqual("nosniff");
+        await expect(response.text()).resolves.toContain("Cannot GET /nope.js");
+    });
+
+    it("escapes the url in the 404 body", async () => {
+        const response = await fetch(`${baseUrl}/%3Cscript%3E`);
+
+        expect(response.status).toStrictEqual(404);
+        await expect(response.text()).resolves.not.toContain("<script>");
+    });
+
+    it("does not serve files outside of the served directory", async () => {
+        // `fetch` resolves `..` and `%2e%2e` segments away before they ever reach the server, so these have to go out raw
+        for (const target of ["/../secret.txt", "/sub/../../secret.txt", "/%2e%2e/secret.txt", "/..%2fsecret.txt"]) {
+            const response = await rawRequest(target);
+
+            expect(response).toContain("404 Not Found");
+            expect(response).not.toContain(SECRET);
+        }
+    });
+
+    it("reports a failure to serve with a 500", async () => {
+        const failing = createStaticFileServer(dir, mockLogError);
+        const failingServer = createServer((request, response) => {
+            const setHeader = response.setHeader.bind(response);
+
+            response.setHeader = (name: string, value: number | string | readonly string[]): never => {
+                if (name === "Content-Security-Policy") {
+                    throw new Error("socket gone");
+                }
+
+                setHeader(name, value);
+
+                return undefined as never;
+            };
+
+            failing(request, response);
+        });
+
+        await new Promise<void>((resolve) => failingServer.listen(0, "127.0.0.1", resolve));
+
+        const port = (failingServer.address() as AddressInfo).port;
+        const response = await fetch(`http://127.0.0.1:${port}/nope.js`);
+
+        expect(response.status).toStrictEqual(500);
+        expect(mockLogError).toHaveBeenCalledWith("Failed to serve '/nope.js': socket gone");
+
+        await new Promise((resolve) => failingServer.close(resolve));
+    });
+});