Skip to content

Commit

Permalink
graph, graphql: Properly validate GraphQL queries before executing them
Browse files Browse the repository at this point in the history
Uses `graphql-tools-rs` for the actual validation.

Validation can be turned off by setting `DISABLE_GRAPHQL_VALIDATIONS=true` in the environment.
  • Loading branch information
dotansimha authored and lutter committed Jan 12, 2022
1 parent b216e3f commit ccb3516
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 18 deletions.
10 changes: 10 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 10 additions & 5 deletions core/tests/interfaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +279,11 @@ async fn follow_interface_reference_invalid() {
.unwrap();

match &res.to_result().unwrap_err()[0] {
QueryError::ExecutionError(QueryExecutionError::UnknownField(_, type_name, field_name)) => {
assert_eq!(type_name, "Legged");
assert_eq!(field_name, "parent");
QueryError::ExecutionError(QueryExecutionError::ValidationError(_, error_message)) => {
assert_eq!(
error_message,
"Cannot query field \"parent\" on type \"Legged\"."
);
}
e => panic!("error {} is not the expected one", e),
}
Expand Down Expand Up @@ -1424,7 +1426,7 @@ async fn recursive_fragment() {
.await
.unwrap();
let data = res.to_result().unwrap_err()[0].to_string();
assert_eq!(data, "query has fragment cycle including `FooFrag`");
assert_eq!(data, "Cannot spread fragment \"FooFrag\" within itself.");

let co_recursive = "
query {
Expand All @@ -1451,5 +1453,8 @@ async fn recursive_fragment() {
.await
.unwrap();
let data = res.to_result().unwrap_err()[0].to_string();
assert_eq!(data, "query has fragment cycle including `BarFrag`");
assert_eq!(
data,
"Cannot spread fragment \"BarFrag\" within itself via \"FooFrag\"."
);
}
5 changes: 5 additions & 0 deletions graph/src/data/query/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub enum QueryExecutionError {
AbstractTypeError(String),
InvalidArgumentError(Pos, String, q::Value),
MissingArgumentError(Pos, String),
ValidationError(Option<Pos>, String),
InvalidVariableTypeError(Pos, String),
MissingVariableError(Pos, String),
ResolveEntitiesError(String),
Expand Down Expand Up @@ -137,6 +138,7 @@ impl QueryExecutionError {
| DeploymentReverted
| SubgraphManifestResolveError(_)
| InvalidSubgraphManifest
| ValidationError(_, _)
| ResultTooBig(_, _) => false,
}
}
Expand All @@ -161,6 +163,9 @@ impl fmt::Display for QueryExecutionError {
OperationNotFound(s) => {
write!(f, "Operation name not found `{}`", s)
}
ValidationError(_pos, message) => {
write!(f, "{}", message)
}
NotSupported(s) => write!(f, "Not supported: {}", s),
NoRootSubscriptionObjectType => {
write!(f, "No root Subscription type defined in the schema")
Expand Down
1 change: 1 addition & 0 deletions graphql/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ crossbeam = "0.8"
futures01 = { package="futures", version="0.1.29" }
graph = { path = "../graph" }
graphql-parser = "0.4.0"
graphql-tools = "0.0.15"
indexmap = "1.7"
Inflector = "0.11.3"
lazy_static = "1.2.0"
Expand Down
58 changes: 55 additions & 3 deletions graphql/src/execution/query.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use graphql_parser::Pos;
use graphql_tools::validation::rules::*;
use graphql_tools::validation::validate::{validate, ValidationPlan};
use lazy_static::lazy_static;
use std::collections::{HashMap, HashSet};
use std::hash::{Hash, Hasher};
use std::sync::Arc;
Expand All @@ -9,11 +12,10 @@ use graph::data::graphql::{
ext::{DocumentExt, TypeExt},
ObjectOrInterface,
};
use graph::data::query::QueryExecutionError;
use graph::data::query::{Query as GraphDataQuery, QueryVariables};
use graph::data::schema::ApiSchema;
use graph::prelude::{
info, o, q, r, s, BlockNumber, CheapClone, Logger, QueryExecutionError, TryFromValue,
};
use graph::prelude::{info, o, q, r, s, BlockNumber, CheapClone, Logger, TryFromValue};

use crate::introspection::introspection_schema;
use crate::query::{ast as qast, ext::BlockConstraint};
Expand All @@ -23,6 +25,41 @@ use crate::{
schema::api::ErrorPolicy,
};

lazy_static! {
static ref GRAPHQL_VALIDATION_PLAN: ValidationPlan = ValidationPlan::from(
if std::env::var("DISABLE_GRAPHQL_VALIDATIONS")
.unwrap_or_else(|_| "false".into())
.parse::<bool>()
.unwrap_or_else(|_| false)
{
vec![]
} else {
vec![
Box::new(UniqueOperationNames {}),
Box::new(LoneAnonymousOperation {}),
Box::new(SingleFieldSubscriptions {}),
Box::new(KnownTypeNames {}),
Box::new(FragmentsOnCompositeTypes {}),
Box::new(VariablesAreInputTypes {}),
Box::new(LeafFieldSelections {}),
Box::new(FieldsOnCorrectType {}),
Box::new(UniqueFragmentNames {}),
Box::new(KnownFragmentNames {}),
Box::new(NoUnusedFragments {}),
Box::new(OverlappingFieldsCanBeMerged {}),
Box::new(NoFragmentsCycle {}),
Box::new(PossibleFragmentSpreads {}),
Box::new(NoUnusedVariables {}),
Box::new(NoUndefinedVariables {}),
Box::new(KnownArgumentNames {}),
Box::new(UniqueArgumentNames {}),
Box::new(UniqueVariableNames {}),
Box::new(ProvidedRequiredArguments {}),
]
}
);
}

#[derive(Clone, Debug)]
pub enum ComplexityError {
TooDeep,
Expand Down Expand Up @@ -115,6 +152,21 @@ impl Query {
max_complexity: Option<u64>,
max_depth: u8,
) -> Result<Arc<Self>, Vec<QueryExecutionError>> {
let validation_errors = validate(
&schema.document(),
&query.document,
&GRAPHQL_VALIDATION_PLAN,
);

if validation_errors.len() > 0 {
return Err(validation_errors
.into_iter()
.map(|e| {
QueryExecutionError::ValidationError(e.locations.first().cloned(), e.message)
})
.collect());
}

let mut operation = None;
let mut fragments = HashMap::new();
for defn in query.document.definitions.into_iter() {
Expand Down
21 changes: 11 additions & 10 deletions graphql/tests/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -895,17 +895,18 @@ fn query_complexity_subscriptions() {
"subscription {
musicians(orderBy: id) {
name
bands(first: 100, orderBy: id) {
t1: bands(first: 100, orderBy: id) {
name
members(first: 100, orderBy: id) {
name
}
}
}
__schema {
types {
name
}
t2: bands(first: 200, orderBy: id) {
name
members(first: 100, orderBy: id) {
name
}
}
}
}",
)
Expand All @@ -932,12 +933,12 @@ fn query_complexity_subscriptions() {
result_size: result_size_metrics(),
};

// The extra introspection causes the complexity to go over.
let result = execute_subscription(Subscription { query }, schema, options);

match result {
Err(SubscriptionError::GraphQLError(e)) => match e[0] {
QueryExecutionError::TooComplex(1_010_200, _) => (), // Expected
_ => panic!("did not catch complexity"),
Err(SubscriptionError::GraphQLError(e)) => match &e[0] {
QueryExecutionError::TooComplex(3_030_100, _) => (), // Expected
e => panic!("did not catch complexity: {:?}", e),
},
_ => panic!("did not catch complexity"),
}
Expand Down

0 comments on commit ccb3516

Please sign in to comment.