Reject queries that are recursive during the resolution #2330
Conversation
There was a problem hiding this comment.
Spotted a race condition that can cause false positive recursion detections. It's not obvious to me whether this is a showstopper or not depending on how pragmatic we want to be. Let me know your thoughts. Meanwhile, I'll think about potential ways to handle this problem.
Cargo.toml
Outdated
|
|
||
| # Add caching of the RocksDB for local development | ||
| librocksdb-sys = "0.11.0+8.1.1" | ||
|
|
||
| [patch.crates-io] | ||
| librocksdb-sys = { git = "https://github.com/FuelLabs/rust-rocksdb/", branch = "release/v0.21.0" } |
There was a problem hiding this comment.
Nit: I assume we don't want to commit this part.
| # Add caching of the RocksDB for local development | |
| librocksdb-sys = "0.11.0+8.1.1" | |
| [patch.crates-io] | |
| librocksdb-sys = { git = "https://github.com/FuelLabs/rust-rocksdb/", branch = "release/v0.21.0" } |
|
|
||
| struct ValidationInner { | ||
| recursion_limit: usize, | ||
| visited: parking_lot::Mutex<HashMap<String, usize>>, |
There was a problem hiding this comment.
Is this so performance critical that we need the parking lot mutex instead of the std one? If we introduce parking lot, I assume we'd likely want to follow up with using their synchronization primitives everywhere (which could be a separate issue).
crates/fuel-core/src/graphql_api/validation_extension/visitor.rs
Outdated
Show resolved
Hide resolved
|
|
||
| struct ValidationInner { | ||
| recursion_limit: usize, | ||
| errors: Mutex<Option<Vec<RuleError>>>, |
There was a problem hiding this comment.
Do we need the option here? I.e. is it meaningful to differentiate between an empty vec of errors and no errors? Otherwise I think we can simplify this to
| errors: Mutex<Option<Vec<RuleError>>>, | |
| errors: Mutex<Vec<RuleError>>, |
Co-authored-by: Mårten Blankfors <[email protected]>
…ve-queries-during-resolution
There was a problem hiding this comment.
The code looks fine. I'm not fan of this idea at all, as having GraphQL without recursive ("graph") queries is nonsensical and just worse than a normal REST API. Have we really carried all this complexity for nothing? But given the timelines, this is the practical solution so I'm approving it anyway.
xgreenx
dismissed stale reviews from Dentosal, rymnc, and netrome
The base branch was changed.
## Version v0.39.0 ### Added - [2324](#2324): Added metrics for sync, async processor and for all GraphQL queries. - [2320](#2320): Added new CLI flag `graphql-max-resolver-recursive-depth` to limit recursion within resolver. The default value it "1". ## Fixed - [2320](#2320): Prevent `/health` and `/v1/health` from being throttled by the concurrency limiter. - [2322](#2322): Set the salt of genesis contracts to zero on execution. - [2324](#2324): Ignore peer if we already are syncing transactions from it. #### Breaking - [2320](#2330): Reject queries that are recursive during the resolution of the query. ### Changed #### Breaking - [2311](#2311): Changed the text of the error returned by the executor if gas overflows. ## What's Changed * chore(executor): instrument errors to be more meaningful by @rymnc in #2311 * fix(dummy_da_block_costs): remove dependency on polling_interval and use channel instead by @rymnc in #2314 * fix(txpool): Error in GossipsubMessageAcceptance variant docstrings by @netrome in #2316 * refactor: Eager returns in txpool_v2::service::Task::run by @netrome in #2325 * fix(api_service): exclude health checks from throttling with ConcurrencyLimitLayer by @rymnc in #2320 * Remove the `normalize_rewards_and_costs()` function by @rafal-ch in #2293 * fix(genesis): set salt of contract on execution of genesis state configuration by @rymnc in #2322 * Fixing the issue with duplicate transaction synchronization processes by @xgreenx in #2324 * Reject queries that are recursive during the resolution by @xgreenx in #2330 **Full Changelog**: v0.38.0...v0.39.0
If during resolution of the query we hit the same type, reject further execution. The code to do this check uses
Visitorlogic from theasync-graphqllibrary, but this logic is private, so I duplicated it in our branch, Later it would be nice to create PR to them in make it public. Or add a way to pass our own query validators into theasync-graphql(which will be more performant).Checklist
Before requesting review