Skip to content

Security: Guard against stack overflow for deeply nested input #43

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

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

That3Percent
Copy link
Contributor

This prevents a stack overflow when parsing deeply nested input.

The default depth is hardcoded to 50, but a non-public API allows the value to be overridden. How to surface this (eg: overloads, or an Options object) is left to the core maintainers.

@That3Percent
Copy link
Contributor Author

I made a couple of other small changes along the way that were not necessary, and will describe the rationale here.

peek_token was renamed to take_token because typically the word peek when using a queue or similar refers to looking at what the next item is, without actually popping the item. This method does "pop" the token because the offset in the input is advanced. Because of this, I thought peek_token was confusing.

The common pattern of advancing the column, offset, and returning a token with the same length was moved into the method advance_token. This was done to avoid copy-pasting that code into the newly added branches that handle recursion.

@tailhook tailhook merged commit 45167b5 into graphql-rust:master Nov 16, 2020
@tailhook
Copy link
Collaborator

Merged. Thanks!

Do you know where 'Expected ]' comes from? This is not critical, but I think a bit weird to see in this message.

@That3Percent
Copy link
Contributor Author

I don't know particularly where the ] comes from, but interestingly it is a continuation character that would have prevented the recursion limit from being exceeded at that position. Whatever error paradigm exists above the tokenizer is working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants