feat(router): add security complexity mode#2753
feat(router): add security complexity mode#2753AdrienPoupa wants to merge 5 commits intowundergraph:mainfrom
Conversation
AdrienPoupa
requested review from
a team,
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds 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}
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
3 times, most recently
from
520c502 to
064d844
Compare
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2753 +/- ##
==========================================
- Coverage 64.45% 57.80% -6.65%
==========================================
Files 306 235 -71
Lines 43621 26191 -17430
Branches 4690 0 -4690
==========================================
- Hits 28114 15141 -12973
+ Misses 15485 9594 -5891
- Partials 22 1456 +1434
🚀 New features to boost your workflow:
|
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
Few things, otherwise looks good. I would expect that config_defaults.json gets updated as well, but I don't see that in the diff?
I applied the same pattern as "ComplexityLimits": null,
"CostControl": null,That makes it default to |
AdrienPoupa
force-pushed
the
feat/add-complexity-limits-measure-mode
branch
from
1699f13 to
54c2855
Compare
AdrienPoupa
force-pushed
the
feat/add-complexity-limits-measure-mode
branch
from
54c2855 to
199bc21
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/config/config_test.go (1)
1764-1780: Consider adding a direct env-binding test (without YAML interpolation).Current coverage proves
${SECURITY_COMPLEXITY_MODE}interpolation works, but not whether plain env parsing alone setssecurity.complexity_limits.modewhen YAML omits it. A dedicated case would harden regression coverage forSECURITY_COMPLEXITY_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 1764 - 1780, Add a new unit test that verifies direct environment binding (no YAML interpolation) sets security.complexity_limits.mode: in config_test.go add a t.Run similar to the existing "mode can be set from environment via YAML expansion" but create the fixture YAML with the mode key omitted (or set to empty) and call t.Setenv("SECURITY_COMPLEXITY_MODE", "measure"), then call LoadConfig([]string{f}) and assert require.NoError(t, err) and require.Equal(t, ComplexityLimitsModeMeasure, cfg.Config.SecurityConfiguration.ComplexityLimits.Mode); reference createTempFileFromFixture, LoadConfig and ComplexityLimitsModeMeasure to locate where to add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/config/config_test.go`:
- Around line 1764-1780: Add a new unit test that verifies direct environment
binding (no YAML interpolation) sets security.complexity_limits.mode: in
config_test.go add a t.Run similar to the existing "mode can be set from
environment via YAML expansion" but create the fixture YAML with the mode key
omitted (or set to empty) and call t.Setenv("SECURITY_COMPLEXITY_MODE",
"measure"), then call LoadConfig([]string{f}) and assert require.NoError(t, err)
and require.Equal(t, ComplexityLimitsModeMeasure,
cfg.Config.SecurityConfiguration.ComplexityLimits.Mode); reference
createTempFileFromFixture, LoadConfig and ComplexityLimitsModeMeasure to locate
where to add the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9d1e32a-12d3-4d44-9c5d-97e2349d2930
📒 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 (1)
- router/pkg/config/fixtures/full.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- router/pkg/config/testdata/config_full.json
- router/core/router.go
- router/core/operation_processor.go
- docs-website/router/configuration.mdx
- router/pkg/config/config.go
- router-tests/operations/complexity_limits_test.go
|
Also, would you mind doing merge to update instead of re-squashing down to one commit? We squash at the end anyway so the commit history doesn't matter, and it bugs the GitHub review UI out a bit. |
|
It turned out the extra check in |
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
I'm fine with no environment variable here, if it's running into a pre-existing issue.
Please remove it entirely including docs and the now weird test
2 participants
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|enforce, defaultenforce) to control whether complexity is measured only or enforced; missing mode defaults toenforce.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.