From 2e07a90492d3eb3d5d2957a09a92d2ccbc7cd993 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Wed, 10 Apr 2019 21:03:16 +0200 Subject: [PATCH] =?UTF-8?q?fix(users):=20hide=20sensitive=20data=20from=20?= =?UTF-8?q?errors=20=F0=9F=90=9B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- config/defaults/development.js | 17 ++++++++++++++-- lib/middlewares/model.js | 5 ++++- modules/users/models/user.schema.js | 4 ++-- modules/users/services/user.service.js | 26 ++++-------------------- modules/users/tests/user.crud.tests.js | 20 ++++++++++++++++++ modules/users/tests/user.schema.tests.js | 2 +- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/config/defaults/development.js b/config/defaults/development.js index c841c93de..564310190 100644 --- a/config/defaults/development.js +++ b/config/defaults/development.js @@ -76,8 +76,21 @@ module.exports = { host: 'localhost', port: '4200', }, - illegalUsernames: ['waos', 'weareopensource', 'administrator', 'password', 'admin', 'user', 'unknown', 'anonymous', 'null', 'undefined', 'api'], - roles: ['user', 'admin'], + // Data filter whitelist & Blacklist + blacklists: { + users: { + usernames: ['waos', 'weareopensource', 'administrator', 'password', 'admin', 'user', 'unknown', 'anonymous', 'null', 'undefined', 'api'], + }, + }, + whitelists: { + users: { + default: ['_id', 'id', 'firstName', 'lastName', 'displayName', 'username', 'email', 'roles', 'profileImageURL', 'updated', 'created', 'resetPasswordToken', 'resetPasswordExpires'], + update: ['firstName', 'lastName', 'username', 'email', 'profileImageURL'], + updateAdmin: ['firstName', 'lastName', 'username', 'email', 'profileImageURL', 'roles'], + recover: ['password', 'resetPasswordToken', 'resetPasswordExpires'], + roles: ['user', 'admin'], + }, + }, uploads: { profile: { avatar: { diff --git a/lib/middlewares/model.js b/lib/middlewares/model.js index 11735c928..c08b54f3f 100644 --- a/lib/middlewares/model.js +++ b/lib/middlewares/model.js @@ -43,7 +43,10 @@ module.exports.isValid = schema => (req, res, next) => { // Validate req.body using the schema and validation options const result = getResultFromJoi(req.body, schema, options); // if error - if (result && result.error) return responses.error(res, 422, 'schema validation error')(result.error); + if (result && result.error) { + if (result.error.original && (result.error.original.password || result.error.original.firstname)) result.error.original = _.pick(result.error.original, config.whitelists.users.default); + return responses.error(res, 422, 'schema validation error')(result.error); + } // else return req.body with the data after Joi validation req.body = result; return next(); diff --git a/modules/users/models/user.schema.js b/modules/users/models/user.schema.js index ea5021895..f8964d5b3 100644 --- a/modules/users/models/user.schema.js +++ b/modules/users/models/user.schema.js @@ -19,9 +19,9 @@ const UserSchema = Joi.object().keys({ email: Joi.string().email({ minDomainAtoms: 2 }), username: Joi.string().lowercase().trim() .regex(/^(?=[\w.-]+$)(?!.*[._-]{2})(?!\.)(?!.*\.$).{3,34}$/) - .invalid(config.illegalUsernames), + .invalid(config.blacklists.users.usernames), profileImageURL: Joi.string(), - roles: Joi.array().items(Joi.string().valid(config.roles)).min(1).default(['user']), + roles: Joi.array().items(Joi.string().valid(config.whitelists.users.roles)).min(1).default(['user']), /* Extra */ updated: Joi.date(), created: Joi.date().default(Date.now, 'current date'), diff --git a/modules/users/services/user.service.js b/modules/users/services/user.service.js index 1783011e7..0fee2e3ff 100644 --- a/modules/users/services/user.service.js +++ b/modules/users/services/user.service.js @@ -15,24 +15,6 @@ const imageFileFilter = require(path.resolve('./lib/services/multer')).imageFile const UserRepository = require('../repositories/user.repository'); const saltRounds = 10; -// update whitelist -const whitelistUpdate = ['firstName', 'lastName', 'username', 'email', 'profileImageURL']; -const whitelistUpdateAdmin = whitelistUpdate.concat(['roles']); -const whitelistRecover = ['password', 'resetPasswordToken', 'resetPasswordExpires']; -// Data filter whitelist -const whitelist = ['_id', - 'id', - 'firstName', - 'lastName', - 'displayName', - 'username', - 'email', - 'roles', - 'profileImageURL', - 'updated', - 'created', - 'resetPasswordToken', - 'resetPasswordExpires']; /** * @desc Local function to removeSensitive data from user @@ -41,7 +23,7 @@ const whitelist = ['_id', */ const removeSensitive = (user) => { if (!user || typeof user !== 'object') return null; - return _.assignIn(user, _.pick(user, whitelist)); + return _.assignIn(user, _.pick(user, config.whitelists.users.default)); }; @@ -98,9 +80,9 @@ exports.create = async (user) => { * @return {Promise} user - */ exports.update = async (user, body, option) => { - if (!option) user = _.assignIn(user, _.pick(body, whitelistUpdate)); - else if (option === 'admin') user = _.assignIn(user, _.pick(body, whitelistUpdateAdmin)); - else if (option === 'recover') user = _.assignIn(user, _.pick(body, whitelistRecover)); + if (!option) user = _.assignIn(user, _.pick(body, config.whitelists.users.update)); + else if (option === 'admin') user = _.assignIn(user, _.pick(body, config.whitelists.users.updateAdmin)); + else if (option === 'recover') user = _.assignIn(user, _.pick(body, config.whitelists.users.recover)); user.updated = Date.now(); user.displayName = `${user.firstName} ${user.lastName}`; diff --git a/modules/users/tests/user.crud.tests.js b/modules/users/tests/user.crud.tests.js index ab1e3864d..f57aa4ab5 100644 --- a/modules/users/tests/user.crud.tests.js +++ b/modules/users/tests/user.crud.tests.js @@ -104,6 +104,26 @@ describe('User CRUD Unit Tests :', () => { } }); + test('if should not be able to register, should not return sensible data', async () => { + // Init user edited + _userEdited.username = 'register_new_user'; + _userEdited.email = 'register_new_user_@test.com'; + _userEdited.password = 'azerty'; + + try { + const result = await agent.post('/api/auth/signup') + .send(_userEdited) + .expect(422); + expect(result.body.type).toBe('error'); + expect(result.body.message).toBe('schema validation error'); + expect(result.body.error.details[0].message).toBe('password Password strength score 0 does not suffice the minimum of 3'); + expect(result.body.error.original.password).toBeUndefined(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + test('should be able to register a new user ', async () => { // Init user edited _userEdited.username = 'register_new_user'; diff --git a/modules/users/tests/user.schema.tests.js b/modules/users/tests/user.schema.tests.js index 241f335b6..e917d71e8 100644 --- a/modules/users/tests/user.schema.tests.js +++ b/modules/users/tests/user.schema.tests.js @@ -419,7 +419,7 @@ describe('User Schema Unit Tests:', () => { }); test('should be able to show an error when try to valid with not allowed username', (done) => { - user.username = config.illegalUsernames[Math.floor(Math.random() * config.illegalUsernames.length)]; + user.username = config.blacklists.users.usernames[Math.floor(Math.random() * config.blacklists.users.usernames.length)]; const result = Joi.validate(user, schema.User, options); expect(typeof result).toBe('object');