search

node-formidable / formidable

The most used, flexible, fast and streaming parser for multipart form data. Supports uploading to serverless environments, AWS S3, Azure, GCP or the filesystem. Used in production.
MIT License
7k stars 680 forks source link

Mimetype Parsing for Non-File Parts in MultipartForm #805

Closed sabihismail closed 2 years ago

sabihismail commented 2 years ago

Support plan

Context

What are you trying to achieve or the steps to reproduce?

When I use the C# methods to generate a MultipartFormDataContent object, passing in a StringContent object includes a mimetype (Content-Type) for that header.

Based on formidible, it assumes any part with a mimetype is a file, which doesn't seem to be the case for some core libraries (according to Microsoft's implementation).

I'm not entirely sure which is the expected and correct implementation based on the RFC (sorry, not enough time to research this myself). I was wondering, back in v1 before I upgraded to v2, this seemed to have been accounted for and so I wonder if this upgrade introduced a bug?

What was the result you got?

Using form.parse, err was empty, fields was not populated, and files had 2 entries, one which was the file and one which was a string with a mimetype of text/plain; charset=utf-8. When I removed the header from the string part manually in the client, fields was populated as expected (1 entry) and so was files (also 1 entry).

What result did you expect?

It should maybe be smarter in recognizing the mimetype, or maybe outlining in the documentation that the mimetype should be null for non-file parts. Or maybe, Microsoft is the one in the wrong and there is no action to be performed :)

GrosSacASac commented 2 years ago

Files is more a concept than a thing on the web platform. What you do with the incoming data is up to you. You could, after form.parse handle files with text/plain like fields.

this is how we think it is a field if (!part.mimetype) {

and this is how it was done in 1.2.6 if (part.filename === undefined)

In the usual case a file has both filename and mimetype when upload through a regular web html form.

Should we change something ?

sabihismail commented 2 years ago

Hey, thanks for the quick response. I think for this it's worth having it outlined on the documentation and that would be sufficient enough of a solution, specifically the criteria for when a field is populated. It took myself a while to figure out how fields was populated since I was unfamiliar with the codebase.

Thanks!

GrosSacASac commented 2 years ago

20b4dd2512570d68abc5ef2d43994e3968926b92

Good enough ?

sabihismail commented 2 years ago

Yes thank you!

tunnckoCore commented 2 years ago

Yea, in v1 it was part.filename == '', then i tried to shorten it and make some more sense or refactor, and changed it to !part.filename then some weird bugs appear and failing tests. So i thought.. okay, in the general case, fields "don't have" mimetype and if there is it most probably is file and that's the expected.

So.. yea, that was the case. Good adding it to the changelog, we probably missed a lot of things in the long process. Thank god we published it as v2, so it's acceptable there can be breaking changes, haha.

@GrosSacASac great, we may add it to the readme docs somewhere too?