fix(compiler): apply limits to recursive functions in validation (#748)

* chore(compiler): add a failing test for #742

* Add max depth to RecursionStack

* Use the CycleError with path for directive and input object cycles

* Reuse length of `RecursionGuard::seen` set as the current recursion depth

* Add limit when recursively walking selection set

* Add `ValidationOptions` structure to pass in recursion limit

Possibly also a diagnostic limit in the future.

Maybe there would be options that could differ between executable and
schema validation, but not right now

* Revert "Add `ValidationOptions` structure to pass in recursion limit"

This reverts commit 7206c61.

* Hardcode recursion limit

* Add a passing fragment chain test; add directive chain tests; lower recursion limit

* Test input objects

* chglg

* Add an error type for hitting recursion limit

* Track max recursion depth in RecursionStack

* Lower limits: 32 for directives and types, 100 for fragments and selections

* update limit in chglg

crates/apollo-compiler/CHANGELOG.md

@@ -130,6 +130,19 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 [pull/724]: https://github.com/apollographql/apollo-rs/pull/724
 [pull/752]: https://github.com/apollographql/apollo-rs/pull/752
 
+## Fixes
+
+- **Limit recursion in validation - [goto-bus-stop], [pull/748] fixing [issue/742]**
+  Validation now bails out of very long chains of definitions that refer to each other,
+  even if they don't strictly form a cycle. These could previously cause extremely long validation
+  times or stack overflows.
+
+  The limit for input objects and directives is set at 32. For fragments, the limit is set at 100.
+  Based on our datasets, real-world documents don't come anywhere close to this.
+
+[goto-bus-stop]: https://github.com/goto-bus-stop
+[issue/742]: https://github.com/apollographql/apollo-rs/issues/742
+[pull/748]: https://github.com/apollographql/apollo-rs/pull/748
 
 # [1.0.0-beta.7](https://crates.io/crates/apollo-compiler/1.0.0-beta.7) - 2023-11-17
 

crates/apollo-compiler/src/diagnostics.rs

@@ -129,6 +129,8 @@ pub(crate) enum DiagnosticData {
     RecursiveInputObjectDefinition { name: String },
     #[error("`{name}` fragment cannot reference itself")]
     RecursiveFragmentDefinition { name: String },
+    #[error("`{name}` contains too much nesting")]
+    DeeplyNestedType { name: String },
     #[error("type does not satisfy interface `{interface}`: missing field `{field}`")]
     MissingInterfaceField { interface: String, field: String },
     #[error("the required argument `{name}` is not provided")]
@@ -238,6 +240,8 @@ pub(crate) enum DiagnosticData {
         /// Name of the argument where variable is used
         arg_name: String,
     },
+    #[error("too much recursion")]
+    RecursionError {},
 }
 
 impl ApolloDiagnostic {

crates/apollo-compiler/src/validation/directive.rs

@@ -3,6 +3,8 @@ use crate::validation::{NodeLocation, RecursionGuard, RecursionStack};
 use crate::{ast, schema, Node, ValidationDatabase};
 use std::collections::{HashMap, HashSet};
 
+use super::CycleError;
+
 /// This struct just groups functions that are used to find self-referential directives.
 /// The way to use it is to call `FindRecursiveDirective::check`.
 struct FindRecursiveDirective<'s> {
@@ -14,7 +16,7 @@ impl FindRecursiveDirective<'_> {
         &self,
         seen: &mut RecursionGuard<'_>,
         def: &schema::ExtendedType,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         match def {
             schema::ExtendedType::Scalar(scalar_type_definition) => {
                 self.directives(seen, &scalar_type_definition.directives)?;
@@ -49,7 +51,7 @@ impl FindRecursiveDirective<'_> {
         &self,
         seen: &mut RecursionGuard<'_>,
         input_value: &Node<ast::InputValueDefinition>,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         for directive in &input_value.directives {
             self.directive(seen, directive)?;
         }
@@ -66,7 +68,7 @@ impl FindRecursiveDirective<'_> {
         &self,
         seen: &mut RecursionGuard<'_>,
         enum_value: &Node<ast::EnumValueDefinition>,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         for directive in &enum_value.directives {
             self.directive(seen, directive)?;
         }
@@ -78,7 +80,7 @@ impl FindRecursiveDirective<'_> {
         &self,
         seen: &mut RecursionGuard<'_>,
         directives: &[schema::Component<ast::Directive>],
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         for directive in directives {
             self.directive(seen, directive)?;
         }
@@ -89,17 +91,18 @@ impl FindRecursiveDirective<'_> {
         &self,
         seen: &mut RecursionGuard<'_>,
         directive: &Node<ast::Directive>,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         if !seen.contains(&directive.name) {
             if let Some(def) = self.schema.directive_definitions.get(&directive.name) {
-                self.directive_definition(seen.push(&directive.name), def)?;
+                self.directive_definition(seen.push(&directive.name)?, def)
+                    .map_err(|error| error.trace(directive))?;
             }
         } else if seen.first() == Some(&directive.name) {
             // Only report an error & bail out early if this is the *initial* directive.
             // This prevents raising confusing errors when a directive `@b` which is not
             // self-referential uses a directive `@a` that *is*. The error with `@a` should
             // only be reported on its definition, not on `@b`'s.
-            return Err(directive.clone());
+            return Err(CycleError::Recursed(vec![directive.clone()]));
         }
 
         Ok(())
@@ -109,7 +112,7 @@ impl FindRecursiveDirective<'_> {
         &self,
         mut seen: RecursionGuard<'_>,
         def: &Node<ast::DirectiveDefinition>,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         for input_value in &def.arguments {
             self.input_value(&mut seen, input_value)?;
         }
@@ -120,7 +123,7 @@ impl FindRecursiveDirective<'_> {
     fn check(
         schema: &schema::Schema,
         directive_def: &Node<ast::DirectiveDefinition>,
-    ) -> Result<(), Node<ast::Directive>> {
+    ) -> Result<(), CycleError<ast::Directive>> {
         let mut recursion_stack = RecursionStack::with_root(directive_def.name.clone());
         FindRecursiveDirective { schema }
             .directive_definition(recursion_stack.guard(), directive_def)
@@ -143,22 +146,59 @@ pub(crate) fn validate_directive_definition(
     // references itself directly.
     //
     // Returns Recursive Definition error.
-    if let Err(directive) = FindRecursiveDirective::check(&db.schema(), &def) {
-        let definition_location = def.location();
-        let head_location = NodeLocation::recompose(def.location(), def.name.location());
-        let directive_location = directive.location();
-
-        diagnostics.push(
-            ApolloDiagnostic::new(
+    match FindRecursiveDirective::check(&db.schema(), &def) {
+        Ok(_) => {}
+        Err(CycleError::Recursed(trace)) => {
+            let definition_location = def.location();
+            let head_location = NodeLocation::recompose(def.location(), def.name.location());
+            let mut diagnostic = ApolloDiagnostic::new(
                 db,
                 definition_location,
                 DiagnosticData::RecursiveDirectiveDefinition {
                     name: def.name.to_string(),
                 },
             )
-            .label(Label::new(head_location, "recursive directive definition"))
-            .label(Label::new(directive_location, "refers to itself here")),
-        );
+            .label(Label::new(head_location, "recursive directive definition"));
+
+            if let Some((cyclical_application, path)) = trace.split_first() {
+                let mut prev_name = &def.name;
+                for directive_application in path.iter().rev() {
+                    diagnostic = diagnostic.label(Label::new(
+                        directive_application.location(),
+                        format!(
+                            "`{}` references `{}` here...",
+                            prev_name, directive_application.name
+                        ),
+                    ));
+                    prev_name = &directive_application.name;
+                }
+
+                diagnostic = diagnostic.label(Label::new(
+                    cyclical_application.location(),
+                    format!(
+                        "`{}` circularly references `{}` here",
+                        prev_name, cyclical_application.name
+                    ),
+                ));
+            }
+
+            diagnostics.push(diagnostic);
+        }
+        Err(CycleError::Limit(_)) => {
+            let diagnostic = ApolloDiagnostic::new(
+                db,
+                def.location(),
+                DiagnosticData::DeeplyNestedType {
+                    name: def.name.to_string(),
+                },
+            )
+            .label(Label::new(
+                def.location(),
+                "directive references a very long chain of directives",
+            ));
+
+            diagnostics.push(diagnostic);
+        }
     }
 
     diagnostics

crates/apollo-compiler/src/validation/fragment.rs

@@ -1,10 +1,9 @@
 use crate::diagnostics::{ApolloDiagnostic, DiagnosticData, Label};
-use crate::validation::{FileId, NodeLocation, RecursionGuard, RecursionStack};
+use crate::validation::operation::OperationValidationConfig;
+use crate::validation::{CycleError, FileId, NodeLocation, RecursionGuard, RecursionStack};
 use crate::{ast, schema, Node, ValidationDatabase};
 use std::collections::{HashMap, HashSet};
 
-use super::operation::OperationValidationConfig;
-
 /// Given a type definition, find all the type names that can be used for fragment spreading.
 ///
 /// Spec: https://spec.graphql.org/October2021/#GetPossibleTypes()
@@ -300,13 +299,13 @@ pub(crate) fn validate_fragment_cycles(
         named_fragments: &HashMap<ast::Name, Node<ast::FragmentDefinition>>,
         selection_set: &[ast::Selection],
         visited: &mut RecursionGuard<'_>,
-    ) -> Result<(), Vec<Node<ast::FragmentSpread>>> {
+    ) -> Result<(), CycleError<ast::FragmentSpread>> {
         for selection in selection_set {
             match selection {
                 ast::Selection::FragmentSpread(spread) => {
                     if visited.contains(&spread.fragment_name) {
                         if visited.first() == Some(&spread.fragment_name) {
-                            return Err(vec![spread.clone()]);
+                            return Err(CycleError::Recursed(vec![spread.clone()]));
                         }
                         continue;
                     }
@@ -315,12 +314,9 @@ pub(crate) fn validate_fragment_cycles(
                         detect_fragment_cycles(
                             named_fragments,
                             &fragment.selection_set,
-                            &mut visited.push(&fragment.name),
+                            &mut visited.push(&fragment.name)?,
                         )
-                        .map_err(|mut trace| {
-                            trace.push(spread.clone());
-                            trace
-                        })?;
+                        .map_err(|error| error.trace(spread))?;
                     }
                 }
                 ast::Selection::InlineFragment(inline) => {
@@ -335,45 +331,63 @@ pub(crate) fn validate_fragment_cycles(
         Ok(())
     }
 
-    let mut visited = RecursionStack::with_root(def.name.clone());
+    let mut visited = RecursionStack::with_root(def.name.clone()).with_limit(100);
 
-    if let Err(cycle) =
-        detect_fragment_cycles(&named_fragments, &def.selection_set, &mut visited.guard())
-    {
-        let head_location = NodeLocation::recompose(def.location(), def.name.location());
+    match detect_fragment_cycles(&named_fragments, &def.selection_set, &mut visited.guard()) {
+        Ok(_) => {}
+        Err(CycleError::Recursed(trace)) => {
+            let head_location = NodeLocation::recompose(def.location(), def.name.location());
 
-        let mut diagnostic = ApolloDiagnostic::new(
-            db,
-            def.location(),
-            DiagnosticData::RecursiveFragmentDefinition {
-                name: def.name.to_string(),
-            },
-        )
-        .label(Label::new(head_location, "recursive fragment definition"));
+            let mut diagnostic = ApolloDiagnostic::new(
+                db,
+                def.location(),
+                DiagnosticData::RecursiveFragmentDefinition {
+                    name: def.name.to_string(),
+                },
+            )
+            .label(Label::new(head_location, "recursive fragment definition"));
+
+            if let Some((cyclical_spread, path)) = trace.split_first() {
+                let mut prev_name = &def.name;
+                for spread in path.iter().rev() {
+                    diagnostic = diagnostic.label(Label::new(
+                        spread.location(),
+                        format!(
+                            "`{}` references `{}` here...",
+                            prev_name, spread.fragment_name
+                        ),
+                    ));
+                    prev_name = &spread.fragment_name;
+                }
 
-        if let Some((cyclical_spread, path)) = cycle.split_first() {
-            let mut prev_name = &def.name;
-            for spread in path.iter().rev() {
                 diagnostic = diagnostic.label(Label::new(
-                    spread.location(),
+                    cyclical_spread.location(),
                     format!(
-                        "`{}` references `{}` here...",
-                        prev_name, spread.fragment_name
+                        "`{}` circularly references `{}` here",
+                        prev_name, cyclical_spread.fragment_name
                     ),
                 ));
-                prev_name = &spread.fragment_name;
             }
 
-            diagnostic = diagnostic.label(Label::new(
-                cyclical_spread.location(),
-                format!(
-                    "`{}` circularly references `{}` here",
-                    prev_name, cyclical_spread.fragment_name
-                ),
-            ));
+            diagnostics.push(diagnostic);
         }
+        Err(CycleError::Limit(_)) => {
+            let head_location = NodeLocation::recompose(def.location(), def.name.location());
 
-        diagnostics.push(diagnostic);
+            let diagnostic = ApolloDiagnostic::new(
+                db,
+                def.location(),
+                DiagnosticData::DeeplyNestedType {
+                    name: def.name.to_string(),
+                },
+            )
+            .label(Label::new(
+                head_location,
+                "fragment references a very long chain of fragments",
+            ));
+
+            diagnostics.push(diagnostic);
+        }
     }
 
     diagnostics