Repository: apollographql/apollo-rs
Pull request #733 (2023-11-09 14:08:25)

Fix validation stack overflow for fragment cycles inside nested fields
Previously fragment cycle detection only counted fragments that are
directly recursive, where the top-level selection for the fragment
included a fragment spread of itself. But spreading a fragment anywhere
inside a nested field is also problematic and prohibited by spec,
because even if the field you're spreading into is nullable, the client
can't know how deep that recursion goes, and it could be infinite.

Fixes https://github.com/apollographql/apollo-rs/issues/716



Patch:
diff --git a/crates/apollo-compiler/src/validation/fragment.rs b/crates/apollo-compiler/src/validation/fragment.rs
index c92e3d9..d1b4b03 100644
--- a/crates/apollo-compiler/src/validation/fragment.rs
+++ b/crates/apollo-compiler/src/validation/fragment.rs
@@ -298,17 +298,19 @@ pub(crate) fn validate_fragment_cycles(
     // synthetic trees.
     let named_fragments = db.ast_named_fragments(file_id);
 
+    /// If a fragment spread is recursive, returns a vec containing the spread that refers back to
+    /// the original fragment, and a trace of each fragment spread back to the original fragment.
     fn detect_fragment_cycles(
         named_fragments: &HashMap<ast::Name, Node<ast::FragmentDefinition>>,
         selection_set: &[ast::Selection],
         visited: &mut RecursionGuard<'_>,
-    ) -> Result<(), ast::Selection> {
+    ) -> Result<(), Vec<Node<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(selection.clone());
+                            return Err(vec![spread.clone()]);
                         }
                         continue;
                     }
@@ -318,13 +320,19 @@ pub(crate) fn validate_fragment_cycles(
                             named_fragments,
                             &fragment.selection_set,
                             &mut visited.push(&fragment.name),
-                        )?;
+                        )
+                        .map_err(|mut trace| {
+                            trace.push(spread.clone());
+                            trace
+                        })?;
                     }
                 }
                 ast::Selection::InlineFragment(inline) => {
                     detect_fragment_cycles(named_fragments, &inline.selection_set, visited)?;
                 }
-                _ => {}
+                ast::Selection::Field(field) => {
+                    detect_fragment_cycles(named_fragments, &field.selection_set, visited)?;
+                }
             }
         }
 
@@ -338,17 +346,38 @@ pub(crate) fn validate_fragment_cycles(
     {
         let head_location = NodeLocation::recompose(def.location(), def.name.location());
 
-        diagnostics.push(
-            ApolloDiagnostic::new(
-                db,
-                def.location(),
-                DiagnosticData::RecursiveFragmentDefinition {
-                    name: def.name.to_string(),
-                },
-            )
-            .label(Label::new(head_location, "recursive fragment definition"))
-            .label(Label::new(cycle.location(), "refers to itself here")),
-        );
+        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)) = cycle.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;
+            }
+
+            diagnostic = diagnostic.label(Label::new(
+                cyclical_spread.location(),
+                format!(
+                    "`{}` circularly references `{}` here",
+                    prev_name, cyclical_spread.fragment_name
+                ),
+            ));
+        }
+
+        diagnostics.push(diagnostic);
     }
 
     diagnostics


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql
index 57bb3a9..08733dc 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql
@@ -9,7 +9,16 @@ type Query {
 # Query
 {
   human { ...fragA }
+  __schema { types { ...cycle1 } }
 }
 fragment fragA on Human { name, ...fragB }
 fragment fragB on Human { name, ...fragC }
 fragment fragC on Human { name, ...fragA }
+
+# Indirect cycle
+fragment cycle1 on __Type { kind, ...cycle2 }
+
+fragment cycle2 on __Type {
+  kind
+  ofType { ...cycle1 }
+}
diff --git a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt
index 680c43a..ab0b539 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt
@@ -1,12 +1,29 @@
 Error: `fragA` fragment cannot reference itself
-    ╭─[0092_recursive_fragment_spread.graphql:13:1]
+    ╭─[0092_recursive_fragment_spread.graphql:14:1]
     │
- 13 │ fragment fragA on Human { name, ...fragB }
-    │ ───────┬──────  
-    │        ╰──────── recursive fragment definition
-    │ 
- 15 │ fragment fragC on Human { name, ...fragA }
+ 14 │ fragment fragA on Human { name, ...fragB }
+    │ ───────┬──────                  ────┬───  
+    │        ╰────────────────────────────────── recursive fragment definition
+    │                                     │     
+    │                                     ╰───── `fragA` references `fragB` here...
+ 15 │ fragment fragB on Human { name, ...fragC }
+    │                                 ────┬───  
+    │                                     ╰───── `fragB` references `fragC` here...
+ 16 │ fragment fragC on Human { name, ...fragA }
     │                                 ────┬───  
-    │                                     ╰───── refers to itself here
+    │                                     ╰───── `fragC` circularly references `fragA` here
+────╯
+Error: `cycle1` fragment cannot reference itself
+    ╭─[0092_recursive_fragment_spread.graphql:19:1]
+    │
+ 19 │ fragment cycle1 on __Type { kind, ...cycle2 }
+    │ ───────┬───────                   ────┬────  
+    │        ╰───────────────────────────────────── recursive fragment definition
+    │                                       │      
+    │                                       ╰────── `cycle1` references `cycle2` here...
+    │ 
+ 23 │   ofType { ...cycle1 }
+    │            ────┬────  
+    │                ╰────── `cycle2` circularly references `cycle1` here
 ────╯
 
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql
index 4a50fd3..4280aa7 100644
--- a/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql
@@ -10,6 +10,11 @@ query {
   human {
     ...fragA
   }
+  __schema {
+    types {
+      ...cycle1
+    }
+  }
 }
 
 fragment fragA on Human {
@@ -26,3 +31,15 @@ fragment fragC on Human {
   name
   ...fragA
 }
+
+fragment cycle1 on __Type {
+  kind
+  ...cycle2
+}
+
+fragment cycle2 on __Type {
+  kind
+  ofType {
+    ...cycle1
+  }
+}