feat(router): add security complexity mode#2753
Open
AdrienPoupa wants to merge 1 commit intowundergraph:mainfrom
Open
feat(router): add security complexity mode#2753AdrienPoupa wants to merge 1 commit intowundergraph:mainfrom
AdrienPoupa wants to merge 1 commit intowundergraph:mainfrom
Conversation
AdrienPoupa
requested review from
a team,
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
WalkthroughAdds a new complexity limits mode Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
AdrienPoupa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
router/pkg/config/config_test.go (1)
1691-1765: Add env-path coverage forSECURITY_COMPLEXITY_MODE.This suite validates YAML/default behavior well, but it doesn’t verify env parsing for the new mode setting. Adding one env-based subtest would close the regression gap for configuration source parity.
Proposed test addition
func TestComplexityLimitsModeConfig(t *testing.T) { t.Parallel() @@ t.Run("measure mode is set correctly", func(t *testing.T) { t.Parallel() @@ require.Equal(t, ComplexityLimitsModeMeasure, cfg.Config.SecurityConfiguration.ComplexityLimits.Mode) }) + + t.Run("mode can be set from environment", func(t *testing.T) { + t.Parallel() + + t.Setenv("SECURITY_COMPLEXITY_MODE", "measure") + f := createTempFileFromFixture(t, ` +version: "1" +security: + complexity_limits: + depth: + enabled: true + limit: 5 +`) + cfg, err := LoadConfig([]string{f}) + require.NoError(t, err) + require.Equal(t, ComplexityLimitsModeMeasure, cfg.Config.SecurityConfiguration.ComplexityLimits.Mode) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config_test.go` around lines 1691 - 1765, Add an env-path subtest inside TestComplexityLimitsModeConfig that verifies SECURITY_COMPLEXITY_MODE is respected: create a fixture YAML that omits the mode (use createTempFileFromFixture), set the env var via t.Setenv("SECURITY_COMPLEXITY_MODE", "measure") in the subtest, call LoadConfig([]string{f}), assert no error, and assert cfg.Config.SecurityConfiguration.ComplexityLimits.Mode equals ComplexityLimitsModeMeasure; reference TestComplexityLimitsModeConfig, createTempFileFromFixture, LoadConfig and ComplexityLimitsModeMeasure when locating where to add the test.docs-website/router/configuration.mdx (1)
2150-2150: Split this into shorter declarative sentences.This line is hard to scan due to multiple clauses. Split it into short statements to match the docs style guide.
✍️ Suggested rewrite
-For all of the limits, if the limit is 0, or `enabled` isn't true, the limit isn't applied. The `mode` field controls whether limits are enforced or only measured. When set to `measure`, complexity is calculated and reported via telemetry but requests are not rejected. When set to `enforce` (the default), requests exceeding limits are rejected with an error. All of them have the same configuration fields: +For all limits, the limit is not applied when `enabled` is not `true` or when `limit` is `0`. +The `mode` field controls behavior. +`measure` calculates complexity and reports telemetry without rejecting requests. +`enforce` rejects requests that exceed limits. This is the default mode. +All limits use the same configuration fields:As per coding guidelines: "Prefer short, declarative sentences. If a sentence has more than one comma-separated clause, consider splitting it."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs-website/router/configuration.mdx` at line 2150, The long sentence beginning "For all of the limits, if the limit is 0, or `enabled` isn't true, the limit isn't applied..." should be split into short, declarative sentences: state that a limit is not applied when its value is 0 or `enabled` is not true; state that the `mode` field controls enforcement vs measurement; state that `measure` calculates complexity and reports it via telemetry but does not reject requests; and state that `enforce` (the default) rejects requests that exceed limits; preserve the final line "All of them have the same configuration fields:" as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs-website/router/configuration.mdx`:
- Line 2150: The long sentence beginning "For all of the limits, if the limit is
0, or `enabled` isn't true, the limit isn't applied..." should be split into
short, declarative sentences: state that a limit is not applied when its value
is 0 or `enabled` is not true; state that the `mode` field controls enforcement
vs measurement; state that `measure` calculates complexity and reports it via
telemetry but does not reject requests; and state that `enforce` (the default)
rejects requests that exceed limits; preserve the final line "All of them have
the same configuration fields:" as-is.
In `@router/pkg/config/config_test.go`:
- Around line 1691-1765: Add an env-path subtest inside
TestComplexityLimitsModeConfig that verifies SECURITY_COMPLEXITY_MODE is
respected: create a fixture YAML that omits the mode (use
createTempFileFromFixture), set the env var via
t.Setenv("SECURITY_COMPLEXITY_MODE", "measure") in the subtest, call
LoadConfig([]string{f}), assert no error, and assert
cfg.Config.SecurityConfiguration.ComplexityLimits.Mode equals
ComplexityLimitsModeMeasure; reference TestComplexityLimitsModeConfig,
createTempFileFromFixture, LoadConfig and ComplexityLimitsModeMeasure when
locating where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2086130-a46a-43f6-82c0-0ba36902a64c
📒 Files selected for processing (9)
docs-website/router/configuration.mdxrouter-tests/operations/complexity_limits_test.gorouter/core/operation_processor.gorouter/core/router.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_full.json
AdrienPoupa
force-pushed
the
feat/add-complexity-limits-measure-mode
branch
2 times, most recently
from
13be87d to
520c502
Compare
AdrienPoupa
force-pushed
the
feat/add-complexity-limits-measure-mode
branch
from
520c502 to
064d844
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/config/config.go`:
- Around line 1406-1409: The code only defaults a blank ComplexityLimits.Mode
but allows invalid non-empty strings; update the logic around
cfg.Config.SecurityConfiguration.ComplexityLimits to validate Mode against the
allowed set (e.g., ComplexityLimitsModeEnforce and ComplexityLimitsModeMeasure)
and if Mode is nil/empty or not one of the allowed constants, set it to
ComplexityLimitsModeEnforce; locate the initialization that references
cfg.Config.SecurityConfiguration.ComplexityLimits.Mode and replace the simple
empty-check with a validation function or conditional that checks membership in
the allowed modes and assigns ComplexityLimitsModeEnforce for any invalid or
missing value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 378c36b9-9069-461d-9394-be1b378844a7
📒 Files selected for processing (9)
docs-website/router/configuration.mdxrouter-tests/operations/complexity_limits_test.gorouter/core/operation_processor.gorouter/core/router.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_full.json
✅ Files skipped from review due to trivial changes (3)
- router/pkg/config/fixtures/full.yaml
- router/core/router.go
- router/pkg/config/testdata/config_full.json
🚧 Files skipped from review as they are similar to previous changes (4)
- router/core/operation_processor.go
- router/pkg/config/config.schema.json
- docs-website/router/configuration.mdx
- router-tests/operations/complexity_limits_test.go
Comment on lines
+1406
to
+1409
| // Default complexity limits mode to "enforce" for backward compatibility | ||
| if cfg.Config.SecurityConfiguration.ComplexityLimits != nil && cfg.Config.SecurityConfiguration.ComplexityLimits.Mode == "" { | ||
| cfg.Config.SecurityConfiguration.ComplexityLimits.Mode = ComplexityLimitsModeEnforce | ||
| } |
Contributor
There was a problem hiding this comment.
Validate non-empty mode values, not just empty mode.
Line 1407 only defaults empty values. Invalid non-empty values can still pass through this path and be treated as enforce at runtime (because only measure gets special handling), which can cause accidental blocking on misconfiguration.
🛠️ Proposed fix
- // Default complexity limits mode to "enforce" for backward compatibility
- if cfg.Config.SecurityConfiguration.ComplexityLimits != nil && cfg.Config.SecurityConfiguration.ComplexityLimits.Mode == "" {
- cfg.Config.SecurityConfiguration.ComplexityLimits.Mode = ComplexityLimitsModeEnforce
- }
+ // Normalize and validate complexity limits mode
+ if limits := cfg.Config.SecurityConfiguration.ComplexityLimits; limits != nil {
+ switch limits.Mode {
+ case "":
+ limits.Mode = ComplexityLimitsModeEnforce
+ case ComplexityLimitsModeMeasure, ComplexityLimitsModeEnforce:
+ // valid
+ default:
+ return nil, fmt.Errorf(
+ "invalid security.complexity_limits.mode %q: must be %q or %q",
+ limits.Mode, ComplexityLimitsModeMeasure, ComplexityLimitsModeEnforce,
+ )
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Default complexity limits mode to "enforce" for backward compatibility | |
| if cfg.Config.SecurityConfiguration.ComplexityLimits != nil && cfg.Config.SecurityConfiguration.ComplexityLimits.Mode == "" { | |
| cfg.Config.SecurityConfiguration.ComplexityLimits.Mode = ComplexityLimitsModeEnforce | |
| } | |
| // Normalize and validate complexity limits mode | |
| if limits := cfg.Config.SecurityConfiguration.ComplexityLimits; limits != nil { | |
| switch limits.Mode { | |
| case "": | |
| limits.Mode = ComplexityLimitsModeEnforce | |
| case ComplexityLimitsModeMeasure, ComplexityLimitsModeEnforce: | |
| // valid | |
| default: | |
| return nil, fmt.Errorf( | |
| "invalid security.complexity_limits.mode %q: must be %q or %q", | |
| limits.Mode, ComplexityLimitsModeMeasure, ComplexityLimitsModeEnforce, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/config/config.go` around lines 1406 - 1409, The code only defaults
a blank ComplexityLimits.Mode but allows invalid non-empty strings; update the
logic around cfg.Config.SecurityConfiguration.ComplexityLimits to validate Mode
against the allowed set (e.g., ComplexityLimitsModeEnforce and
ComplexityLimitsModeMeasure) and if Mode is nil/empty or not one of the allowed
constants, set it to ComplexityLimitsModeEnforce; locate the initialization that
references cfg.Config.SecurityConfiguration.ComplexityLimits.Mode and replace
the simple empty-check with a validation function or conditional that checks
membership in the allowed modes and assigns ComplexityLimitsModeEnforce for any
invalid or missing value.
Author
There was a problem hiding this comment.
I'm not sure that's a real issue. The JSON schema validation before the YAML is unmarshalled into the config struct. The schema at line 2941 defines "enum": ["measure", "enforce"] for the mode field. Any invalid value is rejected with "value must be one of". LoadConfig returns an error and the router won't start. The post-processing code at line 1407 only runs if validation passed, so it only ever sees "", "measure", or "enforce".
The same pattern applies to CostControl.Mode; it also has no runtime validation beyond the JSON schema enum, and nobody has added one there either.
Contributor
There was a problem hiding this comment.
@AdrienPoupa, you're absolutely right — I missed that. The call to ValidateConfig against the JSON schema happens before yaml.Unmarshal, so any invalid mode value is already rejected with a schema error before the post-processing block is ever reached. By the time execution gets to line 1407, Mode can only be "", "measure", or "enforce". My concern doesn't apply here.
✏️ Learnings added
Learnt from: AdrienPoupa
Repo: wundergraph/cosmo PR: 2753
File: router/pkg/config/config.go:1406-1409
Timestamp: 2026-04-09T21:49:23.798Z
Learning: In the Cosmo router project (router/pkg/config/config.go), `ComplexityLimits.Mode` (type `ComplexityLimitsMode`) is validated at the JSON schema level in `config.schema.json` with `"enum": ["measure", "enforce"]`. The JSON schema validation in `LoadConfig` runs before YAML unmarshalling, so by the time post-processing code runs, `Mode` is guaranteed to be `""`, `"measure"`, or `"enforce"`. Runtime validation of this field in Go code is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 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: endigma
Repo: wundergraph/cosmo 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
Repo: wundergraph/cosmo PR: 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.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Learnt from: SkArchon
Repo: wundergraph/cosmo 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: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2689
File: demo/pkg/subgraphs/projects/src/service/service.go:597-622
Timestamp: 2026-03-25T11:32:18.421Z
Learning: In this repository, protobuf-generated getter methods (e.g., GetPriority(), GetPrimaryItem()) are nil-safe in Go: they check for a nil receiver and return the appropriate zero value instead of panicking. During review, do not require external nil guards before calling these generated getters, even if the parent protobuf message pointer may be nil. Only request nil checks for non-generated/custom methods that are not guaranteed nil-safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a new option,
mode, to the complexity limits. Similarly to themodeoption recently introduced in Cost Control, this lets the user choose between measurement or enforcement for complexity limits.This is useful to fine tune the limits, and will allow us to:
I have tested my changes locally and validated that requests that would have been blocked in enforce mode are allowed in measure mode.
Disclaimer: I used AI (Claude Opus 4.6) to generate the majority of this code. I have followed the HumanOSS guidelines.
Summary by CodeRabbit
New Features
measure(records complexity via telemetry, does not reject) andenforce(default, rejects over-limit requests).Documentation
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.