search

vektah / gqlparser

A port of the parser from graphql-js into golang
MIT License
498 stars 123 forks source link

Token limit fix CVE-2023-49559 #291

Closed uvzz closed 4 months ago

uvzz commented 9 months ago

Hi,

I added a default token limit value of 15000 and a token count inside next() function in parser.go

the limit should be configured by callers of the parser (i.e. gqlgen, genqlient, etc.). - Can you assist with that?

coveralls commented 9 months ago

Coverage Status

coverage: 88.477% (-0.1%) from 88.575% when pulling d6689addacc778a4cac572bcb2d41f1c4e6b4395 on uvzz:token-limit-fix-CVE-2023-49559 into 591c98bb315b8c840f94afbaa566dc6770f9d98c on vektah:master.

benjaminjkraft commented 9 months ago

This doesn't seem to let callers set the token limit? All the structs in question are private. We will need to figure out how to wire it in, which is a bit painful since there is no existing options API.

One kind of strange way is to make it a field of Source. It's a bit weird at first blush but it actually kinda makes sense that the parsing limit might vary from source to source. That wouldn't make it easy to set a default, and doesn't work for the main LoadQuery API -- but that one could have a default and callers who care could use ParseQuery + Validate, I think?

BTW, my opinion is still that this should be off by default; it is not a vulnerability for a parser to parse what you give it in its entirety, only for an HTTP server to parse input from the network with no limitations. (And I'm still unconvinced that request body size is not a sufficient limitation!) But I understand others might feel differently, so as long as it's possible to disable I'm fine being outvoted on that one.