fix(users) MIME-type checking fixed on both client and server-side #1465

hyperreality · 2016-08-29T23:41:10Z

fix(users) Client-side MIME-type checking for picture upload

@mleanos pointed out that the MIME-type checking for profile pictures
wasn't working on the client-side as expected, so I fixed it to display
a warning message if the user attempts to upload a different kind of file.

In the meantime I realised that MIME-type checking was broken on the
server-side too, as noted in #1272, so I applied the necessary fixes and
added a test that attempts to upload a JS file as a profile picture.

As @simison said on that thread, the multer isn't infallible and magic bytes
checking would be best, so perhaps that is the next stage.

…s bar (#1443) * Add ng-file-upload and picture cropping * Update bower.json Remove bower dependency for angular-file-upload

hyperreality · 2016-08-30T02:07:43Z

Note that this doesn't fix the issue reported in #1272, it is still possible to rename any file to a .jpg for instance and upload it through the API route. To fix that we need to apply a fix like @simison implemented on his website - temporarily upload the file, magic-byte check it, then discard if it doesn't match. If @simison is pushed for time then I am willing to port his code.

simison · 2016-08-30T16:44:21Z

@hyperreality yeah I'm pretty thin on time. Ping me if ya need any help understanding Trustroots codebase (see my chat contact infos). Freenode IRC channel #trustroots might be easiest.

lirantal · 2016-08-30T16:57:48Z

@hyperreality the temp upload and mime type checking is a bit of an overkill for the framework so I don't want to go there.

lirantal · 2016-08-30T16:59:01Z

So what's the status here?

simison · 2016-08-30T17:04:24Z

modules/core/server/controllers/errors.server.controller.js

      case 'LIMIT_FILE_SIZE':
-        message = 'Image too big. Please maximum ' + (config.uploads.profileUpload.limits.fileSize / (1024 * 1024)).toFixed(2) + ' Mb files.';
+        message = 'Image too big. The maximum size allowed is ' + (config.uploads.profileUpload.limits.fileSize / (1024 * 1024)).toFixed(2) + ' Mb';


There could be a test for this, too.

@lirantal I just saw some bad English there and took the opportunity to clean it up.

@simison is right, while I'm at it I should add tests to make sure that those errors are correctly triggered too.

https://github.com/Trustroots/trustroots/blob/master/modules/users/tests/server/user.server.routes.tests.js#L1165-L1191

https://github.com/Trustroots/trustroots/blob/master/config/env/test.js#L26

;-)

hyperreality · 2016-08-30T17:42:58Z

@lirantal right, I won't bother adding that. But I do have a concern about malicious users finding out that a particular website uses the MEAN framework and reading issues like this, and then realising they can upload arbitrary data through the route.
The data will not get executed, however it just seems wrong to allow it, and it's a potential vulnerability if somebody misconfigures their server in other ways.

lirantal · 2016-08-30T18:02:31Z

I get what you mean, and because of that you should delete your comment and this PR altogether once we merge it so no one can find out about it!!!

On a serious note :) - mime-type checking in real environments actually happen on an isolated system which needs to run some tests, kind of like a service. So a real secure handling is more complex.

P.S. we can also sort out server-side security in a later PR. I don't want to hold PRs for too long because they easily get outdated and un-maintained

hyperreality · 2016-08-30T19:32:53Z

Haha! In that case I think we should add something to the docs about rolling your own MIME-type checking for production environments.

You're right, there are a lots of great pull requests here that got abandoned as developers apparently found all their free time taken up. I really appreciate your dedication to pushing towards version 0.5!

lirantal · 2016-08-30T23:14:02Z

Honestly I think it will overload the README.md but if you meant to add it to the docs at meanjs.org then definitely!

lirantal · 2016-08-30T23:14:36Z

@hyperreality ping me back when you've added tests and this PR is ready to merge

hyperreality · 2016-09-01T22:32:38Z

@lirantal added the image size test. Might be worth noting that the crop directive already tends to shrink the picture size down to <150kB, so it's highly unlikely that people will hit the 1 megabyte limit through the profile picture upload form anyway.

lirantal · 2016-09-02T17:27:24Z

@simison any other comments on this PR before merging it in?

lirantal · 2016-09-02T17:28:17Z

@hyperreality could you check the build erroring out due to bower conflicts for an angular library?

simison · 2016-09-02T17:29:09Z

bower.json

@@ -13,7 +13,7 @@
    "angular-ui-router": "~0.2.18",
    "bootstrap": "~3.3.6",
    "ng-file-upload": "^12.1.0",
-    "ng-img-crop": "ngImgCrop#^0.3.2",
+    "ng-img-crop-full-extended": "ngImgCropFullExtended#~6.0.1",


Why ngImgCropFullExtended#~6.0.1 and not just ~6.0.1?

Bower does this automatically, I think because the package is named ng-img-crop-full-extended but the Github repo is ngImgCropFullExtended.

