For Buffers, set the content type to application/octet-stream by default. by UndeadKernel · Pull Request #54 · form-data/form-data

UndeadKernel · 2013-12-12T13:54:41Z

When buffers are attached to a form they were added with no content type; therefore, the content type would default to 'text/plan'. This modification changes this behaviour to default to 'application/octet-stream' for buffers.
The user is also able to add a 'filename' optional field to the header in case a file is being sent in binary form.

…ation/octet-stream by default.

alexindigo · 2013-12-12T17:48:15Z

It would be nice to have a test to test new functionality.

felixge · 2013-12-13T04:57:10Z

The patch removes the ability to set a custom content type for buffers, which is an unacceptable regression. Please add a test / make sure that custom options will always be honored. (you're already doing this for filename correctly)

UndeadKernel · 2013-12-13T10:07:14Z

Ay Ay Captain!
Working on the test and the honoring of custom content type.

felixge · 2013-12-13T10:49:12Z

Ay Ay Captain! Working on the test and the honoring of custom content type.

😄 - thank you so much for spending time on making this library better!

A new unit test was created to verify that Buffers are being sent correctly and all custom options are taken into account.

UndeadKernel · 2013-12-18T22:56:00Z

Are there any other things left to make this pull?

alexindigo · 2013-12-18T22:58:11Z

At first glance looks exactly what we need, I'll play with it tonight.
(Somehow there was no notification on your commit 4 days ago).

alexindigo · 2013-12-19T04:36:34Z

Interesting your test is not failing if I run it against old codebase. :)

alexindigo · 2013-12-19T18:27:50Z

This one form.append('buffer_two' , buffer, {filename: 'buffer_two.bin'}); sent with Content-Type: application/octet-stream without your changes.

This is how request body looks like:

----------------------------055007228935537416881819
Content-Disposition: form-data; name="buffer_one"

ABCD
----------------------------055007228935537416881819
Content-Disposition: form-data; name="buffer_two"; filename="buffer_two.bin"
Content-Type: application/octet-stream

ABCD
----------------------------055007228935537416881819
Content-Disposition: form-data; name="buffer_three"; filename="buffer_three.bin"
Content-Type: application/custom-type

ABCD
----------------------------055007228935537416881819--

And it's been parsed by Formidable the way you expect it in your test.

Do you mind to share your setup and what exactly doesn't work?

alexindigo · 2013-12-20T04:49:19Z

Btw, what is your environment on receiving end?

UndeadKernel · 2013-12-22T15:58:49Z

My receiving environment is basically plain express.
Maybe the problem is seen when sending actual binary data. For the test I'm sending a buffer encoding the "binary data" in ASCII format. I will try to send not ASCII characters and see if this has any impact.

I opened this pull request because I noticed that when I wanted to transfer a binary file that I had previously fetched into a buffer to express the data sent would not be the same in the receiving end. I then noticed that if you where sending an arbitrary "buffer" without a filename, the content ype was chosen to be "plan/text" by default.

alexindigo · 2013-12-22T17:37:12Z

I see, maybe it's Formidable being smart enough.

Oh, and in Express, can you set default encoding to application/octet-stream – would it make help?

UndeadKernel · 2013-12-23T09:36:06Z

I'm not sure I completely understand what you mean with "set the default encoding..." in express.
What I noticed would work when dealing with express and binary data is to set the "filename" option explicit when appending the buffer to the form. I then noticed that this library then tries to find the right encodign type for the filename extension using the "mime" library. The "mime" library then defaults to "application/octet-stream" when the file extension doesn't match anything known.

UndeadKernel · 2014-01-04T19:28:06Z

Hey, I just wanted to follow up on this.
Are things looking good or is the pull request going to be rejected?

alexindigo · 2014-01-04T19:29:50Z

I thought you're going to update the test to make it fail without your changes, otherwise it's not really a test, or it's working and no need to change anything. :)

UndeadKernel · 2014-01-04T20:58:56Z

I see, I misunderstood your second to last message.
Let me see what I can do.

alexindigo · 2014-01-04T22:49:08Z

Sorry for not being clear and let me know if you need a hand with something.

alexindigo · 2014-06-23T17:23:04Z

