Repository: graphql-rust/juniper (language: rust)

Issue:
Registry should reject types with invalid names
While the spec doesn't explicitly specify what a valid name of a type is, it's reasonable to assume that they should be valid GraphQL name tokens, i.e. `/[_A-Za-z][_0-9A-Za-z]*/` since they're written as identifiers in e.g. fragment type conditions. This seems to be in line with what the [reference implementation](https://github.com/graphql/graphql-js/blob/7ddd8dd44cb7f8a3a9f7a760d5ab76ac91802931/src/utilities/assertValidName.js) does. This causes tools like GraphiQL to stop working if an invalid type name is exposed.

Since Juniper's macros use `stringify!`, invalid names can be generated by using e.g. multiple modules:

```rust
// Generates a GraphQL type named "auth::User"
graphql_object!(auth::User: () |&self| {
});
```

It's probably not possible to do this validation check at compile time, but the registry could probably panic on invalid names.


Discussion / hints:
I'd be happy to tackle this one!
@srijs go for it!

Fix patch:
diff --git a/juniper/src/executor.rs b/juniper/src/executor.rs
index 56c57e146..c830a896e 100644
--- a/juniper/src/executor.rs
+++ b/juniper/src/executor.rs
@@ -13,6 +13,7 @@ use schema::meta::{Argument, EnumMeta, EnumValue, Field, InputObjectMeta, Interf
 use schema::model::{RootNode, SchemaType};
 
 use types::base::GraphQLType;
+use types::name::Name;
 
 /// A type registry used to build schemas
 ///
@@ -21,7 +22,7 @@ use types::base::GraphQLType;
 /// into `Type` instances and automatically registers them.
 pub struct Registry<'r> {
     /// Currently registered types
-    pub types: HashMap<String, MetaType<'r>>,
+    pub types: HashMap<Name, MetaType<'r>>,
 }
 
 #[derive(Clone)]
