tj / node-querystring

querystring parser for node and the browser - supporting nesting (used by Express, Connect, etc)
MIT License
455 stars 66 forks source link

Fix possibility to create huge arrays of null objects. #60

Closed abecciu closed 11 years ago

abecciu commented 11 years ago

This fixes a serious bug. Someone hitting an app with a param like a[999999999]=1 could make a node process crash by running out memory very quickly.

Example:

var qs = require('qs');

console.log(qs.parse('a[999999999]=1'));
abecciu commented 11 years ago

@visionmedia any feedback on this?

tj commented 11 years ago

hmmm they're sparse internally, for example qs.parse('foo[100000000000000000000000000000000000000000000]=1') wont exhaust memory, however for iteration and console.log() this is obviously not ideal :D haha I'll check out the fix

tj commented 11 years ago

instead of altering the already super messy functions a bunch we could wrap the parseObject response with something like:

function sparse(obj) {
  if (Array.isArray(obj)) {
    var ret = [];
    for (var i in obj) ret.push(obj[i]);
    return ret;
  }

  for (var key in obj) {
    obj[key] = sparse(obj[key]);
  }

  return obj;
}

and make sure it recurses properly

abecciu commented 11 years ago

@visionmedia sounds like a very clean solution. However, if I understand correctly, you'd have to iterate over potentially very long arrays. And that consumes a lot of cpu time.

tj commented 11 years ago

@abecciu for/in just iterates the hash keys

abecciu commented 11 years ago

Oh didn't know that it works like that even in arrays. Great then! I'll make the change.

abecciu commented 11 years ago

@visionmedia done! Way cleaner now. Thanks for the feedback! :)

tj commented 11 years ago

merged! thanks man

caligo-mentis commented 11 years ago

So. This PR breaks my production code when I try to send array which starts with non-zero index and expect what non-defined array elements will be undefined. How should I send empty array element to server now? Of course, I can send JSON instead of form data but this behavior of querystring library was not expected.