Skip to content

Commit ccb3516

Browse files
dotansimhalutter
authored andcommitted
graph, graphql: Properly validate GraphQL queries before executing them
Uses `graphql-tools-rs` for the actual validation. Validation can be turned off by setting `DISABLE_GRAPHQL_VALIDATIONS=true` in the environment.
1 parent b216e3f commit ccb3516

File tree

6 files changed

+92
-18
lines changed

6 files changed

+92
-18
lines changed

Cargo.lock

+10
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

core/tests/interfaces.rs

+10-5
Original file line numberDiff line numberDiff line change
@@ -279,9 +279,11 @@ async fn follow_interface_reference_invalid() {
279279
.unwrap();
280280

281281
match &res.to_result().unwrap_err()[0] {
282-
QueryError::ExecutionError(QueryExecutionError::UnknownField(_, type_name, field_name)) => {
283-
assert_eq!(type_name, "Legged");
284-
assert_eq!(field_name, "parent");
282+
QueryError::ExecutionError(QueryExecutionError::ValidationError(_, error_message)) => {
283+
assert_eq!(
284+
error_message,
285+
"Cannot query field \"parent\" on type \"Legged\"."
286+
);
285287
}
286288
e => panic!("error {} is not the expected one", e),
287289
}
@@ -1424,7 +1426,7 @@ async fn recursive_fragment() {
14241426
.await
14251427
.unwrap();
14261428
let data = res.to_result().unwrap_err()[0].to_string();
1427-
assert_eq!(data, "query has fragment cycle including `FooFrag`");
1429+
assert_eq!(data, "Cannot spread fragment \"FooFrag\" within itself.");
14281430

14291431
let co_recursive = "
14301432
query {
@@ -1451,5 +1453,8 @@ async fn recursive_fragment() {
14511453
.await
14521454
.unwrap();
14531455
let data = res.to_result().unwrap_err()[0].to_string();
1454-
assert_eq!(data, "query has fragment cycle including `BarFrag`");
1456+
assert_eq!(
1457+
data,
1458+
"Cannot spread fragment \"BarFrag\" within itself via \"FooFrag\"."
1459+
);
14551460
}

graph/src/data/query/error.rs

+5
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ pub enum QueryExecutionError {
4141
AbstractTypeError(String),
4242
InvalidArgumentError(Pos, String, q::Value),
4343
MissingArgumentError(Pos, String),
44+
ValidationError(Option<Pos>, String),
4445
InvalidVariableTypeError(Pos, String),
4546
MissingVariableError(Pos, String),
4647
ResolveEntitiesError(String),
@@ -137,6 +138,7 @@ impl QueryExecutionError {
137138
| DeploymentReverted
138139
| SubgraphManifestResolveError(_)
139140
| InvalidSubgraphManifest
141+
| ValidationError(_, _)
140142
| ResultTooBig(_, _) => false,
141143
}
142144
}
@@ -161,6 +163,9 @@ impl fmt::Display for QueryExecutionError {
161163
OperationNotFound(s) => {
162164
write!(f, "Operation name not found `{}`", s)
163165
}
166+
ValidationError(_pos, message) => {
167+
write!(f, "{}", message)
168+
}
164169
NotSupported(s) => write!(f, "Not supported: {}", s),
165170
NoRootSubscriptionObjectType => {
166171
write!(f, "No root Subscription type defined in the schema")

graphql/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ crossbeam = "0.8"
88
futures01 = { package="futures", version="0.1.29" }
99
graph = { path = "../graph" }
1010
graphql-parser = "0.4.0"
11+
graphql-tools = "0.0.15"
1112
indexmap = "1.7"
1213
Inflector = "0.11.3"
1314
lazy_static = "1.2.0"

graphql/src/execution/query.rs

+55-3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
use graphql_parser::Pos;
2+
use graphql_tools::validation::rules::*;
3+
use graphql_tools::validation::validate::{validate, ValidationPlan};
4+
use lazy_static::lazy_static;
25
use std::collections::{HashMap, HashSet};
36
use std::hash::{Hash, Hasher};
47
use std::sync::Arc;
@@ -9,11 +12,10 @@ use graph::data::graphql::{
912
ext::{DocumentExt, TypeExt},
1013
ObjectOrInterface,
1114
};
15+
use graph::data::query::QueryExecutionError;
1216
use graph::data::query::{Query as GraphDataQuery, QueryVariables};
1317
use graph::data::schema::ApiSchema;
14-
use graph::prelude::{
15-
info, o, q, r, s, BlockNumber, CheapClone, Logger, QueryExecutionError, TryFromValue,
16-
};
18+
use graph::prelude::{info, o, q, r, s, BlockNumber, CheapClone, Logger, TryFromValue};
1719

1820
use crate::introspection::introspection_schema;
1921
use crate::query::{ast as qast, ext::BlockConstraint};
@@ -23,6 +25,41 @@ use crate::{
2325
schema::api::ErrorPolicy,
2426
};
2527

28+
lazy_static! {
29+
static ref GRAPHQL_VALIDATION_PLAN: ValidationPlan = ValidationPlan::from(
30+
if std::env::var("DISABLE_GRAPHQL_VALIDATIONS")
31+
.unwrap_or_else(|_| "false".into())
32+
.parse::<bool>()
33+
.unwrap_or_else(|_| false)
34+
{
35+
vec![]
36+
} else {
37+
vec![
38+
Box::new(UniqueOperationNames {}),
39+
Box::new(LoneAnonymousOperation {}),
40+
Box::new(SingleFieldSubscriptions {}),
41+
Box::new(KnownTypeNames {}),
42+
Box::new(FragmentsOnCompositeTypes {}),
43+
Box::new(VariablesAreInputTypes {}),
44+
Box::new(LeafFieldSelections {}),
45+
Box::new(FieldsOnCorrectType {}),
46+
Box::new(UniqueFragmentNames {}),
47+
Box::new(KnownFragmentNames {}),
48+
Box::new(NoUnusedFragments {}),
49+
Box::new(OverlappingFieldsCanBeMerged {}),
50+
Box::new(NoFragmentsCycle {}),
51+
Box::new(PossibleFragmentSpreads {}),
52+
Box::new(NoUnusedVariables {}),
53+
Box::new(NoUndefinedVariables {}),
54+
Box::new(KnownArgumentNames {}),
55+
Box::new(UniqueArgumentNames {}),
56+
Box::new(UniqueVariableNames {}),
57+
Box::new(ProvidedRequiredArguments {}),
58+
]
59+
}
60+
);
61+
}
62+
2663
#[derive(Clone, Debug)]
2764
pub enum ComplexityError {
2865
TooDeep,
@@ -115,6 +152,21 @@ impl Query {
115152
max_complexity: Option<u64>,
116153
max_depth: u8,
117154
) -> Result<Arc<Self>, Vec<QueryExecutionError>> {
155+
let validation_errors = validate(
156+
&schema.document(),
157+
&query.document,
158+
&GRAPHQL_VALIDATION_PLAN,
159+
);
160+
161+
if validation_errors.len() > 0 {
162+
return Err(validation_errors
163+
.into_iter()
164+
.map(|e| {
165+
QueryExecutionError::ValidationError(e.locations.first().cloned(), e.message)
166+
})
167+
.collect());
168+
}
169+
118170
let mut operation = None;
119171
let mut fragments = HashMap::new();
120172
for defn in query.document.definitions.into_iter() {

graphql/tests/query.rs

+11-10
Original file line numberDiff line numberDiff line change
@@ -895,17 +895,18 @@ fn query_complexity_subscriptions() {
895895
"subscription {
896896
musicians(orderBy: id) {
897897
name
898-
bands(first: 100, orderBy: id) {
898+
t1: bands(first: 100, orderBy: id) {
899899
name
900900
members(first: 100, orderBy: id) {
901901
name
902902
}
903903
}
904-
}
905-
__schema {
906-
types {
907-
name
908-
}
904+
t2: bands(first: 200, orderBy: id) {
905+
name
906+
members(first: 100, orderBy: id) {
907+
name
908+
}
909+
}
909910
}
910911
}",
911912
)
@@ -932,12 +933,12 @@ fn query_complexity_subscriptions() {
932933
result_size: result_size_metrics(),
933934
};
934935

935-
// The extra introspection causes the complexity to go over.
936936
let result = execute_subscription(Subscription { query }, schema, options);
937+
937938
match result {
938-
Err(SubscriptionError::GraphQLError(e)) => match e[0] {
939-
QueryExecutionError::TooComplex(1_010_200, _) => (), // Expected
940-
_ => panic!("did not catch complexity"),
939+
Err(SubscriptionError::GraphQLError(e)) => match &e[0] {
940+
QueryExecutionError::TooComplex(3_030_100, _) => (), // Expected
941+
e => panic!("did not catch complexity: {:?}", e),
941942
},
942943
_ => panic!("did not catch complexity"),
943944
}

0 commit comments

Comments
 (0)