feat: Add option to disable rate limiting#2708
Open
leechristensen wants to merge 3 commits into
Open
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
📝 WalkthroughWalkthroughThreads configuration through API rate-limiting and login-protection code: adds ChangesAPI Rate Limiting & Login Protections
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Middleware as RateLimitMiddleware
participant DB as Store/DB
participant Handler as Handler
Client->>Router: HTTP request
Router->>Middleware: invoke middleware
alt DisableLoginProtections / limit <= 0
Middleware->>Handler: passthrough (call next)
Handler-->>Client: 200 OK
else rate limiting enabled
Middleware->>DB: fetch/update per-IP state
DB-->>Middleware: allow / deny decision
alt allow
Middleware->>Handler: call next
Handler-->>Client: 200 OK
else deny
Middleware-->>Client: 429 Too Many Requests
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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)
cmd/api/src/api/middleware/rate_limit.go (1)
130-136: Consider logging a startup warning when rate limiting is disabled.The passthrough is correct, but disabling rate limiting (particularly on
/api/v2/login) removes brute-force protection on authentication. Since this flag is intended for controlled environments, emitting a one-timeslog.Warn(e.g., at server startup or on first middleware construction) helps make the security trade-off visible in operational logs and reduces the risk of it being silently enabled in production.Also, ensure the new
DisableRateLimitingsetting is documented in the user-facing configuration reference / sample config so operators understand the security impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/middleware/rate_limit.go` around lines 130 - 136, The RateLimitMiddleware currently returns a passthrough when cfg.DisableRateLimiting is true; add a one-time startup warning via the standard logger (e.g., slog.Warn) when this flag is set so operators see that rate limiting (and brute-force protection) is disabled; implement the warning at middleware construction in RateLimitMiddleware (guarded so it only logs once) and ensure the message references DisableRateLimiting and the security impact (e.g., removal of brute-force protection on login). Also update the configuration docs/sample to document the new DisableRateLimiting setting and its security implications.cmd/api/src/api/middleware/rate_limit_test.go (1)
115-144: Addt.Helper()to test helpers.Both
newRateLimitedRouterandnewTestRequestaccept*testing.Tand may invoket.Fatal. Marking them as helpers ensures failure locations are reported at the call site in the subtest rather than inside the helper.♻️ Proposed fix
func newRateLimitedRouter(t *testing.T, cfg config.Configuration, useDefaultRateLimit bool, limit int64) (*mux.Router, *CountingHandler) { + t.Helper() mockCtl := gomock.NewController(t) mockDatabase := mocks.NewMockDatabase(mockCtl) @@ func newTestRequest(t *testing.T) *http.Request { + t.Helper() req, err := http.NewRequest("GET", "/teapot", nil) if err != nil { t.Fatal(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/middleware/rate_limit_test.go` around lines 115 - 144, Both test helper functions newRateLimitedRouter and newTestRequest take *testing.T and may call t.Fatal; add t.Helper() as the first statement inside each function (i.e., at the top of newRateLimitedRouter and newTestRequest) so failures are attributed to the caller location. Ensure you add the call in both functions (newRateLimitedRouter(...) and newTestRequest(...)) before any other logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/api/src/api/middleware/rate_limit_test.go`:
- Around line 115-144: Both test helper functions newRateLimitedRouter and
newTestRequest take *testing.T and may call t.Fatal; add t.Helper() as the first
statement inside each function (i.e., at the top of newRateLimitedRouter and
newTestRequest) so failures are attributed to the caller location. Ensure you
add the call in both functions (newRateLimitedRouter(...) and
newTestRequest(...)) before any other logic.
In `@cmd/api/src/api/middleware/rate_limit.go`:
- Around line 130-136: The RateLimitMiddleware currently returns a passthrough
when cfg.DisableRateLimiting is true; add a one-time startup warning via the
standard logger (e.g., slog.Warn) when this flag is set so operators see that
rate limiting (and brute-force protection) is disabled; implement the warning at
middleware construction in RateLimitMiddleware (guarded so it only logs once)
and ensure the message references DisableRateLimiting and the security impact
(e.g., removal of brute-force protection on login). Also update the
configuration docs/sample to document the new DisableRateLimiting setting and
its security implications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e9a778a2-5e0b-43f6-945e-32dc7980ec86
📒 Files selected for processing (11)
cmd/api/src/api/middleware/rate_limit.gocmd/api/src/api/middleware/rate_limit_test.gocmd/api/src/api/registration/registration.gocmd/api/src/api/registration/v2.gocmd/api/src/config/config.gocmd/api/src/config/config_test.gocmd/api/src/config/default.gocmd/api/src/services/entrypoint.godocker-compose.dev.ymlexamples/docker-compose/.env.exampleexamples/docker-compose/docker-compose.yml
leechristensen
force-pushed
the
disable-rate-limiting-option
branch
from
50d56e9 to
24c5408
Compare
Author
|
recheck |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/api/middleware/rate_limit_test.go (1)
115-137: Optional: split the helper to avoid the unusedlimitwhen using the default middleware.The
useDefaultRateLimit bool, limit int64shape requires callers to pass a no-op0when picking the default middleware (e.g., line 76 and line 96). Two small helpers (or a functional-option style) would make call sites self-documenting and remove the dead parameter:♻️ Suggested refactor
-func newRateLimitedRouter(t *testing.T, cfg config.Configuration, useDefaultRateLimit bool, limit int64) (*mux.Router, *CountingHandler) { - t.Helper() - - mockCtl := gomock.NewController(t) - mockDatabase := mocks.NewMockDatabase(mockCtl) - - if !cfg.DisableRateLimiting { - mockDatabase.EXPECT().GetConfigurationParameter(gomock.Any(), appcfg.TrustedProxiesConfig).Return(appcfg.Parameter{}, nil).AnyTimes() - } - - testHandler := &CountingHandler{} - router := mux.NewRouter() - - if useDefaultRateLimit { - router.Use(middleware.DefaultRateLimitMiddleware(cfg, mockDatabase)) - } else { - router.Use(middleware.RateLimitMiddleware(cfg, mockDatabase, limit)) - } - - router.Handle("/teapot", testHandler) - - return router, testHandler -} +func newRateLimitedRouterWithLimit(t *testing.T, cfg config.Configuration, limit int64) (*mux.Router, *CountingHandler) { + t.Helper() + return buildRouter(t, cfg, func(db database.Database) mux.MiddlewareFunc { + return middleware.RateLimitMiddleware(cfg, db, limit) + }) +} + +func newDefaultRateLimitedRouter(t *testing.T, cfg config.Configuration) (*mux.Router, *CountingHandler) { + t.Helper() + return buildRouter(t, cfg, func(db database.Database) mux.MiddlewareFunc { + return middleware.DefaultRateLimitMiddleware(cfg, db) + }) +}(Plus a small private
buildRouterhelper holding the shared mock + handler wiring.)Feel free to skip if you prefer to keep diffs minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/middleware/rate_limit_test.go` around lines 115 - 137, The helper newRateLimitedRouter currently takes (useDefaultRateLimit bool, limit int64) which forces callers to pass a dummy limit when using DefaultRateLimitMiddleware; split it into two small helpers—e.g., newRateLimitedRouterWithDefault(cfg, ...) that calls router.Use(middleware.DefaultRateLimitMiddleware(cfg, mockDatabase)) and newRateLimitedRouterWithLimit(cfg, limit, ...) that calls router.Use(middleware.RateLimitMiddleware(cfg, mockDatabase, limit))—or implement a small private buildRouter to factor shared setup (mockDatabase creation, CountingHandler, router.Handle) and then two thin wrappers that call DefaultRateLimitMiddleware or RateLimitMiddleware respectively; update call sites to use the clearer helpers and remove the unused limit parameter path in the original function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/api/src/api/middleware/rate_limit_test.go`:
- Around line 115-137: The helper newRateLimitedRouter currently takes
(useDefaultRateLimit bool, limit int64) which forces callers to pass a dummy
limit when using DefaultRateLimitMiddleware; split it into two small
helpers—e.g., newRateLimitedRouterWithDefault(cfg, ...) that calls
router.Use(middleware.DefaultRateLimitMiddleware(cfg, mockDatabase)) and
newRateLimitedRouterWithLimit(cfg, limit, ...) that calls
router.Use(middleware.RateLimitMiddleware(cfg, mockDatabase, limit))—or
implement a small private buildRouter to factor shared setup (mockDatabase
creation, CountingHandler, router.Handle) and then two thin wrappers that call
DefaultRateLimitMiddleware or RateLimitMiddleware respectively; update call
sites to use the clearer helpers and remove the unused limit parameter path in
the original function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1a17c085-20da-48b6-b204-86681973039a
📒 Files selected for processing (11)
cmd/api/src/api/middleware/rate_limit.gocmd/api/src/api/middleware/rate_limit_test.gocmd/api/src/api/registration/registration.gocmd/api/src/api/registration/v2.gocmd/api/src/config/config.gocmd/api/src/config/config_test.gocmd/api/src/config/default.gocmd/api/src/services/entrypoint.godocker-compose.dev.ymlexamples/docker-compose/.env.exampleexamples/docker-compose/docker-compose.yml
✅ Files skipped from review due to trivial changes (5)
- examples/docker-compose/docker-compose.yml
- cmd/api/src/config/default.go
- cmd/api/src/services/entrypoint.go
- examples/docker-compose/.env.example
- docker-compose.dev.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/api/src/config/config.go
- cmd/api/src/api/registration/registration.go
- cmd/api/src/config/config_test.go
- cmd/api/src/api/registration/v2.go
leechristensen
changed the title
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.
Description
Adds an environment variables to disable API/login rate limiting:
Addresses #1444 as well.
Motivation and Context
For testing purposes or in controlled networks(where BloodHound has restricted access), the rate limits get in the way. Examples include AI automations or when wanting to do security testing against the APIs.
Why is this change required? What problem does it solve?
The rate limiting slows down testing and presents an unecesary hurdle in controlled/testing environments.
How Has This Been Tested?
Enabled the new configuration flag and tested that the rate limits no longer existed.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
Tests