fix(parser): ensure all loops advance parsing, fuzz with arbitrary bytes (#828)

* fix(parser): ensure all loops advance parsing

* fix(parser): write root operation parsing with a loop

This also fixes a case where bogus input was accepted, and did not raise
a parse error:
```graphql
schema {
  query: Query
  { mutation: Mutation
  { subscription: Subscription
}
```

* chore(parser): add peek_while_kind variant for simple loops

* fix(parser): add parse_separated_list helper; remove recursion from directive locations parser

* add test that fails on main and is fixed here

* Add fuzz target parsing arbitrary strings with token limit

* fix(parser): always consume token in operation_type() parser

* add failing test: stray StringValue at token limit

* fix(parser): remove unwrap that may trigger with token limits

* fix(parser): fix panic if token limit is reached mid-type

* fix(parser): remove unwrap from enum_value parser

* Use peek_while_kind

* Tweak fuzz comment

Author: goto-bus-stop

crates/apollo-parser/src/lexer/token.rs

@@ -3,7 +3,7 @@ use std::fmt;
 use crate::TokenKind;
 
 /// A token generated by the lexer.
-#[derive(Clone)]
+#[derive(Clone, PartialEq, Eq)]
 pub struct Token<'a> {
     pub(crate) kind: TokenKind,
     pub(crate) data: &'a str,

crates/apollo-parser/src/parser/grammar/argument.rs

@@ -1,6 +1,7 @@
 use crate::parser::grammar::value::Constness;
 use crate::parser::grammar::{input, name, value};
 use crate::{Parser, SyntaxKind, TokenKind, S, T};
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#Argument
 ///
@@ -27,9 +28,9 @@ pub(crate) fn arguments(p: &mut Parser, constness: Constness) {
     } else {
         p.err("expected an Argument");
     }
-    while let Some(TokenKind::Name) = p.peek() {
+    p.peek_while_kind(TokenKind::Name, |p| {
         argument(p, constness);
-    }
+    });
     p.expect(T![')'], S![')']);
 }
 
@@ -45,9 +46,13 @@ pub(crate) fn arguments_definition(p: &mut Parser) {
     } else {
         p.err("expected an Argument Definition");
     }
-    while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
-        input::input_value_definition(p);
-    }
+    p.peek_while(|p, kind| match kind {
+        TokenKind::Name | TokenKind::StringValue => {
+            input::input_value_definition(p);
+            ControlFlow::Continue(())
+        }
+        _ => ControlFlow::Break(()),
+    });
     p.expect(T![')'], S![')']);
 }
 

crates/apollo-parser/src/parser/grammar/directive.rs

@@ -1,6 +1,7 @@
 use crate::parser::grammar::value::Constness;
 use crate::parser::grammar::{argument, description, input, name};
 use crate::{Parser, SyntaxKind, TokenKind, S, T};
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#DirectiveDefinition
 ///
@@ -31,9 +32,13 @@ pub(crate) fn directive_definition(p: &mut Parser) {
         } else {
             p.err("expected an Argument Definition");
         }
-        while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
-            input::input_value_definition(p);
-        }
+        p.peek_while(|p, kind| match kind {
+            TokenKind::Name | TokenKind::StringValue => {
+                input::input_value_definition(p);
+                ControlFlow::Continue(())
+            }
+            _ => ControlFlow::Break(()),
+        });
         p.expect(T![')'], S![')']);
     }
 
@@ -52,29 +57,27 @@ pub(crate) fn directive_definition(p: &mut Parser) {
 
     if let Some(TokenKind::Name | T![|]) = p.peek() {
         let _g = p.start_node(SyntaxKind::DIRECTIVE_LOCATIONS);
-        if let Some(T![|]) = p.peek() {
-            p.bump(S![|]);
-        }
-        directive_locations(p, false);
+        directive_locations(p);
     } else {
         p.err("expected valid Directive Location");
     }
 }
 
-/// See: https://spec.graphql.org/October2021/#DirectiveLocations
+/// https://spec.graphql.org/October2021/#DirectiveLocation
 ///
