Disable ratelimits on saas#4073
Conversation
|
Review Complete Files Reviewed: 1 By Severity:
The PR introduces a Files Reviewed (1 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (72/100) — 1 high finding · 33 LOC across 1 file
Overview
This PR modifies controllers/rate_limit.go to gate the authentication rate-limiting middleware behind a DeployedByOperator() check. When the operator/SaaS deployment mode is active, the entire rate-limiting and lockout logic for POST /api/users/adm/authenticate becomes a no-op.
Security Concern
controllers/rate_limit.go:115— Theif !servercfg.DeployedByOperator()conditional wraps lines 116-132, silently disabling brute-force protection for the auth endpoint.- No startup warning —
main.gohas no logging or validation to alert operators that in-app rate limiting is disabled whenDEPLOYED_BY_OPERATORis set. - No defense-in-depth — The rate limiter struct and cleanup goroutine are still allocated (
controllers/controller.go), but the middleware is dead code in operator mode, creating a hidden posture gap. - A single misconfiguration (env var or YAML field) propagates silently, leaving the auth endpoint vulnerable to credential brute-forcing if external ingress rate limiting is not properly configured.
Recommendation
Add a startup warning log when DeployedByOperator() is true, and consider retaining a relaxed defense-in-depth rate limit as a backstop against misconfigured external rate limiting.
| if !servercfg.DeployedByOperator() { | ||
| route, err := mux.CurrentRoute(r).GetPathTemplate() | ||
| if err != nil { | ||
| logic.ReturnErrorResponse(w, r, logic.FormatError(err, "badrequest")) | ||
| return | ||
| } | ||
|
|
||
| if r.Method == http.MethodPost && route == "/api/users/adm/authenticate" { | ||
| ip := clientIP(r) | ||
| if r.Method == http.MethodPost && route == "/api/users/adm/authenticate" { | ||
| ip := clientIP(r) | ||
|
|
||
| ok, retryAfter := rl.allowAuthenticate(ip) | ||
| if !ok { | ||
| w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) | ||
| http.Error(w, "too many requests", http.StatusTooManyRequests) | ||
| return | ||
| ok, retryAfter := rl.allowAuthenticate(ip) | ||
| if !ok { | ||
| w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) | ||
| http.Error(w, "too many requests", http.StatusTooManyRequests) | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 DeployedByOperator flag silently disables all auth rate limiting without operational signal (security)
The PR introduces a conditional at controllers/rate_limit.go:115 — if !servercfg.DeployedByOperator() — that wraps the entire rate-limiting and lockout logic for the POST /api/users/adm/authenticate endpoint. When DeployedByOperator() returns true (controlled by the DEPLOYED_BY_OPERATOR environment variable or server.deployed_by_operator config field), the rate limiter middleware becomes a complete passthrough with no defense-in-depth fallback, no log warning, and no startup validation. An operator deploying with deployed_by_operator=true but without properly configured external ingress rate limiting (e.g., nginx ingress annotations) would have an unprotected auth endpoint vulnerable to credential brute-forcing. The DeployedByOperator() function (servercfg/serverconf.go:749-754) checks an env var or YAML config field — a single misconfiguration propagates silently. The rate limiter struct and its cleanup goroutine are still allocated (controllers/controller.go), but the middleware is dead code when operator-mode is active, creating a hidden security posture gap.
💡 Suggestion: At minimum, log a prominent warning at startup when DeployedByOperator() is true to alert operators that in-app auth rate limiting is disabled and external rate limiting must be configured on the ingress/API gateway. Consider retaining a relaxed defense-in-depth rate limit even in operator mode (e.g., higher threshold like 100 req/min) as a backstop against misconfigured external rate limiting. Alternatively, require a separate explicit config flag (e.g., OPERATOR_RATE_LIMIT_EXTERNAL=true) before fully disabling local rate limiting.
📋 Prompt for AI Agents
In main.go, inside the initialize() function (around line 110-117, near the existing HA mode logging block), add a conditional check: if servercfg.DeployedByOperator() is true, log a warning-level message such as 'Operator/SaaS mode detected: in-app auth rate limiting on POST /api/users/adm/authenticate is disabled. Ensure external rate limiting is configured on the ingress or API gateway.' This gives operators visibility into the security posture change. The logger package is already imported in main.go (line 27).
2 participants
Describe your changes
Provide Issue ticket number if applicable/not in title
Provide testing steps
Checklist before requesting a review