fix(graphql): disable introspection endpoint on production

[derrabauke]

No description provided.

[winged]

That's not quite the right place to run the validation. This will run at startup, but you'll need to validate the actual GQL query coming in, not just the schema

The validate() function (sadly, not a method) used is in graphql.validation.validate, and uses a default list of rules from here: graphql.validation.specified_rules.specified_rules. Unfortunately there doesn't appear to be a "right way" to add more rules.

But - even though I don't think it's meant to do that way, if we'd extend that list by adding the DisableIntrospection rule, we'd probably get the desired effect.

Something like:

Suggested change

	if settings.DISABLE_INTROSPECTION:
	validate = partial(validate, rules=(DisableIntrospection,))
	validation_errors = validate(
	schema=schema.graphql_schema,
	)
	if settings.DISABLE_INTROSPECTION:
	from graphql.validation.specified_rules import specified_rules
	specified_rules.append(DisableIntrospection)

Sadly I don't see any better way right now to inject the rule as needed - feel free to dig deeper :-)

[winged]

BTW the call traces along these lines:

• caluma.caluma_user.views.AuthenticationGraphQLView.dispatch()
• graphene_django.views.GraphQLView.dispatch()
• graphene_django.views.GraphQLView.get_response()
• graphene_django.views.GraphQLView.execute_graphql_request()
• graphql.validation.validate.validate() <-- this could accept the rules param, but it's not passed, so the default noted in the previous comment is used

[derrabauke]

Thanks for the review! My approach was a mix of this blobpost and the graphene doc

Otherwise we could implement it in the View by overwriting execute_graphql_request?!

[derrabauke]

But your proposal of extending graphql.validation.specified_rules looks also good!

[winged]

Thanks for the review! My approach was a mix of this blobpost and the graphene doc

The first example is just showing the calling conventions of the validate() function.

Otherwise we could implement it in the View by overwriting execute_graphql_request?!

Indeed, the "correct" way would be to overload execute_graphql_request(), but that contains a ton of code that we'd need to duplicate, just so we could then insert the correct call to validate(). Seems excessive and problematic, as we'd need to keep track of the upstream implementation. Thus my suggestion of extending the specified_rules.

After thinking about it some more, I think since this is only used for development mode, it's probably not too bad to mess with specified_rules - if the implementation changes, we'd notice relatively quickly, which might not be the case in the other variant.

[winged]

Oh, also note the way the blog post calls validate(), they're replacing the whole rules parameter with just one rule - so all the specified_rules get disabled, which may allow malformed requests to penetrate much deeper into the code, possibly opening up a ton of security problems. Iff we go that route, we should at least do rules=specified_rules+[DisableIntrospection]

[derrabauke]

There seems to be an easier way in the making

[derrabauke]

graphene-django 3.2.0 is out now so we can utilize the latest feature to do the job. 🎉

[derrabauke]

Couldn't get it running locally with the new version. Could we have a small timed-boxed session @winged to hack this together?

[winged]

Thanks for your awesome work! We've paired a bit over this, and the introspection disable switch is now in #2134, the dependency update was just merged in #2133.