@@ -408,7 +409,7 @@ where
 
 impl<'r> Registry<'r> {
     /// Construct a new registry
-    pub fn new(types: HashMap<String, MetaType<'r>>) -> Registry<'r> {
+    pub fn new(types: HashMap<Name, MetaType<'r>>) -> Registry<'r> {
         Registry { types: types }
     }
 
@@ -421,12 +422,16 @@ impl<'r> Registry<'r> {
         T: GraphQLType,
     {
         if let Some(name) = T::name(info) {
-            if !self.types.contains_key(&name.to_string()) {
-                self.insert_placeholder(&name, Type::NonNullNamed(Cow::Owned(name.to_string())));
+            let validated_name = name.parse::<Name>().unwrap();
+            if !self.types.contains_key(name) {
+                self.insert_placeholder(
+                    validated_name.clone(),
+                    Type::NonNullNamed(Cow::Owned(name.to_string())),
+                );
                 let meta = T::meta(info, self);
-                self.types.insert(name.to_string(), meta);
+                self.types.insert(validated_name, meta);
             }
-            self.types[&name.to_string()].as_type()
+            self.types[name].as_type()
         } else {
             T::meta(info, self).as_type()
         }
@@ -483,10 +488,10 @@ impl<'r> Registry<'r> {
         Argument::new(name, self.get_type::<Option<T>>(info)).default_value(value.to())
     }
 
-    fn insert_placeholder(&mut self, name: &str, of_type: Type<'r>) {
-        if !self.types.contains_key(name) {
+    fn insert_placeholder(&mut self, name: Name, of_type: Type<'r>) {
+        if !self.types.contains_key(&name) {
             self.types.insert(
-                name.to_owned(),
+                name,
                 MetaType::Placeholder(PlaceholderMeta { of_type: of_type }),
             );
         }
diff --git a/juniper/src/schema/model.rs b/juniper/src/schema/model.rs
index 5bb3bb4fc..2515b1511 100644
--- a/juniper/src/schema/model.rs
+++ b/juniper/src/schema/model.rs
@@ -2,6 +2,7 @@ use std::collections::HashMap;
 use std::fmt;
 
 use types::base::GraphQLType;
+use types::name::Name;
 use executor::{Context, Registry};
 use ast::Type;
 use schema::meta::{Argument, InterfaceMeta, MetaType, ObjectMeta, PlaceholderMeta, UnionMeta};
@@ -25,7 +26,7 @@ pub struct RootNode<'a, QueryT: GraphQLType, MutationT: GraphQLType> {
 
 /// Metadata for a schema
 pub struct SchemaType<'a> {
-    types: HashMap<String, MetaType<'a>>,
+    types: HashMap<Name, MetaType<'a>>,
     query_type_name: String,
     mutation_type_name: Option<String>,
     directives: HashMap<String, DirectiveType<'a>>,
diff --git a/juniper/src/types/mod.rs b/juniper/src/types/mod.rs
index db97e46d8..5ced41f31 100644
--- a/juniper/src/types/mod.rs
+++ b/juniper/src/types/mod.rs
@@ -3,3 +3,4 @@ pub mod scalars;
 pub mod pointers;
 pub mod containers;
 pub mod utilities;
+pub mod name;
diff --git a/juniper/src/types/name.rs b/juniper/src/types/name.rs
new file mode 100644
index 000000000..9208c6d23
--- /dev/null
+++ b/juniper/src/types/name.rs
@@ -0,0 +1,90 @@
+use std::borrow::Borrow;
+use std::error::Error;
+use std::fmt::{Display, Formatter, Result as FmtResult};
+use std::str::FromStr;
+
+// Helper functions until the corresponding AsciiExt methods
+// stabilise (https://github.com/rust-lang/rust/issues/39658).
+
+fn is_ascii_alphabetic(c: char) -> bool {
+    return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z';
+}
+
+fn is_ascii_digit(c: char) -> bool {
+    return c >= '0' && c <= '9';
+}
+
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Name(String);
+
+impl Name {
+    pub fn is_valid(input: &str) -> bool {
+        for (i, c) in input.chars().enumerate() {
+            if i == 0 {
+                if !is_ascii_alphabetic(c) && c != '_' {
+                    return false;
+                }
+            } else {
+                if !is_ascii_alphabetic(c) && !is_ascii_digit(c) && c != '_' {
+                    return false;
+                }
+            }
+        }
+        return input.len() > 0;
+    }
+}
+
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
+pub struct NameParseError(String);
+
+impl Display for NameParseError {
+    fn fmt(&self, f: &mut Formatter) -> FmtResult {
+        self.0.fmt(f)
+    }
+}
+
+impl Error for NameParseError {
+    fn description(&self) -> &str {
+        &self.0
+    }
+}
+
+impl FromStr for Name {
+    type Err = NameParseError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        if Name::is_valid(s) {
+            Ok(Name(s.to_string()))
+        } else {
+            Err(NameParseError(format!(
+                "Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but \"{}\" does not",
+                s
+            )))
+        }
+    }
+}
+
+impl Borrow<String> for Name {
+    fn borrow(&self) -> &String {
+        &self.0
+    }
+}
+
+impl Borrow<str> for Name {
+    fn borrow(&self) -> &str {
+        &self.0
+    }
+}
+
+#[test]
+fn test_name_is_valid() {
+    assert!(Name::is_valid("Foo"));
+    assert!(Name::is_valid("foo42"));
+    assert!(Name::is_valid("_Foo"));
+    assert!(Name::is_valid("_Foo42"));
+    assert!(Name::is_valid("_foo42"));
+    assert!(Name::is_valid("_42Foo"));
+
+    assert!(!Name::is_valid("42_Foo"));
+    assert!(!Name::is_valid("Foo-42"));
+    assert!(!Name::is_valid("Foo???"));
+}


Test patch: