Fragment definitions not sufficiently validated in v3.0.2?

[soustelle]

The following query got accepted even if there are unused, duplicated fragments or even fragments depending on nonexistent types:

query packageList {
    packages {
      ...frag1
      ...frag2
    }
}

# ok
fragment frag1 on Package {
      id
      weight
}

# ok
fragment frag2 on Package {
      cost
      policy
      reference
}

# duplicated fragment frag1
# <!> 5.5.1.1 Fragment Name Uniquenes
fragment frag1 on Package {
      id
      weight
      box
}

# duplicated fragment frag1 with typo: 
# <!> 5.5.1.1 Fragment Name Uniquenes
# <!> 5.5.1.2 Fragment Spread Type Existence
fragment frag1 on xPackage {
      id
      weight
}

# unused fragment
# <!> 5.5.1.4 Fragments Must Be Used
# <!> 5.5.1.2 Fragment Spread Type Existence
fragment frag3 on xPackage {
      cost
      policy
      reference
}

[wravery]

Yes, that's right, GraphQLService is more permissive than the spec says it should be. Another example of something the spec says is not allowed but which it doesn't prevent is cycles in fragment spreads. I actually found that really useful in one of my prototypes where I was creating a tree structure, but eventually backed away from that since other tools (including Relay) complained.

Thanks for enumerating the other cases, I'll see what I can do. I might need to put it behind a "strict" mode switch in Request::resolve to avoid breaking backwards compatibility.

[soustelle]

With v3.0.2, there are more such problems. For example, name and argument count of field and fragment directives are not sufficiently checked against their corresponding definitions. Again playing with field tags within a subscription document in preparation of this comment, I just observed that one could graft an arbitrarily named tag on a field, that is without having defined any corresponding directive in the subjacent schema. This can only mean mean that there are no checks regarding field tags. Hence, probably any pattern of argument names and types gets accepted.

With some assistance of an an absent-minded service developer, a similar problem may also occur on the other side, within the schema definition itself:

    directive @fst(fax: Integer) on INLINE_FRAGMENT

The non-existing type "Integer" (instead of the predefined, irregularly named "Int" type) leads to the dereference of an invalid pointer in graphql::introspection::Schema::LookupType(.), which is the first vulnerable point of a call chain starting from generated code with AddTypesToSchema(.). The typo itself gets accepted because "schemagen" apparently does not check for the definition of an "Integer" type here. And when finally running the service, LookupType(.) does not check the result of _typeMap.find(name) in Introspection.cpp +35:

33 const std::shared_ptr<object::Type>& Schema::LookupType(const response::StringType& name) const
34 {
35  return _types[_typeMap.find(name)->second].second;
36 }

[wravery]

The crash accessing a nonexistent type in LookupType has been fixed in #87.

[wravery]

Fully fixed in PR #88.