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

Wrong behavior of application/wasm mime type #220

Closed
rqrqrqrq opened this issue Mar 7, 2018 · 12 comments
Closed

Wrong behavior of application/wasm mime type #220

rqrqrqrq opened this issue Mar 7, 2018 · 12 comments

Comments

@rqrqrqrq
Copy link

rqrqrqrq commented Mar 7, 2018

Current webassebmly spec says

Note: extra parameters are not allowed, including the empty application/wasm;.
https://webassembly.github.io/spec/web-api/index.html#streaming-modules

For now ecstatic adds charset in Content-Type for all filetypes, so mimetype for wasm files will be application/wasm; charset=utf-8. Can we somehow disable this for files with application/wasm mimetype (or for binary files at all?)?

I'll be happy to create PR if you tell me right way to figure out exclusions.

@dcodeIO
Copy link

dcodeIO commented Apr 19, 2018

Just got hit by this as well. This basically breaks WebAssembly.instantiateStreaming incl. WebAssembly source map support.

@jfhbrook
Copy link
Owner

https://github.com/jfhbrook/node-ecstatic/blob/master/lib/ecstatic.js#L242-L247 potentially a bug in mime@1 ? upgrading mime is part of a bigger project.

@dcodeIO
Copy link

dcodeIO commented Apr 20, 2018

Yeah, I think that's the cause. Seems that the mime package removed the charset functionality entirely in v2. Ideally, if there'd be a way to detect that a mime type is indicating a binary file, appending the charset could simply be skipped, or something like that.

@takahirox
Copy link

takahirox commented Apr 24, 2018

I'd like to request "charset=utf-8" skipping for binary files. createImageBitmap() doesn't work for images whose header includes "charset=utf-8".

https://bugzilla.mozilla.org/show_bug.cgi?id=1413806

@bytex64
Copy link

bytex64 commented Apr 27, 2018

Seems like ecstatic isn't using mime.charsets.lookup() correctly. Here's the entire source for the function from the mime module:

lookup: function(mimeType, fallback) {
  // Assume text types are utf8
  return (/^text\/|^application\/(javascript|json)/).test(mimeType) ? 'UTF-8' : fallback;
}

And here's how ecstatic is using it:

charSet = mime.charsets.lookup(contentType, 'utf-8');
if (charSet) {
  contentType += `; charset=${charSet}`;
}

mime.charsets.lookup() returns either the detected charset ('UTF-8') or the given fallback (in ecstatic's case, 'utf-8'), but ecstatic is treating it like it returns a bool. Since a non-empty string is always true, the charset 'utf-8' is always added. The fix is to remove the fallback ('utf-8') parameter (making the fallback case a falsy undefined) and/or check specifically for charset == 'UTF-8'.

I was wondering if maybe that changed lately, but it seems mime.charsets.lookup() has worked that way as far back as I looked into the history (~2011).

@jfhbrook
Copy link
Owner

We're using mime@1, which hasn't had an api update in Some Time. Presumably, this code would change if we upgraded to v2, which will happen Eventually if someone puts the time in.

I agree with @bytex64 's assessment and I think this was just a mistake. Though, I think that at least for some mime types, defaulting to UTF-8 is appropriate? Maybe this can be thrown behind a feature switch, though. Will accept a pull request that implements this!

@BigBlueHat
Copy link
Contributor

FWIW, folks using http-server have found that this breaks image usage (via Fetch + createImageBitmap()) in Firefox (at least): http-party/http-server#296 (comment)

I'm be submitting a PR shortly to remove the defaulting to charset=utf-8, but a follow-up PR for switching to upgrading the mime package would be the best long term solution (but obviously comes with greater risk/overhead).

BigBlueHat added a commit to BigBlueHat/node-ecstatic that referenced this issue Jun 19, 2018
@jfhbrook
Copy link
Owner

The mime upgrade has been a long time coming. If you're willing to put the work into it I'd love to merge that and just bang out a major release.

@BigBlueHat
Copy link
Contributor

It looks like the biggest thing missing (according to #218 and some follow-up research) is the loss of the .load() system. They do support passing in custom media type lists in JSON, but we'd need to provide some parsing/handling of the .types format (as seen in Apache's mime.types).

For now I'll reach out to see if the mime library plans to add this (even as a separate module/script) or if we would need to directly.

@jfhbrook
Copy link
Owner

If you like! Though I don't think they were interested in supporting apache mime files anymore.

One suggestion from that ticket was to make it so you could implement your own lookup function, which I think is actually pretty good and a solid way to punt on this. But I'll take it either way.

@BigBlueHat
Copy link
Contributor

The new Mime() constructor can take the JSON files created by mime-db, so it's really about (afaict) getting .types into the .json format mime (and mime-db) use.

Just filed such an issue here, fwiw: jshttp/mime-db#133

jfhbrook pushed a commit that referenced this issue Apr 5, 2019
@jfhbrook
Copy link
Owner

jfhbrook commented Apr 5, 2019

I think #240 fixes this.

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.

6 participants