chjj / parted

Streaming body parser for node.js.
MIT License
63 stars 14 forks source link

Fixed bug where passing in invalid data would throw errors instead of passing them through next() #14

Closed 0chroma closed 9 years ago

0chroma commented 9 years ago

Hi!

I discovered a problem where invalid data would cause the middleware to throw rather than passing it to next().

The first part of the problem was in Parser._error() (for both affected parsers). Since Parser.destroy() resets all event listeners, and Parser._error() was calling destroy() before emitting the error, the event listeners never got notified, causing EventEmitter to just throw the error.

The second part of the problem was in the Parser class of multipart.js. Since the boundary key check was happening in the constructor, any external code wouldn't have the opportunity to listen on the error event + catch it. I fixed this by delaying the error emission to any write() calls, which seems to work for my cases but definitely let me know if you think there's a better spot to move that check.

Let me know if there's anything else you need to get this merged. Thanks!

P.S.: Here's the test case that I used to reproduce the error:

parted = require("./index");

var _request = function(type) {
  var req = new (require('stream').Stream)();

  req.headers = {
    'content-type': type
  };

  req.destroy = function() {
    return this;
  };

  return req;
};

var makeReq = function(type, stream) {
  var baddata = "dfsgkads==jhf-ga}asd?fs=&df&";

  var req = _request(type);

  var m = parted({ stream: stream });

  m(req, {}, function(err) {
    if(err) console.log("yay we got an err for "+type+"!", err);

    var obj = req.body;
    console.log(type+" parsed:", obj);

  });

  var half = baddata.length / 2 << 0;
  req.emit('data', new Buffer(baddata.slice(0, half), 'utf8'));
  req.emit('data', new Buffer(baddata.slice(half), 'utf8'));
  req.emit('end');
}

makeReq("application/x-www-form-urlencoded", true);
makeReq("application/x-www-form-urlencoded", false);
makeReq("application/json", true);
makeReq("application/json", false);
makeReq("multipart/form-data", true);
makeReq("multipart/form-data", false);
chjj commented 9 years ago

Good catch. Thanks.

0chroma commented 9 years ago

Awesome, thanks for merging so quickly? Could you tag a release + npm publish?