diff --git a/src/index.js b/src/index.js index f654bc5e9..7cbdeddee 100644 --- a/src/index.js +++ b/src/index.js @@ -424,7 +424,8 @@ function koaWrapper(compiler, options) { ctx.status = statusCode; }; - res.getReadyReadableStreamState = () => "open"; + let isFinished = false; + let needNext = false; try { await new Promise( @@ -438,12 +439,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(); }; /** @@ -452,6 +459,9 @@ function koaWrapper(compiler, options) { res.finish = (data) => { ctx.status = status; res.end(data); + + isFinished = true; + resolve(); }; devMiddleware(req, res, (err) => { @@ -460,7 +470,11 @@ function koaWrapper(compiler, options) { return; } - resolve(); + needNext = true; + + if (!isFinished) { + resolve(); + } }); }, ); @@ -475,7 +489,9 @@ function koaWrapper(compiler, options) { }; } - await next(); + if (needNext) { + await next(); + } } webpackDevMiddleware.devMiddleware = devMiddleware; @@ -575,11 +591,10 @@ function honoWrapper(compiler, options) { // Do nothing, because we set it before }; - res.getReadyReadableStreamState = () => "readable"; - res.getHeadersSent = () => context.env.outgoing.headersSent; let body; + let isFinished = false; try { await new Promise( @@ -593,7 +608,9 @@ function honoWrapper(compiler, options) { */ res.stream = (stream) => { body = stream; - // responseHandler(stream); + + isFinished = true; + resolve(); }; /** @@ -604,9 +621,10 @@ function honoWrapper(compiler, options) { context.res.headers.delete("Content-Length"); body = data; - }; - let isFinished = false; + isFinished = true; + resolve(); + }; /** * @param {(string | Buffer)=} data data @@ -620,8 +638,8 @@ function honoWrapper(compiler, options) { } body = isDataExist ? data : null; - isFinished = true; + isFinished = true; resolve(); }; 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..101bdbbbd 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('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