fix: enforce parser limits #2068
Conversation
|
""" WalkthroughThis change introduces parser hard limits for GraphQL queries in the router, including configuration, schema, and test updates. It adds enforcement of approximate depth and total fields limits during parsing, updates the operation processor to centrally manage these limits, and adjusts complexity validation logic and its invocation order. Related configuration files and tests are updated accordingly. Changes
Estimated code review effort4 (~70 minutes) Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
router-tests/go.mod(1 hunks)router-tests/security_test.go(1 hunks)router/core/cache_warmup.go(1 hunks)router/core/graph_server.go(1 hunks)router/core/graphql_prehandler.go(1 hunks)router/core/operation_processor.go(7 hunks)router/core/websocket.go(1 hunks)router/go.mod(1 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: wundergraph/cosmo#1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
router-tests/go.mod (1)
Learnt from: SkArchon
PR: #2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
router/pkg/config/testdata/config_defaults.json (3)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: #2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
router/core/graph_server.go (1)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
router/pkg/config/config.go (2)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: #2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
router-tests/security_test.go (1)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
router/pkg/config/config.schema.json (3)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Learnt from: SkArchon
PR: #2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
router/core/operation_processor.go (1)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
router/pkg/config/testdata/config_full.json (3)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: #2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.833Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
router/go.mod (1)
34-34: LGTM! Dependency update supports new parser hard limits feature.The version bump from
rc.207torc.208ofgithub.com/wundergraph/graphql-go-tools/v2is consistent with the PR objective to introduce parser hard limits and complexity validation features.router/core/websocket.go (1)
897-901: LGTM! Complexity validation correctly added to WebSocket flow.The addition of
operationKit.ValidateQueryComplexity()before the main validation step ensures WebSocket operations are subject to the same parser hard limits as HTTP operations. The error handling and early return pattern is consistent with the existing validation flow.router-tests/go.mod (1)
26-29: LGTM! Test dependencies properly synchronized with router changes.The dependency updates maintain consistency between the router and test suite:
graphql-go-tools/v2updated torc.208(matches router/go.mod)- Cosmo modules updated to latest commits from July 21st
These updates ensure the test suite supports the new parser hard limits functionality.
router/pkg/config/testdata/config_defaults.json (1)
313-316: LGTM! Parser hard limits defaults are well-configured.The new
ParserHardLimitsconfiguration provides sensible default values:
ApproximateDepthLimit: 100- reasonable depth protectionParserTotalFieldsLimit: 500- appropriate field count limitThe placement within
SecurityConfigurationis logical and consistent with the security-focused nature of these limits.router/pkg/config/testdata/config_full.json (1)
660-663: LGTM! Full configuration correctly includes parser hard limits.The
ParserHardLimitsconfiguration in the full config example is consistent with the defaults:
ApproximateDepthLimit: 100ParserTotalFieldsLimit: 500This provides a complete configuration reference for users while maintaining consistency with the default values.
router/core/graph_server.go (1)
1205-1209: LGTM! Proper integration of parser hard limits and complexity limits.The addition of
ParserHardLimitsandComplexityLimitsfields to theOperationProcessorconfiguration correctly integrates the new security limits into the operation processing pipeline. The fields are appropriately sourced from the security configuration, maintaining proper separation of concerns.router/core/cache_warmup.go (1)
308-309: Sound design decision to skip complexity validation for cache warmup.The explanatory comment clearly justifies why query complexity validation is omitted for cache warmup operations. Since these queries originate from analytics data (previously executed and validated queries), re-validating them would be redundant and could slow down the warmup process. This aligns well with the centralized complexity validation approach implemented elsewhere in the router.
router/pkg/config/config.go (2)
411-414: LGTM! Well-structured parser limits configuration.The new
ParserHardLimitsConfigurationstruct follows the established patterns in the codebase with appropriate YAML tags and reasonable default values. The field names clearly convey their purpose and the defaults (100 for depth, 500 for total fields) provide sensible starting points for GraphQL query parsing limits.
401-409: LGTM! Clean integration into security configuration.The addition of
ParserHardLimitsto theSecurityConfigurationstruct is well-placed alongside other security-related configurations like complexity limits and depth limits. The field follows the established naming conventions and integrates seamlessly with the existing security configuration structure.router-tests/security_test.go (1)
13-112: LGTM! Well-structured integration tests for parser hard limits.The test coverage is comprehensive, testing both approximate depth and total fields limits with appropriate assertions for error cases and success cases.
router/core/graphql_prehandler.go (1)
901-921: Well-structured refactoring of complexity validation placement.The complexity validation has been appropriately moved earlier in the validation sequence and simplified to use internal state. The telemetry attributes and error handling remain intact.
router/core/operation_processor.go (4)
117-118: Configuration fields properly integrated.The new parser hard limits and complexity limits configuration fields are correctly added to both the options and processor structs, with proper initialization in the constructor.
Also applies to: 132-133, 1248-1249
529-538: Parser limits properly enforced.The parsing methods now correctly enforce the configured hard limits for approximate depth and total fields, with appropriate HTTP 400 error responses when limits are exceeded.
Also applies to: 766-775
1101-1130: Complexity validation refactored effectively.The method signature has been simplified to use internal state, with a performance optimization that short-circuits when no limits are configured. The complexity calculation logic remains unchanged.
1101-1104: Verify the intended validation order.The complexity validation now runs earlier in the validation pipeline. While this provides a fail-fast mechanism for resource-intensive operations, please confirm this order change aligns with the intended behavior, as it means complexity limits are checked before other operation validations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/core/operation_processor.go (1)
1240-1241: Address past review feedback: Consider initializing parser limits once during parseKit creation.Based on the past review comment about initializing "this just once when we create the parse kits," consider whether
parserTokenizerLimitsshould be stored inparseKitOptionsand initialized once per parseKit rather than accessing it from the processor each time during parsing.This could improve performance by avoiding repeated field access during parsing operations. However, the current approach maintains flexibility if limits need to be changed dynamically.
type parseKitOptions struct { apolloCompatibilityFlags config.ApolloCompatibilityFlags apolloRouterCompatibilityFlags config.ApolloRouterCompatibilityFlags disableExposingVariablesContentOnValidationError bool + parserTokenizerLimits astparser.TokenizerLimits }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
router-tests/security_test.go(1 hunks)router/core/graph_server.go(3 hunks)router/core/operation_processor.go(7 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: wundergraph/cosmo#1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
router/core/operation_processor.go (1)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
🚧 Files skipped from review as they are similar to previous changes (6)
- router/pkg/config/testdata/config_defaults.json
- router/pkg/config/config.go
- router/pkg/config/testdata/config_full.json
- router/core/graph_server.go
- router-tests/security_test.go
- router/pkg/config/config.schema.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
Learnt from: SkArchon
PR: wundergraph/cosmo#1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
router/core/operation_processor.go (1)
Learnt from: endigma
PR: #2009
File: router/pkg/config/config.go:0-0
Timestamp: 2025-07-03T10:33:25.778Z
Learning: The CardinalityLimit field in the Metrics struct (router/pkg/config/config.go) is validated at the JSON schema level in config.schema.json with a minimum value constraint of 1, preventing zero or negative values without requiring runtime validation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
router/core/operation_processor.go (7)
117-118: LGTM: New configuration fields properly added.The addition of
ComplexityLimitsandParserTokenizerLimitsfields toOperationProcessorOptionsenables centralized configuration of parser limits, which aligns with the PR objective of enforcing parser hard limits.
132-133: LGTM: Processor fields correctly added for limit enforcement.The new fields in
OperationProcessorprovide centralized storage for the parser limits, enabling consistent enforcement across all operations processed by this instance.
529-534: Proper error handling for parser limits enforcement.The switch to
ParseWithLimitscorrectly enforces the tokenizer limits and returns HTTP 400 Bad Request for parsing errors, which is appropriate for malformed or oversized queries that exceed the configured limits.
762-767: Consistent parser limits application in operation document parsing.The
setAndParseOperationDocmethod correctly applies the same parser limits and error handling as the mainParsemethod, ensuring consistent enforcement across all parsing paths.
1093-1096: Excellent refactoring: Simplified method signature with early return optimization.The refactored
ValidateQueryComplexitymethod signature removes external parameters and uses the storedcomplexityLimitsfrom the processor. The early return when no limits are configured (line 1094-1096) is an efficient optimization that avoids unnecessary complexity calculations.
1105-1105: LGTM: Complexity calculation using processor's client schema.The complexity calculation correctly uses
o.operationProcessor.executor.ClientSchema, maintaining consistency with other validation operations in the codebase.
1122-1122: LGTM: Consistent complexity validation for cache misses.The complexity validation call correctly uses the processor's stored limits and maintains the same logic flow as the cached path.
devsergiy
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Checklist