-/// *DirectiveLocations*:
-///     DirectiveLocations **|** DirectiveLocation
-///     **|**? DirectiveLocation
-pub(crate) fn directive_locations(p: &mut Parser, is_location: bool) {
-    if let Some(T![|]) = p.peek() {
-        p.bump(S![|]);
-        directive_locations(p, false);
-    }
+/// *DirectiveLocation*:
+///     *ExecutableDirectiveLocation*
+///     *TypeSystemDirectiveLocation*
+///
+/// (This function does not distinguish between the two groups of
+/// locations.)
+fn directive_location(p: &mut Parser) {
+    let Some(token) = p.peek_token() else {
+        return;
+    };
 
-    if let Some(TokenKind::Name) = p.peek() {
-        let loc = p.peek_data().unwrap();
-        match loc {
+    if token.kind == TokenKind::Name {
+        match token.data {
             "QUERY" => {
                 let _g = p.start_node(SyntaxKind::DIRECTIVE_LOCATION);
                 p.bump(SyntaxKind::QUERY_KW);
@@ -152,21 +155,23 @@ pub(crate) fn directive_locations(p: &mut Parser, is_location: bool) {
                 p.bump(SyntaxKind::INPUT_FIELD_DEFINITION_KW);
             }
             _ => {
-                if !is_location {
-                    p.err("expected valid Directive Location");
-                }
-                return;
+                p.err("expected valid Directive Location");
             }
         }
-        if p.peek_data().is_some() {
-            return directive_locations(p, true);
-        }
-    }
-    if !is_location {
-        p.err("expected Directive Locations");
+    } else {
+        p.err("expected Directive Location");
     }
 }
 
+/// See: https://spec.graphql.org/October2021/#DirectiveLocations
+///
+/// *DirectiveLocations*:
+///     DirectiveLocations **|** DirectiveLocation
+///     **|**? DirectiveLocation
+pub(crate) fn directive_locations(p: &mut Parser) {
+    p.parse_separated_list(T![|], S![|], directive_location);
+}
+
 /// See: https://spec.graphql.org/October2021/#Directive
 ///
 /// *Directive[Const]*:
@@ -188,9 +193,9 @@ pub(crate) fn directive(p: &mut Parser, constness: Constness) {
 ///     Directive[?Const]*
 pub(crate) fn directives(p: &mut Parser, constness: Constness) {
     let _g = p.start_node(SyntaxKind::DIRECTIVES);
-    while let Some(T![@]) = p.peek() {
+    p.peek_while_kind(T![@], |p| {
         directive(p, constness);
-    }
+    });
 }
 
 #[cfg(test)]

crates/apollo-parser/src/parser/grammar/document.rs

@@ -5,6 +5,7 @@ use crate::{
     },
     Parser, SyntaxKind, TokenKind,
 };
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#Document
 ///
@@ -13,16 +14,19 @@ use crate::{
 pub(crate) fn document(p: &mut Parser) {
     let doc = p.start_node(SyntaxKind::DOCUMENT);
 
-    while let Some(node) = p.peek() {
+    p.peek_while(|p, kind| {
         assert_eq!(
             p.recursion_limit.current, 0,
             "unbalanced limit increment / decrement"
         );
 
-        match node {
+        match kind {
             TokenKind::StringValue => {
-                let def = p.peek_data_n(2).unwrap();
-                select_definition(def, p);
+                if let Some(def) = p.peek_data_n(2) {
+                    select_definition(def, p);
+                } else {
+                    p.err_and_pop("expected a definition after this StringValue");
+                }
             }
             TokenKind::Name => {
                 let def = p.peek_data().unwrap();
@@ -32,10 +36,12 @@ pub(crate) fn document(p: &mut Parser) {
                 let def = p.peek_data().unwrap();
                 select_definition(def, p);
             }
-            TokenKind::Eof => break,
+            TokenKind::Eof => return ControlFlow::Break(()),
             _ => p.err_and_pop("expected a StringValue, Name or OperationDefinition"),
         }
-    }
+
+        ControlFlow::Continue(())
+    });
 
     p.push_ignored();
 
@@ -274,4 +280,21 @@ enum join__Graph {
             }
         }
     }
+
+    #[test]
+    fn trailing_description_at_limit() {
+        let parser = crate::Parser::new(
+            r#"
+            "All our queries"
+            type Query {
+                a: Int
+            }
+
+            "Imagine another type below!"
+        "#,
+        )
+        .token_limit(18);
+        // Must not panic
+        let _cst = parser.parse();
+    }
 }

crates/apollo-parser/src/parser/grammar/enum_.rs

@@ -3,6 +3,7 @@
 use crate::parser::grammar::value::Constness;
 use crate::parser::grammar::{description, directive, name, value};
 use crate::{Parser, SyntaxKind, TokenKind, S, T};
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#EnumTypeDefinition
 ///
@@ -78,9 +79,13 @@ pub(crate) fn enum_values_definition(p: &mut Parser) {
         _ => p.err("expected Enum Value Definition"),
     }
 
-    while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
-        enum_value_definition(p);
-    }
+    p.peek_while(|p, kind| match kind {
+        TokenKind::Name | TokenKind::StringValue => {
+            enum_value_definition(p);
+            ControlFlow::Continue(())
+        }
+        _ => ControlFlow::Break(()),
+    });
 
     p.expect(T!['}'], S!['}']);
 }

crates/apollo-parser/src/parser/grammar/field.rs

@@ -3,6 +3,7 @@
 use crate::parser::grammar::value::Constness;
 use crate::parser::grammar::{argument, description, directive, name, selection, ty};
 use crate::{Parser, SyntaxKind, TokenKind, S, T};
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#Field
 ///
@@ -40,10 +41,13 @@ pub(crate) fn field(p: &mut Parser) {
 pub(crate) fn fields_definition(p: &mut Parser) {
     let _g = p.start_node(SyntaxKind::FIELDS_DEFINITION);
     p.bump(S!['{']);
-    while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
-        // Guaranteed to eat at least one token if the next token is a Name or StringValue
-        field_definition(p);
-    }
+    p.peek_while(|p, kind| match kind {
+        TokenKind::Name | TokenKind::StringValue => {
+            field_definition(p);
+            ControlFlow::Continue(())
+        }
+        _ => ControlFlow::Break(()),
+    });
     p.expect(T!['}'], S!['}']);
 }
 

crates/apollo-parser/src/parser/grammar/input.rs

@@ -1,6 +1,7 @@
 use crate::parser::grammar::value::Constness;
 use crate::parser::grammar::{description, directive, name, ty, value};
 use crate::{Parser, SyntaxKind, TokenKind, S, T};
+use std::ops::ControlFlow;
 
 /// See: https://spec.graphql.org/October2021/#InputObjectTypeDefinition
 ///
@@ -76,9 +77,14 @@ pub(crate) fn input_fields_definition(p: &mut Parser) {
     } else {
         p.err("expected an Input Value Definition");
     }
-    while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
-        input_value_definition(p);
-    }
+    p.peek_while(|p, kind| {
+        if matches!(kind, TokenKind::Name | TokenKind::StringValue) {
+            input_value_definition(p);
+            ControlFlow::Continue(())
+        } else {
+            ControlFlow::Break(())
+        }
+    });
 
     p.expect(T!['}'], S!['}']);
 }

crates/apollo-parser/src/parser/grammar/object.rs

@@ -90,24 +90,13 @@ pub(crate) fn implements_interfaces(p: &mut Parser) {
     let _g = p.start_node(SyntaxKind::IMPLEMENTS_INTERFACES);
     p.bump(SyntaxKind::implements_KW);
 
-    if let Some(T![&]) = p.peek() {
-        p.bump(S![&]);
-    }
-
-    if let Some(TokenKind::Name) = p.peek() {
-        ty::named_type(p);
-    } else {
-        p.err("expected an Interface name");
-    }
-
-    while let Some(T![&]) = p.peek() {
-        p.bump(S![&]);
+    p.parse_separated_list(T![&], S![&], |p| {
         if let Some(TokenKind::Name) = p.peek() {
             ty::named_type(p);
         } else {
             p.err("expected an Interface name");
         }
-    }
+    });
 }
 
 #[cfg(test)]

crates/apollo-parser/src/parser/grammar/operation.rs

@@ -1,46 +1,6 @@
 use crate::parser::grammar::value::Constness;
-use crate::parser::grammar::{directive, name, selection, ty, variable};
-use crate::{Parser, SyntaxKind, TokenKind, S, T};
-
-/// RootOperationTypeDefinition is used in a SchemaDefinition. Not to be confused
-/// with OperationDefinition.
-///
-/// See: https://spec.graphql.org/October2021/#RootOperationTypeDefinition
-///
-/// *RootOperationTypeDefinition*:
-///    OperationType **:** NamedType
-pub(crate) fn root_operation_type_definition(p: &mut Parser, is_operation_type: bool) {
-    if let Some(T!['{']) = p.peek() {
-        p.bump(S!['{']);
-    }
-
-    if let Some(TokenKind::Name) = p.peek() {
-        let guard = p.start_node(SyntaxKind::ROOT_OPERATION_TYPE_DEFINITION);
-        operation_type(p);
-        if let Some(T![:]) = p.peek() {
-            p.bump(S![:]);
-            ty::named_type(p);
-            if p.peek().is_some() {
-                guard.finish_node();
-
-                // TODO use a loop instead of recursion
-                if p.recursion_limit.check_and_increment() {
-                    p.limit_err("parser recursion limit reached");
-                    return;
-                }
-                root_operation_type_definition(p, true);
-                p.recursion_limit.decrement();
-                return;
-            }
-        } else {
-            p.err("expected a Name Type");
-        }
-    }
-
-    if !is_operation_type {
-        p.err("expected an Operation Type");
-    }
-}
+use crate::parser::grammar::{directive, name, selection, variable};
+use crate::{Parser, SyntaxKind, TokenKind, T};
 
 /// See: https://spec.graphql.org/October2021/#OperationDefinition
 ///
@@ -92,7 +52,7 @@ pub(crate) fn operation_type(p: &mut Parser) {
             "query" => p.bump(SyntaxKind::query_KW),
             "subscription" => p.bump(SyntaxKind::subscription_KW),
             "mutation" => p.bump(SyntaxKind::mutation_KW),
-            _ => p.err("expected either a 'mutation', a 'query', or a 'subscription'"),
+            _ => p.err_and_pop("expected either a 'mutation', a 'query', or a 'subscription'"),
         }
     }
 }