Skip to content

Commit

Permalink
Simplify error handling within the function processRequest.
Browse files Browse the repository at this point in the history
  • Loading branch information
krasivyy3954 committed May 28, 2022
1 parent 7e100bf commit a7ae2fe
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 24 deletions.
2 changes: 1 addition & 1 deletion changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@

- 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.

### Patch

- 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
Expand Down
30 changes: 11 additions & 19 deletions processRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ function processRequest(
/** @type {Error} */
let exitError;

/** @type {import("stream").Readable} */
let lastFileStream;

/**
* @type {{ [key: string]: unknown } | Array<
* { [key: string]: unknown }
Expand Down Expand Up @@ -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
Expand All @@ -106,7 +96,9 @@ function processRequest(
setImmediate(() => {
request.resume();
});
};

reject(exitError);
}

parser.on("field", (fieldName, value, { valueTruncated }) => {
if (valueTruncated)
Expand Down Expand Up @@ -228,8 +220,6 @@ function processRequest(
parser.on(
"file",
(fieldName, stream, { filename, encoding, mimeType: mimetype }) => {
lastFileStream = stream;

if (!map) {
ignoreStream(stream);
return exit(
Expand Down Expand Up @@ -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;
Expand Down
14 changes: 10 additions & 4 deletions processRequest.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
);
};
Expand Down Expand Up @@ -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,
});
};

Expand Down

0 comments on commit a7ae2fe

Please sign in to comment.