fix(router): refactor complexity limits #1364

df-wg · 2024-11-12T14:18:59Z

Motivation and Context

Previous work (#1153) added in the opportunity to limit allowed query depth, rejecting a query if it passes the acceptable limit. This PR moves that configuration into a newly created security.complexity_limitssection, while still maintaining the now deprecatedsecurity. depth_limit` section for the next 60 days

It will now be configured using:

security:
    block_mutations: false
    block_subscriptions: false
    block_non_persisted_operations: false
    complexity_calculation_cache: # This is for a local in-memory cache, to persist the calculation results
      enabled: true
      size: 1024
    complexity_limits:
        depth: # the equivalent of `security.depth_limit` prevoiusly
          enabled: false
          limit: 7
          ignore_persisted_operations: true
        total_fields:
          enabled: false
          limit: 128
          ignore_persisted_operations: true
        root_fields:
          enabled: false
          limit: 3
          ignore_persisted_operations: true
        root_field_aliases:
          enabled: false
          limit: 3
          ignore_persisted_operations: true

Checklist

I have discussed my proposed changes in an issue and have received approval to proceed.
I have followed the coding standards of the project.
Tests or benchmarks have been added or updated.
Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
I have read the Contributors Guide.

router/core/operation_processor.go

jensneuse · 2024-11-14T09:36:17Z

router/pkg/config/config.schema.json

+              },
+              "size": {
+                "type": "integer",
+                "default": 1024,


Can you make this 101024=10240?
Also, can you make planner cache, validation cache, and normalization cache the same default amount?
We've seen with some customers that 1024 is too low and 101024 doesn't increase memory usage dramatically.

jensneuse · 2024-11-14T09:38:55Z

router/pkg/config/config.schema.json

        "depth_limit": {
          "type": "object",
-          "description": "The configuration for adding a max depth limit for query (how many nested levels you can have in a query). This limit prevents infinite querying, and also limits the size of the data returned. If the limit is 0, this limit isn't applied.",
+          "description": "DEPRECATED (move to complexity_limits.depth): The configuration for adding a max depth limit for query (how many nested levels you can have in a query).",


Have you considered following this spec?
https://www.learnjsonschema.com/2020-12/meta-data/deprecated/

jensneuse · 2024-11-14T09:39:22Z

router/pkg/otel/attributes.go

@@ -36,7 +36,10 @@ const (
 	WgValidationCacheHit               = attribute.Key("wg.engine.validation_cache_hit")
 	WgVariablesValidationSkipped       = attribute.Key("wg.engine.variables_validation_skipped")
 	WgQueryDepth                       = attribute.Key("wg.operation.query_depth")
-	WgQueryDepthCacheHit               = attribute.Key("wg.operation.query_depth_cache_hit")
+	WgQueryTotalFields                 = attribute.Key("wg.operation.total_fields")


@StarpTech is this fine?

I think we should scope it further e.g. wg.operation.complexity.total_fields because it is not a common attribute and only set when complexity limits are enabled.

jensneuse · 2024-11-14T09:39:58Z

router/pkg/config/config.go

@@ -334,6 +336,28 @@ type QueryDepthConfiguration struct {
 	IgnorePersistedOperations bool  `yaml:"ignore_persisted_operations,omitempty" envDefault:"false" env:"SECURITY_QUERY_DEPTH_IGNORE_PERSISTED_OPERATIONS"`
 }

+type ComplexityCalculationCache struct {
+	Enabled   bool  `yaml:"enabled" envDefault:"false" env:"SECURITY_COMPLEXITY_CACHE_ENABLED"`
+	CacheSize int64 `yaml:"size,omitempty" envDefault:"1024" env:"SECURITY_COMPLEXITY_CACHE_SIZE"`


Update defaults here accordingly as discussed in the other thread.

jensneuse · 2024-11-14T09:41:04Z

router/core/operation_processor.go

+	}
+	if complexityLimitConfig.RootFieldAliases != nil && complexityLimitConfig.RootFieldAliases.ApplyLimit(isPersisted) {
+		testComparisons = append(testComparisons,
+			comp{complexityLimitConfig.RootFieldAliases.Limit, cachedComplexity.RootFieldAliases, fmt.Sprintf("The number of root fields aliases %d exceeds the root field aliases limit allowed (%d)", cachedComplexity.RootFieldAliases, complexityLimitConfig.RootFieldAliases.Limit)})


The number of root field aliases %d exceeds the root field aliases limit allowed (%d)

jensneuse · 2024-11-14T09:42:01Z

router/core/operation_processor.go

+	}
+	if complexityLimitConfig.TotalFields != nil && complexityLimitConfig.TotalFields.ApplyLimit(isPersisted) {
+		testComparisons = append(testComparisons,
+			comp{complexityLimitConfig.TotalFields.Limit, cachedComplexity.TotalFields, fmt.Sprintf("The query total fields %d exceeds the max total fields allowed (%d)", cachedComplexity.TotalFields, complexityLimitConfig.TotalFields.Limit)})


The total number of fields %d exceeds the limit allowed (%d)

StarpTech · 2024-11-14T11:06:01Z

router/core/operation_processor.go

+}
+
+func (o *OperationKit) runComplexityComparisons(complexityLimitConfig *config.ComplexityLimits, cachedComplexity ComplexityCacheEntry, isPersisted bool) error {
+	type comp struct {


We should discourage inline structs. Please place them at the top of the file.

StarpTech · 2024-11-14T11:51:08Z

router-tests/complexity_limits_test.go

+				failedRes, _ := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
+					Query: `{ employee(id:1) { id details { forename surname } } }`,
+				})
+				require.Equal(t, 400, failedRes.Response.StatusCode)


Please add the tests to the telemetry tests.

StarpTech · 2024-11-14T11:58:47Z

router/pkg/otel/attributes.go

@@ -36,7 +36,10 @@ const (
 	WgValidationCacheHit               = attribute.Key("wg.engine.validation_cache_hit")
 	WgVariablesValidationSkipped       = attribute.Key("wg.engine.variables_validation_skipped")
 	WgQueryDepth                       = attribute.Key("wg.operation.query_depth")
-	WgQueryDepthCacheHit               = attribute.Key("wg.operation.query_depth_cache_hit")
+	WgQueryTotalFields                 = attribute.Key("wg.operation.total_fields")


I think we should scope it further e.g. wg.operation.complexity.total_fields because it is not a common attribute and only set when complexity limits are enabled.

github-actions bot added the router label Nov 12, 2024

df-wg force-pushed the dave/eng-5663-refactor-improve-security-complexity-limits branch from dcf43fa to 8f70c7a Compare November 12, 2024 14:21

df-wg added 3 commits November 12, 2024 16:41

feat(router): move depth limit to full complexity limits section

7bb630b

feat: add complexity tests

c26f0ec

fix: fix linter and config tests

ee032c8

df-wg force-pushed the dave/eng-5663-refactor-improve-security-complexity-limits branch from 1fa616d to ee032c8 Compare November 12, 2024 14:41

devsergiy reviewed Nov 12, 2024

View reviewed changes

router/core/operation_processor.go Outdated Show resolved Hide resolved

fix: simplify enabled checks

19ed9ee

jensneuse requested changes Nov 14, 2024

View reviewed changes

StarpTech requested changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): refactor complexity limits #1364

fix(router): refactor complexity limits #1364

df-wg commented Nov 12, 2024

jensneuse Nov 14, 2024

jensneuse Nov 14, 2024

jensneuse Nov 14, 2024

StarpTech Nov 14, 2024

jensneuse Nov 14, 2024

jensneuse Nov 14, 2024

jensneuse Nov 14, 2024

StarpTech Nov 14, 2024

StarpTech Nov 14, 2024 •

edited

Loading

StarpTech Nov 14, 2024

fix(router): refactor complexity limits #1364

Are you sure you want to change the base?

fix(router): refactor complexity limits #1364

Conversation

df-wg commented Nov 12, 2024

Motivation and Context

Checklist

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

StarpTech Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech Nov 14, 2024 •

edited

Loading