Closed ravangen closed 4 weeks ago
We've gotten more requests for this from users of Apollo Server. Would graphql-js be open to a PR that adds a flag to validate
's options
allowing you to disable the didYouMean
suggestions? The alternatives would be to provide alternate versions of all of the specified rules that provide suggestions, or manually stripping didYouMean
from returned errors, neither of which feels particularly maintainable.
Based on the OP intent, should there be a single flag to disable introspection that also disables didYouMean?
I favour a more granular mechanism to disable didYouMean functionality if desired, whether for the introspection angle, or if there is a performance angle for large schemas (I haven't done any profiling).
@leebyron I think that would satisfy the particular use cases we've heard recommended, though it might also be nice to have the more granular version (eg, for people using validation rules exported by graphql-js directly).
For what it's worth, Apollo Server implements "no introspection" by adding the following validation rule:
const NoIntrospection = (context: ValidationContext) => ({
Field(node: FieldDefinitionNode) {
if (node.name.value === '__schema' || node.name.value === '__type') {
context.reportError(
new GraphQLError(
'GraphQL introspection is not allowed by Apollo Server, but the query contained __schema or __type. To enable introspection, pass introspection: true to ApolloServer in production',
[node],
),
);
}
},
});
Is your proposal a flag to validate
that would add a rule like the above and change the behavior of didYouMean
rules?
I don't really have a crisp proposal - just trying to clarify the need.
That seems like a reasonable way to implement introspection blocking, though I would worry about a theoretical misuse that creates a security risk where a query skips validation in some way and is still executed and exposes introspection. It might be nice if this was implemented within GraphQL.js that in addition to the clear error, it also simply wasn't possible to execute introspection queries on a schema with introspection disabled.
My concern with granular controls is that a user would disable introspection but not consider disabling didYouMean and inadvertently open this internal information leak. I would expect that a schema with introspection disabled would also disable didYouMean.
I can't think of a reason why you would want to disable introspection but enable didYouKnow or vice-versa.
Yes, I would expect that Apollo Server would take its introspection
option and use it to control both of these features if they are separate. So for the AS use case it doesn't really matter too much if it's one feature or two... though if the features were separate we might consider still using our own NoIntrospection rule just to keep the behavior that the error message tells the user what option to pass to our API to control it?
Re your theoretical risk, I do feel that if disableIntrospection
were an option to validate
, then structurally it would be pretty clear that you need to run validate
in order to disable introspection? So my inclination would be to add disableIntrospection
as an option to validate
and allow that to be enough.
If it were an execute
option too then I guess the behavior of resolving an introspection field without validation would be to just leave the __schema
field out of the response, just as it appears execution currently does for unknown fields without validation?
I'm happy to help here and I do agree with @leebyron where if introspection
is set to false didYouMean
method should not return any suggestions.
Can the options
object be used here to accept the introspection
value from the consumer?
export function validate(
schema: GraphQLSchema,
documentAST: DocumentNode,
rules: ReadonlyArray<ValidationRule> = specifiedRules,
options?: { maxErrors?: number },
/** @deprecated will be removed in 17.0.0 */
typeInfo: TypeInfo = new TypeInfo(schema),
)
In addition to this we could also create a method getSpecifiedRules({ introspection: false })
so that consumers could pass in option so that errors returned would respect the options passed in.
Please let me know if this needs to go through an RFC process and gather feedback.
Hi guys, any updates?
Any updates in 2024?
Does this mean that, by default, graphql-js is not production-ready (without workarounds)? Because schema leaking is an issue.
This will be implemented in v17 and has been in #4214
Similar to introspection, another way to probe a server for details about its schema is to submit invalid GraphQL documents so that the default validation rules provide data back. A variety of rules use
didYouMean.js
to give the developer suggestions about possible changes to get a valid document. This is ideal when developing against an API, but also is a channel for leaking information (like prototype features) with validation rules likeFieldsOnCorrectType
.In production, some companies with internal schemas elect to disable introspection, where internally they can acquire their schema via other channels. Seemingly security through obscurity.
The validation rules still need to run, but it would be ideal if we could toggle the ability to provide suggestions as part of the error message. The
ValidationContext
does provideonError
handler, but it seems a bit reactive and wasteful to then remove the computedsuggestionList
. This also ties into some ideas outlined in https://github.com/graphql/graphql-js/pull/2074.For example, Apollo Server provides a convenient constructor option
introspection
flag to determine if an introspection query should be allowed. It would be an improved developer experience if this flag could inform the underlyingGraphQL.js
validation rules to restrict its error messaging without any additional configuration required.