-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Token limit fix CVE-2023-49559 #291
Token limit fix CVE-2023-49559 #291
Conversation
…previous directive limit
coverage: 88.477% (-0.1%) from 88.575% |
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 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. |
@@ -20,7 +20,8 @@ func ParseSchemas(inputs ...*Source) (*SchemaDocument, error) { | |||
|
|||
func ParseSchema(source *Source) (*SchemaDocument, error) { | |||
p := parser{ | |||
lexer: lexer.New(source), | |||
lexer: lexer.New(source), | |||
maxTokenLimit: 15000, // default value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be 0 too I think? especially since parsing a schema is usually if anything the safer operation
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?