Nodebb: Mime problems

[RoiEXLab]

I ran across this issue while reporting my last one. Currently when a file is uploaded its Content-type param is checked and the extension is renamed to a file extension matching this mime type.
This mime type is controlled by the browser.
While I was able to upload zipfiles without problems, a friend of mine had his files being renamed to undefined.
The reason behind this was, that chrome seems to pick application/x-zip-compressed as content type which is not supported by the mime module nodebb is using, although it supports zip in general.
I was able to work around this thanks to my own Plugin, mentioned in #5448, but you can't expect this from everyone.
My suggestion would be to either get rid of the mime extension system, or to set the content type based on the mime module, if present.

[revir]

Also if you upload a mp3 file on chrome, the file mime would be "audio/mp3", which is not supported by node-mime, so you would get a file extension named ".undefined", that's bad. And I guess "audio/mp3" is a long-time issue of chrome:

https://bugs.chromium.org/p/chromium/issues/detail?id=227004

But could you stop to use node-mime to get the extension name of uploaded file?

[RoiEXLab]

@revir You can use my plugin to fix that "issue"
just install nodebb-plugin-custom-file-extensions in your nodebb and add audio/mp3 mp3 in the settings.
Not the best way to solve this, but it should work pretty good...

[revir]

@RoiEXLab
Yeah I found that, thanks.
It's brilliant to extend node-mime to more types, great job!

[RoiEXLab]

Thanks :D
I ran into a similar problem, and since I haven't found anything doing something similar, I decided to code it myself...
This was my first nodebb plugin...

[revir]

And it works :)
Thanks.

[barisusakli]

application/x-zip-compressed

is not supported by https://github.com/jshttp/mime-types either.

IIRC there are some security implications if we just take the file extension from the filename.

[RoiEXLab]

@barisusakli Regarding security: Using Mime types isn't making the files any safer, since the mime type can be freely chosen by the client.
I believe a blacklist of forbidden extensions would be better. This blacklist should contain all sorts of executable file extensions, from exe or shell scripts to js scripts, so the forum won't host malware which can accidentally be downloaded by a "stupid user" who though that his pc was about to explode, because a flashing gif told him that it would unless he installed this app.

[barisusakli]

Yeah I think if allow file uploads and you don't set allowedFileExtensions in the ACP you are allowing all kinds of files to be uploaded right now. And when we were taking the extension from the filename it was possible to craft a malicious image file and submit it with content-type: image/png but change its extension to .html. That is not possible now since when content-type is image/png the extension will be png.

[RoiEXLab]

@barisusakli NodeBB created such a tag <img src="actuallyanhtmlfile.png"> which caused browsers to perform a GET request, which returned text/html with js?
Couldn't this problem be solved, by determining the filetype also by the extension, ignoring the mime type at all, or am I getting something wrong here?

[barisusakli]

Nah it was happening when the uploaded link is clicked by a user.

[pitaj]

Right but the file extension should be .html in the request on the server, so just filtering by file extension would work, right?

[barisusakli]

Yes this is not an issue if you filter extensions like html, and therefore disallowing uploads of that type.

[pitaj]

So how does it work right now? Any mime type besides images is not given an extension at all?

Perhaps a better solution is to use a more sensible default extension allowance?

[barisusakli]

They are given an extension based on their mime-type, but in this case since application/x-zip-compressed is not supported by the mime-type libraries they get undefined as extension.

[pitaj]

Right, but why do that at all?

It seems like just checking file extensions would work fine.

[barisusakli]

We can go back to using the extension from the filename, but we would have to use a default whitelist of extensions if the user does not specify one in the ACP.

[RoiEXLab]

@barisusakli Just asking, because my plugin will be useless, once this change is applied...
How would I edit

"nbbpm": {
  "compatibility": "^1.4.4"
}

to be only compatible with the versions using mime?
(I believe this mime types in general were introduced in version 0.7.0 according to #725, but the forced extension renaming in v1.4.3 or 1.4.4)

[barisusakli]

I think you will just change it to

"nbbpm": {
  "compatibility": "1.4.4"
}

[RoiEXLab]

@julianlam @barisusakli Just a little question:
Shouldn't the mime dependency get removed?
The mime module is still being used by image uploads, but since the whitelist also contains image file extensions this should be big of a deal.

[RoiEXLab]

@barisusakli Hate to say it, but this issue needs to be reopened...
Although the file extension is checked against a whitelist, this extension is still determined by mime, which causes some file types not to be recognized correctly!
In other words: The old problems still exist.

[barisusakli]

Which uploads are not recognized? I think the uploads in the composer are no longer using mime.

[RoiEXLab]

Whenever you upload a file which has no official file type, resulting in a browser specifying application/octet-stream this "unknown file type" is checked against the whitelist and being renamed to .bin afterwards.
If .bin is not in the whitelist, the file get's dropped no matter what file extension it actually got...

[barisusakli]

Are you saying the file you upload has no extension? Also looks like this issue isn't resolved in 1.4.6 it is committed in develop and will be available in 1.5.0

[RoiEXLab]

I could swear this change was applied to the 1.X.X branch at some time...
Ok then, nevermind...