Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In v1 fileSize limit is 1 byte off #297

Open
jaydenseric opened this issue Apr 26, 2022 · 5 comments · May be fixed by #315
Open

In v1 fileSize limit is 1 byte off #297

jaydenseric opened this issue Apr 26, 2022 · 5 comments · May be fixed by #315

Comments

@jaydenseric
Copy link

jaydenseric commented Apr 26, 2022

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
Copy link
Author

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
Copy link
Author

jaydenseric commented May 25, 2022

@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 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
Copy link
Owner

mscdex commented May 26, 2022

I haven't had time to look into it.

wickedest pushed a commit to wickedest/busboy that referenced this issue Aug 25, 2022
@wickedest wickedest linked a pull request Aug 25, 2022 that will close this issue
@wickedest
Copy link

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
Copy link

zhanglingthai commented Apr 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants