search

jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 131 forks source link

Update the `busboy` dependency to v1 #311

Closed jaydenseric closed 2 years ago

jaydenseric commented 2 years ago

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

https://github.com/advisories/GHSA-wm7h-9275-46v2

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

https://github.com/mscdex/busboy/issues/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 commented 2 years ago

@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 commented 2 years ago

I'm hoping for an overnight response to https://github.com/mscdex/busboy/issues/297#issuecomment-1137963988 , 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 commented 2 years ago

@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 commented 2 years ago

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

maximzah commented 2 years ago

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

jaydenseric commented 2 years ago

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

jaydenseric commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

This seems to be a problem:

https://github.com/jaydenseric/graphql-upload/blob/5d4c75891929857bd47a8364b08d2db3db0a8c4e/processRequest.js#L91

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?

https://github.com/jaydenseric/graphql-upload/blob/5d4c75891929857bd47a8364b08d2db3db0a8c4e/processRequest.js#L106-L112

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

maximzah commented 2 years ago

@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 commented 2 years ago

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: https://github.com/jaydenseric/graphql-upload/commit/7906f951e50a802807fafc6a28b0b07634644c40 .

jaydenseric commented 2 years ago

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 commented 2 years ago

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:

https://github.com/jaydenseric/graphql-upload/blob/ef566f39d1f0addc7a98493514a24b38e4eb13ef/processRequest.test.mjs#L1531-L1534

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 commented 2 years ago

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 🙏