For that reason it won't work if we do:
"ng-img-crop-full-extended": "~6.0.1",

But we could do this:
"ngImgCropFullExtended": "~6.0.1",

simison · 2016-09-02T17:31:17Z

config/assets/default.js

@@ -9,7 +9,7 @@ module.exports = {
        // bower:css
        'public/lib/bootstrap/dist/css/bootstrap.css',
        'public/lib/bootstrap/dist/css/bootstrap-theme.css',
-        'public/lib/ng-img-crop/compile/unminified/ng-img-crop.css'
+        'public/lib/ng-img-crop-full-extended/compile/minified/ng-img-crop.css'


unminified instead of minified

@hyperreality FYI

hyperreality · 2016-09-02T18:11:47Z

Hmm. The ngImgCropFullExtended module requires angular 1.4.0 which is why the bower builds failed. Furthermore they appear to have messed up their version numbers by jumping from 0.6.0 to 6.0.1 in the last update. Not sure at all about this module, what do you think @simison?

simison · 2016-09-02T18:12:55Z

@hyperreality Yeah, not very convincing. :-/ Not sure if there's anything very stable out there.

They seem to be on issues quite actively, tho: CrackerakiUA/ngImgCropFullExtended#154

hyperreality · 2016-09-02T18:15:03Z

In that case I'm not averse to rolling back the cropping functionality and just leaving it around here in case somewhat specifically wants it.

lirantal · 2016-09-02T18:26:57Z

@hyperreality any chance we can find something else for cropping? there surely must be something that is widely used

simison · 2016-09-02T19:17:39Z

@lirantal @hyperreality they merged my PR's regarding version issues really fast. It's much better than that badly outdated package, anyway.

Quick googling gives just lots of (well maintained) jQuery packages.

hyperreality · 2016-09-02T19:56:03Z

@simison yes work appears to be underway on that package very fast. Thanks for going there and fixing the obvious issues. Do you think it is appropriate to go into the MEAN framework?

lirantal · 2016-09-04T20:07:23Z

@hyperreality did that fix got merged? if so, can you please update this PR status?

simison · 2016-09-04T20:12:47Z

@lirantal @hyperreality it's still a fork of a fork, which is kinda off-putting.

I opened a question about it CrackerakiUA/ngImgCropFullExtended#161

lirantal · 2016-09-04T20:14:37Z

Ugh :(

hyperreality · 2016-09-09T16:39:48Z

No response from the Angular UI team yet about including that crop module.

mleanos · 2016-10-15T21:16:10Z

@lirantal @simison @hyperreality Any update here? Can we move this to the backlog?

hyperreality · 2016-10-15T23:17:12Z

@mleanos Status:

Image cropping currently uses a now unmaintained library so this should be acted on
The search for newer libraries wasn't very fruitful, and I'm not convinced that the seeming best choice https://github.com/CrackerakiUA/ngImgCropFullExtended is good enough for MEAN.js. In particular, it seems to have poor compatibility with older browsers and smartphones.
My conclusion: rollback the cropping functionality. The other stuff in this PR is good and should be included.

I'll look into updating this tomorrow.

Additionally, I know that @lirantal said above that magic byte checking is outside the scope of the project but want to re-open a discussion on including it. Because right now, the framework is vulnerable out of the box.

mleanos · 2016-10-16T01:48:53Z

@hyperreality Thank you for the update. We're getting ready to release 0.5.0, and would like to wrap up all remaining PR's that are critical or fix bugs.

Does it seem reasonable to move it to the backlog? Or is this something that cannot wait?

@meanjs/contributors

hyperreality · 2016-10-21T21:31:36Z

@mleanos Apologies for not getting back to this in a timely fashion, it's been another busy week. Because there have been so many intervening changes I'm continuing work at #1589

Client-side MIME type checking for profile picture

ad71421

hyperreality referenced this pull request Aug 29, 2016

feat(profile): Profile picture cropping, mobile uploading and progres…

246c5d9

…s bar (#1443) * Add ng-file-upload and picture cropping * Update bower.json Remove bower dependency for angular-file-upload

Fixed multer, added test for it

b508661

hyperreality force-pushed the master branch from 4fb02b2 to b508661 Compare August 30, 2016 00:54

hyperreality changed the title ~~fix(users) Client-side MIME-type checking for picture upload~~ fix(users) MIME-type checking fixed on both client and server-side Aug 30, 2016

lirantal self-assigned this Aug 30, 2016

lirantal added this to the 0.5.0 milestone Aug 30, 2016

lirantal added the enhance:feature label Aug 30, 2016

simison mentioned this pull request Aug 30, 2016

fix(users) Fix bug file mime-type validate does not work #1272

Closed

simison reviewed Aug 30, 2016
View reviewed changes

Test for too large files

4bd849d

speed up image cropping by resizing large pics

fbd8781

hyperreality mentioned this pull request Sep 2, 2016

feat(users): Profile picture cropping, mobile uploading and progress bar #1443

Merged

Swithcing out old ngImgCrop dependency

9d91cb6

hyperreality force-pushed the master branch from d35337a to 9d91cb6 Compare September 2, 2016 16:29

simison reviewed Sep 2, 2016
View reviewed changes

mleanos mentioned this pull request Oct 15, 2016

Bad example of file type valiator for multer.fileFilter #1270

Open

hyperreality mentioned this pull request Oct 21, 2016

fix(users): Better MIME-type checking, remove image cropping library #1589

Merged

hyperreality closed this Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(users) MIME-type checking fixed on both client and server-side #1465

fix(users) MIME-type checking fixed on both client and server-side #1465

hyperreality commented Aug 29, 2016 •

edited

Loading

hyperreality commented Aug 30, 2016 •

edited

Loading

simison commented Aug 30, 2016 •

edited

Loading

lirantal commented Aug 30, 2016

lirantal commented Aug 30, 2016

simison Aug 30, 2016

hyperreality Aug 30, 2016 •

edited

Loading

simison Aug 30, 2016 •

edited

Loading

hyperreality commented Aug 30, 2016 •

edited

Loading

lirantal commented Aug 30, 2016 •

edited

Loading

hyperreality commented Aug 30, 2016

lirantal commented Aug 30, 2016

lirantal commented Aug 30, 2016

hyperreality commented Sep 1, 2016 •

edited

Loading

lirantal commented Sep 2, 2016

lirantal commented Sep 2, 2016

simison Sep 2, 2016

hyperreality Sep 2, 2016

simison Sep 2, 2016

lirantal Sep 2, 2016

hyperreality commented Sep 2, 2016 •

edited

Loading

simison commented Sep 2, 2016 •

edited

Loading

hyperreality commented Sep 2, 2016

lirantal commented Sep 2, 2016

simison commented Sep 2, 2016 •

edited

Loading

hyperreality commented Sep 2, 2016

lirantal commented Sep 4, 2016

simison commented Sep 4, 2016

lirantal commented Sep 4, 2016

hyperreality commented Sep 9, 2016

mleanos commented Oct 15, 2016

hyperreality commented Oct 15, 2016

mleanos commented Oct 16, 2016 •

edited

Loading

hyperreality commented Oct 21, 2016

fix(users) MIME-type checking fixed on both client and server-side #1465

fix(users) MIME-type checking fixed on both client and server-side #1465

Conversation

hyperreality commented Aug 29, 2016 • edited Loading

hyperreality commented Aug 30, 2016 • edited Loading

simison commented Aug 30, 2016 • edited Loading

lirantal commented Aug 30, 2016

lirantal commented Aug 30, 2016

simison Aug 30, 2016

Choose a reason for hiding this comment

hyperreality Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

simison Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

hyperreality commented Aug 30, 2016 • edited Loading

lirantal commented Aug 30, 2016 • edited Loading

hyperreality commented Aug 30, 2016

lirantal commented Aug 30, 2016

lirantal commented Aug 30, 2016

hyperreality commented Sep 1, 2016 • edited Loading

lirantal commented Sep 2, 2016

lirantal commented Sep 2, 2016

simison Sep 2, 2016

Choose a reason for hiding this comment

hyperreality Sep 2, 2016

Choose a reason for hiding this comment

simison Sep 2, 2016

Choose a reason for hiding this comment

lirantal Sep 2, 2016

Choose a reason for hiding this comment

hyperreality commented Sep 2, 2016 • edited Loading

simison commented Sep 2, 2016 • edited Loading

hyperreality commented Sep 2, 2016

lirantal commented Sep 2, 2016

simison commented Sep 2, 2016 • edited Loading

hyperreality commented Sep 2, 2016

lirantal commented Sep 4, 2016

simison commented Sep 4, 2016

lirantal commented Sep 4, 2016

hyperreality commented Sep 9, 2016

mleanos commented Oct 15, 2016

hyperreality commented Oct 15, 2016

mleanos commented Oct 16, 2016 • edited Loading

hyperreality commented Oct 21, 2016

hyperreality commented Aug 29, 2016 •

edited

Loading

hyperreality commented Aug 30, 2016 •

edited

Loading

simison commented Aug 30, 2016 •

edited

Loading

hyperreality Aug 30, 2016 •

edited

Loading

simison Aug 30, 2016 •

edited

Loading

hyperreality commented Aug 30, 2016 •

edited

Loading

lirantal commented Aug 30, 2016 •

edited

Loading

hyperreality commented Sep 1, 2016 •

edited

Loading

hyperreality commented Sep 2, 2016 •

edited

Loading

simison commented Sep 2, 2016 •

edited

Loading

simison commented Sep 2, 2016 •

edited

Loading

mleanos commented Oct 16, 2016 •

edited

Loading