Error on non-number limit rather than ignoring

[jdleesmiller]

Currently if you set a configuration limit, such as fileSize, to a string, the default is silently used instead.

Clearly passing a string is wrong (mea culpa!), but it is an easy mistake to make, because these limits are often set from environment variables. The result is that fileSize gets set to its default value of Infinity rather than the intended limit in the env var, leaving the app with no limit on the size of the files it would try to process.

A few seconds of searching on GitHub reveals I am not alone in making this blunder:

• https://github.com/duyk16/fullstack-app-js/blob/46019e3c541a6ea61b8c7c8fec4bbc17611ad499/server/middlewares/upload.middlerware.js#L21-L26
• https://github.com/templetonpr/fcc-file-metadata-microservice/blob/f2f9c7a8d8d1126f8a634329ee5d0bb29b5d471d/server.js#L7
• https://github.com/merq-rodriguez/geoprogram/blob/df8127fa74c420f4400161dc468f4ec2f93f4e4d/src/common/config/multer.config.ts#L18
• https://github.com/b0939261761/coffee-print-server/blob/ba46c933ebaccdc728759bd3feeaa824ac36bd7e/routes/galleries.js#L66-L70

In my case, I had an app using node-config:

limits: {
    files: 1,
    fileSize: config.get('maxFileUploadSize')
  }

which was OK when the default limit came from a JSON config file and was a number. But then one day I added an environment variable override, so config.get returned a string, which caused fileSize to actually get the default value of Infinity.

Here I've proposed making it an error to do this. Doing so without a lot of repetition required making a helper function to get the limit from config and validate it.

I think this would be considered a breaking change. Some apps that are currently running insecurely without upload limits would instead crash on boot. That's not great either, of course, but I think on balance it would be better to fail fast rather than to run silently in an unsafe way.

[jdleesmiller]

Thanks for the quick feedback @mscdex ! I have accepted code review suggestions and added the tests for null and NaN.

[jdleesmiller]

@charmander @mscdex Thanks! Now using strictEqual.