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

Developer set Content type header deleted if data is blob/file. (Caused by #1773) #3232

Closed
barryspearce opened this issue Aug 26, 2020 · 8 comments · Fixed by #3289
Closed
Projects
Milestone

Comments

@barryspearce
Copy link

barryspearce commented Aug 26, 2020

Describe the bug

I have a chunked file uploader, all data is to be sent as application/octet-stream in PUT in a binary stream.
This worked in 0.19.2.

However, 0.20.0 fails because of code added under #1773 "Fixing overwrite Blob/File type as Content-Type in browser.".

Line 21 lib/adapters/xhr.js:

    if (
      (utils.isBlob(requestData) || utils.isFile(requestData)) &&
      requestData.type
    ) {
      delete requestHeaders['Content-Type']; // Let the browser set it
    }

This "fix" now breaks my application as it assumes the browser knows best - it doesn't. It also is semantically inaccurate to set the content type to something like image/jpeg when the data is 20MB chunk from the middle of a jpeg file - the data is NOT of content type image/jpeg.

The "fix" applied in #1773 appears to be fixing overwrite caused by setting of default content-type headers but this solution removes ANY control over the header by the developer when dealing with Blobs.

To Reproduce

Simply set content-type in a request where the payload is a blob.

Expected behavior

Axios should NOT delete the content type header set by the developer specifically on the request. The power to control this should remain with the calling code and not be dictated by the library.

Environment

  • Axios Version 0.20.0
  • Adapter XHR
  • Browser ALL
  • Browser Version ALL
  • Node.js Version n/a
  • OS: ALL (Linux, Windows, mac)

Additional context/Screenshots

@adamdehaven
Copy link

Can confirm, this also broke for me with the 0.20.0 release. I explicitly set the header to application/octet-stream; however, the browser sends image/png when sending a PNG file, disregarding the hard-coded header.

As @barryspearce commented:

Axios should NOT delete the content type header set by the developer specifically on the request. The power to control this should remain with the calling code and not be dictated by the library.

Totally agree. Any idea when v0.20.1 will be released?

@Gerhut
Copy link
Contributor

Gerhut commented Sep 10, 2020

Firstly I think what you want to do is to upload a blob with type application/octet-stream so what should to be done is to build a blob with the type application/octet-stream from a PNG image, I write a codepen to show how to do it: https://codepen.io/Gerhut/pen/YzqaOQY?editors=0012

Alternatively, if what you need is to just send the bytes without type annotation as body data, you should consider use array buffer.

Fundamentally, for the 'content-type' header:

  • The 0.19 behavior is: body type overwritten by default type overwritten by config type
  • The 0.20 behavior is: default type overwritten by config type overwritten by body type
  • The correct behavior should be: default type overwritten by body type overwritten by config type

@barryspearce
Copy link
Author

barryspearce commented Sep 11, 2020

In my code the PUT is binary. Semantically my file uploader deals with metadata outside of the transfer of the data.
The transfer is therefore n * chunks of application/octet-stream. In Spring Framework in Java I therefore restrict the endpoint to receiving 'application/octet-stream'.

However, rightly or wrongly I send the data in 2 ways, in one case I slice (because I need to process the data before sending) and therefore use the sliced blob in the call to axios, in another (maximum performance upload where I do not need to access the data) I just pass the file object directly.

Whilst re-assigning the type with slice is fine where I slice anyway, it now means in the 2nd use case I now have to apply an additional slicing of the blob simply because axios has chosen to overwrite my choice. I cannot see anything to be gained at all in calling Bob.slice on a file object simply to change a string that has been overwritten by the library. Let me make a statement of the situation and see if that sounds right:

"In order to make the library output the correct string, and not override the string that I passed as a parameter, I have to slice the file object, simply to correct the string outputted by the library."

In terms of software engineering that statement worries me. Does it worry you? Does it sound like the tail wagging the dog?

@Gerhut
Copy link
Contributor

Gerhut commented Sep 11, 2020

Hi @barryspearce

Thanks for your reply, in the original PR I gave my original needs and misses so I am ok with revert this PR because of too much break reported. Whatever the result is, the decision maker is the maintainer of axios. In addition, I have 2 points to write down here for everyone to see if they make a little sense.

  1. Blobs have their metadata which is the content type, if you do not need such metadata you could consider ArrayBuffer, which is raw byte lists without such extra metadata, and the headers in config just work fine. Blob.slice is just a simple workaround that prevents you make the sync code asynchronously.

  2. Content type is one of entity headers in HTTP, which describe the body. As we know, there is another entity header called Content length. Previously, we are not able to overwrite the Content length (if the body is not stream so has a length) but be able to overwrite Content type if the body also has one. I think it's not consistent.

@barryspearce
Copy link
Author

barryspearce commented Sep 11, 2020

Of course the decision maker is Axios. Library maintainers are within their rights to make their library work their way. Surely the aim of a library is to share and help other developers solve a common problem, and avoid common pitfalls. The more restrictive the library the more it becomes a decision between one library and another. If the restriction we are talking about was necessary why do the browsers not enforce it?

Regarding 2. It isn't inconsistent.

The content length is an absolute fact. It is either known, or unknown (in the case of a stream). To state one amount and send another is an obvious problem, one that was used (and still can be used) in many of attacks in order to cause buffer overflow and so on, hence it is no longer modifiable (that said server code shouldn't rely on that).

However, content type is based on some particularly flimsy logic which is the browser equivalent of sticking a finger in the air to which way the wind is blowing. It is not absolute fact. To make browsers mis-report the type simply change the file suffix, many do not use any kind of sniffing to determine file types (Firefox according to MDN uses various methods by generally not sniffing). Therefore quite frequently I find my software on the server "correcting" the mis-reporting of a file type by examining the actual file headers - a more typical strategy on Unix/Linux machines and has always generated groans where systems rely on filenames for file type.

As an example rename a jpg file as .png and upload via Chromium. Chromium will report the file type as 'image/png'. Which it isn't. It's a jpeg.

Given that the browsers are so fickle in their determination of mime type it does not seem to be an inconsistency that the mime-type can be altered by the client where the content-length cannot.

@adamdehaven
Copy link

@Gerhut Any idea when this change will be reverted/fixed? I'd hate to have to revert to 0.19.x just for this one issue.

@jasonsaayman
Copy link
Member

I have merged as this makes sense to me that Axios should not dictate headers it seems a bit counterproductive or if we do try to sense a header we should at least allow overrides to work.

@jasonsaayman jasonsaayman added this to the v0.21.0 milestone Oct 23, 2020
@jasonsaayman jasonsaayman added this to To do in v0.21.0 via automation Oct 23, 2020
@jasonsaayman jasonsaayman moved this from To do to Done in v0.21.0 Oct 23, 2020
@bblanchon
Copy link

How can we get the old behavior back?
I mean, how can we let the browser set Content-Type, as in 0.20?
I know there is Blob.type, but it's not supported by all browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.21.0
  
Done
Development

Successfully merging a pull request may close this issue.

6 participants