BusBoy replacement?

[Uzlopak]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the issue has not already been raised

Issue

We should consider replacing busboy. It is actually quite buggy and has security issues. @mscdex does not really merge PRs, which would fix various bugs.

e.g.
mscdex/dicer#25
mscdex/dicer#22

Maybe we should write it completely new from scratch. The interfaces used by busboy seem to be a success, so we should keep the interfaces but replace the internals. Maybe with actual Streams instead of EventEmitter as baseclass.

Any thoughts?

[climba03003]

An replacement means it should be able to provide 1:1 feature compatibility.

1. stream handling for files - it should be the core of this module
2. faster parsing for multipart? maybe

[Uzlopak]

yes, I would also prefer a drop-in replacement

As far as i can see there are following streams to be potentially reimplemented:

StreamSearch
https://github.com/mscdex/streamsearch

HeaderParser
PartParser
Dicer
https://github.com/mscdex/dicer

Decoder (?)
UrlEncoded(?)
Multipart(?) + FileStream
BusBoy
https://github.com/mscdex/busboy

To solve this I would do a top to bottom approach. This means, first busboy and then all the way down to StreamSearch (if necessary).

[mcollina]

Multipart is a complex spec, however it can be reimplemented.
Those libraries are complex and written in an early node era.

If you'd like to improve parsing speed, I think the only way is to move it to wasm.

[StarpTech]

This reminds me to nodejs/node#38943

[mcollina]

Totally

[Uzlopak]

Actually it is not about performance, but about using the "new" nodejs streams and making it more resilient against malicious payloads.

Is there any interest in such a refactoring/reimplementation?

I would not invest time into this, if it will be denied because e.g. it is considered not worth it in the first place.

[mcollina]

I would not land an implementation that is slower than busboy.

[Uzlopak]

Do you expect that a streams implementation would be slower?

[Uzlopak]

Do we have some benchmarks regarding Busboy?

[mcollina]

Do you expect that a streams implementation would be slower?

I expect a stream.Readable / stream.Writable to be slower.

Here are the bench I know about:

https://github.com/mscdex/dicer/tree/master/bench

[kibertoad]

@Uzlopak We've ended up forking busboy: https://github.com/fastify/busboy
If there are known existing issues with the old version of it, we would be more than grateful if you could help with fixing them!

[Uzlopak]

@kibertoad
Oha. That was unexpected. Man I love this hands-on community.

I am Currently shopping with my wife, but if I am at home I would participate.

You should integrate npm package Dicer into the forked busboy to fix the crashing issue. Yesterday I tested the example mentioned in the Dicer GitHub issue/pr and could crash some public nodejs services.

[kibertoad]

@Uzlopak I'm open to that. Could you do a PR?

[Uzlopak]

Will do

[kibertoad]

@Uzlopak Note that it would be preferable to first integrate it as-is, and then do a fix PR separately, with benchmark results before and after the change, since it is a performance-sensitive part of the code, so we have to tread carefully.