[SECURITY] Implement a feature to disable the suggestion when a GraphQL query fails #3411
Conversation
| func TestExecutorDisableSuggestion(t *testing.T) { | ||
| exec := testexecutor.New() | ||
| t.Run("by default, the error message will include suggestions", func(t *testing.T) { | ||
| resp := query(exec, "", "{nam}") | ||
| assert.Equal(t, "", string(resp.Data)) | ||
| assert.Equal(t, "input:1: Cannot query field \"nam\" on type \"Query\". Did you mean \"name\"?\n", resp.Errors.Error()) | ||
| }) | ||
|
|
||
| t.Run("disable suggestion, the error message will not include suggestions", func(t *testing.T) { | ||
| exec.SetDisableSuggestion(true) | ||
| resp := query(exec, "", "{nam}") | ||
| assert.Equal(t, "", string(resp.Data)) | ||
| assert.Equal(t, "input:1: Cannot query field \"nam\" on type \"Query\".\n", resp.Errors.Error()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Since the settings of the Executor are being modified, I have prepared a separate test from the TestExecutor functions. This ensures that it does not affect anyone writing tests in the future.
| @@ -24,7 +25,8 @@ type Executor struct { | |||
| recoverFunc graphql.RecoverFunc | |||
| queryCache graphql.Cache[*ast.QueryDocument] | |||
|
|
|||
| parserTokenLimit int | |||
| parserTokenLimit int | |||
| disableSuggestion bool | |||
There was a problem hiding this comment.
Unless SetDisableSuggestion(true) is called, the behavior will remain exactly the same as before, so it does not affect backward compatibility.
Only those who are aware of this feature will be able to turn off the suggestions.
StevenACoffman
changed the title
|
Thanks for this (and the gqlparser contribution)! Just noticed you are in Yokohama, I miss the food there, so be sure to appreciate it for me! I'm always wistful when my Ramen here doesn't come with wood-ear mushrooms like in sanmamen. |
|
@StevenACoffman |
|
@StevenACoffman @tomoikey I think this might have accidentally been a breaking change. Maybe I'm wrong though! Previously, (obviously, yes, users like me should just make validation pass on schemas, but I'm not entirely sure if this change was intentional?) EDIT: investigating more, it seems like this was probably always the intended behavior, and that validate should always have imported the rules like this. But due to importing bits-and-pieces of the library piece-meal, we never actually ended up importing these rules. |
Description
Hello! I recently contributed to gqlparser to disable the suggestion feature for security reasons. Since gqlgen also uses the updated version of gqlparser, I have made modifications to allow gqlgen users to disable the suggestion feature as well.
The suggestion feature can be convenient from the client's perspective, but it may pose security risks. Therefore, developers using gqlgen should have the option to enable or disable this feature as needed.
Thanks.
The relevant pull request
vektah/gqlparser#319
Changes Made
I have:
Updated any relevant documentation (see docs)