Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bower.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"angular-ui-router": "~0.2.18",
"bootstrap": "~3.3.6",
"ng-file-upload": "^12.1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, this could be changed to ~.

"ng-img-crop": "ngImgCrop#^0.3.2",
"ng-img-crop-full-extended": "ngImgCropFullExtended#~6.0.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ngImgCropFullExtended#~6.0.1 and not just ~6.0.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

"owasp-password-strength-test": "~1.3.0"
},
"overrides": {
Expand Down
4 changes: 2 additions & 2 deletions config/assets/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unminified instead of minified

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// endbower
],
js: [
Expand All @@ -18,7 +18,7 @@ module.exports = {
'public/lib/angular-animate/angular-animate.js',
'public/lib/angular-bootstrap/ui-bootstrap-tpls.js',
'public/lib/ng-file-upload/ng-file-upload.js',
'public/lib/ng-img-crop/compile/unminified/ng-img-crop.js',
'public/lib/ng-img-crop-full-extended/compile/minified/ng-img-crop.js',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better include unminified version, easier for debugging and our build process should do the minification instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'public/lib/angular-messages/angular-messages.js',
'public/lib/angular-mocks/angular-mocks.js',
'public/lib/angular-resource/angular-resource.js',
Expand Down
8 changes: 8 additions & 0 deletions config/env/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ module.exports = {
app: {
title: defaultEnvConfig.app.title + ' - Test Environment'
},
uploads: {
profileUpload: {
Copy link
Member

@simison simison Sep 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be called just —

uploads: {
  profile: {
  ...

— for simplicity? I take it's like this so that it would be easier to add other upload settings here. Otherwise it could be just one level deep.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I see that in Trustroots you simplified function names like profileUploadFileFilter too, because that function really works on any image, not just the profile uploader.

dest: './modules/users/client/img/profile/uploads/', // Profile upload destination path
limits: {
fileSize: 100000 // Limit filesize (100kb) for testing purposes
}
}
},
facebook: {
clientID: process.env.FACEBOOK_ID || 'APP_ID',
clientSecret: process.env.FACEBOOK_SECRET || 'APP_SECRET',
Expand Down
8 changes: 5 additions & 3 deletions config/lib/multer.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
'use strict';

module.exports.profileUploadFileFilter = function (req, file, cb) {
module.exports.profileUploadFileFilter = function (req, file, callback) {
if (file.mimetype !== 'image/png' && file.mimetype !== 'image/jpg' && file.mimetype !== 'image/jpeg' && file.mimetype !== 'image/gif') {
return cb(new Error('Only image files are allowed!'), false);
var err = new Error();
err.code = 'UNSUPPORTED_MEDIA_TYPE';
return callback(err, false);
}
cb(null, true);
callback(null, true);
};
5 changes: 4 additions & 1 deletion modules/core/server/controllers/errors.server.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@ exports.getErrorMessage = function (err) {
case 11001:
message = getUniqueErrorMessage(err);
break;
case 'UNSUPPORTED_MEDIA_TYPE':
message = 'Unsupported filetype';
break;
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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be a test for this, too.

Copy link
Contributor Author

@hyperreality hyperreality Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@simison simison Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break;
case 'LIMIT_UNEXPECTED_FILE':
message = 'Missing `newProfilePicture` field';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
var vm = this;

vm.user = Authentication.user;
vm.fileSelected = false;

vm.upload = function (dataUrl, name) {
vm.success = vm.error = null;
Expand All @@ -32,6 +31,14 @@
});
};

// Called after the user has selected any file and it's been cropped
vm.onFileSelection = function() {
vm.success = vm.error = vm.nowResizing = vm.pictureSelected = null;

// proceed to crop if they didn't cancel and it's a valid image file
if (vm.picFile && !vm.userForm.$error.pattern) vm.pictureSelected = true;
};

// Called after the user has successfully uploaded a new picture
function onSuccessItem(response) {
// Show success message
Expand All @@ -41,13 +48,13 @@
vm.user = Authentication.user = response;

// Reset form
vm.fileSelected = false;
vm.pictureSelected = false;
vm.progress = 0;
}

// Called after the user has failed to uploaded a new picture
function onErrorItem(response) {
vm.fileSelected = false;
vm.pictureSelected = false;

// Show error message
vm.error = response.message;
Expand Down
1 change: 1 addition & 0 deletions modules/users/client/css/users.css
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
background: #E4E4E4;
width: 300px;
height: 300px;
margin: 0 auto;
}

.social {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
<section class="row">
<div class="col-xs-offset-1 col-xs-10 col-md-offset-3 col-md-4">
<form class="signin form-horizontal">
<div class="col-xs-12 col-md-offset-3 col-md-5">
<form name="vm.userForm" class="signin form-horizontal">
<fieldset>
<div ng-show="vm.fileSelected" class="text-center form-group">
<p>Crop your picture then press upload below:</p>
<div ngf-drop ng-model="picFile" ngf-pattern="image/*" class="cropArea">
<img-crop image="picFile | ngfDataUrl" result-image="croppedDataUrl" ng-init="croppedDataUrl=''"></img-crop>
<div ng-show="vm.nowResizing" class="text-center">
<p><span class="glyphicon glyphicon-refresh glyphicon-refresh-animate"></span> Preparing for cropping...</p>
</div>
<div ng-show="vm.pictureSelected" class="text-center">
<p>Crop your picture then press upload below</p>
<div class="cropArea">
<img-crop image="vm.picFile | ngfDataUrl" result-image="croppedDataUrl" ng-init="croppedDataUrl=''" chargement="'Loading'"></img-crop>
</div>
</div>
<div class="form-group text-center">
<img ng-src="{{vm.fileSelected ? croppedDataUrl : vm.user.profileImageURL}}" alt="{{vm.user.displayName}}" class="img-thumbnail user-profile-picture" ngf-drop>
<img ng-src="{{vm.pictureSelected ? croppedDataUrl : vm.user.profileImageURL}}" alt="{{vm.user.displayName}}" class="img-thumbnail user-profile-picture">
</div>
<div ng-show="vm.userForm.$error.pattern" class="text-center text-danger">
<strong>You may only select an image file</strong>
</div>
<div ng-show="!vm.fileSelected" class="text-center form-group">
<button class="btn btn-default btn-file" ngf-select="vm.fileSelected = true; vm.success = null" ng-model="picFile" accept="image/*">Select Picture</button>
<div ng-show="!vm.pictureSelected" class="text-center form-group">
<button class="btn btn-default btn-file" ngf-select="vm.onFileSelection()" ng-model="vm.picFile" ngf-pattern="'image/*'" accept="image/*" ngf-resize="{ width:700, height:700 }" ngf-before-model-change="vm.nowResizing = true">Select Picture</button>
</div>
<div ng-show="vm.fileSelected" class="text-center form-group">
<button class="btn btn-primary" ng-click="vm.upload(croppedDataUrl, picFile.name)">Upload</button>
<button class="btn btn-default" ng-click="vm.fileSelected = false">Cancel</button>
<div ng-show="vm.pictureSelected" class="text-center form-group">
<button class="btn btn-primary" ng-click="vm.upload(croppedDataUrl, vm.picFile.name)">Upload</button>
<button class="btn btn-default" ng-click="vm.pictureSelected = false">Cancel</button>
</div>
<div ng-show="vm.fileSelected" class="progress text-center">
<div ng-show="vm.pictureSelected" class="progress text-center">
<div class="progress-bar" role="progressbar" aria-valuenow="{{vm.progress}}" aria-valuemin="0" aria-valuemax="100" style="width:{{vm.progress}}%" ng-bind="vm.progress + '%'">
<span class="sr-only">{{vm.progress}}% Complete</span>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ exports.update = function (req, res) {
*/
exports.changeProfilePicture = function (req, res) {
var user = req.user;
var upload = multer(config.uploads.profileUpload).single('newProfilePicture');
var profileUploadFileFilter = require(path.resolve('./config/lib/multer')).profileUploadFileFilter;
var existingImageUrl;

// Filtering to upload only images
upload.fileFilter = profileUploadFileFilter;
var multerConfig = config.uploads.profileUpload;
multerConfig.fileFilter = require(path.resolve('./config/lib/multer')).profileUploadFileFilter;
var upload = multer(multerConfig).single('newProfilePicture');

if (user) {
existingImageUrl = user.profileImageURL;
Expand Down
1 change: 1 addition & 0 deletions modules/users/tests/server/img/text-file.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Should not be able to upload this as a profile picture!
Binary file added modules/users/tests/server/img/too-big-file.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
40 changes: 40 additions & 0 deletions modules/users/tests/server/user.server.routes.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,46 @@ describe('User CRUD tests', function () {
});
});

it('should not be able to upload a non-image file as a profile picture', function (done) {
agent.post('/api/auth/signin')
.send(credentials)
.expect(200)
.end(function (signinErr, signinRes) {
// Handle signin error
if (signinErr) {
return done(signinErr);
}

agent.post('/api/users/picture')
.attach('newProfilePicture', './modules/users/tests/server/img/text-file.txt')
.send(credentials)
.expect(400)
.end(function (userInfoErr, userInfoRes) {
done(userInfoErr);
});
});
});

it('should not be able to change profile picture to too big of a file', function (done) {
agent.post('/api/auth/signin')
.send(credentials)
.expect(200)
.end(function (signinErr) {
// Handle signin error
if (signinErr) {
return done(signinErr);
}

agent.post('/api/users/picture')
.attach('newProfilePicture', './modules/users/tests/server/img/too-big-file.png')
.send(credentials)
.expect(400)
.end(function (userInfoErr, userInfoRes) {
done(userInfoErr);
});
});
});

it('should not be able to change profile picture if attach a picture with a different field name', function (done) {
agent.post('/api/auth/signin')
.send(credentials)
Expand Down