Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport NoSchemaIntrospectionCustomRule from graphql@15.2.0 #2662

Merged
merged 1 commit into from Sep 3, 2020

Conversation

joeyAghion
Copy link
Contributor

@joeyAghion joeyAghion commented Sep 2, 2020

For reasons described in PLATFORM-2793, it would be nice to be able to require authentication for introspection queries.

I tried to upgrade graphql to 15.2.0, which has a built-in rule added (graphql/graphql-js#2600), but encountered tons of failures. For now I'd be satisfied to adapt the rule to our codebase. The goal, ultimately, is for introspection to be enabled in development, but require an authorization header in staging/production.

TODO:

  • Introspection shouldn't be possible locally in this version of the code, but for some reason still is. (This was caused by the ENABLE_APOLLO flag being true locally.)
  • Improve typing of rule responses (currently any). (Fixed, at the expense of some slightly ugly array concatenation.)
  • Maybe fix tests? (What tests? 馃槤 )

@joeyAghion
Copy link
Contributor Author

Updated to require an Authorization header if an INTROSPECT_TOKEN is configured and the env is staging or production.

Migration

hokusai [staging|production] env set INTROSPECT_TOKEN=<some new cryptographically random token>

We plan to try this^ only in staging until we can surface and fix clients.

.env.example Outdated Show resolved Hide resolved
@dzucconi
Copy link
Member

dzucconi commented Sep 2, 2020

All this looks good to me

This rule is adapted from graphql/graphql-js#2600
and should be replaced once using graphql >=15.2.0.

Co-authored by: dzucconi <mail@damonzucconi.com>
@joeyAghion joeyAghion marked this pull request as ready for review September 2, 2020 21:55
@dzucconi dzucconi merged commit 9ada1a9 into artsy:master Sep 3, 2020
@artsy-peril artsy-peril bot mentioned this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants