search

jaydenseric / graphql-upload

Middleware and a scalar Upload to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 132 forks source link

Security issue - can kill server by using a malformed operations header #154

Closed juona closed 5 years ago

juona commented 5 years ago

Hello,

There have been issues like this one in the past, here's a new one I have found.

When sending a multipart request using some arbitrary REST client I can kill the express server. Namely, if the variants field in the operations header has a value other than an object, the server dies. A normal operations header would look something like this:

{"operationName": null, "variables": {"images": [null, null]}, "query": "mutation ($images: [Upload!]!) { uploadImages(images: $images) { id image } }"}

If you change the value of variables ({"images": [null, null]}) to any of the following, sending such request will kill the server:

Array [] and object {} work fine. The error that is thrown is this:

[0] TypeError: Cannot create property 'images' on string '123'
[0]     at set (/home/juona/dev/podbase/node_modules/object-path/index.js:118:28)
[0]     at set (/home/juona/dev/podbase/node_modules/object-path/index.js:124:14)
[0]     at set (/home/juona/dev/podbase/node_modules/object-path/index.js:104:16)
[0]     at Function.objectPath.set (/home/juona/dev/podbase/node_modules/object-path/index.js:157:14)
[0]     at Busboy.parser.on (/home/juona/dev/podbase/node_modules/graphql-upload/lib/processRequest.js:173:30)
[0]     at Busboy.emit (events.js:182:13)
[0]     at Busboy.emit (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/busboy/lib/main.js:37:33)
[0]     at PartStream.onEnd (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/busboy/lib/types/multipart.js:262:15)
[0]     at PartStream.emit (events.js:187:15)
[0]     at Dicer.onPart (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/busboy/lib/types/multipart.js:120:13)
[0]     at Dicer.emit (events.js:182:13)
[0]     at Dicer.emit (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/dicer/lib/Dicer.js:79:35)
[0]     at Dicer._oninfo (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/dicer/lib/Dicer.js:180:12)
[0]     at SBMH.<anonymous> (/home/juona/dev/podbase/node_modules/graphql-upload/node_modules/dicer/lib/Dicer.js:126:10)
[0]     at SBMH.emit (events.js:182:13)
[0]     at SBMH._sbmh_feed (/home/juona/dev/podbase/node_modules/streamsearch/lib/sbmh.js:188:10)
jaydenseric commented 5 years ago

Thanks for the report, I think the problem is here:

https://github.com/jaydenseric/graphql-upload/blob/v8.0.6/src/processRequest.mjs#L274

It's not specifically to do with variables, as the object path could be to anywhere in the operations object.

We need to detect the path is nonsense by putting a try catch around it or something, then on error return and exit with a relevant error like this:

https://github.com/jaydenseric/graphql-upload/blob/v8.0.6/src/processRequest.mjs#L267

A test needs to be added to cover this situation.

PR would be a big help!

jaydenseric commented 5 years ago

Don't worry about a PR, I'm working on a fix…

juona commented 5 years ago

Thank you, Jayden. I could have done a PR, you just reacted way sooner than I anticipated : ]

jaydenseric commented 5 years ago

Published the fix in v8.0.7 🚀