apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
576 stars 45 forks source link

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

Closed goto-bus-stop closed 11 months ago

goto-bus-stop commented 11 months ago

Fixes https://github.com/apollographql/apollo-rs/issues/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 commented 11 months ago

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!

goto-bus-stop commented 11 months ago

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.