m4nuC / async-busboy

Promise based multipart form parser for KoaJS
MIT License
167 stars 58 forks source link

Processing unindexed array fields cause a whole process to crash #44

Open pepkin88 opened 3 years ago

pepkin88 commented 3 years ago

A common use case is to use array fields with no specified indexes, e.g.:

<input type="checkbox" name="fruits[]" value="apple">
<input type="checkbox" name="fruits[]" value="orange">
<input type="checkbox" name="fruits[]" value="banana">

However, upon receiving more than one such field, e.g. fruits[]=apple and fruits[]=banana, the whole Node process crashes with this stack trace:

RangeError: Maximum call stack size exceeded
    at Function.keys (<anonymous>)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:193:22)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)
    at reconcile ([redacted]/node_modules/async-busboy/index.js:202:12)

There are 2 problems here. I propose the following solutions:

  1. There should be no process crash, in any circumstances. Upon any error, even internal, the exception should be thrown. It seams that the underlying busboy module poorly handles errors inside registered event handlers. Suggested solution: wrap any internal field and file processing function with a try..catch, which would reject the main promise.
  2. This use case is very common and should be handled properly. And making your own parser can be difficult and sometimes even unsafe. I think, in general, TJ’s qs module could (and maybe even should) be used here for robust field parsing and aggregating. Off the top of my head:
    const qs = require('qs')
    const fieldList = [
    {name: 'fruits[]', value: 'apple'},
    {name: 'fruits[]', value: 'banana'},
    ]
    const fields = qs.parse(fieldList
    .map(({name, value}) => encodeURIComponent(name) + '=' + encodeURIComponent(value))
    .join('&'))