keystonejs / keystone-classic

Node.js CMS and web app framework
http://v4.keystonejs.com
MIT License
14.64k stars 2.21k forks source link

GET parameters are not always strings #4541

Open ttsirkia opened 6 years ago

ttsirkia commented 6 years ago

Expected behavior

req.query contains only strings.

Actual/Current behavior

There can be lists and objects.

Steps to reproduce the actual/current behavior

Create a GET request that has the same parameter twice.

Environment

https://github.com/keystonejs/keystone/blob/master/server/bindBodyParser.js#L11

Is there a reason why extended option is set to true? With this setting, a developer can never be sure that the parameter is actually a string. If a malicious user creates requests that are extended to rich objects, it is likely that the application will crash because of the req.query.param data type. At least I sometimes call methods such as toLowerCase, toUpperCase, split etc. which do not work with arrays or objects.

ttsirkia commented 6 years ago

It seems that in Keystone 4 this is needed for the Admin UI but for that is is defined here: https://github.com/keystonejs/keystone/blob/master/admin/server/app/createDynamicRouter.js#L19 Could the bindBodyParser read this option from a configuration?

This is kind of a nasty way to crash the process just by adding the same GET parameter twice to the request if the code assumes to get a simple string instead of something else.