Ok, we will incorporate this feature into 0.2 release so users would aware of the change. (Unless they're using carrot top) :)

UndeadKernel · 2014-06-24T11:58:16Z

That's good to know.

Let me know if other things need to be done to this pull request.

alexindigo · 2014-06-24T16:41:13Z

Do we really need to have this header?

I believe it is required according to the documentation here:
http://www.w3.org/Protocols/rfc1341/5_Content-Transfer-Encoding.html

On Tue, Jun 24, 2014 at 6:41 PM, Alex Indigo [email protected]
wrote:

In lib/form_data.js:

if (Buffer.isBuffer(value)) {

// let the user provide a filename for the buffer.

if(options.filename) {

header += '; filename="' + options.filename + '"';

}

header += FormData.LINE_BREAK;

// let the user set a custom content-type

if (options.contentType) {

header += 'Content-Type: ' + options.contentType;

} else {

header += 'Content-Type: application/octet-stream';

}

// if a buffer is being sent then set the encoding as binary.

header += FormData.LINE_BREAK + 'Content-Transfer-Encoding: binary';

Do we really need to have this header?

—
Reply to this email directly or view it on GitHub
https://github.com/felixge/node-form-data/pull/54/files#r14139683.

Quote: "As of the publication of this document, there are no standardized Internet transports for which it is legitimate to include unencoded 8-bit or binary data in mail bodies. Thus there are no circumstances in which the "8bit" or "binary" Content-Transfer-Encoding is actually legal on the Internet. However, in the event that 8-bit or binary mail transport becomes a reality in Internet mail, or when this document is used in conjunction with any other 8-bit or binary-capable transport mechanism, 8-bit or binary bodies should be labeled as such using this mechanism."

And it seems like they very concerned about mail transport in the future, I don't want to sound Bill Gates here, but I don't see using [email protected] to send emails.

And my main concern, that if it does make the difference (not yet so far) it will introduce different behavior when you send binary files and binary buffers, which is totally unexpected from user's point of view.

With this that you show me, I agree that the header is not needed.

alexindigo · 2014-06-24T16:49:27Z

@UndeadKernel I still can't make your test fail without your modifications to form_data.js, means it doesn't really test the problem. Can you refactor it?
(And tabs instead of spaces are frowned upon here) :)

Thank you.

UndeadKernel · 2014-06-24T16:55:36Z

I'm using tabs instead of spaces!? Silly me :p

Last time I checked the tests were not failing because I was using
"formidable" which parses the data correctly by magic in this situation.

On Tue, Jun 24, 2014 at 6:49 PM, Alex Indigo [email protected]
wrote:

@UndeadKernel https://github.com/UndeadKernel I still can't make your
test fail without your modifications to form_data.js, means it doesn't
really test the problem. Can you refactor it?
(And tabs instead of spaces are frowned upon here) :)

Thank you.

—
Reply to this email directly or view it on GitHub
#54 (comment).

alexindigo · 2014-06-24T17:06:57Z

It's what we're using for tests, maybe in your test you don't need to rely on Formidable but rather get naked request and check the difference "manually". Otherwise there is no point to have the test if it doesn't really test stuff, right? :)

zensh · 2015-07-09T12:44:12Z

Here should be change to options.contentType || mime.lookup(options.filename || value.path) || 'application/octet-stream'.

mime.lookup will return false when lookup failed, then data will be 'Content-Type: false'

zensh · 2015-07-09T12:47:02Z

I have a problem with superagent, cause by "Content-Type: false"

When attaching buffers to the form, the content type is set to applic…

5ff6e3d

…ation/octet-stream by default.

Allow the user to set a custom "content-type" when a Buffer is sent.

20e3778

A new unit test was created to verify that Buffers are being sent correctly and all custom options are taken into account.

alexindigo self-assigned this Jun 23, 2014

alexindigo added this to the v0.2 milestone Jun 23, 2014

alexindigo added improvement labels Jun 23, 2014

alexindigo reviewed Jun 24, 2014
View reviewed changes

alexindigo added the v1.0.0-rc3 label Jun 13, 2015

zensh reviewed Jul 9, 2015
View reviewed changes

DylanPiercey mentioned this pull request Jul 23, 2015

Default content-type to 'application/octect-stream' #128

Merged

alexindigo closed this in #128 Jul 24, 2015

alexindigo added the bug label Jul 24, 2015

Conversation

UndeadKernel commented Dec 12, 2013

Uh oh!

alexindigo commented Dec 12, 2013

Uh oh!

felixge commented Dec 13, 2013

Uh oh!

UndeadKernel commented Dec 13, 2013

Uh oh!

felixge commented Dec 13, 2013

Uh oh!

UndeadKernel commented Dec 18, 2013

Uh oh!

alexindigo commented Dec 18, 2013

Uh oh!

alexindigo commented Dec 19, 2013

Uh oh!

alexindigo commented Dec 19, 2013

Uh oh!

alexindigo commented Dec 20, 2013

Uh oh!

UndeadKernel commented Dec 22, 2013

Uh oh!

alexindigo commented Dec 22, 2013

Uh oh!

UndeadKernel commented Dec 23, 2013

Uh oh!

UndeadKernel commented Jan 4, 2014

Uh oh!

alexindigo commented Jan 4, 2014

Uh oh!

UndeadKernel commented Jan 4, 2014

Uh oh!

alexindigo commented Jan 4, 2014

Uh oh!

alexindigo commented Jun 23, 2014

Uh oh!

UndeadKernel commented Jun 24, 2014

Uh oh!

alexindigo Jun 24, 2014

Choose a reason for hiding this comment

Uh oh!

UndeadKernel Jun 24, 2014

Choose a reason for hiding this comment

Uh oh!

alexindigo Jun 24, 2014

Choose a reason for hiding this comment

Uh oh!

UndeadKernel Jun 24, 2014

Choose a reason for hiding this comment

Uh oh!

alexindigo commented Jun 24, 2014

Uh oh!

UndeadKernel commented Jun 24, 2014

Uh oh!

alexindigo commented Jun 24, 2014

Uh oh!

zensh Jul 9, 2015

Choose a reason for hiding this comment

Uh oh!

zensh commented Jul 9, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants