Skip to content

Commit

Permalink
fix(users): hide sensitive data from errors 🐛
Browse files Browse the repository at this point in the history
  • Loading branch information
PierreBrisorgueil committed Apr 10, 2019
1 parent cdc06e4 commit 2e07a90
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 28 deletions.
17 changes: 15 additions & 2 deletions config/defaults/development.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
5 changes: 4 additions & 1 deletion lib/middlewares/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions modules/users/models/user.schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
26 changes: 4 additions & 22 deletions modules/users/services/user.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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));
};


Expand Down Expand Up @@ -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}`;
Expand Down
20 changes: 20 additions & 0 deletions modules/users/tests/user.crud.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
2 changes: 1 addition & 1 deletion modules/users/tests/user.schema.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down

0 comments on commit 2e07a90

Please sign in to comment.