From 9fe1c298892c8718c06bb19a1a8d9ac83889aabf Mon Sep 17 00:00:00 2001 From: Massimo Cairo Date: Mon, 16 Dec 2019 10:17:13 +0200 Subject: [PATCH] Validate variables of the executed operation only (#462) * Validate variables of the executed operation only * Use `unreachable!` in `validate_var_defs`. Use `unreachable!` instead of `panic!` on invalid variable types, since thay have already been checked during document validation. * Fix formatting in `validation/input_value.rs` --- juniper/CHANGELOG.md | 21 +------ juniper/src/executor/mod.rs | 83 +++++++++++++++---------- juniper/src/executor_tests/executor.rs | 5 +- juniper/src/executor_tests/variables.rs | 46 -------------- juniper/src/lib.rs | 39 +++++++----- juniper/src/validation/input_value.rs | 37 ++++++----- 6 files changed, 95 insertions(+), 136 deletions(-) diff --git a/juniper/CHANGELOG.md b/juniper/CHANGELOG.md index 55e49418..36d5c53e 100644 --- a/juniper/CHANGELOG.md +++ b/juniper/CHANGELOG.md @@ -1,24 +1,7 @@ # master -## Features - -- Support raw identifiers in field and argument names. (#[object] macro) - -## Breaking Changes - -- remove old `graphql_object!` macro, rename `object` proc macro to `graphql_object` - -- Remove deprecated `ScalarValue` custom derive (renamed to GraphQLScalarValue) - -- `graphql_union!` macro removed, replaced by `#[graphql_union]` proc macro - -- ScalarRefValue trait removed - Trait was not required. - -- Changed return type of GraphQLType::resolve to `ExecutionResult` - This was done to unify the return type of all resolver methods - The previous `Value` return type was just an internal artifact of - error handling. +- Fix incorrect validation with non-executed operations [#455](https://github.com/graphql-rust/juniper/issues/455) +- Correctly handle raw identifiers in field and argument names. # [[0.14.1] 2019-10-24](https://github.com/graphql-rust/juniper/releases/tag/juniper-0.14.1) diff --git a/juniper/src/executor/mod.rs b/juniper/src/executor/mod.rs index 8af6ab87..9c5c87f4 100644 --- a/juniper/src/executor/mod.rs +++ b/juniper/src/executor/mod.rs @@ -4,8 +4,8 @@ use fnv::FnvHashMap; use crate::{ ast::{ - Definition, Document, Fragment, FromInputValue, InputValue, OperationType, Selection, - ToInputValue, Type, + Definition, Document, Fragment, FromInputValue, InputValue, Operation, OperationType, + Selection, ToInputValue, Type, }, parser::SourcePosition, value::Value, @@ -31,6 +31,7 @@ pub use self::look_ahead::{ Applies, ChildSelection, ConcreteLookAheadSelection, LookAheadArgument, LookAheadMethods, LookAheadSelection, LookAheadValue, }; +use crate::parser::Spanning; /// A type registry used to build schemas /// @@ -652,9 +653,9 @@ impl ExecutionError { } } -pub fn execute_validated_query<'a, QueryT, MutationT, CtxT, S>( - document: Document, - operation_name: Option<&str>, +pub fn execute_validated_query<'a, 'b, QueryT, MutationT, CtxT, S>( + document: &'b Document, + operation: &'b Spanning>, root_node: &RootNode, variables: &Variables, context: &CtxT, @@ -663,38 +664,17 @@ where S: ScalarValue, QueryT: GraphQLType, MutationT: GraphQLType, + for<'c> &'c S: ScalarRefValue<'c>, { let mut fragments = vec![]; - let mut operation = None; - - for def in document { + for def in document.iter() { match def { - Definition::Operation(op) => { - if operation_name.is_none() && operation.is_some() { - return Err(GraphQLError::MultipleOperationsProvided); - } - - let move_op = operation_name.is_none() - || op.item.name.as_ref().map(|s| s.item) == operation_name; - - if move_op { - operation = Some(op); - } - } Definition::Fragment(f) => fragments.push(f), + _ => (), }; } - let op = match operation { - Some(op) => op, - None => return Err(GraphQLError::UnknownOperationName), - }; - - if op.item.operation_type == OperationType::Subscription { - return Err(GraphQLError::IsSubscription); - } - - let default_variable_values = op.item.variable_definitions.map(|defs| { + let default_variable_values = operation.item.variable_definitions.as_ref().map(|defs| { defs.item .items .iter() @@ -723,7 +703,7 @@ where final_vars = &all_vars; } - let root_type = match op.item.operation_type { + let root_type = match operation.item.operation_type { OperationType::Query => root_node.schema.query_type(), OperationType::Mutation => root_node .schema @@ -738,16 +718,16 @@ where .map(|f| (f.item.name.item, &f.item)) .collect(), variables: final_vars, - current_selection_set: Some(&op.item.selection_set[..]), + current_selection_set: Some(&operation.item.selection_set[..]), parent_selection_set: None, current_type: root_type, schema: &root_node.schema, context, errors: &errors, - field_path: FieldPath::Root(op.start), + field_path: FieldPath::Root(operation.start), }; - value = match op.item.operation_type { + value = match operation.item.operation_type { OperationType::Query => executor.resolve_into_value(&root_node.query_info, &root_node), OperationType::Mutation => { executor.resolve_into_value(&root_node.mutation_info, &root_node.mutation_type) @@ -882,6 +862,41 @@ where Ok((value, errors)) } +pub fn get_operation<'a, 'b, S>( + document: &'b Document<'b, S>, + operation_name: Option<&str>, +) -> Result<&'b Spanning>, GraphQLError<'a>> +where + S: ScalarValue, +{ + let mut operation = None; + for def in document { + match def { + Definition::Operation(op) => { + if operation_name.is_none() && operation.is_some() { + return Err(GraphQLError::MultipleOperationsProvided); + } + + let move_op = operation_name.is_none() + || op.item.name.as_ref().map(|s| s.item) == operation_name; + + if move_op { + operation = Some(op); + } + } + _ => (), + }; + } + let op = match operation { + Some(op) => op, + None => return Err(GraphQLError::UnknownOperationName), + }; + if op.item.operation_type == OperationType::Subscription { + return Err(GraphQLError::IsSubscription); + } + Ok(op) +} + impl<'r, S> Registry<'r, S> where S: ScalarValue + 'r, diff --git a/juniper/src/executor_tests/executor.rs b/juniper/src/executor_tests/executor.rs index 5dd443a0..f5310856 100644 --- a/juniper/src/executor_tests/executor.rs +++ b/juniper/src/executor_tests/executor.rs @@ -1067,7 +1067,7 @@ mod named_operations { #[crate::graphql_object_internal] impl Schema { - fn a() -> &str { + fn a(p: Option) -> &str { "b" } } @@ -1112,7 +1112,8 @@ mod named_operations { #[test] fn uses_named_operation_if_name_provided() { let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); - let doc = r"query Example { first: a } query OtherExample { second: a }"; + let doc = + r"query Example($p: String!) { first: a(p: $p) } query OtherExample { second: a }"; let vars = vec![].into_iter().collect(); diff --git a/juniper/src/executor_tests/variables.rs b/juniper/src/executor_tests/variables.rs index 6fbbc6b5..2dbb03fb 100644 --- a/juniper/src/executor_tests/variables.rs +++ b/juniper/src/executor_tests/variables.rs @@ -830,52 +830,6 @@ fn allow_non_null_lists_of_non_null_to_contain_values() { ); } -#[test] -fn does_not_allow_invalid_types_to_be_used_as_values() { - let schema = RootNode::new(TestType, EmptyMutation::<()>::new()); - - let query = r#"query q($input: TestType!) { fieldWithObjectInput(input: $input) }"#; - let vars = vec![( - "value".to_owned(), - InputValue::list(vec![InputValue::scalar("A"), InputValue::scalar("B")]), - )] - .into_iter() - .collect(); - - let error = crate::execute(query, None, &schema, &vars, &()).unwrap_err(); - - assert_eq!( - error, - ValidationError(vec![RuleError::new( - r#"Variable "$input" expected value of type "TestType!" which cannot be used as an input type."#, - &[SourcePosition::new(8, 0, 8)], - ),]) - ); -} - -#[test] -fn does_not_allow_unknown_types_to_be_used_as_values() { - let schema = RootNode::new(TestType, EmptyMutation::<()>::new()); - - let query = r#"query q($input: UnknownType!) { fieldWithObjectInput(input: $input) }"#; - let vars = vec![( - "value".to_owned(), - InputValue::list(vec![InputValue::scalar("A"), InputValue::scalar("B")]), - )] - .into_iter() - .collect(); - - let error = crate::execute(query, None, &schema, &vars, &()).unwrap_err(); - - assert_eq!( - error, - ValidationError(vec![RuleError::new( - r#"Variable "$input" expected value of type "UnknownType!" which cannot be used as an input type."#, - &[SourcePosition::new(8, 0, 8)], - ),]) - ); -} - #[test] fn default_argument_when_not_provided() { run_query(r#"{ fieldWithDefaultArgumentValue }"#, |result| { diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index be21a809..38f7e9e6 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -152,6 +152,7 @@ mod executor_tests; pub use crate::util::to_camel_case; use crate::{ + executor::{execute_validated_query, get_operation}, introspection::{INTROSPECTION_QUERY, INTROSPECTION_QUERY_WITHOUT_DESCRIPTIONS}, parser::{parse_document_source, ParseError, Spanning}, validation::{validate_input_values, visit_all_rules, ValidatorContext}, @@ -206,13 +207,6 @@ where MutationT: GraphQLType, { let document = parse_document_source(document_source, &root_node.schema)?; - { - let errors = validate_input_values(variables, &document, &root_node.schema); - - if !errors.is_empty() { - return Err(GraphQLError::ValidationError(errors)); - } - } { let mut ctx = ValidatorContext::new(&root_node.schema, &document); @@ -224,7 +218,17 @@ where } } - executor::execute_validated_query(document, operation_name, root_node, variables, context) + let operation = get_operation(&document, operation_name)?; + + { + let errors = validate_input_values(variables, operation, &root_node.schema); + + if !errors.is_empty() { + return Err(GraphQLError::ValidationError(errors)); + } + } + + execute_validated_query(document, operation_name, root_node, variables, context) } /// Execute a query in a provided schema @@ -245,14 +249,7 @@ where CtxT: Send + Sync, { let document = parse_document_source(document_source, &root_node.schema)?; - { - let errors = validate_input_values(variables, &document, &root_node.schema); - - if !errors.is_empty() { - return Err(GraphQLError::ValidationError(errors)); - } - } - + { let mut ctx = ValidatorContext::new(&root_node.schema, &document); visit_all_rules(&mut ctx, &document); @@ -263,6 +260,16 @@ where } } + let operation = get_operation(&document, operation_name)?; + + { + let errors = validate_input_values(variables, operation, &root_node.schema); + + if !errors.is_empty() { + return Err(GraphQLError::ValidationError(errors)); + } + } + executor::execute_validated_query_async(document, operation_name, root_node, variables, context) .await } diff --git a/juniper/src/validation/input_value.rs b/juniper/src/validation/input_value.rs index 02da737a..ab0ae206 100644 --- a/juniper/src/validation/input_value.rs +++ b/juniper/src/validation/input_value.rs @@ -1,9 +1,9 @@ use std::{collections::HashSet, fmt}; use crate::{ - ast::{Definition, Document, InputValue, VariableDefinitions}, + ast::{InputValue, Operation, VariableDefinitions}, executor::Variables, - parser::SourcePosition, + parser::{SourcePosition, Spanning}, schema::{ meta::{EnumMeta, InputObjectMeta, MetaType, ScalarMeta}, model::{SchemaType, TypeType}, @@ -21,7 +21,7 @@ enum Path<'a> { pub fn validate_input_values( values: &Variables, - document: &Document, + operation: &Spanning>, schema: &SchemaType, ) -> Vec where @@ -29,12 +29,8 @@ where { let mut errs = vec![]; - for def in document { - if let Definition::Operation(ref op) = *def { - if let Some(ref vars) = op.item.variable_definitions { - validate_var_defs(values, &vars.item, schema, &mut errs); - } - } + if let Some(ref vars) = operation.item.variable_definitions { + validate_var_defs(values, &vars.item, schema, &mut errs); } errs.sort(); @@ -59,22 +55,25 @@ fn validate_var_defs( errors.push(RuleError::new( &format!( r#"Variable "${}" of required type "{}" was not provided."#, - name.item, - def.var_type.item, + name.item, def.var_type.item, ), &[name.start], )); } else if let Some(v) = values.get(name.item) { - errors.append(&mut unify_value(name.item, &name.start, v, &ct, schema, Path::Root)); + errors.append(&mut unify_value( + name.item, + &name.start, + v, + &ct, + schema, + Path::Root, + )); } } - _ => errors.push(RuleError::new( - &format!( - r#"Variable "${}" expected value of type "{}" which cannot be used as an input type."#, - name.item, def.var_type.item, - ), - &[ name.start ], - )), + _ => unreachable!( + r#"Variable "${}" has invalid input type "{}" after document validation."#, + name.item, def.var_type.item, + ), } } }