From 60759ea0530bdcab712a9dc7cbdc80cf3592c27a Mon Sep 17 00:00:00 2001 From: "alexander.akait" Date: Tue, 19 Mar 2024 15:28:16 +0300 Subject: [PATCH 1/5] fix: cleaup stream and handle errors --- .cspell.json | 3 +- package-lock.json | 15 +++- package.json | 2 + src/utils/compatibleAPI.js | 141 ++++++++++++++++++++++++++++++++++++- src/utils/escapeHtml.js | 58 +++++++++++++++ test/middleware.test.js | 4 +- 6 files changed, 215 insertions(+), 8 deletions(-) create mode 100644 src/utils/escapeHtml.js diff --git a/.cspell.json b/.cspell.json index c1064bc80..490b85431 100644 --- a/.cspell.json +++ b/.cspell.json @@ -17,7 +17,8 @@ "myhtml", "configurated", "mycustom", - "commitlint" + "commitlint", + "nosniff" ], "ignorePaths": [ "CHANGELOG.md", diff --git a/package-lock.json b/package-lock.json index 83fc67549..87b0ccffd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,6 +12,7 @@ "colorette": "^2.0.10", "memfs": "^4.6.0", "mime-types": "^2.1.31", + "on-finished": "^2.4.1", "range-parser": "^1.2.1", "schema-utils": "^4.0.0" }, @@ -25,6 +26,7 @@ "@types/express": "^4.17.13", "@types/mime-types": "^2.1.1", "@types/node": "^20.11.16", + "@types/on-finished": "^2.3.4", "@webpack-contrib/eslint-config-webpack": "^3.0.0", "babel-jest": "^29.3.1", "chokidar": "^3.5.1", @@ -4259,6 +4261,15 @@ "integrity": "sha512-37i+OaWTh9qeK4LSHPsyRC7NahnGotNuZvjLSgcPzblpHB3rrCJxAOgI5gCdKm7coonsaX1Of0ILiTcnZjbfxA==", "dev": true }, + "node_modules/@types/on-finished": { + "version": "2.3.4", + "resolved": "https://registry.npmjs.org/@types/on-finished/-/on-finished-2.3.4.tgz", + "integrity": "sha512-Ld4UQD3udYcKPaAWlI1EYXKhefkZcTlpqOLkQRmN3u5Ml/tUypMivUHbNH8LweP4H4FlhGGO+uBjJI1Y1dkE1g==", + "dev": true, + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/qs": { "version": "6.9.12", "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.9.12.tgz", @@ -8205,8 +8216,7 @@ "node_modules/ee-first": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/ee-first/-/ee-first-1.1.1.tgz", - "integrity": "sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow==", - "dev": true + "integrity": "sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow==" }, "node_modules/electron-to-chromium": { "version": "1.4.695", @@ -13847,7 +13857,6 @@ "version": "2.4.1", "resolved": "https://registry.npmjs.org/on-finished/-/on-finished-2.4.1.tgz", "integrity": "sha512-oVlzkg3ENAhCk2zdv7IJwd/QUD4z2RxRwpkcGY8psCVcCYZNq4wYnVWALHM+brtuJjePWiYF/ClmuDr8Ch5+kg==", - "dev": true, "dependencies": { "ee-first": "1.1.1" }, diff --git a/package.json b/package.json index 5b8aed90b..333cf31b6 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "colorette": "^2.0.10", "memfs": "^4.6.0", "mime-types": "^2.1.31", + "on-finished": "^2.4.1", "range-parser": "^1.2.1", "schema-utils": "^4.0.0" }, @@ -69,6 +70,7 @@ "@types/express": "^4.17.13", "@types/mime-types": "^2.1.1", "@types/node": "^20.11.16", + "@types/on-finished": "^2.3.4", "@webpack-contrib/eslint-config-webpack": "^3.0.0", "babel-jest": "^29.3.1", "chokidar": "^3.5.1", diff --git a/src/utils/compatibleAPI.js b/src/utils/compatibleAPI.js index bd094daa3..db526129c 100644 --- a/src/utils/compatibleAPI.js +++ b/src/utils/compatibleAPI.js @@ -1,3 +1,8 @@ + +const onFinishedStream = require("on-finished"); + +const escapeHtml = require("./escapeHtml"); + /** @typedef {import("../index.js").IncomingMessage} IncomingMessage */ /** @typedef {import("../index.js").ServerResponse} ServerResponse */ @@ -88,6 +93,33 @@ function setHeaderForResponse(res, name, value) { res.setHeader(name, value); } +/** + * @template {ServerResponse} Response + * @param {Response} res + * @param {Record} headers + */ +function setHeadersForResponse(res, headers) { + const keys = Object.keys(headers); + + for (let i = 0; i < keys.length; i++) { + const key = keys[i]; + + setHeaderForResponse(res, key, headers[key]); + } +} + +/** + * @template {ServerResponse} Response + * @param {Response} res + */ +function clearHeadersForResponse(res) { + const headers = getHeaderNames(res); + + for (let i = 0; i < headers.length; i++) { + res.removeHeader(headers[i]); + } +} + /** * @template {ServerResponse} Response * @param {Response} res @@ -108,6 +140,83 @@ function setStatusCode(res, code) { res.statusCode = code; } +/** + * @param {import("fs").ReadStream} stream stream + * @param {boolean} suppress do need suppress? + * @returns {void} + */ +function destroyStream(stream, suppress) { + if (typeof stream.destroy === "function") { + stream.destroy(); + } + + if (typeof stream.close === "function") { + // Node.js core bug workaround + stream.on( + "open", + /** + * @this {import("fs").ReadStream} + */ + function onOpenClose() { + // @ts-ignore + if (typeof this.fd === "number") { + // actually close down the fd + this.close(); + } + }, + ); + } + + if (typeof stream.addListener === "function" && suppress) { + stream.removeAllListeners("error"); + stream.addListener("error", () => {}); + } +} + +/** @type {Record} */ +const statuses = { + 404: "Not Found", + 500: "Internal Server Error", +}; + +/** + * @template {ServerResponse} Response + * @param {Response} res response + * @param {number} status status + * @param {Error & { headers?: Record}} err error + * @returns {void} + */ +function sendError(res, status, err) { + const msg = statuses[status] || String(status); + const doc = ` + + + +Error + + +
${escapeHtml(msg)}
+ +`; + + // Clear existing headers + clearHeadersForResponse(res); + + // Add error headers + if (err && err.headers) { + setHeadersForResponse(res, err.headers); + } + + // Send basic response + setStatusCode(res, status); + setHeaderForResponse(res, "Content-Type", "text/html; charset=UTF-8"); + setHeaderForResponse(res, "Content-Length", Buffer.byteLength(doc)); + setHeaderForResponse(res, "Content-Security-Policy", "default-src 'none'"); + setHeaderForResponse(res, "X-Content-Type-Options", "nosniff"); + + res.end(doc); +} + /** * @template {IncomingMessage} Request * @template {ServerResponse} Response @@ -125,13 +234,42 @@ function send(req, res, bufferOtStream, byteLength) { if (req.method === "HEAD") { res.end(); - return; } /** @type {import("fs").ReadStream} */ (bufferOtStream).pipe(res); + // Cleanup + const cleanup = () => { + destroyStream( + /** @type {import("fs").ReadStream} */ (bufferOtStream), + true, + ); + }; + + // Response finished, cleanup + onFinishedStream(res, cleanup); + + // error handling + /** @type {import("fs").ReadStream} */ + (bufferOtStream).on("error", (error) => { + // clean up stream early + cleanup(); + + // Handle Error + switch (/** @type {NodeJS.ErrnoException} */ (error).code) { + case "ENAMETOOLONG": + case "ENOENT": + case "ENOTDIR": + sendError(res, 404, error); + break; + default: + sendError(res, 500, error); + break; + } + }); + return; } @@ -141,7 +279,6 @@ function send(req, res, bufferOtStream, byteLength) { ) { /** @type {Response & ExpectedResponse} */ (res).send(bufferOtStream); - return; } diff --git a/src/utils/escapeHtml.js b/src/utils/escapeHtml.js new file mode 100644 index 000000000..4a131ef30 --- /dev/null +++ b/src/utils/escapeHtml.js @@ -0,0 +1,58 @@ +const matchHtmlRegExp = /["'&<>]/; + +/** + * @param {string} string raw HTML + * @returns {string} escaped HTML + */ +function escapeHtml(string) { + const str = `${string}`; + const match = matchHtmlRegExp.exec(str); + + if (!match) { + return str; + } + + let escape; + let html = ""; + let index = 0; + let lastIndex = 0; + + for ({ index } = match; index < str.length; index++) { + switch (str.charCodeAt(index)) { + // " + case 34: + escape = """; + break; + // & + case 38: + escape = "&"; + break; + // ' + case 39: + escape = "'"; + break; + // < + case 60: + escape = "<"; + break; + // > + case 62: + escape = ">"; + break; + default: + // eslint-disable-next-line no-continue + continue; + } + + if (lastIndex !== index) { + html += str.substring(lastIndex, index); + } + + lastIndex = index + 1; + html += escape; + } + + return lastIndex !== index ? html + str.substring(lastIndex, index) : html; +} + +module.exports = escapeHtml; diff --git a/test/middleware.test.js b/test/middleware.test.js index 160114aac..95a492c9f 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -19,7 +19,7 @@ import webpackQueryStringConfig from "./fixtures/webpack.querystring.config"; import webpackClientServerConfig from "./fixtures/webpack.client.server.config"; // Suppress unnecessary stats output -global.console.log = jest.fn(); +// global.console.log = jest.fn(); describe.each([ ["express", express], @@ -62,7 +62,7 @@ describe.each([ } describe("basic", () => { - describe("should work", () => { + describe.only("should work", () => { let compiler; let codeContent; let codeLength; From 12c29ffdbedad297930542459142573d5f2e30ba Mon Sep 17 00:00:00 2001 From: "alexander.akait" Date: Tue, 19 Mar 2024 15:34:49 +0300 Subject: [PATCH 2/5] test: more --- src/utils/compatibleAPI.js | 1 - test/middleware.test.js | 2 +- test/utils/escapeHtml.test.js | 11 +++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 test/utils/escapeHtml.test.js diff --git a/src/utils/compatibleAPI.js b/src/utils/compatibleAPI.js index db526129c..9aaaebe1f 100644 --- a/src/utils/compatibleAPI.js +++ b/src/utils/compatibleAPI.js @@ -1,4 +1,3 @@ - const onFinishedStream = require("on-finished"); const escapeHtml = require("./escapeHtml"); diff --git a/test/middleware.test.js b/test/middleware.test.js index 95a492c9f..c63ebf8a9 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -19,7 +19,7 @@ import webpackQueryStringConfig from "./fixtures/webpack.querystring.config"; import webpackClientServerConfig from "./fixtures/webpack.client.server.config"; // Suppress unnecessary stats output -// global.console.log = jest.fn(); +global.console.log = jest.fn(); describe.each([ ["express", express], diff --git a/test/utils/escapeHtml.test.js b/test/utils/escapeHtml.test.js new file mode 100644 index 000000000..f89d5799d --- /dev/null +++ b/test/utils/escapeHtml.test.js @@ -0,0 +1,11 @@ +import escapeHtml from "../../src/utils/escapeHtml"; + +describe("escapeHtml", () => { + it("should work", () => { + expect(escapeHtml("")).toBe(""); + expect(escapeHtml("test")).toBe("test"); + expect(escapeHtml("\"&'<>test")).toBe(""&'<>test"); + expect(escapeHtml("\"&'test<>")).toBe(""&'test<>"); + expect(escapeHtml("test\"&'<>")).toBe("test"&'<>"); + }); +}); From 8b12b869cc81dba480d519d64ee9593febae6424 Mon Sep 17 00:00:00 2001 From: "alexander.akait" Date: Tue, 19 Mar 2024 15:36:54 +0300 Subject: [PATCH 3/5] fix: types --- types/utils/escapeHtml.d.ts | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 types/utils/escapeHtml.d.ts diff --git a/types/utils/escapeHtml.d.ts b/types/utils/escapeHtml.d.ts new file mode 100644 index 000000000..f7982bda1 --- /dev/null +++ b/types/utils/escapeHtml.d.ts @@ -0,0 +1,6 @@ +export = escapeHtml; +/** + * @param {string} string raw HTML + * @returns {string} escaped HTML + */ +declare function escapeHtml(string: string): string; From 4a3f11c488c9aea48ae3877151fe8686122ec091 Mon Sep 17 00:00:00 2001 From: "alexander.akait" Date: Tue, 19 Mar 2024 16:33:51 +0300 Subject: [PATCH 4/5] test: more --- test/middleware.test.js | 196 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 195 insertions(+), 1 deletion(-) diff --git a/test/middleware.test.js b/test/middleware.test.js index c63ebf8a9..97adf5f24 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -62,7 +62,7 @@ describe.each([ } describe("basic", () => { - describe.only("should work", () => { + describe("should work", () => { let compiler; let codeContent; let codeLength; @@ -1959,6 +1959,200 @@ describe.each([ expect(response.statusCode).toEqual(200); }); }); + + describe("should handle custom fs errors and response 500 code", () => { + let compiler; + let codeContent; + let codeLength; + + const outputPath = path.resolve( + __dirname, + "./outputs/basic-test-errors", + ); + + beforeAll((done) => { + compiler = getCompiler({ + ...webpackConfig, + output: { + filename: "bundle.js", + path: outputPath, + }, + }); + + instance = middleware(compiler); + + app = framework(); + app.use(instance); + + listen = listenShorthand(() => { + compiler.hooks.afterCompile.tap("wdm-test", (params) => { + codeContent = params.assets["bundle.js"].source(); + codeLength = Buffer.byteLength(codeContent); + + done(); + }); + }); + + instance.context.outputFileSystem.mkdirSync(outputPath, { + recursive: true, + }); + instance.context.outputFileSystem.writeFileSync( + path.resolve(outputPath, "image.svg"), + "svg image", + ); + + instance.context.outputFileSystem.createReadStream = + function createReadStream(...args) { + const brokenStream = new this.ReadStream(...args); + + // eslint-disable-next-line no-underscore-dangle + brokenStream._read = function _read() { + this.emit("error", new Error("test")); + this.end(); + this.destroy(); + }; + + return brokenStream; + }; + + req = request(app); + }); + + afterAll(close); + + it('should return the "200" code for the "GET" request to the bundle file', async () => { + const response = await req.get("/bundle.js"); + + expect(response.statusCode).toEqual(200); + expect(response.headers["content-length"]).toEqual( + String(codeLength), + ); + expect(response.headers["content-type"]).toEqual( + "application/javascript; charset=utf-8", + ); + expect(response.text).toEqual(codeContent); + }); + + it('should return the "500" code for the "GET" request to the "image.svg" file', async () => { + const response = await req.get("/image.svg").set("Range", "bytes=0-"); + + expect(response.statusCode).toEqual(500); + expect(response.headers["content-type"]).toEqual( + "text/html; charset=UTF-8", + ); + expect(response.text).toEqual( + "\n" + + '\n' + + "\n" + + '\n' + + "Error\n" + + "\n" + + "\n" + + "
Internal Server Error
\n" + + "\n" + + "", + ); + }); + }); + + describe("should handle known fs errors and response 404 code", () => { + let compiler; + let codeContent; + let codeLength; + + const outputPath = path.resolve( + __dirname, + "./outputs/basic-test-errors", + ); + + beforeAll((done) => { + compiler = getCompiler({ + ...webpackConfig, + output: { + filename: "bundle.js", + path: outputPath, + }, + }); + + instance = middleware(compiler); + + app = framework(); + app.use(instance); + + listen = listenShorthand(() => { + compiler.hooks.afterCompile.tap("wdm-test", (params) => { + codeContent = params.assets["bundle.js"].source(); + codeLength = Buffer.byteLength(codeContent); + + done(); + }); + }); + + instance.context.outputFileSystem.mkdirSync(outputPath, { + recursive: true, + }); + instance.context.outputFileSystem.writeFileSync( + path.resolve(outputPath, "image.svg"), + "svg image", + ); + + instance.context.outputFileSystem.createReadStream = + function createReadStream(...args) { + const brokenStream = new this.ReadStream(...args); + + // eslint-disable-next-line no-underscore-dangle + brokenStream._read = function _read() { + const error = new Error("test"); + + error.code = "ENAMETOOLONG"; + + this.emit("error", error); + this.end(); + this.destroy(); + }; + + return brokenStream; + }; + + req = request(app); + }); + + afterAll(close); + + it('should return the "200" code for the "GET" request to the bundle file', async () => { + const response = await req.get("/bundle.js"); + + expect(response.statusCode).toEqual(200); + expect(response.headers["content-length"]).toEqual( + String(codeLength), + ); + expect(response.headers["content-type"]).toEqual( + "application/javascript; charset=utf-8", + ); + expect(response.text).toEqual(codeContent); + }); + + it('should return the "404" code for the "GET" request to the "image.svg" file', async () => { + const response = await req.get("/image.svg").set("Range", "bytes=0-"); + + expect(response.statusCode).toEqual(404); + expect(response.headers["content-type"]).toEqual( + "text/html; charset=UTF-8", + ); + expect(response.text).toEqual( + "\n" + + '\n' + + "\n" + + '\n' + + "Error\n" + + "\n" + + "\n" + + "
Not Found
\n" + + "\n" + + "", + ); + }); + }); }); describe("mimeTypes option", () => { From 5fa75bcc3dfd5608c37d66ae7dd770b01f37b879 Mon Sep 17 00:00:00 2001 From: "alexander.akait" Date: Tue, 19 Mar 2024 16:39:15 +0300 Subject: [PATCH 5/5] refactor: avoid unnecessary API --- src/utils/compatibleAPI.js | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/src/utils/compatibleAPI.js b/src/utils/compatibleAPI.js index 9aaaebe1f..83301478f 100644 --- a/src/utils/compatibleAPI.js +++ b/src/utils/compatibleAPI.js @@ -92,21 +92,6 @@ function setHeaderForResponse(res, name, value) { res.setHeader(name, value); } -/** - * @template {ServerResponse} Response - * @param {Response} res - * @param {Record} headers - */ -function setHeadersForResponse(res, headers) { - const keys = Object.keys(headers); - - for (let i = 0; i < keys.length; i++) { - const key = keys[i]; - - setHeaderForResponse(res, key, headers[key]); - } -} - /** * @template {ServerResponse} Response * @param {Response} res @@ -182,10 +167,9 @@ const statuses = { * @template {ServerResponse} Response * @param {Response} res response * @param {number} status status - * @param {Error & { headers?: Record}} err error * @returns {void} */ -function sendError(res, status, err) { +function sendError(res, status) { const msg = statuses[status] || String(status); const doc = ` @@ -200,12 +184,6 @@ function sendError(res, status, err) { // Clear existing headers clearHeadersForResponse(res); - - // Add error headers - if (err && err.headers) { - setHeadersForResponse(res, err.headers); - } - // Send basic response setStatusCode(res, status); setHeaderForResponse(res, "Content-Type", "text/html; charset=UTF-8"); @@ -261,10 +239,10 @@ function send(req, res, bufferOtStream, byteLength) { case "ENAMETOOLONG": case "ENOENT": case "ENOTDIR": - sendError(res, 404, error); + sendError(res, 404); break; default: - sendError(res, 500, error); + sendError(res, 500); break; } });