diff --git a/changelog.md b/changelog.md index 2515ca6..11b9f25 100644 --- a/changelog.md +++ b/changelog.md @@ -6,7 +6,6 @@ - Updated the [`busboy`](https://npm.im/busboy) dependency to v1, fixing [#311](https://github.com/jaydenseric/graphql-upload/issues/311). - This important update addresses the vulnerability [CVE-2022-24434](https://nvd.nist.gov/vuln/detail/CVE-2022-24434) ([GHSA-wm7h-9275-46v2](https://github.com/advisories/GHSA-wm7h-9275-46v2)). - - It now emits an error on a file upload stream if the request disconnects partway through, which is different to the error that used to come from [`graphql-upload`](https://npm.im/graphql-upload). - Some error messages have changed. - Temporarily until [mscdex/busboy#297](https://github.com/mscdex/busboy/issues/297) is fixed upstream, for the function `processRequest` and the middleware `graphqlUploadExpress` and `graphqlUploadKoa` the option `maxFileSize` is actually 1 byte less than the amount specified. @@ -14,6 +13,7 @@ - Updated the [`typescript`](https://npm.im/typescript) dev dependency. - In the function `processRequest` use the `on` method instead of `once` to listen for `error` events on the [`busboy`](https://npm.im/busboy) parser, as in edge cases the same parser could have multiple `error` events and all must be handled to prevent the Node.js process exiting with an error. +- Simplified error handling within the function `processRequest`. - Added a test for the function `processRequest` with a maliciously malformed multipart request. ## 14.0.0 diff --git a/processRequest.js b/processRequest.js index 2dfc71e..3d1b679 100644 --- a/processRequest.js +++ b/processRequest.js @@ -42,9 +42,6 @@ function processRequest( /** @type {Error} */ let exitError; - /** @type {import("stream").Readable} */ - let lastFileStream; - /** * @type {{ [key: string]: unknown } | Array< * { [key: string]: unknown } @@ -76,27 +73,20 @@ function processRequest( /** * Exits request processing with an error. Successive calls have no effect. * @param {Error} error Error instance. + * @param {boolean} [isParserError] Is the error from the parser. */ - const exit = (error) => { + function exit(error, isParserError = false) { if (exitError) return; exitError = error; - reject(exitError); - - parser.destroy(); - - if ( - lastFileStream && - !lastFileStream.readableEnded && - !lastFileStream.destroyed - ) - lastFileStream.destroy(exitError); - if (map) for (const upload of map.values()) if (!upload.file) upload.reject(exitError); + // If the error came from the parser, don’t cause it to be emitted again. + isParserError ? parser.destroy() : parser.destroy(exitError); + request.unpipe(parser); // With a sufficiently large request body, subsequent events in the same @@ -106,7 +96,9 @@ function processRequest( setImmediate(() => { request.resume(); }); - }; + + reject(exitError); + } parser.on("field", (fieldName, value, { valueTruncated }) => { if (valueTruncated) @@ -228,8 +220,6 @@ function processRequest( parser.on( "file", (fieldName, stream, { filename, encoding, mimeType: mimetype }) => { - lastFileStream = stream; - if (!map) { ignoreStream(stream); return exit( @@ -331,7 +321,9 @@ function processRequest( // could have multiple `error` events and all must be handled to prevent the // Node.js process exiting with an error. One edge case is if there is a // malformed part header as well as an unexpected end of the form. - parser.on("error", exit); + parser.on("error", (/** @type {Error} */ error) => { + exit(error, true); + }); response.once("close", () => { released = true; diff --git a/processRequest.test.mjs b/processRequest.test.mjs index 677fda7..5d5a854 100644 --- a/processRequest.test.mjs +++ b/processRequest.test.mjs @@ -789,8 +789,11 @@ export default (tests) => { stream.once("error", reject).once("end", resolve).resume(); }), { - name: "Error", - message: "Unexpected end of file", + name: "BadRequestError", + message: + "Request disconnected during file upload stream parsing.", + status: 499, + expose: true, } ); }; @@ -931,8 +934,11 @@ export default (tests) => { strictEqual(upload.mimetype, "text/plain"); strictEqual(upload.encoding, "7bit"); throws(() => upload.createReadStream(), { - name: "Error", - message: "Unexpected end of file", + name: "BadRequestError", + message: + "Request disconnected during file upload stream parsing.", + status: 499, + expose: true, }); };