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

feat: Add configurable validation rules #1239

Open
wants to merge 2 commits into
base: 8.x-4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions config/schema/graphql.schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ graphql.graphql_servers.*:
batching:
type: boolean
label: 'Batching'
disable_introspection:
type: boolean
label: 'Disable Introspection'
depth:
type: number
Copy link
Contributor

Choose a reason for hiding this comment

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

does number exist? I think this should be integer?

label: 'Max query depth'
complexity:
type: number
label: 'Max query complexity'
bypass_validation_token:
type: string
label: 'Bypass validation token'
schema_configuration:
type: 'graphql.schema.[%parent.schema]'
persisted_queries_settings:
Expand Down
6 changes: 6 additions & 0 deletions graphql.services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ services:
tags:
- { name: event_subscriber }

# Reset the current language during operations.
graphql.explorer_subscriber:
class: Drupal\graphql\EventSubscriber\ExplorerEventSubscriber
tags:
- { name: event_subscriber }

# Plugin manager for schemas
plugin.manager.graphql.schema:
class: Drupal\graphql\Plugin\SchemaPluginManager
Expand Down
25 changes: 23 additions & 2 deletions src/Entity/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
use GraphQL\Server\Helper;
use GraphQL\Type\Definition\ResolveInfo;
use GraphQL\Validator\DocumentValidator;
use GraphQL\Validator\Rules\DisableIntrospection;
use GraphQL\Validator\Rules\QueryComplexity;
use GraphQL\Validator\Rules\QueryDepth;

/**
* The main GraphQL configuration and request entry point.
Expand Down Expand Up @@ -59,7 +62,11 @@
* "endpoint",
* "debug_flag",
* "caching",
* "batching"
* "batching",
* "disable_introspection",
* "depth",
* "complexity",
* "bypass_validation_token"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* },
* links = {
* "collection" = "/admin/config/graphql/servers",
Expand Down Expand Up @@ -498,7 +505,21 @@ protected function getValidationRules() {
return [];
}

return array_values(DocumentValidator::defaultRules());
$rules = array_values(DocumentValidator::defaultRules());

if (\Drupal::request()->get('bypass_validation') !== $this->get('bypass_validation_token')) {
if ($this->get('disable_introspection')) {
$rules[DisableIntrospection::class] = new DisableIntrospection();
}
if ($this->get('depth')) {
$rules[QueryDepth::class] = new QueryDepth($this->get('depth'));
}
if ($this->get('complexity')) {
$rules[QueryComplexity::class] = new QueryComplexity($this->get('complexity'));
}
}

return $rules;
};
}

Expand Down
50 changes: 50 additions & 0 deletions src/EventSubscriber/ExplorerEventSubscriber.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

namespace Drupal\graphql\EventSubscriber;

use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class ExplorerEventSubscriber implements EventSubscriberInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


/**
* {@inheritdoc}
*/
public static function getSubscribedEvents() {
$events[KernelEvents::REQUEST][] = ['bypassValidation'];
return $events;
}

/**
* Add the bypass_validation url parameter to graphql admin routes.
*
* @param RequestEvent $event
*/
public function bypassValidation(RequestEvent $event) {
$route = \Drupal::routeMatch()->getRouteName();

// If a bypass_validation param is already set, skip.
if ($event->getRequest()->get('bypass_validation')) {
return;
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/** @var \Drupal\graphql\Entity\Server $server */
$server = $event->getRequest()->get('graphql_server');

$url = $event->getRequest()->getUri();

// Get the bypass_validation_token from the server settings.
$bypass_validation_token = $server->get('bypass_validation_token');

// Add the bypass_validation parameter to the current url.
$url .= (parse_url($url, PHP_URL_QUERY) ? '&' : '?') . 'bypass_validation=' . $bypass_validation_token;

// Redirect to the new url.
$event->setResponse(new RedirectResponse($url));
}
}
}
33 changes: 33 additions & 0 deletions src/Form/ServerForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,39 @@ public function form(array $form, FormStateInterface $formState): array {
'#description' => $this->t('Whether caching of queries and partial results is enabled.'),
];

$form['validation'] = [
'#title' => $this->t('Validation rules'),
'#type' => 'fieldset',
];

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

Choose a reason for hiding this comment

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

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

'#description' => $this->t('Whether introspection should be disabled.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's absolutely fine :D

];

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

'#description' => $this->t('The maximum allowed depth of nested queries. Leave empty to set unlimited.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

"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."

];

$form['validation']['complexity'] = [
'#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.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

];

$form['validation']['bypass_validation_token'] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

'#title' => $this->t('Bypass validation token'),
'#default_value' => $server->get('bypass_validation_token'),
'#type' => 'textfield',
'#description' => $this->t('A string token that can be used as the "bypass_validation" parameter when doing a GraphQL request. This bypasses the above validation rules. Could be used when generating types for front-end applications.'),
];

$debug_flags = $server->get('debug_flag') ?? 0;
$form['debug_flag'] = [
'#title' => $this->t('Debug settings'),
Expand Down