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

different mime type for .bmp files since 4.16.0 #3486

Closed
Janpot opened this issue Nov 22, 2017 · 18 comments
Closed

different mime type for .bmp files since 4.16.0 #3486

Janpot opened this issue Nov 22, 2017 · 18 comments
Assignees
Milestone

Comments

@Janpot
Copy link

Janpot commented Nov 22, 2017

Mime type for .bmp files use to be image/bmp in express < 4.15.0. Now it is image/x-ms-bmp.

I've been looking at the mime package and it looks like between version 1.3.5 and 2.0.0 it resolves to image/x-ms-bmp. in version > 2.0.0 it resolves to image/bmp again

@dougwilson
Copy link
Contributor

The issue is that 2.0 is a different API (and we expose the mime API as parts of our API) and also requires Node.js 6+, both of which means we cannot use mime 2.0 in Express 4. The mime 1.x we are using right now is the only 1.x that does not have a security vulnerability so we cannot downgrade.

Perhaps get it backported to mime 1.x and we can then upgrade our dependency. We treat changes to mime reaolutions as semver minor here, as we have done for many many years. This change was introduced in a minor version of Express so we don't really see an issue here.

Basically if you can get mime package to release a new 1.x version with your desired mapping, we can update mime in our next minor version as usual, but otherwise there isn't an Express issue here (or anything we can change in this repository to make any adjustments).

The mime module allows you to adjust mappings though their API for the entire process if you require('mime') and redefine .bmp which since it's global will change what Express will see as well.

@Janpot
Copy link
Author

Janpot commented Nov 22, 2017

Ok, makes sense. Good to know.
Looks like there already was an issue about backporting mime broofa/mime#181

@dougwilson
Copy link
Contributor

👍 also feel free to ping me here or whatever if there is a mime release with the change so we can get moving on it. We'll of course see the release some point after as well but a ping won't be a bother either :)

@broofa
Copy link

broofa commented Nov 22, 2017

@dougwilson What's the security issue with mime 1.x? This is the first I've heard of that.

[Edit: Oh, are you talking about the RegExp DoS issue in https://github.com/broofa/mime/issues/167 ?]

@dougwilson
Copy link
Contributor

Oh, are you talking about the RegExp DoS issue in broofa/mime#167 ?

Yep. Those tools like nsp and snyk have 1.4.1 as the only "safe" version in the 1.x series. They flag modules that depend on (even just having a version ranges thag includes) older versions.

@broofa
Copy link

broofa commented Nov 22, 2017

Just published [email protected], built on [email protected]... however I don't believe that fixes the bmp issue mentioned here, since the mappings for image/bmp types haven't changed in mime-db for quite awhile.

kieffer@MacBook-Pro-3$ npm install mime@^1
...

+ [email protected]
added 1 package in 1.251s

kieffer@MacBook-Pro-3$ type mime
mime is hashed (./node_modules/.bin/mime)

kieffer@MacBook-Pro-3$ mime bmp
image/x-ms-bmp

@dougwilson Unfortunately I think the root issue here is the lack of facet prioritization I discuss in jshttp/mime-types#36 broofa/mime#160

This is fixed in mime@^2. I'd be okay refactoring mime@^2 to remove the dependency on ES6 support if that'd help. (that ES6 requirement was a bit of hubris on my part that was probably a mistake, truth be told.) Thoughts?

@dougwilson
Copy link
Contributor

Isn't that issue about changing the mime-types module? I don't think that the mime module uses that (and Express doesn't use it for this, either, just the mime module). I was just re-reading that issue and besides the question I had for myself on the octet type over the others, I don't really see an issue, though I feel like you did make that change in 1.x but then reverted it at some point?

As for removing the ES6 from mime 2.x, not sure if that's super necessary as far as Express 4.x is concerned, because the different API of mime 2.x also prevents us from upgrading, because we provide a direct reference to your 1.x module as public API in the form of express.static.mime so it would break all the apps out there currently doing express.static.mime.load('/path/to/types/file') etc.

@broofa
Copy link

broofa commented Nov 22, 2017

The fundamental issue here is that neither mime@^1 nor mime-types handles the issue of file extension contention well. mime@^1 uses "first one wins" (which leads to bugs like this one), and mime-types has some ad-hoc logic based on the type "source", which is neither standard nor canonical.

mime@^2 "solves" this in what I believe is the best/most-nuanced approach. I filed that issue with mime-types because (imho) the JS community as a whole would be best served by having mime libraries that use consistent logic for resolving these contention cases.

@dougwilson
Copy link
Contributor

Right, and I don't disagree with anything you're saying. The issue is still open as well, though I was just noting for those subscribed to this issue that the mime-types module is not used by Express for the behavior the OP is reporting, so subscribing to that issue wouldn't be relevant to this issur is all.

@Janpot
Copy link
Author

Janpot commented Nov 23, 2017

@broofa but mime before 1.3.5 also resolved to image/bmp. Is 1.3.6 the version when the image/x-ms-bmp type was introduced?

@broofa
Copy link

broofa commented Nov 23, 2017

The bmp issue was introduced in [email protected] when the mime-db dependency was upgraded from [email protected] to [email protected]. (The type definition for image/x-ms-bmp was introduced in [email protected])

@broofa
Copy link

broofa commented Nov 25, 2017

@Janpot @dougwilson : [email protected] contains a fix for the bmp issue. (tl;dr: [email protected] and [email protected] now both use the new https://github.com/broofa/mime-score module to resolve extension conflicts.)

@Janpot
Copy link
Author

Janpot commented Nov 25, 2017

@broofa thanks a lot!

@Janpot

This comment has been minimized.

@dougwilson

This comment has been minimized.

@Janpot

This comment has been minimized.

@01binary

This comment has been minimized.

@dougwilson

This comment has been minimized.

@dougwilson dougwilson added this to the 4.17 milestone Oct 27, 2018
@dougwilson dougwilson mentioned this issue May 3, 2019
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants