Update the busboy dependency to v1

[jaydenseric]

The busboy v1 update has a lot of great improvements, and importantly fixes this security flaw:

GHSA-wm7h-9275-46v2

Unfortunately, it also introduces a bug where the file size limit is 1 byte off:

mscdex/busboy#297

If we were to update the graphql-upload dependency busboy to v1 with that bug, imagine how many apps have public facing labels on file upload inputs saying things like "max 4 MB file size" and users would try to upload an exactly 4 MB file and it would be erroring. To avoid having to update our front ends to say "max 3.999999 MB" we would have to change our graphql-upload maxFileSize config in GraphQL APIs to be the real limit we want + 1. But then, if this busboy bug is fixed in a patch release, suddenly files 1 byte too big will start being accepted which could have who knows what problems further down the line in our systems depending how the files are used.

We might be forced to publish a major release of graphql-upload that only bumps busboy to v1, but with a big warning in the changelog entry that explains this outstanding busboy bug and that people should be aware of the dilemma and deal with it as best as they can.

[drbobbeaty]

@jaydenseric I am very interested in what you plan to do - and the "off by one" error is not nearly as troubling to me, as the Security Flaw... but that's just one vote... I know I may not be typical.

I would be very interested in whatever steps you take to remediate this issue - as I need to include this in my work as well. Thanks.

[jaydenseric]

I'm hoping for an overnight response to mscdex/busboy#297 (comment) , either a fix or a timeline for a fix otherwise I will probably just update to busboy v1 tomorrow, disable or modify the test that will start failing, and publish a new major version with the changelog caveat as described above.

[ken-do]

@jaydenseric Have you made any decision regarding this? The Security Flaw is blocking our work as well, and I also think the "off by one" error is definitely something that we can tolerate... while waiting for busboy to fix this issue.

[jaydenseric]

Yes, I'm about to start work on it now.

[maximzah]

@jaydenseric we have the same issue and it blocks our releases. Is there any estimated time for the fix? sorry for the disturbance

[jaydenseric]

ETA within the hour. I'm about to push a commit up.

[jaydenseric]

I was curious to see how graphql-upload fares with this vulnerability, and sure enough sending a request as described here to the Apollo upload examples API causes the Node.js server to crash like this:

/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/HeaderParser.js:85
      this.header[h][this.header[h].length - 1] += lines[i];
                                    ^

TypeError: Cannot read properties of undefined (reading 'length')
    at HeaderParser._parseHeader (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/HeaderParser.js:85:37)
    at HeaderParser._finish (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/HeaderParser.js:60:10)
    at SBMH.<anonymous> (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/HeaderParser.js:40:12)
    at SBMH.emit (node:events:527:28)
    at SBMH._sbmh_feed (/[redacted]/apollo-upload-examples/api/node_modules/streamsearch/lib/sbmh.js:159:14)
    at SBMH.push (/[redacted]/apollo-upload-examples/api/node_modules/streamsearch/lib/sbmh.js:56:14)
    at HeaderParser.push (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/HeaderParser.js:46:19)
    at Dicer._oninfo (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/Dicer.js:196:25)
    at SBMH.<anonymous> (/[redacted]/apollo-upload-examples/api/node_modules/dicer/lib/Dicer.js:126:10)
    at SBMH.emit (node:events:527:28)

[jaydenseric]

So, there might need to be more work to do. Because even with busboy v1.6.0 such malicious requests cause the Node.js server to crash; this time with an Unexpected end of form error.

[jaydenseric]

This seems to be a problem:

graphql-upload/processRequest.js

Line 91 in 5d4c758

parser.destroy();

If you change it to parser.end() it doesn't cause the server to crash anymore, but it's causing error messages in several tests to change. There is technical debt here because we had to deal with a lot of quirks relating to streams in older Node.js versions we no longer support anyway, and busboy itself behaves a lot differently to how it used to regarding the streams. I'm not sure anymore about the right way to do a lot of this, and changing it seems pretty risky. It's 9:33 pm on Friday night my time 😩

I mean, what is this for?

graphql-upload/processRequest.js

Lines 106 to 112 in 5d4c758

	// With a sufficiently large request body, subsequent events in the same
	// event frame cause the stream to pause after the parser is destroyed. To
	// ensure that the request resumes, the call to .resume() is scheduled for
	// later in the event loop.
	setImmediate(() => {
	request.resume();
	});

I vaguely remember it took us ages to figure out it was really important. Is it still important? IDK.

[maximzah]

@jaydenseric probably the issue should be reopened because as you've already mentioned the issue is still actual and there wasn't any release

[jaydenseric]

I pushed up a failing test for the function processRequest with a maliciously malformed multipart request, in case it helps someone put together a PR before I am able to figure out the right fixes: 7906f95 .

[jaydenseric]

I've figured a lot of it out, putting polish on it. I'm going as fast as I can but not sure I'm going to be able to publish a new graphql-upload version in time before I have to go away from keyboard for a couple of hours. If that happens, I'll get back on this after.

[jaydenseric]

I've pushed up a few commits that get all the tests passing across all our supported Node.js versions. I'm a little scared to rush this out without adequate testing or review because it changes the way errors a handled in the function processRequest quite a lot, but I think it's worth publishing anyway to deal with the current security issue.

Something that I noticed testing this out, is that when there are certain kinds of busboy parser errors we just just reject the async function processRequest with what the error was instead of wrapping it in a custom HTTP error, for example:

graphql-upload/processRequest.test.mjs

Lines 1531 to 1534 in ef566f3

	await rejects(processRequest(request, response), {
	name: "Error",
	message: "Unexpected end of form",
	});

The result of a multipart request that is so malformed that the error comes from the busboy parser and not graphql-upload checks for GraphQL multipart request spec compliance when using the Express or Koa middleware is that the HTTP response status code is 500, when ideally it should be something like a 400 Bad Request.

Improving that can happen in future work and doesn't need to block this release.

[jaydenseric]

This work has been published in v15.0.0! Please test this update in your projects and hopefully we didn't introduce any new bugs in the rush 🙏