[SECURITY PROBLEM] Implement a configuration option to disable the suggestion feature when a GraphQL query fails

[tomoikey]

Purpose

gqlparser is highly convenient because it offers suggestions when we mistakenly input field names or type names. However, when deploying a GraphQL server that uses gqlparser, leaving the suggestion feature enabled may pose a risk of disclosing information to attackers. Therefore, it is important to provide developers implementing GraphQL servers with the option to enable or disable the suggestion feature.

For your reference, Rust-based GraphQL library also offer the capability to disable suggestions.
https://github.com/async-graphql/async-graphql/blob/3046ae7f06ac08a9b912d8655d17f5c7f8c663c0/src/schema.rs#L217-L221

I would be delighted if you are pleased with this change. Thank you.

---

I have:

• Added tests covering the bug / feature
  - [ ] Updated any relevant documentation

Updated any relevant documentation

Additionally, it was mentioned that updating the relevant documentation is mandatory, but I was unsure about the specific sections that need to be revised. If you find this PR satisfactory, I would greatly appreciate it if you could advise me on which documents to update and how to modify them.

[coveralls]

coverage: 87.527% (-0.6%) from 88.147%
when pulling fc729b1 on tomoikey:impl/disable_suggestion
into eedca08 on vektah:master.

[tomoikey]

The golangci-lint CI is currently failing, but since the same issue is occurring on the master branch, I will not be addressing this in my branch.

[kgrigorev]

Hi @tomoikey we have run into similar problem by using this library (that suggesting types in the error message is a security risk for a public GraphQL server). So we are also interested in this change. But I am afraid your change may break a lot of existing code, because of adding additional parameter to the exported functions (which are used by other packages). Have you maybe considered ways for keeping an old API intact, i.e. that gqlparser.MustLoadQuery(schema, query) etc would still work?

Another idea would be just to drop suggested types from the error message

[tomoikey]

@kgrigorev
I have come up with a better solution than the one mentioned above. The idea is to use variable-length arguments for each function. This way, even if we update from the previous version, no compilation errors will occur. Furthermore, if nothing is passed to the variable-length arguments, we can implement it to behave the same as previous version, which will resolve our issue.
Thank you.

func LoadQuery(schema *ast.Schema, str string, options ...validator.ValidateOptionFactor) (*ast.QueryDocument, gqlerror.List)
func MustLoadQuery(schema *ast.Schema, str string, options ...validator.ValidateOptionFactor) *ast.QueryDocument
func Validate(schema *Schema, doc *QueryDocument, options ...ValidateOptionFactor) gqlerror.List

Accordingly, we can disable the suggestion as shown below.

gqlparser.LoadQuery(schema, str)
gqlparser.LoadQuery(schema, str, validator.DisableSuggestion{}) // if we want to disable suggestions

gqlparser.MustLoadQuery(schema, str)
gqlparser.MustLoadQuery(schema, str, validator.DisableSuggestion{}) // if we want to disable suggestions

validator.Validate(schema, doc)
validator.Validate(schema, doc, validator.DisableSuggestion{}) // if we want to disable suggestions

fixed on: c01da5c (test implemention: 14c9ed8)

[StevenACoffman]

Would you mind putting this import down with the non-standard library imports?

[tomoikey]

@StevenACoffman
Thanks! I fixed it like

import (
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/vektah/gqlparser/v2"
	"github.com/vektah/gqlparser/v2/ast"
	"github.com/vektah/gqlparser/v2/parser"
	"github.com/vektah/gqlparser/v2/validator"
	"github.com/vektah/gqlparser/v2/validator/rules"
)

[StevenACoffman]

Suggested change

	type ruleFunc func(observers *Events, validateOption ValidateOption, addError AddErrFunc)
	type ruleFunc func(observers *Events, addError AddErrFunc, options ...ValidateOption)

Currently people are using the AddRule which takes a function as an argument that uses this ruleFunc signature, so your change here would break backwards compatibility unless you made it variadic.

[StevenACoffman]

Hey, thanks for working on this.
There is some tension between avoiding breaking backwards compatibility and adding more configurable behavior. I don't think this is entirely free from breaking changes.

Some recent changes ( #320 ) allow people to reset the rules, so that might be an alternative method that would avoid breaking backwards compatibility.

[tomoikey]

@StevenACoffman
Thank you for your review! I’ve incorporated the changes from #320 and adjusted the implementation to introduce a new rule while maintaining full backward compatibility. What are your thoughts on this updated approach?

[StevenACoffman]

Thanks! Yeah, that's a better way since it avoids breaking backward compatibility.