From eeba005c0816d2d4e63a696bb7f6cb8d913cc128 Mon Sep 17 00:00:00 2001 From: alexander-akait Date: Wed, 3 Sep 2025 19:08:52 +0300 Subject: [PATCH 1/5] fix: do not call the next middleware when request is finished or errored --- src/index.js | 4 - src/middleware.js | 82 +++++++------- src/utils/compatibleAPI.js | 16 --- test/middleware.test.js | 188 +++++++++++++++++++++++++++++++-- types/utils/compatibleAPI.d.ts | 15 --- 5 files changed, 226 insertions(+), 79 deletions(-) diff --git a/src/index.js b/src/index.js index f654bc5e9..dc6a9ade0 100644 --- a/src/index.js +++ b/src/index.js @@ -424,8 +424,6 @@ function koaWrapper(compiler, options) { ctx.status = statusCode; }; - res.getReadyReadableStreamState = () => "open"; - try { await new Promise( /** @@ -575,8 +573,6 @@ function honoWrapper(compiler, options) { // Do nothing, because we set it before }; - res.getReadyReadableStreamState = () => "readable"; - res.getHeadersSent = () => context.env.outgoing.headersSent; let body; diff --git a/src/middleware.js b/src/middleware.js index 824a21b79..500c0dbe2 100644 --- a/src/middleware.js +++ b/src/middleware.js @@ -9,7 +9,6 @@ const { finish, getHeadersSent, getOutgoing, - getReadyReadableStreamState, getRequestHeader, getRequestMethod, getRequestURL, @@ -135,16 +134,13 @@ const MAX_MAX_AGE = 31536000000; */ function wrapper(context) { return async function middleware(req, res, next) { - const acceptedMethods = context.options.methods || ["GET", "HEAD"]; - - initState(res); - /** + * @param {NodeJS.ErrnoException=} err an error * @returns {Promise} */ - async function goNext() { + async function goNext(err) { if (!context.options.serverSideRender) { - return next(); + return next(err); } return new Promise((resolve) => { @@ -152,13 +148,19 @@ function wrapper(context) { context, () => { setState(res, "webpack", { devMiddleware: context }); - resolve(next()); + resolve(next(err)); }, req, ); }); } + const acceptedMethods = context.options.methods || ["GET", "HEAD"]; + // TODO do we need an option here? + const forwardError = false; + + initState(res); + const method = getRequestMethod(req); if (method && !acceptedMethods.includes(method)) { @@ -167,11 +169,21 @@ function wrapper(context) { } /** + * @param {NodeJS.ErrnoException} err ann error * @param {number} status status * @param {Partial>=} options options - * @returns {void} + * @returns {Promise} */ - function sendError(status, options) { + async function sendError(err, status, options) { + if (forwardError) { + const error = + /** @type {Error & { statusCode: number }} */ + (new Error(err.message)); + error.statusCode = status; + + await goNext(error); + } + const escapeHtml = require("./utils/escapeHtml"); const content = statuses[status] || String(status); @@ -230,19 +242,19 @@ function wrapper(context) { /** * @param {NodeJS.ErrnoException} error error - * @returns {void} + * @returns {Promise} */ - function errorHandler(error) { + async function errorHandler(error) { switch (error.code) { case "ENAMETOOLONG": case "ENOENT": case "ENOTDIR": - sendError(404, { + await sendError(error, 404, { modifyResponseData: context.options.modifyResponseData, }); break; default: - sendError(500, { + await sendError(error, 500, { modifyResponseData: context.options.modifyResponseData, }); break; @@ -496,10 +508,15 @@ function wrapper(context) { context.logger.error(`Malicious path "${filename}".`); } - sendError(extra.errorCode, { - modifyResponseData: context.options.modifyResponseData, - }); - await goNext(); + await sendError( + extra.errorCode === 400 + ? new Error("Bad Request") + : new Error("Forbidden"), + extra.errorCode, + { + modifyResponseData: context.options.modifyResponseData, + }, + ); return; } @@ -649,8 +666,7 @@ function wrapper(context) { value = result.bufferOrStream; ({ bufferOrStream, byteLength } = result); } catch (error) { - errorHandler(/** @type {NodeJS.ErrnoException} */ (error)); - await goNext(); + await errorHandler(/** @type {NodeJS.ErrnoException} */ (error)); return; } } @@ -691,10 +707,9 @@ function wrapper(context) { // Conditional GET support if (isConditionalGET()) { if (isPreconditionFailure()) { - sendError(412, { + await sendError(new Error("Precondition Failed"), 412, { modifyResponseData: context.options.modifyResponseData, }); - await goNext(); return; } @@ -744,13 +759,12 @@ function wrapper(context) { getValueContentRangeHeader("bytes", size), ); - sendError(416, { + await sendError(new Error("Range Not Satisfiable"), 416, { headers: { "Content-Range": getResponseHeader(res, "Content-Range"), }, modifyResponseData: context.options.modifyResponseData, }); - await goNext(); return; } else if (parsedRanges === -2) { context.logger.error( @@ -793,8 +807,7 @@ function wrapper(context) { end, )); } catch (error) { - errorHandler(/** @type {NodeJS.ErrnoException} */ (error)); - await goNext(); + await errorHandler(/** @type {NodeJS.ErrnoException} */ (error)); return; } } @@ -823,7 +836,6 @@ function wrapper(context) { } finish(res); - await goNext(); return; } @@ -838,7 +850,6 @@ function wrapper(context) { if (!isPipeSupports) { send(res, /** @type {Buffer} */ (bufferOrStream)); - await goNext(); return; } @@ -852,16 +863,11 @@ function wrapper(context) { // Error handling /** @type {import("fs").ReadStream} */ - (bufferOrStream) - .on("error", (error) => { - // clean up stream early - cleanup(); - errorHandler(error); - goNext(); - }) - .on(getReadyReadableStreamState(res), () => { - goNext(); - }); + (bufferOrStream).on("error", (error) => { + // clean up stream early + cleanup(); + errorHandler(error); + }); pipe(res, /** @type {ReadStream} */ (bufferOrStream)); diff --git a/src/utils/compatibleAPI.js b/src/utils/compatibleAPI.js index a28f3a0ad..4cd1da15c 100644 --- a/src/utils/compatibleAPI.js +++ b/src/utils/compatibleAPI.js @@ -24,7 +24,6 @@ * @property {((data: any) => void)=} stream stream * @property {(() => any)=} getOutgoing get outgoing * @property {((name: string, value: any) => void)=} setState set state - * @property {(() => "ready" | "open" | "readable")=} getReadyReadableStreamState get ready readable streamState */ /** @@ -301,26 +300,11 @@ function setState(res, name, value) { (res.locals)[name] = value; } -/** - * @template {ServerResponse & ExpectedServerResponse} Response - * @param {Response} res res - * @returns {"ready" | "open" | "readable"} state - */ -function getReadyReadableStreamState(res) { - // Pseudo API and Express API and Koa API - if (typeof res.getReadyReadableStreamState === "function") { - return res.getReadyReadableStreamState(); - } - - return "ready"; -} - module.exports = { createReadStreamOrReadFileSync, finish, getHeadersSent, getOutgoing, - getReadyReadableStreamState, getRequestHeader, getRequestMethod, getRequestURL, diff --git a/test/middleware.test.js b/test/middleware.test.js index 7a2c3815d..6211b3d9a 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -3325,12 +3325,7 @@ describe.each([ }); middlewares.push(middleware.koaWrapper(compiler, {})); } else if (name === "hono") { - middlewares.unshift(async (c, next) => { - await next(); - - return new Response("Hello Node.js!"); - }); - middlewares.push(middleware.honoWrapper(compiler, {})); + // Hono doesn't have this problem due design } else { middlewares.push({ route: "/", @@ -3379,6 +3374,187 @@ describe.each([ expect(response.text).toBeUndefined(); }); }); + + describe("should work and don't call the next middleware for finished or errored requests by default", () => { + let compiler; + + const outputPath = path.resolve( + __dirname, + "./outputs/basic-test-errors-headers-sent", + ); + + let nextWasCalled = false; + + beforeAll(async () => { + compiler = getCompiler({ + ...webpackConfig, + output: { + filename: "bundle.js", + path: outputPath, + }, + }); + + [server, req, instance] = await frameworkFactory( + name, + framework, + compiler, + { + etag: "weak", + }, + { + setupMiddlewares: (middlewares) => { + if (name === "hapi") { + // There's no such thing as "the next route handler" in hapi. One request is matched to one or no route handlers. + } else if (name === "koa") { + middlewares.push(middleware.koaWrapper(compiler, {})); + middlewares.push(async () => { + nextWasCalled = true; + }); + } else if (name === "hono") { + // Hono doesn't have this problem due design + } else { + middlewares.push(middleware(compiler, {})); + middlewares.push(() => { + nextWasCalled = true; + }); + } + + return middlewares; + }, + }, + ); + + instance.context.outputFileSystem.mkdirSync(outputPath, { + recursive: true, + }); + instance.context.outputFileSystem.writeFileSync( + path.resolve(outputPath, "index.html"), + "HTML", + ); + instance.context.outputFileSystem.writeFileSync( + path.resolve(outputPath, "image.svg"), + "svg image", + ); + + const originalMethod = + instance.context.outputFileSystem.createReadStream; + + instance.context.outputFileSystem.createReadStream = + function createReadStream(...args) { + if (args[0].endsWith("image.svg")) { + const brokenStream = new this.ReadStream(...args); + + brokenStream._read = function _read() { + const error = new Error("test"); + error.code = "ENAMETOOLONG"; + this.emit("error", error); + this.end(); + this.destroy(); + }; + + return brokenStream; + } + + return originalMethod(...args); + }; + }); + + afterAll(async () => { + await close(server, instance); + }); + + it("should work with piping stream", async () => { + const response1 = await req.get("/"); + + expect(response1.statusCode).toBe(200); + expect(nextWasCalled).toBe(false); + }); + + it("should not allow to get files above root", async () => { + const response = await req.get("/public/..%2f../middleware.test.js"); + + expect(response.statusCode).toBe(403); + expect(response.headers["content-type"]).toBe( + "text/html; charset=utf-8", + ); + expect(response.text).toBe(` + + + +Error + + +
Forbidden
+ +`); + expect(nextWasCalled).toBe(false); + }); + + it('should return the "412" code for the "GET" request to the bundle file with etag and wrong "if-match" header', async () => { + const response1 = await req.get("/"); + + expect(response1.statusCode).toBe(200); + expect(response1.headers.etag).toBeDefined(); + expect(response1.headers.etag.startsWith("W/")).toBe(true); + + const response2 = await req.get("/").set("if-match", "test"); + + expect(response2.statusCode).toBe(412); + expect(nextWasCalled).toBe(false); + }); + + it('should return the "416" code for the "GET" request with the invalid range header', async () => { + const response = await req.get("/").set("Range", "bytes=9999999-"); + + expect(response.statusCode).toBe(416); + expect(response.headers["content-type"]).toBe( + "text/html; charset=utf-8", + ); + expect(response.text).toBe( + ` + + + +Error + + +
Range Not Satisfiable
+ +`, + ); + expect(nextWasCalled).toBe(false); + }); + + it('should return the "404" code for the "GET" request to the "image.svg" file when it throws a reading error', async () => { + const response = await req.get("/image.svg"); + + expect(response.statusCode).toBe(404); + expect(response.headers["content-type"]).toBe( + "text/html; charset=utf-8", + ); + expect(response.text).toEqual( + "\n" + + '\n' + + "\n" + + '\n' + + "Error\n" + + "\n" + + "\n" + + "
Not Found
\n" + + "\n" + + "", + ); + expect(nextWasCalled).toBe(false); + }); + + it.only('should return the "200" code for the "HEAD" request to the bundle file', async () => { + const response = await req.head("/"); + + expect(response.statusCode).toBe(200); + expect(response.text).toBeUndefined(); + expect(nextWasCalled).toBe(false); + }); + }); }); describe("mimeTypes option", () => { diff --git a/types/utils/compatibleAPI.d.ts b/types/utils/compatibleAPI.d.ts index 37aa8f073..87b2905bd 100644 --- a/types/utils/compatibleAPI.d.ts +++ b/types/utils/compatibleAPI.d.ts @@ -69,12 +69,6 @@ export type ExpectedServerResponse = { * set state */ setState?: ((name: string, value: any) => void) | undefined; - /** - * get ready readable streamState - */ - getReadyReadableStreamState?: - | (() => "ready" | "open" | "readable") - | undefined; }; /** * @param {string} filename filename @@ -116,14 +110,6 @@ export function getHeadersSent< export function getOutgoing< Response extends ServerResponse & ExpectedServerResponse, >(res: Response): Response; -/** - * @template {ServerResponse & ExpectedServerResponse} Response - * @param {Response} res res - * @returns {"ready" | "open" | "readable"} state - */ -export function getReadyReadableStreamState< - Response extends ServerResponse & ExpectedServerResponse, ->(res: Response): "ready" | "open" | "readable"; /** @typedef {import("../index.js").IncomingMessage} IncomingMessage */ /** @typedef {import("../index.js").ServerResponse} ServerResponse */ /** @typedef {import("../index").OutputFileSystem} OutputFileSystem */ @@ -147,7 +133,6 @@ export function getReadyReadableStreamState< * @property {((data: any) => void)=} stream stream * @property {(() => any)=} getOutgoing get outgoing * @property {((name: string, value: any) => void)=} setState set state - * @property {(() => "ready" | "open" | "readable")=} getReadyReadableStreamState get ready readable streamState */ /** * @template {IncomingMessage & ExpectedIncomingMessage} Request From d99e394e238fd734ff64619c2c469c2db0b4abac Mon Sep 17 00:00:00 2001 From: alexander-akait Date: Thu, 4 Sep 2025 14:29:28 +0300 Subject: [PATCH 2/5] test: fix --- test/middleware.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/middleware.test.js b/test/middleware.test.js index 6211b3d9a..101bdbbbd 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -3547,7 +3547,7 @@ describe.each([ expect(nextWasCalled).toBe(false); }); - it.only('should return the "200" code for the "HEAD" request to the bundle file', async () => { + it('should return the "200" code for the "HEAD" request to the bundle file', async () => { const response = await req.head("/"); expect(response.statusCode).toBe(200); From 92cdc6d60210d3e6b5dc547e34abb49d0177b0b6 Mon Sep 17 00:00:00 2001 From: alexander-akait Date: Thu, 4 Sep 2025 15:03:38 +0300 Subject: [PATCH 3/5] refactor: code --- src/index.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/index.js b/src/index.js index dc6a9ade0..f8967d56b 100644 --- a/src/index.js +++ b/src/index.js @@ -576,6 +576,7 @@ function honoWrapper(compiler, options) { res.getHeadersSent = () => context.env.outgoing.headersSent; let body; + let isFinished = false; try { await new Promise( @@ -589,7 +590,9 @@ function honoWrapper(compiler, options) { */ res.stream = (stream) => { body = stream; - // responseHandler(stream); + + isFinished = true; + resolve(); }; /** @@ -600,9 +603,10 @@ function honoWrapper(compiler, options) { context.res.headers.delete("Content-Length"); body = data; - }; - let isFinished = false; + isFinished = true; + resolve(); + }; /** * @param {(string | Buffer)=} data data @@ -616,8 +620,8 @@ function honoWrapper(compiler, options) { } body = isDataExist ? data : null; - isFinished = true; + isFinished = true; resolve(); }; From 1a8f2a5ad03a1bc1ba9c69e6b32dfa519207fbfa Mon Sep 17 00:00:00 2001 From: alexander-akait Date: Thu, 4 Sep 2025 16:31:39 +0300 Subject: [PATCH 4/5] refactor: code --- src/index.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index f8967d56b..6d7cefdd7 100644 --- a/src/index.js +++ b/src/index.js @@ -424,6 +424,8 @@ function koaWrapper(compiler, options) { ctx.status = statusCode; }; + let isFinished = false; + try { await new Promise( /** @@ -436,12 +438,18 @@ function koaWrapper(compiler, options) { */ res.stream = (stream) => { ctx.body = stream; + + isFinished = true; + resolve(); }; /** * @param {string | Buffer} data data */ res.send = (data) => { ctx.body = data; + + isFinished = true; + resolve(); }; /** @@ -450,6 +458,9 @@ function koaWrapper(compiler, options) { res.finish = (data) => { ctx.status = status; res.end(data); + + isFinished = true; + resolve(); }; devMiddleware(req, res, (err) => { @@ -458,7 +469,9 @@ function koaWrapper(compiler, options) { return; } - resolve(); + if (!isFinished) { + resolve(); + } }); }, ); From 8c9d6909a88aa883c3682b750c001b632bd92a29 Mon Sep 17 00:00:00 2001 From: alexander-akait Date: Thu, 4 Sep 2025 16:40:48 +0300 Subject: [PATCH 5/5] refactor: code --- src/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 6d7cefdd7..7cbdeddee 100644 --- a/src/index.js +++ b/src/index.js @@ -425,6 +425,7 @@ function koaWrapper(compiler, options) { }; let isFinished = false; + let needNext = false; try { await new Promise( @@ -469,6 +470,8 @@ function koaWrapper(compiler, options) { return; } + needNext = true; + if (!isFinished) { resolve(); } @@ -486,7 +489,9 @@ function koaWrapper(compiler, options) { }; } - await next(); + if (needNext) { + await next(); + } } webpackDevMiddleware.devMiddleware = devMiddleware;