-
Notifications
You must be signed in to change notification settings - Fork 201
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
base: 8.x-4.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -59,7 +62,11 @@ | |
* "endpoint", | ||
* "debug_flag", | ||
* "caching", | ||
* "batching" | ||
* "batching", | ||
* "disable_introspection", | ||
* "depth", | ||
* "complexity", | ||
* "bypass_validation_token" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
@@ -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; | ||
}; | ||
} | ||
|
||
|
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'] = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'), | ||
|
There was a problem hiding this comment.
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?