Skip to content
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

fix(compiler): apply limits to recursive functions in validation #748

Merged
merged 16 commits into from
Nov 29, 2023

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Nov 17, 2023

Fixes #742

The validation code already had protections for cycles. However if you provided a very long chain of types or fragments referencing each other, it could chase each reference and still cause a stack overflow. This does pretty much require malicious input, but the failure mode is quite bad, so it's worth protecting against.

Self-referential directives and input objects now also show the chain that was followed in diagnostics. The directives one can be a bit incomplete because there may be a different kind of node in between (eg there might be a directive applied to an enum used inside an input type used as an argument to the same directive... and only directive names are tracked in the chain.) I think we can improve that with other diagnostics work soon, this is already an improvement.

@goto-bus-stop goto-bus-stop marked this pull request as ready for review November 21, 2023 17:04
Possibly also a diagnostic limit in the future.

Maybe there would be options that could differ between executable and
schema validation, but not right now
Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let’s merge this with an additional test (see inline comment) and a changelog entry

#[test]
fn long_fragment_chains_do_not_overflow_stack() {
// Build a query that applies thousands of fragments
// Validating it would take a lot of recursion and blow the stack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it you’ve manually reproduced the stack overflow when the limit is set too high?

Please also add a similar test that goes right under the limit and succeeds.

@goto-bus-stop
Copy link
Member Author

Also added tests for input objects and directives. Directives in particular didn't cause a stack overflow even with thousands of definitions, but they did take over 60 seconds to validate, so the limit also helps with that.

I lowered the limit to 200 because fragments could still overflow with 500 (the cycles detection works with 500, but the recursion in validate_fragment_definition is more expensive as it cycles through a few different and bigger methods).
Thinking about the serde_json limit, 200 is still a lot!


let expected = expect_test::expect![[r#"
Error: too much recursion
Error: `typeFragment1` fragment cannot reference itself
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is not actually recursive, right? Shouldn’t there be a different error message for reaching the limit?

Comment on lines 115 to 117
// The final 199 directives do not cause recursion errors because the chain is less than 200
// directives deep.
assert_eq!(partial.errors.len(), 801);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, do we restart following the whole chain for every step in it? Quadratic time is why it took 60 seconds… Maybe this limit should be much lower still

Copy link
Member Author

@goto-bus-stop goto-bus-stop Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dooo. Fragment definitions are validated when they are used, but for directives and input objects each definition is validated, including for recursion depth. (would be a good use case for caching hehe)

Long fragment chains do happen in the wild, but long input object chains and directive chains might not, so we could have a low limit for those. The risk is lower for directives and input objects because they are in the schema and not likely to be provided over the internet by a malicious user.

@goto-bus-stop
Copy link
Member Author

Lowered the limits more after trying many real-world inputs. I basically found no practical example of deeply nested input objects (for the purposes of schema validation, only non-null fields are chased, which are very rare) or nested directive applications (found no example at all, which makes sense to me, i've never seen someone take complicated arguments in directives aside from custom scalars). 32 is still an order of magnitude above what I've seen :)

For fragments I don't have great data, my suspicion is that fragment chains can be long in practice, but like...5 or 6 would already be quite long. I put that limit at 100 so it won't trigger even on extreme use cases but not grind to a halt.

@goto-bus-stop goto-bus-stop merged commit 67f8f91 into main Nov 29, 2023
11 checks passed
@goto-bus-stop goto-bus-stop deleted the deeply-nested-references branch November 29, 2023 16:04
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.

Apply recursion limit in validation
2 participants