search

mscdex / busboy

A streaming parser for HTML form data for node.js
MIT License
2.86k stars 213 forks source link

In v1 `fileSize` limit is 1 byte off #297

Open jaydenseric opened 2 years ago

jaydenseric commented 2 years ago

Before busboy v1, the fileSize limit would be the number of bytes that are allowed. Now in v1, it's changed to being the number of bytes that first exceeds the limit.

Basically, if in the busboy limits you set fileSize: 1 and a text file contains only 1 character, it used to be allowed without truncating the file contents. Now with v1 in the same situation the limit event is emitted in the file stream and truncation occurs.

I think the new behavior is incorrect, and the old behavior should be restored in a bug fix patch release. I noticed this issue because tests started failing in graphql-upload when attempting to update the busboy dependency.

jaydenseric commented 2 years ago

This issue is currently blocking upgrading busboy to v1 in graphql-upload, which is unfortunate because I plan to release a major version very soon and hoped this upgrade would make it in this release.

I tried looking at a PR to fix this issue, but got confused about what the right way to fix it was. There was a spot mindlessly adding a + 1 seemed to work, but I hesitate to raise a PR without thoroughly understanding the change. Also, there should really be a test for this and I'm not really sure the right way to add one without a fair bit of figuring out as it's a custom setup.

jaydenseric commented 2 years ago

@mscdex can you please at least triage this issue? It hasn't been acknowledged for a whole month now. We really need to know roughly when this will be fixed, or at least have some guidance for contributing a PR.

We are now in this hard position where busboy < v1 should not be used due to the serious security flaw https://github.com/advisories/GHSA-wm7h-9275-46v2 , and upgrading to busboy v1 would push this bug on everyone using graphql-upload. 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 config in APIs to be the real limit we want + 1. But then, if this busboy bug is fixed in a patch release, then 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.

I'm now forced to entertain the option of 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.

mscdex commented 2 years ago

I haven't had time to look into it.

wickedest commented 2 years ago

There is a similar issue for fieldSize where the limit is 1 byte off.

We are also in a similar situation as @jaydenseric and it would impact our users in a similar way too. I looked into the issue, and made an attempt at a fix, which is just a guess, tbh, but hopefully it might start the ball rolling in the right direction.

zhanglingthai commented 7 months ago

I found a problem.If fileSize limit is a Float number it can not working. So I can not set it to 0.4 1024 1024. I have to make sure it's a whole number.