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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove security vulnerability of exposing graph in production environment #3363

Closed
wants to merge 2 commits into from

Conversation

sahanatroam
Copy link

@sahanatroam sahanatroam commented Nov 6, 2021

Resolves and related to:
#3229
#2247

Following Node.js best practices of setting NODE_ENV=production we could use the same pattern to disable this security vulnerability in graphql-js at the core.

Open for feedback to resolve this issue ASAP

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 6, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@saihaj saihaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a node specific fix. As suggested in that issue i think it would be better to have a Boolean configuration.

@@ -12,6 +12,10 @@ export function didYouMean(
firstArg: string | ReadonlyArray<string>,
secondArg?: ReadonlyArray<string>,
) {
if (process.env.NODE_ENV === 'production') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saihaj Thanks for the review, I wonder if by default the library should enforce good practices so that we do not leave vulnerable code in production systems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but this isn’t a platform agnostic fix. This library can be used in non-node environments. So it is something in the environment you are implementing that this can be handled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Keen to see how this progress and I could help with the fix: #2247 (comment)

@IvanGoncharov
Copy link
Member

It shouldn't be a dev vs production configuration.
Production servers can support both introspection and didYouMean (e.g. public APIs, or anyone who does not follow the "security through obscurity" approach).

Having global flags (though env or something else) is also bad since it leads to very non-intuitive behavior (e.g. if calls some function in web worker it will behave differently).

I want to keep #2247 as the single discussion for discussing solutions so closing this one.
I will explore some ideas I have next work week and write my findings in #2247

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

3 participants