Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

support keys limit size and allow keys #29

Closed
wants to merge 2 commits into from

2 participants

fengmk2 TJ Holowaychuk
TJ Holowaychuk
Owner
tj commented

honestly i dont think anyone will use these settings, you would need to set them in so many places, and those security people even admitted they've never seen it done in practice

fengmk2

@visionmedia
If I use connect, I only put these settings into query and bodyParser middlewares if they support options argument.

e.g:

connect()
  .use(connect.query(options))
  .use(connect.bodyParser(options));

If a query string contain too many key-value pairs, and I dont want to set a request body size too small, I think these limit settings would be helpful.

fengmk2

@visionmedia
Out security member has just implement the "hash algorithm collision" in V8.

And test success in our nodejs web application using Express.

I will send the test codes to your email later, you can check it out.

TJ Holowaychuk
Owner
tj commented

im not saying it cant be done, there are many other attacks as well, im just saying they never even found it performed in practice. you could also do it with formidable, with json(), with urlencoded(), with querystring(), there are too many vectors to cover

fengmk2 fengmk2 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 71 additions and 7 deletions.
  1. +24 −7 lib/querystring.js
  2. +47 −0 test/parse.js
31 lib/querystring.js
View
@@ -107,9 +107,17 @@ function parseObject(obj){
* Parse the given str.
*/
-function parseString(str){
+function parseString(str, options) {
+ var limit = options && options.limit;
+ var keys = options && options.keys;
+ if (keys && Array.isArray(keys)) {
+ keys = {};
+ for (var i = 0, l = options.keys.length; i < l; i++) {
+ keys[options.keys[i]] = 1;
+ }
+ }
return String(str)
- .split('&')
+ .split('&', limit)
.reduce(function(ret, pair){
try{
pair = decodeURIComponent(pair.replace(/\+/g, ' '));
@@ -119,9 +127,12 @@ function parseString(str){
var eql = pair.indexOf('=')
, brace = lastBraceInKey(pair)
- , key = pair.substr(0, brace || eql)
- , val = pair.substr(brace || eql, pair.length)
- , val = val.substr(val.indexOf('=') + 1, val.length);
+ , key = pair.substr(0, brace || eql);
+ if (keys && !keys[key]) {
+ return ret;
+ }
+ var val = pair.substr(brace || eql, pair.length)
+ val = val.substr(val.indexOf('=') + 1, val.length);
// ?foo
if ('' == key) key = pair, val = '';
@@ -133,16 +144,22 @@ function parseString(str){
/**
* Parse the given query `str` or `obj`, returning an object.
*
+ * Options: (only effect on parse string)
+ *
+ * - `limit` parse string split limit.
+ * - `keys` which keys need to be parse.
+ *
* @param {String} str | {Object} obj
+ * @param {Object} options
* @return {Object}
* @api public
*/
-exports.parse = function(str){
+exports.parse = function(str, options) {
if (null == str || '' == str) return {};
return 'object' == typeof str
? parseObject(str)
- : parseString(str);
+ : parseString(str, options);
};
/**
47 test/parse.js
View
@@ -147,6 +147,53 @@ module.exports = {
qs.parse({ 'user[name]': 'tobi', 'user[email][main]': 'tobi@lb.com' })
.should.eql({ user: { name: 'tobi', email: { main: 'tobi@lb.com' } }});
+ },
+
+ 'test parse options.limit': function() {
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World', { limit: 2 })
+ .should.eql({
+ cht: 'p3',
+ chd: 't:60,40'
+ });
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World', { limit: 0 })
+ .should.eql({});
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World', { limit: false })
+ .should.eql({});
+ },
+
+ 'test parse options.keys': function() {
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World',
+ { keys: [ 'chs', 'chl' ] })
+ .should.eql({
+ chs: '250x100',
+ chl: 'Hello|World'
+ });
+ // more fast, no need to change Array to Object
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World',
+ { keys: { chs: 1, chl: 1} })
+ .should.eql({
+ chs: '250x100',
+ chl: 'Hello|World'
+ });
+ // should return {} when set {} or []
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World',
+ { keys: {} })
+ .should.eql({});
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World',
+ { keys: [] })
+ .should.eql({});
+ // should parse all keys when set !keys
+ var falseKeysList = [ false, null, undefined, 0, '' ];
+ for (var i = 0, l = falseKeysList.length; i < l; i++) {
+ var falseKeys = falseKeysList[i];
+ qs.parse('cht=p3&chd=t:60,40&chs=250x100&chl=Hello|World', { keys: falseKeys })
+ .should.eql({
+ cht: 'p3'
+ , chd: 't:60,40'
+ , chs: '250x100'
+ , chl: 'Hello|World'
+ });
+ }
}
// 'test complex': function(){
Something went wrong with that request. Please try again.