diff --git a/juniper/CHANGELOG.md b/juniper/CHANGELOG.md index 1188b6bb..7d10f41e 100644 --- a/juniper/CHANGELOG.md +++ b/juniper/CHANGELOG.md @@ -2,6 +2,10 @@ - No changes yet +# [[0.15.9] 2022-02-02](https://github.com/graphql-rust/juniper/releases/tag/juniper-v0.15.9) + +- Fix infinite recursion on malformed queries with nested recursive fragments. *This is a potential denial-of-service attack vector.* Thanks to [@quapka](https://github.com/quapka) for the detailed vulnerability report and reproduction steps. + # [[0.15.8] 2022-01-26](https://github.com/graphql-rust/juniper/releases/tag/juniper-v0.15.8) - Fix panic on malformed queries with recursive fragments. *This is a potential denial-of-service attack vector.* Thanks to [@quapka](https://github.com/quapka) for the detailed vulnerability report and reproduction steps. diff --git a/juniper/src/validation/context.rs b/juniper/src/validation/context.rs index 9a02ae15..d90e7362 100644 --- a/juniper/src/validation/context.rs +++ b/juniper/src/validation/context.rs @@ -97,6 +97,10 @@ impl<'a, S: Debug> ValidatorContext<'a, S> { self.errors.push(RuleError::new(message, locations)) } + pub(crate) fn has_errors(&self) -> bool { + !self.errors.is_empty() + } + #[doc(hidden)] pub fn into_errors(mut self) -> Vec { self.errors.sort(); diff --git a/juniper/src/validation/mod.rs b/juniper/src/validation/mod.rs index 4a543528..c2759ec9 100644 --- a/juniper/src/validation/mod.rs +++ b/juniper/src/validation/mod.rs @@ -21,6 +21,7 @@ pub use self::{ #[cfg(test)] pub use self::test_harness::{ - expect_fails_rule, expect_fails_rule_with_schema, expect_passes_rule, + expect_fails_fn, expect_fails_fn_with_schema, expect_fails_rule, expect_fails_rule_with_schema, + expect_passes_fn, expect_passes_fn_with_schema, expect_passes_rule, expect_passes_rule_with_schema, }; diff --git a/juniper/src/validation/rules/mod.rs b/juniper/src/validation/rules/mod.rs index 211c1726..60a38d13 100644 --- a/juniper/src/validation/rules/mod.rs +++ b/juniper/src/validation/rules/mod.rs @@ -35,7 +35,14 @@ pub fn visit_all_rules<'a, S: Debug>(ctx: &mut ValidatorContext<'a, S>, doc: &'a where S: ScalarValue, { - let mut mv = MultiVisitorNil + // Some validators are depending on the results of other ones. + // For example, validators checking fragments usually rely on the fact that + // they have no cycles (`no_fragment_cycles`), otherwise may stall in an + // infinite recursion. So, we should run validators in stages, moving to the + // next stage only once the previous succeeds. This is better than making + // every single validator being aware of fragments cycles and/or other + // assumptions. + let mut stage1 = MultiVisitorNil .with(self::arguments_of_correct_type::factory()) .with(self::default_values_of_correct_type::factory()) .with(self::fields_on_correct_type::factory()) @@ -49,7 +56,6 @@ where .with(self::no_undefined_variables::factory()) .with(self::no_unused_fragments::factory()) .with(self::no_unused_variables::factory()) - .with(self::overlapping_fields_can_be_merged::factory()) .with(self::possible_fragment_spreads::factory()) .with(self::provided_non_null_arguments::factory()) .with(self::scalar_leafs::factory()) @@ -60,6 +66,62 @@ where .with(self::unique_variable_names::factory()) .with(self::variables_are_input_types::factory()) .with(self::variables_in_allowed_position::factory()); + visit(&mut stage1, ctx, doc); + if ctx.has_errors() { + return; + } - visit(&mut mv, ctx, doc) + let mut stage2 = MultiVisitorNil.with(self::overlapping_fields_can_be_merged::factory()); + visit(&mut stage2, ctx, doc); +} + +#[cfg(test)] +mod tests { + use crate::{parser::SourcePosition, DefaultScalarValue}; + + use crate::validation::{expect_fails_fn, RuleError}; + + #[test] + fn handles_recursive_fragments() { + expect_fails_fn::<_, DefaultScalarValue>( + super::visit_all_rules, + "fragment f on QueryRoot { ...f }", + &[ + RuleError::new( + "Fragment \"f\" is never used", + &[SourcePosition::new(0, 0, 0)], + ), + RuleError::new( + "Cannot spread fragment \"f\"", + &[SourcePosition::new(26, 0, 26)], + ), + ], + ); + } + + #[test] + fn handles_nested_recursive_fragments() { + expect_fails_fn::<_, DefaultScalarValue>( + super::visit_all_rules, + "fragment f on QueryRoot { a { ...f a { ...f } } }", + &[ + RuleError::new( + "Fragment \"f\" is never used", + &[SourcePosition::new(0, 0, 0)], + ), + RuleError::new( + r#"Unknown field "a" on type "QueryRoot""#, + &[SourcePosition::new(26, 0, 26)], + ), + RuleError::new( + "Cannot spread fragment \"f\"", + &[SourcePosition::new(30, 0, 30)], + ), + RuleError::new( + "Cannot spread fragment \"f\"", + &[SourcePosition::new(39, 0, 39)], + ), + ], + ); + } } diff --git a/juniper/src/validation/rules/overlapping_fields_can_be_merged.rs b/juniper/src/validation/rules/overlapping_fields_can_be_merged.rs index f26dcfdf..30ec9226 100644 --- a/juniper/src/validation/rules/overlapping_fields_can_be_merged.rs +++ b/juniper/src/validation/rules/overlapping_fields_can_be_merged.rs @@ -33,6 +33,7 @@ struct PairSet<'a> { data: HashMap<&'a str, HashMap<&'a str, bool>>, } +#[derive(Debug)] struct OrderedMap { data: HashMap, insert_order: Vec, @@ -172,13 +173,6 @@ impl<'a, S: Debug> OverlappingFieldsCanBeMerged<'a, S> { ); for frag_name2 in &fragment_names[i + 1..] { - // Prevent infinite fragment recursion. This case is - // caught by fragment validators, but because the validation is - // done in parallel we can't rely on fragments being - // non-recursive here. - if frag_name1 == frag_name2 { - continue; - } self.collect_conflicts_between_fragments( &mut conflicts, frag_name1, @@ -202,10 +196,8 @@ impl<'a, S: Debug> OverlappingFieldsCanBeMerged<'a, S> { ) where S: ScalarValue, { - // Prevent infinite fragment recursion. This case is - // caught by fragment validators, but because the validation is - // done in parallel we can't rely on fragments being - // non-recursive here. + // Early return on fragment recursion, as it makes no sense. + // Fragment recursions are prevented by `no_fragment_cycles` validator. if fragment_name1 == fragment_name2 { return; } @@ -293,10 +285,8 @@ impl<'a, S: Debug> OverlappingFieldsCanBeMerged<'a, S> { self.collect_conflicts_between(conflicts, mutually_exclusive, field_map, &field_map2, ctx); for fragment_name2 in fragment_names2 { - // Prevent infinite fragment recursion. This case is - // caught by fragment validators, but because the validation is - // done in parallel we can't rely on fragments being - // non-recursive here. + // Early return on fragment recursion, as it makes no sense. + // Fragment recursions are prevented by `no_fragment_cycles` validator. if fragment_name == fragment_name2 { return; } @@ -2279,26 +2269,6 @@ mod tests { ); } - #[test] - fn handles_recursive_fragments() { - expect_passes_rule_with_schema::< - _, - EmptyMutation<()>, - EmptySubscription<()>, - _, - _, - DefaultScalarValue, - >( - QueryRoot, - EmptyMutation::new(), - EmptySubscription::new(), - factory, - r#" - fragment f on Query { ...f } - "#, - ); - } - #[test] fn error_message_contains_hint_for_alias_conflict() { assert_eq!( diff --git a/juniper/src/validation/test_harness.rs b/juniper/src/validation/test_harness.rs index a2af2194..a7ff1429 100644 --- a/juniper/src/validation/test_harness.rs +++ b/juniper/src/validation/test_harness.rs @@ -1,5 +1,5 @@ use crate::{ - ast::{FromInputValue, InputValue}, + ast::{Document, FromInputValue, InputValue}, executor::Registry, parser::parse_document_source, schema::{ @@ -812,20 +812,13 @@ where } } -pub fn validate<'a, Q, M, Sub, V, F, S>( - r: Q, - m: M, - s: Sub, - q: &'a str, - factory: F, -) -> Vec +pub fn validate<'a, Q, M, Sub, F, S>(r: Q, m: M, s: Sub, q: &'a str, visit_fn: F) -> Vec where S: ScalarValue + 'a, Q: GraphQLType, M: GraphQLType, Sub: GraphQLType, - V: Visitor<'a, S> + 'a, - F: Fn() -> V, + F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document), { let mut root = RootNode::new_with_scalar_value(r, m, s); @@ -864,10 +857,7 @@ where parse_document_source(q, &root.schema).expect(&format!("Parse error on input {:#?}", q)); let mut ctx = ValidatorContext::new(unsafe { ::std::mem::transmute(&root.schema) }, &doc); - let mut mv = MultiVisitorNil.with(factory()); - visit(&mut mv, &mut ctx, unsafe { - ::std::mem::transmute(doc.as_slice()) - }); + visit_fn(&mut ctx, unsafe { ::std::mem::transmute(doc.as_slice()) }); ctx.into_errors() } @@ -881,6 +871,14 @@ where expect_passes_rule_with_schema(QueryRoot, MutationRoot, SubscriptionRoot, factory, q); } +pub fn expect_passes_fn<'a, F, S>(visit_fn: F, q: &'a str) +where + S: ScalarValue + 'a, + F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document), +{ + expect_passes_fn_with_schema(QueryRoot, MutationRoot, SubscriptionRoot, visit_fn, q); +} + pub fn expect_passes_rule_with_schema<'a, Q, M, Sub, V, F, S>( r: Q, m: M, @@ -893,9 +891,12 @@ pub fn expect_passes_rule_with_schema<'a, Q, M, Sub, V, F, S>( M: GraphQLType, Sub: GraphQLType, V: Visitor<'a, S> + 'a, - F: Fn() -> V, + F: FnOnce() -> V, { - let errs = validate(r, m, s, q, factory); + let errs = validate(r, m, s, q, move |ctx, doc| { + let mut mv = MultiVisitorNil.with(factory()); + visit(&mut mv, ctx, unsafe { ::std::mem::transmute(doc) }); + }); if !errs.is_empty() { print_errors(&errs); @@ -903,6 +904,27 @@ pub fn expect_passes_rule_with_schema<'a, Q, M, Sub, V, F, S>( } } +pub fn expect_passes_fn_with_schema<'a, Q, M, Sub, F, S>( + r: Q, + m: M, + s: Sub, + visit_fn: F, + q: &'a str, +) where + S: ScalarValue + 'a, + Q: GraphQLType, + M: GraphQLType, + Sub: GraphQLType, + F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document), +{ + let errs = validate(r, m, s, q, visit_fn); + + if !errs.is_empty() { + print_errors(&errs); + panic!("Expected `visit_fn` to pass, but errors found"); + } +} + pub fn expect_fails_rule<'a, V, F, S>(factory: F, q: &'a str, expected_errors: &[RuleError]) where S: ScalarValue + 'a, @@ -912,6 +934,14 @@ where expect_fails_rule_with_schema(QueryRoot, MutationRoot, factory, q, expected_errors); } +pub fn expect_fails_fn<'a, F, S>(visit_fn: F, q: &'a str, expected_errors: &[RuleError]) +where + S: ScalarValue + 'a, + F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document), +{ + expect_fails_fn_with_schema(QueryRoot, MutationRoot, visit_fn, q, expected_errors); +} + pub fn expect_fails_rule_with_schema<'a, Q, M, V, F, S>( r: Q, m: M, @@ -923,9 +953,18 @@ pub fn expect_fails_rule_with_schema<'a, Q, M, V, F, S>( Q: GraphQLType, M: GraphQLType, V: Visitor<'a, S> + 'a, - F: Fn() -> V, + F: FnOnce() -> V, { - let errs = validate(r, m, crate::EmptySubscription::::new(), q, factory); + let errs = validate( + r, + m, + crate::EmptySubscription::::new(), + q, + move |ctx, doc| { + let mut mv = MultiVisitorNil.with(factory()); + visit(&mut mv, ctx, unsafe { ::std::mem::transmute(doc) }); + }, + ); if errs.is_empty() { panic!("Expected rule to fail, but no errors were found"); @@ -940,6 +979,33 @@ pub fn expect_fails_rule_with_schema<'a, Q, M, V, F, S>( } } +pub fn expect_fails_fn_with_schema<'a, Q, M, F, S>( + r: Q, + m: M, + visit_fn: F, + q: &'a str, + expected_errors: &[RuleError], +) where + S: ScalarValue + 'a, + Q: GraphQLType, + M: GraphQLType, + F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document), +{ + let errs = validate(r, m, crate::EmptySubscription::::new(), q, visit_fn); + + if errs.is_empty() { + panic!("Expected `visit_fn`` to fail, but no errors were found"); + } else if errs != expected_errors { + println!("==> Expected errors:"); + print_errors(expected_errors); + + println!("\n==> Actual errors:"); + print_errors(&errs); + + panic!("Unexpected set of errors found"); + } +} + fn print_errors(errs: &[RuleError]) { for err in errs { for p in err.locations() {