fix(users) Fix bug file mime-type validate does not work #1272
Conversation
For example, we can add console.log(file.mimetype); to
/config/lib/multer.js after line 3 (it is
"module.exports.profileUploadFileFilter = function (req, file, cb) {")
After update user profile picture, nothing output on console, but you
can it after fix this bug.
trendzetter
changed the title
|
Should there be a few extra tests for this? This: |
|
It's good practice to add tests before the code. |
|
Also, (and this might be a place for another PR) but I feel like API should return these depending on error: http://www.restpatterns.org/HTTP_Status_Codes/422_-_Unprocessable_Entity |
|
@trendzetter @simison @incNick can we get some more tests to make sure this isn't an issue in the future after we fully resolve the bug? |
|
I guess we are stuck with this bug and a hint to the solution for now. |
|
I just fixed this for @Trustroots, I'll try to find time to make a PR for meanjs as well. |
|
whoa, nice! so that includes a working multer and the tests or is there still something missing from trustroots? Seems nothing left to do!? |
|
@trendzetter it's all done, just needs restructuring for meanjs. Tho it looks like meanjs doesn't actually process images, just stores them, which might cause issues when trying to determine actual file format (I wouldn't trust mime-types). In our case Graphicsmagick throws an error if it doesn't understand the file. Magic numbers might be a better way to determine if file is an image. ...if someone would like to collaborate on this? https://www.npmjs.com/package/mmmagic |
|
@simison I would be willing to look into the checking of the actual filetype when I see the issue after the initial fix for the unused multer based on mime. You think it's a risk users forging files to upload them to the server and somehow execute them? Would it be possible? |
|
@simison @trendzetter What's the status of this PR? |
|
I thought @simison is about to commit a slightly modified version of the file mime checking code for "trustroots". If he can show me the issue he is having with checking the filetype I will try to fix that issue. |
|
@trendzetter I've had sever lack of time recently, but I might soon be able to tackle this. Feel totally free to work on this though, just copypaste my implementation from TR. One more reason to implement "magic bytes" check: https://imagetragick.com/ |
|
@trendzetter FYI, I implemented magic bytes validation for TR: /~https://github.com/Trustroots/trustroots/blob/629d6015f11fdfa7a7ac242d0ac11b02064fd324/modules/users/server/controllers/users/users.profile.server.controller.js#L172-L186 ...now just gotta transfer this whole thing into mean. Hope to find some time in coming week. (edit; didn't happen) |
|
Cool, I am really willing to look into outstanding issues if there are any further but I am just too busy and not paid to get into another project code (TR). I guess the ImageTragick must be an additional stimulant for you to make you look into this. |
|
@simison @trendzetter is this ready to merge or are there still WIP fixes? |
|
@mleanos @trendzetter @simison anybody wishes to to finalize this? |
|
@trendzetter @lirantal this could be closed and follow ups @ #1465 |
|
Thanks Mikael! |
Bad use of file type valiator for multer.fileFilter
I have been trying some more things and indeed it uploads a text file if you rename it to jpg. So it validates only the extension on the client and not the mime-type in the multer on the server. After applying the suggested change at least config/env/multer.js is called, but it is still not filtering correctly for me: all files appear to be image/jpeg when I add console.log(file.mimetype); on line 4.
Fixes #1270