Repository navigation
feat(ratelimit): add authentication rate limiting - #8
Conversation
📝 WalkthroughWalkthroughThe pull request adds configurable authentication rate limiting across HTTP handlers, memory, PostgreSQL, and Redis stores. It adds startup validation, trusted-proxy handling, audit and Prometheus events, integration tests, Redis CI services, and coverage tooling. ChangesAuthentication rate limiting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler
participant RateLimiter
participant RateLimitStore
participant AuditLogger
Client->>AuthHandler: Send authentication request
AuthHandler->>RateLimiter: Check configured policies
RateLimiter->>RateLimitStore: Allow hashed key
RateLimitStore-->>RateLimiter: Return decision
RateLimiter-->>AuthHandler: Return allowance or retry duration
AuthHandler->>AuditLogger: Record denial or backend failure
AuthHandler-->>Client: Continue, 429, or 503
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
internal/platform/metrics_test.go (1)
17-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the emitted rate-limit dimensions.
The metric test passes if all labels normalize to
unknown. Assert route, policy, backend, outcome, and reason labels. Add coverage forEventRateLimiterUnavailable.Set
RouteinTestAuditLoggerUsesSafeFields. Assert the normalized route attribute in the audit record.Also applies to: 54-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/metrics_test.go` around lines 17 - 38, Strengthen TestAuditLoggerRecordsRateLimitMetric by asserting the emitted metric’s route, policy, backend, outcome, and reason labels retain their expected normalized values rather than all becoming unknown, and add coverage for EventRateLimiterUnavailable. In TestAuditLoggerUsesSafeFields, provide a Route value and assert the audit record contains the normalized route attribute.internal/platform/metrics.go (1)
130-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
normalizeRouteand middleware metric routes in sync.
normalizeRoutemaps every enforced rate-limit path to a known label, butinternal/adapter/inbound/http/middleware/metrics.goalso normalizesMiddlewareRequestCountroutes and is not covered by the current test. Add a table-driven test for both metric route normalizations, using the registered Gin paths that actually receive metrics logging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/metrics.go` around lines 130 - 146, Add a table-driven test covering normalizeRoute and the middleware’s MiddlewareRequestCount route normalization, using the registered Gin paths that emit metrics. Include every enforced rate-limit path and assert known labels for supported routes plus “unknown” for unsupported paths, keeping both normalization implementations synchronized.internal/platform/redis.go (1)
5-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd timeout and TLS configuration options for Redis.
NewRedisClientonly setsAddr,Password, andDB. Many managed Redis deployments require TLS, and missingDialTimeout,ReadTimeout, orWriteTimeoutlets the rate-limit path hang on a stalled Redis connection.Add TLS support behind an optional configuration flag and wire in reasonable connection/read/write timeouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/redis.go` around lines 5 - 11, Update NewRedisClient to configure reasonable dial, read, and write timeouts in redis.Options, and add TLS configuration controlled by an optional configuration flag. Preserve the existing address, password, and database settings, enabling redis.Options.TLSConfig only when the flag requests TLS.go.mod (1)
16-16: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBump
github.com/redis/go-redis/v9fromv9.7.0.
v9.7.0is several major-minor versions behind the latest v9 releases. Later v9 fixes include connection-pool turn-management handling andOptions.clone()use-after-free behavior, both relevant to a request-path rate limiter that uses Redis. Upgrade within v9 to a current release and run the existing unit/race/integration tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 16, Update the github.com/redis/go-redis/v9 dependency in go.mod from v9.7.0 to the current compatible v9 release, refresh the module checksums as needed, and run the existing unit, race, and integration tests to verify the request-path rate limiter remains compatible.internal/bootstrap/ratelimit.go (1)
11-23: 🧹 Nitpick | 🔵 TrivialConsider a startup Redis health check.
platform.NewRedisClientdoes not verify connectivity beforenewRateLimitStorereturns. Sincego-redisconnects lazily, a misconfigured or unreachable Redis instance surfaces only on the first request (as a 503 from the rate limiter), not at boot.Add a
Pingcall after client construction and fail fast on error, consistent with the fail-closed design goal for this feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bootstrap/ratelimit.go` around lines 11 - 23, Update newRateLimitStore’s "redis" branch to synchronously Ping the client returned by platform.NewRedisClient before constructing or returning the Redis store; if Ping fails, return the error with nil store/client values so startup fails fast, while preserving existing behavior for successful connections and other backends.internal/platform/rate_limiter.go (1)
33-49: 🎯 Functional Correctness | 🔵 TrivialUnreachable multi-check
RetryAfteraggregation.The loop returns as soon as one check is denied (Line 46-48). This means the
decision.RetryAfter > retryAftercomparison at Line 43 never sees more than one denied check's value before the function returns. The aggregation logic implies intent to evaluate all checks and return the worst-case wait time, but the early return prevents that.This shares one root cause with
checks[0].Policyininternal/adapter/inbound/http/rate_limit.go'slogRateLimitEvent: bundling several checks into oneAllow(checks...)call removes the caller's ability to know which specific check triggered a denial. See the consolidated comment for the recommended fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/rate_limiter.go` around lines 33 - 49, Update the check loop in the rate limiter’s Allow flow to evaluate every policy before returning, tracking whether any decision was denied and retaining the maximum RetryAfter value. Return the aggregated denial decision only after all checks complete, while preserving immediate propagation of backend errors and valid all-allowed behavior.internal/adapter/inbound/http/oauth_handler.go (1)
22-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd provider-scoped OAuth rate limiting if OAuth quotas need protection.
OAuthLoginandOAuthCallbackonly checkipRateLimit("oauth_ip", c), while the OAuth routes already split by:provider. If separating GitHub and Google request counts is needed to protect upstream OAuth limits, add a separate per-provider rate limit path in addition to the IP check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/inbound/http/oauth_handler.go` around lines 22 - 27, Add provider-scoped rate limiting to the OAuth flow, using the provider parameter in OAuthLogin and the corresponding OAuthCallback path in addition to the existing IP-based check. Reuse the established rate-limit mechanism and ensure requests are rejected when either the IP or provider quota is exceeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 17-24: Add a host-to-container port mapping under the redis
service so host-side integration and coverage tests can reach Redis at
localhost:6379, while preserving the existing healthcheck and Redis image
configuration.
- Around line 38-40: Remove the REDIS_ADDR, REDIS_PASSWORD, and REDIS_DB entries
from the production docker-compose configuration so values loaded from .env.prod
remain authoritative. If local Redis defaults are needed, move these settings to
a separate local-only Compose override.
In `@internal/adapter/inbound/http/rate_limit.go`:
- Around line 29-74: The allowRateLimit method currently bundles checks,
preventing accurate denial attribution; iterate through each check and call
RateLimiter.Allow individually, stopping at the first denial or backend error
and logging that specific check. Update
internal/adapter/inbound/http/rate_limit.go lines 29-74 accordingly. In
internal/platform/rate_limiter.go lines 33-49, simplify the multi-check
RetryAfter aggregation in Allow or document that its multi-check path is no
longer used, without changing single-check behavior.
In `@internal/adapter/outbound/ratelimit/memory.go`:
- Around line 35-51: Remove the full s.entries expiry sweep from
MemoryStore.Allow and keep expiration handling limited to the requested key;
also move evictOldest’s map-wide scan out of request processing where
applicable. Add a periodic background cleanup loop, following
PostgresStore.cleanupLoop, to reclaim expired entries, and update
MemoryStore.Close to stop and await that goroutine so shutdown does not leak
resources.
In `@internal/bootstrap/bootstrap.go`:
- Around line 47-54: Update the deferred shutdown calls after newRateLimitStore
in the bootstrap flow to use small defer functions that invoke
rateLimitStore.Close and redisClient.Close, check each returned error, and log
any shutdown failures through the existing logger. Preserve the conditional
redisClient cleanup and ensure both backend close errors are surfaced.
In `@Makefile`:
- Around line 11-12: Update the coverage recipe that runs go test to create the
parent directory of the configured COVERAGE_FILE before writing coverage output,
rather than relying only on COVERAGE_DIR. Preserve support for the default
COVERAGE_FILE and independently overridden paths.
---
Nitpick comments:
In `@go.mod`:
- Line 16: Update the github.com/redis/go-redis/v9 dependency in go.mod from
v9.7.0 to the current compatible v9 release, refresh the module checksums as
needed, and run the existing unit, race, and integration tests to verify the
request-path rate limiter remains compatible.
In `@internal/adapter/inbound/http/oauth_handler.go`:
- Around line 22-27: Add provider-scoped rate limiting to the OAuth flow, using
the provider parameter in OAuthLogin and the corresponding OAuthCallback path in
addition to the existing IP-based check. Reuse the established rate-limit
mechanism and ensure requests are rejected when either the IP or provider quota
is exceeded.
In `@internal/bootstrap/ratelimit.go`:
- Around line 11-23: Update newRateLimitStore’s "redis" branch to synchronously
Ping the client returned by platform.NewRedisClient before constructing or
returning the Redis store; if Ping fails, return the error with nil store/client
values so startup fails fast, while preserving existing behavior for successful
connections and other backends.
In `@internal/platform/metrics_test.go`:
- Around line 17-38: Strengthen TestAuditLoggerRecordsRateLimitMetric by
asserting the emitted metric’s route, policy, backend, outcome, and reason
labels retain their expected normalized values rather than all becoming unknown,
and add coverage for EventRateLimiterUnavailable. In
TestAuditLoggerUsesSafeFields, provide a Route value and assert the audit record
contains the normalized route attribute.
In `@internal/platform/metrics.go`:
- Around line 130-146: Add a table-driven test covering normalizeRoute and the
middleware’s MiddlewareRequestCount route normalization, using the registered
Gin paths that emit metrics. Include every enforced rate-limit path and assert
known labels for supported routes plus “unknown” for unsupported paths, keeping
both normalization implementations synchronized.
In `@internal/platform/rate_limiter.go`:
- Around line 33-49: Update the check loop in the rate limiter’s Allow flow to
evaluate every policy before returning, tracking whether any decision was denied
and retaining the maximum RetryAfter value. Return the aggregated denial
decision only after all checks complete, while preserving immediate propagation
of backend errors and valid all-allowed behavior.
In `@internal/platform/redis.go`:
- Around line 5-11: Update NewRedisClient to configure reasonable dial, read,
and write timeouts in redis.Options, and add TLS configuration controlled by an
optional configuration flag. Preserve the existing address, password, and
database settings, enabling redis.Options.TLSConfig only when the flag requests
TLS.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e61a6ff-9442-4d5c-809f-888a7ebf9bd6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.env.example.github/workflows/ci.yml.gitignoreMakefileREADME.mddocker-compose.ymlgo.modinternal/adapter/inbound/http/auth_handler.gointernal/adapter/inbound/http/handler.gointernal/adapter/inbound/http/oauth_handler.gointernal/adapter/inbound/http/rate_limit.gointernal/adapter/inbound/http/rate_limit_test.gointernal/adapter/outbound/postgres/db/schema.sqlinternal/adapter/outbound/postgres/models.gointernal/adapter/outbound/ratelimit/contract_test.gointernal/adapter/outbound/ratelimit/memory.gointernal/adapter/outbound/ratelimit/memory_test.gointernal/adapter/outbound/ratelimit/postgres.gointernal/adapter/outbound/ratelimit/postgres_integration_test.gointernal/adapter/outbound/ratelimit/redis.gointernal/adapter/outbound/ratelimit/redis_integration_test.gointernal/bootstrap/bootstrap.gointernal/bootstrap/ratelimit.gointernal/core/port/rate_limiter.gointernal/platform/audit.gointernal/platform/config.gointernal/platform/metrics.gointernal/platform/metrics_test.gointernal/platform/rate_limit_config.gointernal/platform/rate_limit_config_test.gointernal/platform/rate_limiter.gointernal/platform/rate_limiter_test.gointernal/platform/redis.gomigrations/000003_rate_limits.down.sqlmigrations/000003_rate_limits.up.sqlsonar-project.properties
| redis: | ||
| image: redis:7-alpine | ||
| container_name: auth-go-redis | ||
| healthcheck: | ||
| test: ["CMD", "redis-cli", "ping"] | ||
| interval: 3s | ||
| timeout: 3s | ||
| retries: 5 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Expose Redis for host-side integration tests.
README.md Line 35 starts the Compose Redis service, and the example configuration uses REDIS_ADDR=localhost:6379. This service has no host port mapping, so host-side make integration and make coverage runs cannot reach Redis.
Proposed fix
redis:
image: redis:7-alpine
container_name: auth-go-redis
+ ports:
+ - "6379:6379"
healthcheck:📝 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.
| redis: | |
| image: redis:7-alpine | |
| container_name: auth-go-redis | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "ping"] | |
| interval: 3s | |
| timeout: 3s | |
| retries: 5 | |
| redis: | |
| image: redis:7-alpine | |
| container_name: auth-go-redis | |
| ports: | |
| - "6379:6379" | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "ping"] | |
| interval: 3s | |
| timeout: 3s | |
| retries: 5 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 17 - 24, Add a host-to-container port
mapping under the redis service so host-side integration and coverage tests can
reach Redis at localhost:6379, while preserving the existing healthcheck and
Redis image configuration.
| func (h *Handler) allowRateLimit(c *gin.Context, checks ...platform.RateLimitCheck) bool { | ||
| if h.RateLimiter == nil { | ||
| return true | ||
| } | ||
| decision, err := h.RateLimiter.Allow(c.Request.Context(), checks...) | ||
| if err != nil { | ||
| h.logRateLimitEvent(c, platform.EventRateLimiterUnavailable, platform.OutcomeFailure, platform.ReasonBackendUnavailable, checks) | ||
| c.Header("Retry-After", retryAfterHeader(backendRetryAfter)) | ||
| c.Header("Cache-Control", "no-store") | ||
| c.AbortWithStatusJSON(http.StatusServiceUnavailable, gin.H{"error": "authentication temporarily unavailable"}) | ||
| return false | ||
| } | ||
| if decision.Allowed { | ||
| return true | ||
| } | ||
|
|
||
| h.logRateLimitEvent(c, platform.EventAuthRateLimited, platform.OutcomeDetected, platform.ReasonRateLimited, checks) | ||
| c.Header("Retry-After", retryAfterHeader(decision.RetryAfter)) | ||
| c.Header("Cache-Control", "no-store") | ||
| c.AbortWithStatusJSON(http.StatusTooManyRequests, gin.H{"error": "too many requests"}) | ||
| return false | ||
| } | ||
|
|
||
| func (h *Handler) resetRateLimit(c *gin.Context, check platform.RateLimitCheck) { | ||
| if h.RateLimiter == nil { | ||
| return | ||
| } | ||
| if err := h.RateLimiter.Reset(c.Request.Context(), check); err != nil { | ||
| h.logRateLimitEvent(c, platform.EventRateLimiterUnavailable, platform.OutcomeFailure, platform.ReasonBackendUnavailable, []platform.RateLimitCheck{check}) | ||
| } | ||
| } | ||
|
|
||
| func (h *Handler) logRateLimitEvent(c *gin.Context, event, outcome, reason string, checks []platform.RateLimitCheck) { | ||
| if h.Audit == nil || len(checks) == 0 { | ||
| return | ||
| } | ||
| h.Audit.Log(c.Request.Context(), platform.AuditEvent{ | ||
| Name: event, | ||
| Outcome: outcome, | ||
| RequestID: requestID(c), | ||
| Reason: reason, | ||
| Route: c.FullPath(), | ||
| Policy: checks[0].Policy, | ||
| Backend: h.RateLimiter.Backend(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bundling multiple checks into one Allow call breaks audit attribution and leaves aggregation logic unreachable. Both symptoms share one root cause: allowRateLimit passes every check for a handler (for example, IP and email) to a single Allow(checks...) call, so neither the caller nor Allow itself can identify which specific check was denied.
internal/adapter/inbound/http/rate_limit.go#L29-L74: changeallowRateLimitto callh.RateLimiter.Allow(ctx, check)once per check in a loop, stopping and logging at the first denial, sologRateLimitEventrecords the policy of the check that actually failed instead of always usingchecks[0].Policy.internal/platform/rate_limiter.go#L33-L49: onceAllowis only ever invoked with a single check (per the fix above), thedecision.RetryAfter > retryAfteraggregation across multiple denied checks becomes unreachable in practice; simplify the loop accordingly or leave a comment noting the multi-check path is unused, to avoid confusing future maintainers about its intended behavior.
📍 Affects 2 files
internal/adapter/inbound/http/rate_limit.go#L29-L74(this comment)internal/platform/rate_limiter.go#L33-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/adapter/inbound/http/rate_limit.go` around lines 29 - 74, The
allowRateLimit method currently bundles checks, preventing accurate denial
attribution; iterate through each check and call RateLimiter.Allow individually,
stopping at the first denial or backend error and logging that specific check.
Update internal/adapter/inbound/http/rate_limit.go lines 29-74 accordingly. In
internal/platform/rate_limiter.go lines 33-49, simplify the multi-check
RetryAfter aggregation in Allow or document that its multi-check path is no
longer used, without changing single-check behavior.
| func (s *MemoryStore) Allow(ctx context.Context, key string, policy port.RateLimitPolicy) (port.RateLimitDecision, error) { | ||
| if err := ctx.Err(); err != nil { | ||
| return port.RateLimitDecision{}, err | ||
| } | ||
| if policy.Limit < 1 || policy.Window <= 0 { | ||
| return port.RateLimitDecision{}, port.ErrRateLimitBackendUnavailable | ||
| } | ||
|
|
||
| now := s.now() | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| for storedKey, entry := range s.entries { | ||
| if !now.Before(entry.started.Add(entry.window)) { | ||
| delete(s.entries, storedKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Move full-map expiry sweep off the request hot path.
Allow scans the entire s.entries map on every call while holding s.mu (Lines 47-51). This scan is not needed for correctness: the per-key check at Lines 52-59 already handles both the missing-key and expired-key cases for the requested key on its own. The sweep only reclaims memory for unrelated keys, but it does so under lock on every single request, turning an O(1) hot path into an O(n) scan (bounded by maxKeys) that serializes all Allow/Reset calls on the store.
evictOldest (Lines 86-98) has the same O(n) shape and runs whenever a new key arrives at capacity, compounding cost during bursts of new distinct keys.
Move periodic reclamation into a background goroutine, similar to PostgresStore.cleanupLoop in postgres.go (Lines 126-140), with a corresponding shutdown hook in Close() to avoid a goroutine leak.
♻️ Proposed fix to move expiry reclamation off the hot path
type MemoryStore struct {
mu sync.Mutex
entries map[string]memoryEntry
maxKeys int
now func() time.Time
+ cancel context.CancelFunc
+ wg sync.WaitGroup
}
func NewMemoryStore(maxKeys int) *MemoryStore {
if maxKeys < 1 {
maxKeys = 10_000
}
- return &MemoryStore{
+ ctx, cancel := context.WithCancel(context.Background())
+ store := &MemoryStore{
entries: make(map[string]memoryEntry),
maxKeys: maxKeys,
now: time.Now,
+ cancel: cancel,
}
+ store.wg.Add(1)
+ go store.cleanupLoop(ctx)
+ return store
}
func (s *MemoryStore) Allow(ctx context.Context, key string, policy port.RateLimitPolicy) (port.RateLimitDecision, error) {
if err := ctx.Err(); err != nil {
return port.RateLimitDecision{}, err
}
if policy.Limit < 1 || policy.Window <= 0 {
return port.RateLimitDecision{}, port.ErrRateLimitBackendUnavailable
}
now := s.now()
s.mu.Lock()
defer s.mu.Unlock()
- for storedKey, entry := range s.entries {
- if !now.Before(entry.started.Add(entry.window)) {
- delete(s.entries, storedKey)
- }
- }
entry, exists := s.entries[key]
@@
-func (s *MemoryStore) Close() error { return nil }
+func (s *MemoryStore) Close() error {
+ if s.cancel != nil {
+ s.cancel()
+ s.wg.Wait()
+ }
+ return nil
+}
+
+func (s *MemoryStore) cleanupLoop(ctx context.Context) {
+ defer s.wg.Done()
+ ticker := time.NewTicker(time.Minute)
+ defer ticker.Stop()
+ for {
+ select {
+ case <-ctx.Done():
+ return
+ case <-ticker.C:
+ now := s.now()
+ s.mu.Lock()
+ for storedKey, entry := range s.entries {
+ if !now.Before(entry.started.Add(entry.window)) {
+ delete(s.entries, storedKey)
+ }
+ }
+ s.mu.Unlock()
+ }
+ }
+}Also applies to: 86-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/adapter/outbound/ratelimit/memory.go` around lines 35 - 51, Remove
the full s.entries expiry sweep from MemoryStore.Allow and keep expiration
handling limited to the requested key; also move evictOldest’s map-wide scan out
of request processing where applicable. Add a periodic background cleanup loop,
following PostgresStore.cleanupLoop, to reclaim expired entries, and update
MemoryStore.Close to stop and await that goroutine so shutdown does not leak
resources.
| rateLimitStore, redisClient, err := newRateLimitStore(cfg.RateLimit, pool) | ||
| if err != nil { | ||
| log.Fatal("rate-limit backend is unavailable") | ||
| } | ||
| defer rateLimitStore.Close() | ||
| if redisClient != nil { | ||
| defer redisClient.Close() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- ratelimit/redis.go ----"
fd -a redis.go internal/adapter/outbound/ratelimit --exec cat -n {}
echo "---- platform/redis.go ----"
fd -a redis.go internal/platform --exec cat -n {}Repository: Jonathan0823/auth-go
Length of output: 3691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- internal/bootstrap/bootstrap.go relevant section ----"
sed -n '1,90p' internal/bootstrap/bootstrap.go | cat -n
echo "---- all Close() usages near rate-limit/bootstrap ----"
rg -n "newRateLimitStore|RateLimitStore|Red.?is?Store|\\.Close\\(" internal/bootstrap/internal/adapter/outbound/ratelimit internal/platform 2>/dev/null || trueRepository: Jonathan0823/auth-go
Length of output: 4134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- find rate limit store implementations ----"
git ls-files | rg 'internal/adapter/outbound/ratelimit|internal/platform|internal/bootstrap' | rg '(rate|redis|bootstrap)' || true
echo "---- usages of newRateLimitStore ----"
rg -n "newRateLimitStore|func newRateLimitStore|interface|New(Redis|Memory|Rate)Store|Close\\(\\) error" .Repository: Jonathan0823/auth-go
Length of output: 4456
Log the unchecked Close error returns.
Both deferred shutdown calls ignore error returns; wrap them in small defer functions and log any errors so backend shutdown failures surface.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 51-51: Error return value of rateLimitStore.Close is not checked
(errcheck)
[error] 53-53: Error return value of redisClient.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/bootstrap/bootstrap.go` around lines 47 - 54, Update the deferred
shutdown calls after newRateLimitStore in the bootstrap flow to use small defer
functions that invoke rateLimitStore.Close and redisClient.Close, check each
returned error, and log any shutdown failures through the existing logger.
Preserve the conditional redisClient cleanup and ensure both backend close
errors are surfaced.
Source: Linters/SAST tools
| COVERAGE_DIR ?= coverage | ||
| COVERAGE_FILE ?= $(COVERAGE_DIR)/coverage.out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Create the parent directory for COVERAGE_FILE.
COVERAGE_FILE can be overridden independently from COVERAGE_DIR, but the recipe creates only COVERAGE_DIR. make COVERAGE_FILE=artifacts/coverage.out coverage fails when artifacts/ does not exist.
Create the configured file's parent directory before running go test.
Proposed fix
coverage: migrate-up
- mkdir -p "$(COVERAGE_DIR)"
+ mkdir -p "$(COVERAGE_DIR)" "$(dir $(COVERAGE_FILE))"
go test -tags=integration ./... -covermode=atomic -coverprofile="$(COVERAGE_FILE)"Also applies to: 22-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 11 - 12, Update the coverage recipe that runs go test
to create the parent directory of the configured COVERAGE_FILE before writing
coverage output, rather than relying only on COVERAGE_DIR. Preserve support for
the default COVERAGE_FILE and independently overridden paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 22: Update the RATE_LIMIT_KEY configuration validation to reject the
default placeholder value change-me-rate-limit-key when running in production,
while continuing to require a non-empty deployment-specific random value. Locate
the existing startup validation for RATE_LIMIT_KEY and add this production-only
check without changing development behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e2da59-8ffc-428c-b993-bde23df3ff08
📒 Files selected for processing (4)
.env.exampledocker-compose.ymlinternal/adapter/outbound/ratelimit/postgres.gointernal/platform/rate_limit_config.go
💤 Files with no reviewable changes (1)
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/adapter/outbound/ratelimit/postgres.go
| # Authentication rate limiting | ||
| # Options: memory, redis, postgres. | ||
| RATE_LIMIT_BACKEND=memory | ||
| RATE_LIMIT_KEY=change-me-rate-limit-key |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'RATE_LIMIT_KEY|change-me-rate-limit-key|production' internal/platform internal/bootstrap cmdRepository: Jonathan0823/auth-go
Length of output: 7039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
git ls-files | rg '(^|/)(rate_limit_config\.go|bootstrap|main\.go|Dockerfile|docker-compose|compose\.ya?ml|\.env\.example$)' || true
printf '\n--- rate_limit_config.go ---\n'
sed -n '1,140p' internal/platform/rate_limit_config.go
printf '\n--- usage of Validate and environment loading ---\n'
rg -n -C 4 'Validate\(|LoadRateLimitConfig|\bEnvironment\b|\bENVIRONMENT\b|RATE_LIMIT_KEY|change-me-rate-limit-key' -g '*.go' -g 'Dockerfile' -g 'docker-compose*.yml' -g 'compose*.ya?ml' -g '*.yaml' -g '*.yml' .Repository: Jonathan0823/auth-go
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(rate_limit_config\.go|bootstrap|main\.go|Dockerfile|docker-compose|compose\.ya?ml|\.env\.example$)' || true
printf '%s\n' ''
printf '%s\n' '--- rate_limit_config.go ---'
sed -n '1,140p' internal/platform/rate_limit_config.go
printf '%s\n' ''
printf '%s\n' '--- usage of Validate and environment loading ---'
grep -RInE 'Validate\(|LoadRateLimitConfig|\b(Environment|EnvironmentFunc|Environment)\b|\b(ENVIRONMENT|MODE|APP_ENV)\b|RATE_LIMIT_KEY|change-me-rate-limit-key' -g '*.go' -g 'Dockerfile' -g 'docker-compose*.yml' -g 'compose*.ya?ml' -g '*.yaml' -g '*.yml' . || trueRepository: Jonathan0823/auth-go
Length of output: 5230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cmd/auth-go/main.go ---'
sed -n '1,220p' cmd/auth-go/main.go
echo ''
echo '--- internal/bootstrap/ratelimit.go ---'
sed -n '1,180p' internal/bootstrap/ratelimit.go
echo ''
echo '--- internal/bootstrap/bootstrap.go ---'
sed -n '1,220p' internal/bootstrap/bootstrap.go
echo ''
echo '--- Dockerfile ---'
sed -n '1,220p' Dockerfile
echo ''
echo '--- docker-compose.yml ---'
sed -n '1,220p' docker-compose.yml
echo ''
echo '--- .env.example ---'
sed -n '1,80p' .env.exampleRepository: Jonathan0823/auth-go
Length of output: 7537
Reject the placeholder rate-limit HMAC key in production.
RATE_LIMIT_KEY is loaded as-is and only fails validation when empty, so change-me-rate-limit-key can pass startup validation if copied into production. Add a production validation rule that rejects the placeholder and requires a deployment-specific random value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example at line 22, Update the RATE_LIMIT_KEY configuration validation
to reject the default placeholder value change-me-rate-limit-key when running in
production, while continuing to require a non-empty deployment-specific random
value. Locate the existing startup validation for RATE_LIMIT_KEY and add this
production-only check without changing development behavior.
Related issue
Summary
Scope
In scope
Out of scope
Testing
go test ./...go test -race ./...go vet ./...make integrationmake coveragemake sqlcgit diff --checkAll tests passed with PostgreSQL and Redis configured.
Reviewer focus
Risks and migrations
000003_rate_limitsforrate_limit_buckets.redisorpostgres) andRATE_LIMIT_KEY.REDIS_ADDR,REDIS_PASSWORD, andREDIS_DB.make migrate-up.Summary by CodeRabbit
New Features
Documentation
Tests