feat: Add configurable validation security rules #1244

akhomy · 2021-10-01T13:51:36Z

This is just simplified version of the #1239 according to the discussion - #1239 (comment)

Scope:

Provides new config values and getters, setters for them.
Add configurable validation security rules:
- Disable introspection
- Query depth
- Query complexity

- Disable introspection - Query depth - Query complexity

klausi

Thanks, the approach looks good! Type hints are missing for the new methods and I think we should not change the interface as 4.0 was already released (would need 5.x).

Once that is cleaned up I think we can merge.

klausi · 2021-10-05T13:46:49Z

src/Entity/ServerInterface.php

+   * @return bool
+   *   The disable introspection config, FALSE otherwise.
+   */
+  public function getDisableIntrospection();


we should not change the interface at this point because graphql 4.0 has already been released.

Can we only do the changes on the Server class?

klausi · 2021-10-05T13:49:01Z

config/schema/graphql.schema.yml

+      type: integer
+      label: 'Max query depth'
+    query_complexity:
+      type: number


does number exist? should also be integer?

klausi · 2021-10-05T13:51:56Z

src/Entity/Server.php

+  /**
+   * {@inheritdoc}
+   */
+  public function getDisableIntrospection() {


we should add return type hints to the helpers, bool here

klausi · 2021-10-05T13:53:58Z

src/Entity/Server.php

+  /**
+   * {@inheritdoc}
+   */
+  public function setDisableIntrospection($introspection) {


bool param type

akhomy · 2021-10-05T15:23:45Z

@klausi , implemented CRs.

klausi

Thanks - sorry I overlooked a couple of details, one more round?

klausi · 2021-10-05T15:24:26Z

src/Entity/Server.php

+   *
+   * @return $this
+   */
+  public function setQueryDepth(int $depth) {


type hint should be ?int so that you can disable this setting, right?

klausi · 2021-10-05T15:24:36Z

src/Entity/Server.php

+   *
+   * @return $this
+   */
+  public function setQueryComplexity(int $complexity) {


klausi · 2021-10-05T15:32:55Z

src/Entity/Server.php

+        $rules[DisableIntrospection::class] = new DisableIntrospection();
+      }
+      if ($this->getQueryDepth()) {
+        $rules[QueryDepth::class] = new QueryDepth($this->query_depth);


ah, we forgot to add the new properties to the class and document them.

@klausi , my bad, I forgot to use the appropriate getter. Added it in a new commit.

klausi · 2021-10-05T15:33:13Z

src/Form/ServerForm.php

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


hm, should we use the new methods here? they might not exist if somebody has swapped out the entity class and does not inherit them. Maybe an edge case and we don't care, but I think we should use $server->get() to be on the safe side.

…ix server setters and fix rules logic.

klausi

Thanks, please add the class properties for consistency, then I think we should be done.

src/Entity/Server.php

+ *     "batching",
+ *     "disable_introspection",
+ *     "query_depth",
+ *     "query_complexity"


klausi · 2021-10-06T15:25:05Z

Tests are failing, but does not look related to this issue.

klausi

Thanks, I would just improve the comments a bit, but I can do that in a follow-up commit

klausi · 2021-10-07T07:50:18Z

src/Entity/Server.php

+  public $disable_introspection = FALSE;
+
+  /**
+   * The query complexity.


Should be similar to the form description like "The maximum allowed query complexity, NULL means unlimited."

klausi · 2021-10-07T07:50:25Z

src/Entity/Server.php

+  public $query_complexity = NULL;
+
+  /**
+   * The query depth.


* 8.x-4.x: fix(dataproducer): Fix language definition to be single value in entity reference producers (drupal-graphql#1241) docs(server): Improved class property descriptions feat(server): Add configurable validation security rules for introspection and query complexity (drupal-graphql#1244) tests(buffer): Fix ArryObject usage to not depend on Zend (drupal-graphql#1247) test(phpstan): Add missing return type hint fix(file-upldoad): Validate files in the correct order fix(DataProducer): Fix entity reference loading by language (drupal-graphql#1232) tests(assertions): Implement leaked metadata detection for QueryResultAssertionTrait (drupal-graphql#1207) tests(github): Switch testing to Drupal core 9.2.x (drupal-graphql#1215) chore(voyager): yarn audit security updates (drupal-graphql#1214) test(phpstan): Enable PHPStan run on PHP 8.0 (drupal-graphql#1211) refactor(executor): Replace deprecated AST::getOperation() with AST::getOperationAST() (drupal-graphql#1210) fix(executor): Remove swapped errors compatibility mode

…ction and query complexity (drupal-graphql#1244)

feat: Add configurable validation security rules:

8934da3

- Disable introspection - Query depth - Query complexity

klausi requested changes Oct 5, 2021

View reviewed changes

Implements PR CRs.

35da264

akhomy requested a review from klausi October 5, 2021 15:23

klausi requested changes Oct 5, 2021

View reviewed changes

akhomy added 2 commits October 6, 2021 08:17

Fix the server form to use a config property instead of the getter. F…

a30121b

…ix server setters and fix rules logic.

Revert back ServerForm to use ServerInterface.

41ac0fa

akhomy requested a review from klausi October 6, 2021 05:21

klausi requested changes Oct 6, 2021

View reviewed changes

src/Entity/Server.php

* "batching",

* "disable_introspection",

* "query_depth",

* "query_complexity"

This comment was marked as resolved.

Sign in to view

Refactor to use variables instead of direct config call.

01a510d

akhomy requested a review from klausi October 7, 2021 07:04

klausi approved these changes Oct 7, 2021

View reviewed changes

klausi merged commit 0d07207 into drupal-graphql:8.x-4.x Oct 7, 2021

klausi mentioned this pull request Oct 7, 2021

feat: Add configurable validation rules #1239

Open

klausi pushed a commit to klausi/graphql that referenced this pull request Sep 21, 2023

feat(server): Add configurable validation security rules for introspe…

db1bd76

…ction and query complexity (drupal-graphql#1244)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add configurable validation security rules #1244

feat: Add configurable validation security rules #1244

akhomy commented Oct 1, 2021 •

edited

Loading

klausi left a comment

klausi Oct 5, 2021

klausi Oct 5, 2021

klausi Oct 5, 2021

klausi Oct 5, 2021

akhomy commented Oct 5, 2021

klausi left a comment

klausi Oct 5, 2021

klausi Oct 5, 2021

klausi Oct 5, 2021

akhomy Oct 6, 2021

klausi Oct 5, 2021

klausi left a comment

This comment was marked as resolved.

klausi commented Oct 6, 2021

klausi left a comment

klausi Oct 7, 2021

klausi Oct 7, 2021

feat: Add configurable validation security rules #1244

feat: Add configurable validation security rules #1244

Conversation

akhomy commented Oct 1, 2021 • edited Loading

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

akhomy commented Oct 5, 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

klausi left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

klausi commented Oct 6, 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

akhomy commented Oct 1, 2021 •

edited

Loading