feat: Add configurable validation rules #1239

perrydrums · 2021-09-17T12:35:02Z

I noticed there isn't a way to set validation rules like disabling introspection or query depth and complexity. Setting these rules can improve the security of your GraphQL endpoint, so I think it's a welcome addition. I've added the rules config to the Server settings form.

I've also added a way of bypassing the rules by using a simple query parameter which can also be set in the Server settings. Bypassing the rules can be useful for codegen scripts or just when developing.

The event subscriber I added is just there to add the bypass query parameter to the URL of the explorer and voyager pages, but I think this can be done better and cleaner.

klausi

Great work, thank you! I have a couple of questions to improve the approach, but in general the new settings on the server entity make sense.

Not sure if we need the validation token - we could check voyager/explorer permissions of the current user instead to skip validators there.

We should also add a test case if that is possible in a way in our current kernel tests.

klausi · 2021-09-21T11:24:11Z

src/EventSubscriber/ExplorerEventSubscriber.php

+    }
+
+    // Only bypass validation for these two routes.
+    if ($route === 'graphql.explorer' || $route === 'graphql.voyager') {


I think this will not work - the requests performed by explorer and voyager are sent against the normal graphql endpoint, if I understand the code correctly. So you would issue a redirect for the admin UI?

We could also do this redirect directly in the VoyagerController and ExplorerController, then we don't need an extra event listener for our own controllers that we can change anyway.

klausi · 2021-09-21T11:25:57Z

src/EventSubscriber/ExplorerEventSubscriber.php

+use Symfony\Component\HttpKernel\KernelEvents;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+class ExplorerEventSubscriber implements EventSubscriberInterface {


not a big fan that we need our own event subscriber on every single request. Just to do redirects for voyager and explorer admin pages?

klausi · 2021-09-21T11:26:53Z

src/Form/ServerForm.php

+    $form['validation']['disable_introspection'] = [
+      '#title' => $this->t('Disable introspection'),
+      '#type' => 'checkbox',
+      '#default_value' => !!$server->get('disable_introspection'),


Default value if not configured should be TRUE, for security reasons we should have it disabled by default.

klausi · 2021-09-21T11:28:20Z

src/Form/ServerForm.php

+      '#title' => $this->t('Disable introspection'),
+      '#type' => 'checkbox',
+      '#default_value' => !!$server->get('disable_introspection'),
+      '#description' => $this->t('Whether introspection should be disabled.'),


We should mention security here: "GraphQL schema introspection should be disabled on production sites for security reasons. That way the whole schema is not exposed to the public."

While I agree with the general sentiment I do think this falls in the "security through obscurity" part of the security spectrum. Introspection can provide important information to GraphQL tooling and there are legitimate use-cases to keep introspection enabled in production. For example when building an API for third-party developers.

Perhaps we can find wording that isn't as black and white?

Right, there is nuance here. GraphQL introspection is flagged as vulnerability in security audits though, and Drupal tries to be secure by default. You are right that it is no direct vulnerability. I'm open for a better wording, but we should definitely mention the word security in the description and that this should be turned on for a normal GraphQL fronntend API.

Yep that's absolutely fine :D

klausi · 2021-09-21T11:29:10Z

src/Form/ServerForm.php

+    $form['validation']['depth'] = [
+      '#title' => $this->t('Max query depth'),
+      '#type' => 'number',
+      '#default_value' => $server->get('depth'),


We should have a good default value here to prevent arbitrary deep queries. I would suggest 5, does that make sense?

I would plead to keep the default behaviour of the library in case nothing is configured (which is to disable this rule). Any arbitrarily number chosen is going to be too large for some use-cases and too small for others.

klausi · 2021-09-21T11:30:16Z

src/Form/ServerForm.php

+      '#title' => $this->t('Max query depth'),
+      '#type' => 'number',
+      '#default_value' => $server->get('depth'),
+      '#description' => $this->t('The maximum allowed depth of nested queries. Leave empty to set unlimited.'),


"The maximum allowed depth of nested queries. Leave empty to set unlimited, but for security reasons it is recommended to specify a limit like 5."

klausi · 2021-09-21T11:40:17Z

config/schema/graphql.schema.yml

+      type: boolean
+      label: 'Disable Introspection'
+    depth:
+      type: number


does number exist? I think this should be integer?

klausi · 2021-09-21T11:50:18Z

src/Form/ServerForm.php

+      '#title' => $this->t('Max query complexity'),
+      '#default_value' => $server->get('complexity'),
+      '#type' => 'number',
+      '#description' => $this->t('The maximum allowed complexity of a query. Leave empty to set unlimited.'),


Same here - what is a good default value for complexity limits? The docs at https://webonyx.github.io/graphql-php/executing-queries/ use 100 as example.

Unfortunately https://github.com/webonyx/graphql-php/blob/master/src/Validator/Rules/QueryComplexity.php has no docs at all, I assume this counts all the fields that you are requesting in 1 query? Then I think we can use 100 here as default.

This is also a security setting to prevent denial of service attacks where an attacker would slow down the server by requesting lots of fields. This should also be mitigated by the PHP memory limit and execution time, but still makes sense to also limit more strictly here on the GraphQL API level.

Query complexity is slightly more complicated. The default is to count every field as a complexity of one. However, the GraphQL library is elaborate enough that this can be changed by library users on a field level. (💡 an interesting spin-off feature could be to allow data producers to indicate their relative complexity to make this more useful)

So a single field may have a complexity of 1 or 1000 depending on the implementer. Thus complexity of a query depends entirely on what fields are queried (a query with 2 fields can have a higher complexity score than with 20).

Here too I ask that the default follows the default of the library and disables this check. There's a lot of factors that go into determining what a sensible limit is here.

Hm, I don't think many people change the complexity of a field in the library.

But yes, this setting is probably better turned off by default. The server has a PHP memory limit and timeout anyway that should catch DoS attacks on the complexity level.

klausi · 2021-09-21T11:56:45Z

src/Form/ServerForm.php

+      '#description' => $this->t('The maximum allowed complexity of a query. Leave empty to set unlimited.'),
+    ];
+
+    $form['validation']['bypass_validation_token'] = [


Instead of using a token - could we bypass validators if the current user has the permission "administer graphql configuration" or "bypass graphql access"? That way explorer and voyager would still work for a logged in administrator with that permission.

A permission is good idea! But, I think we do need a way to bypass the restrictions when doing an API request. For example, when a codegen script has to generate the schema to use in a decoupled front-end.

Not sure that codegen scripts should pull a graphql schema from a production site. But yes, maybe it makes sense to support this, the token should be good enough as protection.

We have this too in our decoupled set-up since the GraphQL schema depends on what modules are enabled on the platform. So this is not something we know before the site is in production.

Kingdutch · 2021-09-21T15:38:53Z

While I realise this is also not possible currently, I would request that this PR at least leaves room to change these metrics on a per request basis, rather than on the GraphQL server level. i.e. some way to ignore this configuration through the UI entirely.

The use-case is a GraphQL server that has different types of clients. Different clients may need different limits on what their requests may be. For example an unauthenticated client may only request simple (low complexity) queries at a low rate limit with a shallow depth. However, a client with private key authentication may have no complexity or depth limit (since it's a trusted internal client).

We expect to have this use-case of different restrictions for different clients within the next 6 months at Open Social.

klausi · 2021-09-23T15:42:23Z

We could implement an event in the Server code that is invoked. Then an event consumer can disable/change validators on a request basis, for example in a custom opensocial module.

Maybe let's not overengineer this first part yet and just add the options to the Server configuration. The event or the custom validation token can then be follow-ups to improve from there.

klausi · 2021-09-27T08:40:49Z

src/Entity/Server.php

+ *     "disable_introspection",
+ *     "depth",
+ *     "complexity",
+ *     "bypass_validation_token"


we should also add the class properties on Server like we have with the other settings.

klausi · 2021-09-27T08:48:45Z

OK, let's keep it simple to move forward in a first step here:

Add the 3 new config properties (remove the token bypass property, can be follow-up)
Improve descriptions to mention the word "security" in all of them, since those should be set by users
Leave the default values as they are right now (disabled, not adding validators) to not disrupt existing sites

@perrydrums do you have time to work on this and simplify? Otherwise I would task @akhomy-jq to take this over.

klausi · 2021-10-07T08:06:34Z

#1244 has been merged, yay! Will leave this PR open for now with the ideas here for further improvements.

feat: Add configurable validation rules

9bdf79f

klausi added the 4.x label Sep 21, 2021

Merge branch '8.x-4.x' into feature/validation-rules

1738a04

klausi requested changes Sep 21, 2021

View reviewed changes

klausi reviewed Sep 27, 2021

View reviewed changes

akhomy mentioned this pull request Oct 1, 2021

feat: Add configurable validation security rules #1244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add configurable validation rules #1239

feat: Add configurable validation rules #1239

perrydrums commented Sep 17, 2021

klausi left a comment

klausi Sep 21, 2021

klausi Sep 27, 2021

klausi Sep 21, 2021

klausi Sep 21, 2021

klausi Sep 21, 2021

Kingdutch Sep 21, 2021

klausi Sep 23, 2021

Kingdutch Sep 27, 2021

klausi Sep 21, 2021

Kingdutch Sep 21, 2021

klausi Sep 21, 2021

klausi Sep 21, 2021

klausi Sep 21, 2021

Kingdutch Sep 21, 2021

klausi Sep 23, 2021

klausi Sep 21, 2021

perrydrums Sep 21, 2021

klausi Sep 23, 2021

Kingdutch Sep 27, 2021

Kingdutch commented Sep 21, 2021

klausi commented Sep 23, 2021

klausi Sep 27, 2021

klausi commented Sep 27, 2021

klausi commented Oct 7, 2021

feat: Add configurable validation rules #1239

Are you sure you want to change the base?

feat: Add configurable validation rules #1239

Conversation

perrydrums commented Sep 17, 2021

klausi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kingdutch commented Sep 21, 2021

klausi commented Sep 23, 2021

Choose a reason for hiding this comment

klausi commented Sep 27, 2021

klausi commented Oct 7, 2021