Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested field support for sanitize() #12

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 54 additions & 14 deletions lib/express_validator.js
Expand Up @@ -30,18 +30,40 @@ var validator = new Validator();
var expressValidator = function(req, res, next) {

req.updateParam = function(name, value) {
// route params like /user/:id
if (this.params && this.params.hasOwnProperty(name) &&
undefined !== this.params[name]) {
return this.params[name] = value;
}
// query string params
if (undefined !== this.query[name]) {
return this.query[name] = value;
}
// request body params via connect.bodyParser
if (this.body && undefined !== this.body[name]) {
return this.body[name] = value;
var updateList;

if (name.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit concerned about this statement: Notice that also strings have a length property, so 'abc'.length === 3.

I suppose you want to check whether name is an array?

Then rather do

if (Array.isArray(name)) {
...

Copy link
Author

Choose a reason for hiding this comment

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

The input to updateParam (name) will always be an Array, actually. I duplicated some of your code in the validate function so that all fields are represented as arrays.

This code could be further generalized, but I figured it would sacrifice readability.

if (this.params && this.params.hasOwnProperty(name[0])) {
updateList = this.params;
} else if (this.body && undefined !== this.body[name[0]]) {
updateList = this.body;
}
Copy link
Member

Choose a reason for hiding this comment

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

you are missing this.query here. Also I would prefer checking

if (this.params.hasOwnProperty(name[0])) {
...

Copy link
Author

Choose a reason for hiding this comment

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

I can certainly change this. But the reason I omit querystrings is because (as far as I know) there is no standard for arrays as querystrings.

Copy link
Member

Choose a reason for hiding this comment

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

OK, that's a valid point.


if (typeof updateList !== 'undefined') {
name.map(function(item, index) {
if (index == (name.length - 1)) {
return updateList[item] = value;
} else {
updateList = updateList[item];
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what this code-blcok does. @SirensOfTitan can you explain it to me?

Why do you use map when you're not using the result of the map. Wouldn't forEach be more appropriate?

Also what does updateList = updateList[item] do?

Copy link
Author

Choose a reason for hiding this comment

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

It essentially does something very similar to line 48 of the current master. While that code block extracts the the value from params, this code block updates the value in params after that value was sanitized.

The reason map is used is wholly to mimic the current coding style of the project, insofar that the exact same thing is done on line on line 48 of the current master.

} else {
name = name[0];

// route params like /user/:id
if (this.params && this.params.hasOwnProperty(name) &&
undefined !== this.params[name]) {
return this.params[name] = value;
}
// query string params
if (undefined !== this.query[name]) {
return this.query[name] = value;
}
// request body params via connect.bodyParser
if (this.body && undefined !== this.body[name]) {
return this.body[name] = value;
}
}
return false;
};
Expand Down Expand Up @@ -124,12 +146,30 @@ var expressValidator = function(req, res, next) {
req.filter = function(param) {
var self = this;
var filter = new Filter();
var value;

if (!Array.isArray(param)) {
param = typeof param === 'number' ?
[param] : param.split('.').filter(function(e) {
return e !== '';
});
}

// Extract value from params
param.map(function(item) {
if (value === undefined) {
value = req.param(item);
} else {
value = value[item];
}
});

filter.modify = function(str) {
this.str = str;
// Replace the param with the filtered version
self.updateParam(param, str);
};
return filter.sanitize(this.param(param));
return filter.sanitize(value);
};

// Create some aliases - might help with code readability
Expand All @@ -141,4 +181,4 @@ var expressValidator = function(req, res, next) {
};
module.exports = expressValidator;
module.exports.Validator = Validator;
module.exports.Filter = Filter;
module.exports.Filter = Filter;
72 changes: 72 additions & 0 deletions test/sanitize.js
@@ -0,0 +1,72 @@
var assert = require('assert');
var async = require('async');

var App = require('./helpers/app');
var req = require('./helpers/req');

var port = process.env.NODE_HTTP_PORT || 8888;
var url = 'http://localhost:' + port;

var validation = function(req, res) {
req.sanitize(['user', 'email']).trim();
req.sanitize('user.name').trim();
req.sanitize('field').trim();

req.assert(['user', 'email'], 'length').len(16);
req.assert(['user', 'name'], 'length').len(10);
req.assert('field', 'length').len(5);

var errors = req.validationErrors();
if (errors) {
res.json(errors);
return;
}

res.json(req.body);
};

var app = new App(port, validation);
app.start();

function fail(body) {
assert.deepEqual(body[0].msg, 'length');
assert.deepEqual(body[1].msg, 'length');
assert.deepEqual(body[2].msg, 'length');
}

function pass(body) {
assert.deepEqual(body, {
user: {
email: 'test@example.com',
name: 'John Smith'
},
field: 'field'
});
}

var tests = [
async.apply(req, 'post', url + '/', {
json: {
user: {
email: ' test@example.com ',
name: ' John Smith '
},
field: ' field '
}
}, pass),
async.apply(req, 'post', url + '/', {
json: {
user: {
email: '',
name: ''
},
field: ''
}
}, fail)
];

async.parallel(tests, function(err) {
assert.ifError(err);
app.stop();
console.log('All %d tests passed.', tests.length);
})