Make validation execute in stages to avoid possible infinite recursion

tyranron · tyranron · commit bb35c66448b5 · 2022-02-02T18:27:08.000+02:00
diff --git 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
@@ -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<RuleError> {
         self.errors.sort();
diff --git 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
@@ -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
@@ -33,6 +33,7 @@ struct PairSet<'a> {
     data: HashMap<&'a str, HashMap<&'a str, bool>>,
 }
 
+#[derive(Debug)]
 struct OrderedMap<K, V> {
     data: HashMap<K, V>,
     insert_order: Vec<K>,
@@ -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
@@ -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<RuleError>
+pub fn validate<'a, Q, M, Sub, F, S>(r: Q, m: M, s: Sub, q: &'a str, visit_fn: F) -> Vec<RuleError>
 where
     S: ScalarValue + 'a,
     Q: GraphQLType<S, TypeInfo = ()>,
     M: GraphQLType<S, TypeInfo = ()>,
     Sub: GraphQLType<S, TypeInfo = ()>,
-    V: Visitor<'a, S> + 'a,
-    F: Fn() -> V,
+    F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document<S>),
 {
     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<S>),
+{
+    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,16 +891,40 @@ pub fn expect_passes_rule_with_schema<'a, Q, M, Sub, V, F, S>(
     M: GraphQLType<S, TypeInfo = ()>,
     Sub: GraphQLType<S, TypeInfo = ()>,
     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);
         panic!("Expected rule to pass, but errors found");
     }
 }
 
+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<S, TypeInfo = ()>,
+    M: GraphQLType<S, TypeInfo = ()>,
+    Sub: GraphQLType<S, TypeInfo = ()>,
+    F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document<S>),
+{
+    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<S>),
+{
+    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<S, TypeInfo = ()>,
     M: GraphQLType<S, TypeInfo = ()>,
     V: Visitor<'a, S> + 'a,
-    F: Fn() -> V,
-{
-    let errs = validate(r, m, crate::EmptySubscription::<S>::new(), q, factory);
+    F: FnOnce() -> V,
+{
+    let errs = validate(
+        r,
+        m,
+        crate::EmptySubscription::<S>::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<S, TypeInfo = ()>,
+    M: GraphQLType<S, TypeInfo = ()>,
+    F: FnOnce(&mut ValidatorContext<'a, S>, &'a Document<S>),
+{
+    let errs = validate(r, m, crate::EmptySubscription::<S>::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() {
