feat: add per-IP rate limiting for global and auth endpoints

[adnaan]

Summary

• Add brute-force protection via per-IP token-bucket rate limiting with LRU eviction (roadmap 1.2)
• Two rate limiter instances: global (100 rps) wrapping the entire mux, and auth-specific (0.1 rps / 6 req/min) wrapping login/register/reset endpoints
• New pkg/ratelimit/ library with functional options API, single-mutex + sharded implementations, and comprehensive tests
• Inline simplified rate limiter in generated main.go (can't import unreleased pkg/ratelimit, follow-up in Switch rate limiting template from inline to pkg/ratelimit/ #247)
• Hybrid deny handler: redirects browsers, returns JSON 429 for API clients
• All rates configurable via env vars (RATE_LIMIT_RPS, RATE_LIMIT_AUTH_RPS, etc.)

Test plan

• pkg/ratelimit/ — 20 tests covering LRU eviction, stale cleanup, concurrency, custom deny handlers, GetClientIP (race detector enabled)
• internal/generator/ — updated auth test fixtures for new handler signatures
• Integration test (TestGeneratedAppFullFlow) — generates full app with auth, builds, and runs tests
• Full test suite — all 31 packages pass with -race

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a solid implementation with well-structured code, security-conscious IP extraction, comprehensive test coverage, and good use of functional options.

Bugs / Correctness

Double appCancel() call in main.go.tmpl

appCancel is both deferred and called explicitly before graceful shutdown. This is safe (CancelFunc is idempotent) but slightly confusing. Consider relying on just the defer.

Compile-time shard size assertion - one-sided guard

The assertion fires if shard grows past 128 bytes, but passes silently if it shrinks below 128 bytes (padding becomes a no-op). Fine for the current layout; worth noting if the struct changes.

Security

getClientIP returns raw string when net.ParseIP fails

In both main.go.tmpl and pkg/ratelimit/ratelimit.go, when a proxy header contains a malformed value (hostname, ip:port, etc.), net.ParseIP returns nil and the raw trimmed string becomes the LRU map key. This does not create a security hole (a spoofed key just gives the attacker their own bucket), but arbitrary strings from untrusted headers can accumulate in the map. Falling back to the r.RemoteAddr host on parse failure would be safer.

Design and Maintainability

injectAuthRateLimiter anchor is fragile

The insertion point is found via strings.Index(mainContent, "authRL)"). Any prior occurrence of "authRL)" in the file - a comment, a future call - would inject code at the wrong location. A more specific anchor like the full "auth.Handler(queries, authRL)" string would reduce the blast radius. The idempotency check also matches "authRL=" without a space, which could collide with an unrelated identifier.

Inline duplication between main.go.tmpl and pkg/ratelimit

Acknowledged in the PR as blocked on issue 247 - fine to merge as-is. The inline version omits eviction logging and slog integration present in the package; if it stays around longer than expected those become observable gaps in generated apps.

Nits

• w.Write() return value is ignored in the injected JSON deny handler; capturing it silences linters.
• auth_test.go fixtures verify that route calls include authRL, but injectAuthRateLimiter is not tested directly. A test verifying the authRL declaration block is inserted at the correct location would close that gap.

Summary

The malformed XFF fallback and injection anchor fragility are worth addressing before merging. Core rate limiting logic, IP extraction security, sharding implementation, and pkg/ratelimit test coverage are all solid work.

[Copilot]

Pull request overview

Adds per-IP token-bucket rate limiting to generated apps (global + auth endpoints) and introduces a reusable pkg/ratelimit middleware library to support brute-force protection and general abuse mitigation.

Changes:

• Added pkg/ratelimit library with LRU eviction, optional sharded implementation, configurable options, and comprehensive tests.
• Updated the multi-kit main.go template to wrap the server with a global per-IP rate limiter and added inline helper functions (env parsing, IP extraction, LRU limiter).
• Updated auth generation/templates to support an optional authRL middleware and improved route injection to detect handler calls that take (queries, ...).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pkg/ratelimit/ratelimit.go	New rate limiting middleware implementation (single-mutex + sharded) with LRU eviction, cleanup goroutine, and client IP extraction.
pkg/ratelimit/ratelimit_test.go	Test suite covering limiting behavior, eviction, cleanup, concurrency, deny handler, and GetClientIP.
internal/kits/system/multi/templates/auth/handler.go.tmpl	Auth handlers accept optional middleware to apply auth-specific rate limiting.
internal/kits/system/multi/templates/app/main.go.tmpl	Adds global rate limiter + inline limiter implementation and env helpers to generated multi-kit apps.
internal/generator/route_injector.go	Treats handler calls containing (queries, as requiring DB queries initialization.
internal/generator/auth_test.go	Updates auth-related fixtures to match new handler signatures using authRL.
internal/generator/auth.go	Injects auth routes with authRL and adds logic to inject an auth rate limiter block into main.go.
commands/env.go	Documents RATE_LIMIT_* env vars in .env.example output and adds reason strings.

[claude]

PR Review: Per-IP Rate Limiting

Overall this is a solid implementation -- good security design, thoughtful IP detection, and strong test coverage in pkg/ratelimit. A few issues worth addressing before merge.

---

Bugs / Correctness

1. Auth rate-limiter injection may silently no-op

injectAuthRateLimiter returns an error if no auth.XxxHandler(queries, authRL) pattern is found, but the caller only prints a warning. If injection silently fails, the generated app will have authRL undeclared and the build fails at compile time rather than warning at generation time. Consider adding a post-injection validation step.

2. getEnvFloat/getEnvInt silently swallow invalid values

If an operator sets RATE_LIMIT_RPS=abc by mistake, the server starts with default config and no indication anything was wrong. Adding a slog.Warn on parse failure would make misconfiguration easier to diagnose in production. Applies to the inline implementation in main.go.tmpl.

3. RATE_LIMIT_AUTH_BURST=0 is silently clamped to 1

getEnvInt returns 0 successfully, which newRateLimiter clamps to 1. The operator gets burst=1 instead of what they configured, with no feedback.

---

Security

4. XFF header split uses i > 0 instead of i >= 0

A value like ,203.0.113.1 (leading comma) skips the split and the full string fails net.ParseIP, falling back to peer IP -- worst case is a false-negative on proxy trust, not spoofing. But i >= 0 matches the intent. Same pattern in both main.go.tmpl and pkg/ratelimit/ratelimit.go:

if i := strings.IndexByte(xff, ','); i > 0 { // should be i >= 0

5. Duplication between pkg/ratelimit and main.go.tmpl (tracked in issue 247)

Flagging subtle differences that may drift over time:

Aspect	pkg/ratelimit	main.go.tmpl inline
Eviction logging	slog.Warn	None
Graceful shutdown	done channel + Close()	Goroutine exits via appCancel
Content-Type on deny	Explicit	Implicit via http.Error
Sharding	Yes (>= 16 IPs)	No

6. unsafe.Sizeof compile-time assertion is opaque

const _ = uint(128 - unsafe.Sizeof(shard{}))

This produces a confusing "constant overflows uint" error if the struct grows beyond 128 bytes. A comment explaining the intent (two 64-byte cache lines on x86) and the architecture assumption would help future contributors.

7. newTestRL cleanup calls both cancel() and rl.Close()

rl.Close() alone is sufficient since it manages its own derived context. The dual-cancel pattern implies callers need to manage the outer context lifecycle, undermining the Close() contract.

---

Missing Tests

8. No unit tests for injectAuthRateLimiter

This is the most complex injection logic in the PR (regex matching, string splicing, idempotency check). A focused unit test with a known main.go fixture would catch regressions much more precisely than the integration test alone.

9. No tests for getEnvFloat/getEnvInt edge cases

Specifically: invalid strings, zero values, negative values, and burst=0 clamped to 1.

---

Minor

• authDeny sets Retry-After: 60 -- could be commented as derived from 1/rps for clarity.
• withMiddleware is a 3-line nil check used only in the auth template; could be inlined.
• Consider adding X-RateLimit-Limit / X-RateLimit-Remaining headers in deny responses (RFC 6585) to help API clients implement backoff.

---

What's Good

• Trusting XFF only from loopback/private peers is the correct security posture
• LRU eviction with background cleanup goroutine prevents unbounded memory growth
• Sharded implementation with cache-line padding shows careful thinking about lock contention
• pkg/ratelimit functional-options API is clean and well-documented
• Comprehensive test suite with race detector coverage including boundary conditions and cleanup timing
• Hybrid deny handler (redirect for browsers, JSON 429 for API clients) is good UX

Generated with Claude Code

[claude]

PR Review: Per-IP Rate Limiting (#248)

This is well-structured work — functional options API is clean, IP extraction security (only trusting XFF from private/loopback peers) is correct, and the test suite is comprehensive with race-detector coverage. A few issues worth resolving before merging.

---

Bugs / Correctness

1. RATE_LIMIT_BURST=0 silently becomes burst=1

In main.go.tmpl, getEnvInt("RATE_LIMIT_AUTH_BURST", 5) returns 0 successfully if an operator sets the var to 0, but newRateLimiter clamps burst < 1 to 1 with no log message. The operator gets unexpected behavior with no feedback. Add a slog.Warn in newRateLimiter when clamping occurs, parallel to how getEnvFloat/getEnvInt already warn on parse failure.

2. injectAuthRateLimiter idempotency regex too broad

authRLDeclRe := regexp.MustCompile(`\bauthRL\s*:?=`)

This would match authRLConfig := or any identifier starting with authRL. A more precise pattern like \bauthRL\s*:=\s*newRateLimiter avoids false positives.

3. Missing strings import in generated code

injectAuthRateLimiter injects code that calls strings.Contains(r.Header.Get("Accept"), ...) but does not ensure strings is in the generated file's import block. This will cause a compile error if the generated app doesn't already import strings.

---

Security

4. authDeny injects Retry-After: 60 but auth rate is 1/rps = 10s

At RATE_LIMIT_AUTH_RPS=0.1, a token refills every 10 seconds. The hardcoded Retry-After: 60 tells clients to wait 6× longer than necessary. Consider deriving from 1/rps or documenting the intentional conservatism.

5. No X-RateLimit-Remaining or X-RateLimit-Limit headers

RFC 6585 and common API conventions expect these headers so well-behaved clients can implement backoff without hammering the 429 wall. Low-effort win for API consumers.

---

Design

6. Global and auth limiters share RATE_LIMIT_MAX_IPS

Both globalRL and authRL call getEnvInt("RATE_LIMIT_MAX_IPS", 10000), meaning peak memory is up to 2× the configured limit. Consider separate env vars (RATE_LIMIT_AUTH_MAX_IPS) or document the shared behavior.

7. injectAuthRateLimiter has no unit test

This is the riskiest code in the PR — regex-based string surgery with an idempotency check. The integration test catches gross failures but a unit test with a known main.go fixture would catch regressions precisely. The fixtures in auth_test.go test route injection but not the authRL declaration injection.

---

Nits

• w.Write([]byte(...)) in the injected deny handler ignores the return value — silence the linter with _, _ = w.Write(...) or //nolint:errcheck.
• The // Logout route (no rate limiting — already authenticated) comment is a helpful clarification; consider adding it to ConfirmEmailHandler too since confirm tokens are single-use and harder to brute-force.
• withMiddleware helper in the auth template is used only once — inline it to reduce generated file complexity.

---

What's solid

• Trusting XFF only from loopback/private peers is the right security posture
• LRU eviction with background cleanup prevents unbounded memory growth
• Hybrid deny handler (redirect for browsers, JSON 429 for API calls) is good UX
• All rates configurable via env vars with sane defaults
• 20 tests covering eviction, cleanup, concurrency, sharding, and custom deny handlers

The blocking items are #3 (missing strings import in generated code — compile failure) and #7 (no unit test for the injection logic). The rest are improvements.

Generated with Claude Code

[claude]

PR Review: feat: add per-IP rate limiting for global and auth endpoints

Overall this is a well-structured addition with solid test coverage. The design is sound — token bucket per IP, LRU eviction, trusted-proxy detection, configurable via env vars. A few things worth discussing.

Design: Dual Implementation

The PR ships two separate rate limiter implementations: a full-featured pkg/ratelimit/ library and an inline version in main.go.tmpl. The two are already diverging — the library has sharded locking, eviction logging, WithSweepInterval/WithStaleThreshold options, and a Close() method; the inline version uses a single mutex and relies on appCtx cancellation.

This is intentional per the PR description (follow-up in #247), but it creates a maintenance risk. At minimum, add a // TODO(#247): replace with pkg/ratelimit comment in main.go.tmpl so the debt is visible.

Fragile: Regex-based injection in injectAuthRateLimiter

auth.go:175 locates where to inject the authRL declaration by searching for auth handler calls that already contain authRL. This only works because InjectRoute has already been called and written those references into main.go. The ordering dependency is implicit.

If the pattern changes, or if injectAuthRateLimiter is called before route injection, loc is nil, a warning is printed, and the function returns an error — but the caller at auth.go:141-144 continues. The generated main.go ends up with authRL references but no declaration, producing a compile error in the generated app. Consider failing loudly rather than warning-and-continuing.

Security: Trusted proxy assumes RFC-1918 == trusted

getClientIP in both the template and the library trusts X-Forwarded-For from any loopback or private address. In environments where the app shares a private network with untrusted clients (VPCs, shared hosting, Kubernetes without NetworkPolicies), an attacker on the same subnet can spoof X-Forwarded-For to bypass rate limiting.

This is a known trade-off, but a comment would help operators understand the assumption — something like "Trusts proxy headers only from RFC-1918/loopback peers. Correct when deployed behind a single trusted reverse proxy; do not use in multi-tenant private networks."

Design: ConfirmEmailHandler under auth rate limit

ConfirmEmailHandler gets the same 0.1 rps / burst 5 limit as login. Email confirmation links are single-use tokens clicked from a known inbox — the threat model is different from brute-forcing a password. At burst 5, a user clicking the confirmation link from multiple devices on a shared NAT IP could be blocked. Consider excluding it from the auth limiter or giving it a higher burst.

Minor: Hardcoded Retry-After: 60 in auth deny handler

The value 60 is hardcoded in the injected auth deny handler but the auth RPS is configurable. If someone sets RATE_LIMIT_AUTH_RPS=1, the Retry-After header will still say 60. Consider computing this from the configured rate.

Positive highlights

• Test coverage is excellent — 20 tests covering LRU eviction, stale cleanup, concurrency with the race detector, custom deny handlers, and GetClientIP edge cases.
• Compile-time shard size assertion (const _ = uint(128 - unsafe.Sizeof(shard{}))) is a clean way to enforce the cache-line padding invariant.
• needsQueries fix in route_injector.go correctly handles handler calls with extra parameters.
• getClientIP is well-hardened and its test cases cover all the important trust/no-trust combinations.
• Env var configuration with sensible defaults and slog.Warn on bad values is well-designed.

Three asks before merging: (1) add TODO(#247) in the template to track the dual-implementation debt, (2) reconsider whether ConfirmEmailHandler needs auth-level rate limiting, (3) document the trusted-proxy assumption in a comment.

[claude]

PR Review: Per-IP Rate Limiting (#248)

Overall this is a well-designed and thorough implementation. The dual-limiter architecture, LRU eviction, sharded locking for high-concurrency, and the proxy-aware IP extraction are all solid choices. Test coverage is comprehensive. A few things worth addressing:

Security

RPS=0 silently blocks all traffic after the initial burst

When rps=0 is passed, rate.NewLimiter(rate.Limit(0), burst) never refills tokens - only the initial burst is allowed, then every subsequent request is rejected. A misconfigured RATE_LIMIT_RPS=0 would effectively DDOS your own app. Consider clamping to a sane minimum or logging a warning when rps == 0, similar to how negative values are handled.

RATE_LIMIT_MAX_IPS is shared between global and auth limiters

There is no way to independently tune the IP pool for auth vs. global endpoints. Auth endpoints should likely track far fewer IPs (attackers tend to be a smaller set), so a separate RATE_LIMIT_AUTH_MAX_IPS env var (defaulting to something like 1000) would be useful and can be documented alongside the other auth vars in env.go.

Code Quality

w.Write(...) return values silently ignored in pkg/ratelimit/ratelimit.go:defaultDenyHandler and the injected auth deny handler

errcheck linters will flag these. Use _ = w.Write(...) or fmt.Fprint(w, ...) which is more idiomatic in Go HTTP handlers.

getVarReason includes optional rate-limit keys (commands/env.go)

This map is only consulted for required vars, and rate-limit vars are never in getRequiredVars. These dead entries could mislead future contributors. Either remove them or add a separate lookup for optional-but-documented vars.

Regex injection in injectAuthRateLimiter (internal/generator/auth.go)

The approach works and the TODO tracking migration to pkg/ratelimit (#247) is appreciated. Worth confirming: if auth.Handler is the only auth handler (no magic link or password reset), the pattern auth\.\w+Handler\(queries,\s*authRL\) still matches since Handler itself matches \w+Handler. Looks intentional.

Performance

Cleanup goroutine holds the single mutex while scanning all 10k entries (newSingleMutexLimiter)

Acceptable at the expected scale of generated apps, but worth a brief comment acknowledging the tradeoff vs. the sharded version.

Cache-line padding targets 128 bytes (2 cache lines) per shard

Cache lines on x86 are 64 bytes, so 128 bytes is 2x more memory than needed to prevent false sharing. A single 64-byte alignment is sufficient for the goal. The comment says "pad to 128 bytes (2 cache lines)" without explaining the choice.

Testing

Timing-dependent tests may be flaky under CI load (TestStaleCleanup, TestActiveEntrySurvivesCleanup)

400ms sleep with 50ms sweep / 100ms stale threshold is a reasonable buffer, but heavily loaded CI runners could miss it. Consider WithSweepInterval(10*time.Millisecond) with a longer sleep (1-2s), or a polling loop with a timeout.

Minor Nits

• Inconsistent Retry-After values: global deny sets "1", auth deny sets "60". Auth value is more correct for 0.1 rps - worth a comment explaining why they differ.
• The shard size assertion const _ = uint(128 - unsafe.Sizeof(shard{})) is clever. A comment explaining that unsigned constant underflow is a compile error would help Go newcomers.
• auth.LogoutHandler not being rate limited ("already authenticated") is correct and well-documented.

Summary

Category	Verdict
Architecture	Solid - dual limiters, LRU eviction, sharding
Security	RPS=0 footgun; shared MAX_IPS env var
Code quality	Ignored write errors; dead entries in reasons map
Performance	Acceptable for scale; minor sweep lock note
Tests	Timing-sensitive tests may be flaky in CI
Coverage	Comprehensive - 20+ tests, race detector

The RPS=0 footgun and timing-sensitive tests are the most actionable items before merge.

Generated with Claude Code

[claude]

PR Review: Per-IP Rate Limiting (#248)

Overall this is a solid, well-structured addition. The pkg/ratelimit library is clean, the functional options API is idiomatic Go, and the test coverage is comprehensive. A few issues worth addressing before merge:

---

Bugs / Correctness

1. Sharding threshold mismatch

In pkg/ratelimit/ratelimit.go, the sharded path is used when MaxIPs >= defaultNumShards (16). But with MaxIPs=16 and 16 shards, each shard gets a cap of 1 IP — which defeats the purpose of sharding and will cause constant evictions. The threshold should be significantly higher than defaultNumShards, e.g. MaxIPs >= defaultNumShards * 4 (64), or at minimum documented clearly.

// current — triggers sharding at MaxIPs=16, giving 1 IP per shard
if cfg.MaxIPs < defaultNumShards {
    rl.mw, rl.done = newSingleMutexLimiter(ctx, &cfg)
} else {
    rl.mw, rl.done = newShardedLimiter(ctx, &cfg, defaultNumShards)
}

2. injectAuthRateLimiter silently leaves main.go broken on failure

The function is called with a non-fatal error path:

if err := injectAuthRateLimiter(mainGoPath); err != nil {
    fmt.Printf("⚠️  Could not inject auth rate limiter: %v\n", err)
    // continues...
}

But the auth handlers already reference authRL in their HandlerCall (injected earlier in the same GenerateAuth call), so if injection fails, main.go will have undefined-variable compile errors. The warning message asks the user to add it manually — but the generated code is already broken. This should be a hard error, or authRL should only be referenced in handler calls after successful injection.

3. getEnvFloat/getEnvInt allow values that bypass safety clamps

A negative value in env (e.g. RATE_LIMIT_RPS=-5) is parsed successfully and passed to newRateLimiter, which then clamps it. That works, but there's no validation or warning at the env-parsing layer. Low severity, but worth a note in the env docs that invalid values are clamped.

---

Security

4. GetClientIP trusts all RFC-1918 private peers

The proxy-trust logic (peerIP.IsPrivate()) is intentionally broad and already has a comment about multi-tenant networks. However, in cloud deployments (EC2, GCE) where the app server itself has a private IP and receives traffic directly (no proxy), this allows clients to spoof their IP via X-Forwarded-For. The comment acknowledges this, but consider adding a WithTrustedProxyCIDRs option in a follow-up, or at least surfacing this in the generated .env comments as a deployment consideration.

5. Retry-After header in global deny handler is "1"

For the global 100 rps / burst 200 limiter, a legitimate client that exhausts its burst tokens will be told to retry after 1 second — accurate. But for the auth limiter (0.1 rps / burst 5), the authDeny in the generated code also sets Retry-After: "60" which is correct. Just confirming this is intentional and not copy-paste from the global default.

---

Code Quality

6. shard struct size assertion is unusually fragile

const _ = uint(128 - unsafe.Sizeof(shard{}))

This produces a confusing compile error like constant -16 overflows uint if the struct grows. The intent is good (cache line alignment), but the error message gives no context. Consider replacing with a //go:generate check or _ = require.Equal(t, uintptr(128), unsafe.Sizeof(shard{})) in a test — or at least add a //nolint with explanation so future editors know why this is here.

7. FNV-1 reimplemented manually in shardFor

func (sl *shardedLimiter) shardFor(ip string) *shard {
    h := uint32(2166136261)
    for i := 0; i < len(ip); i++ { ... }
}

hash/fnv from stdlib does the same thing and is more recognizable. The manual version is fine for performance, but it's one more thing to audit for correctness.

8. Implicit coupling between injected code and template

injectAuthRateLimiter injects a code block that references appCtx, newRateLimiter, getEnvFloat, and getEnvInt — all of which must be present in main.go. This coupling isn't enforced anywhere; it works because the template generates them, but it's fragile. A comment linking back to the template (or a compile-time test of the generated output) would help maintainers.

---

Tests

9. TestStaleCleanup and TestActiveEntrySurvivesCleanup are timing-sensitive

These use time.Sleep with tight ratios (400ms wait for 100ms threshold). On a slow or heavily loaded CI runner these can be flaky. Consider either:

• Making SweepInterval and StaleThreshold much smaller in tests (10ms/20ms) with a proportionally larger sleep
• Or using a channel-based notification from the sweep goroutine (test-only hook)

10. No test for injectAuthRateLimiter idempotency

The function has an idempotency guard (authRLDeclRe check), but there's no test that calls GenerateAuth twice and verifies main.go isn't double-injected. Given that auth is commonly re-generated during development, this path is real.

11. No IPv6 test for GetClientIP

The XFF parsing code calls strings.TrimSpace + net.ParseIP which handles IPv6, but there's no test with an IPv6 RemoteAddr or XFF value. Edge cases like [::1]:8080 (brackets in RemoteAddr) are handled by net.SplitHostPort but worth a test.

---

Minor

• LogoutHandler correctly has no rate limiting. The comment "no rate limiting — already authenticated" is accurate and appreciated.
• The withMiddleware nil-guard helper is clean and the right abstraction.
• The TODO(Switch rate limiting template from inline to pkg/ratelimit/ #247) for replacing inline code with the library is clearly marked and cross-referenced. Good.
• ConfirmEmailHandler comment says "clicked from a known inbox" — the stronger reason is that confirmation tokens are single-use secrets, making rate-limiting redundant rather than harmful. Minor wording nit.

---

Summary: Issues 2 (silent failure leaving broken main.go) and 1 (sharding threshold) are the most important to fix before merge. The rest are improvements or low-severity observations. The core implementation — token bucket, LRU eviction, GetClientIP proxy logic — looks correct and the test suite is thorough.

🤖 Generated with Claude Code

[claude]

PR Review: Per-IP Rate Limiting (#248)

Several reviews have already been posted with detailed findings. Rather than repeating those, this is a synthesis focused on the items I consider must-fix before merging and a few additional observations not yet raised.

---

Blocking: injectAuthRateLimiter can leave main.go in a non-compilable state

Multiple reviews flagged this, and it's the clearest blocking issue. GenerateAuth first injects auth routes that reference authRL (via InjectRoute), then injects the authRL declaration (via injectAuthRateLimiter). If the second step fails or is skipped (via the routesInjected > 0 guard), the generated main.go has undefined-variable references and won't compile.

The routesInjected > 0 guard only protects the case where zero routes were injected. If even one route was injected, the auth rate limiter injection is attempted — but if that injection then fails, the error is returned up the call chain to GenerateAuth. Worth tracing through: auth.go:406-410 returns the error via fmt.Errorf("failed to inject auth rate limiter: %w", err), which is returned from GenerateAuth. So the CLI does surface this as an error, not a silent warning. The earlier reviews may have misread the error handling path. However, routesInjected > 0 but routesInjected < len(routes) is still a problem: partial injection means some http.Handle(path, auth.XxxHandler(queries, authRL)) calls are in main.go but others are missing, and authRL is declared — so it compiles, but not all auth endpoints have rate limiting applied. A warning to the user about partial injection would be useful.

Additional Issue: injectAuthRateLimiter assumes newRateLimiter exists in main.go

This is not raised by existing reviews. The injected code calls newRateLimiter(appCtx, ...) and references appCtx — both of which are only present in apps generated by the updated main.go.tmpl. If a user runs lvt gen auth on an app generated before this PR (with an older template), the injected declaration will compile-fail because newRateLimiter and appCtx don't exist in their main.go. The injection should check for these prerequisites and fail with a clear message rather than injecting broken code.

Confirmed: strings import not guaranteed in existing apps

The injected authDeny closure uses strings.Contains(r.Header.Get("Accept"), "application/json"). "strings" is added to main.go.tmpl in this PR, so newly generated apps are fine. For older apps (see above), strings won't be imported either. If the newRateLimiter prerequisite check is added, this is resolved transitively.

Sharding threshold

One existing review flagged the threshold as wrong (MaxIPs >= 16 triggers sharding giving 1 IP/shard). Looking at the actual diff, the code already uses defaultNumShards * 4 = 64 as the threshold, which is correct. That review was inaccurate.

---

What's solid

• The pkg/ratelimit library itself is well-designed: functional options, sharding with cache-line padding, clean Close() lifecycle, and a thorough test suite with race-detector coverage.
• The trusted-proxy logic in GetClientIP is the right security posture for single-proxy deployments.
• The routesInjected > 0 guard prevents injecting authRL when no routes need it.
• The needsQueries fix in route_injector.go is correct and necessary.

The main gaps are around lvt gen auth on pre-existing apps. If this command is only intended for apps generated after this PR, that constraint should be documented explicitly. Otherwise, the prerequisite check (for newRateLimiter / appCtx) is needed.

🤖 Generated with Claude Code

[claude]

PR Review: feat: add per-IP rate limiting for global and auth endpoints

Overall this is a solid, well-thought-out feature. The design is clean, the test coverage is thorough, and the security decisions are well-documented. A few things worth discussing below.

---

Security

GetClientIP trust model — The logic of only trusting X-Forwarded-For/X-Real-IP from loopback/private peers is correct for a single-proxy setup and the caveats are documented. One edge case worth noting: in cloud environments where the LB is in 10.0.0.0/8 and an attacker can also reach the app directly on an internal address, they could forge XFF to bypass per-IP rate limiting. The current comment mentions this for multi-tenant private networks. Consider adding a warning in the generated .env comments or README for that scenario.

XFF leftmost-IP extraction — Taking xff[:i] (the first/leftmost IP) is correct when there is exactly one trusted proxy that appends its own IP to the chain. If users deploy behind multiple proxies (e.g., CDN → nginx → app), the leftmost IP could still be spoofed. Worth a brief doc note since the PR surfaces this in generated user-facing code.

Double rate limiting — Auth endpoints go through both the global limiter (100 rps) and the per-handler auth limiter (0.1 rps). The auth limiter is the binding constraint, so this is fine and actually provides defense-in-depth. Worth a comment in the generated main.go to explain why authRL is layered inside the already-rate-limited mux.

---

Potential Bugs

Partial injection + undefined authRL — If InjectRoute succeeds for some routes but fails for others, routesInjected > 0 will trigger injectAuthRateLimiter, injecting an authRL variable. But the routes that failed injection won't reference authRL, while the ones that succeeded will. This won't cause a compilation error (unused var isn't an error in Go), but it's the reverse scenario — if zero routes with authRL were injected but routesInjected > 0 due to non-authRL routes succeeding, injectAuthRateLimiter will return an error ("no auth handler calls with authRL found in main.go"), which bubbles up as a fatal error even though the app might compile fine without it. Might be worth checking hasRateLimiting && routesInjected > 0 more carefully, or only counting routes that actually contain authRL.

evictedIPGetsFreshLimiter test assumption — The test with maxIPs=2 uses the single-mutex path (< defaultNumShards*4 = 64). The comment correctly notes this. But if defaultNumShards ever changes, the test comment will silently become wrong. Nit: use WithMaxIPs(3) with an explicit comment that 3 < 64.

---

Performance

Single mutex in generated main.go — The inline version (no sharding) is fine for most generated apps at 100 rps global. The TODO to replace with pkg/ratelimit is tracked in #247. One thing to flag: the inline version holds the lock while calling order.MoveToFront and updating lastSeen. This is standard for LRU and unavoidable with a single mutex, but it does mean every request goes through a global lock. For apps that grow beyond hobby-scale this will be a bottleneck until #247 lands.

Cleanup goroutine sweeps stale entries back-to-front — For the sharded sweeper, shards are swept sequentially (lock one, sweep, release, next). This is fine for a 5-minute ticker. If StaleThreshold is set very short (e.g., in tests), the sweeper could contend with hot-path requests. The test uses 50ms sweep / 100ms threshold and sleeps 400ms — flakiness risk under CPU load in CI. Consider using a retry loop with timeout rather than a fixed sleep.

---

Code Quality

ipLimiter.ip field — The ip field is stored in the struct only to look up the map key during eviction. Since the shard / outer closure already has items keyed by IP string, you could avoid storing the IP in the struct at all and just use the map key directly. Minor memory micro-optimization, totally fine to leave as-is.

shard struct padding — The compile-time size assertion const _ = uint(128 - unsafe.Sizeof(shard{})) is a nice touch. One note: this asserts the struct is ≤ 128 bytes but doesn't assert it's exactly 128 (i.e., padding is filling correctly). If the struct shrinks due to a field removal, the padding would over-fill and cache lines would still be separate, so it's not a correctness issue — just noting that the assertion is one-sided.

withMiddleware helper — Clean, readable. Good call putting this in the template rather than inlining the nil check everywhere.

injectAuthRateLimiter — regex fragility — The regex auth\.\w+Handler\(queries,\s*authRL\) is reasonably specific. Worth a unit test for injectAuthRateLimiter in auth_test.go covering: (a) already-injected idempotency, (b) no auth handler calls found, (c) successful injection.

---

Minor Nits

• getEnvInt / getEnvFloat in the template are simple and correct; they could be table-tested but not critical.
• The Retry-After: 1 for the global limiter and Retry-After: 60 for the auth limiter are reasonable. The global Retry-After could arguably be computed dynamically from the token refill time, but that's a follow-up concern.
• The env var RATE_LIMIT_MAX_IPS is shared between global and auth limiters, which makes sense (same IP table size). This is clear from the .env comments.

---

Test Coverage

Strong overall — LRU eviction, stale cleanup, concurrency (race detector), custom deny handlers, GetClientIP variants. The TestActiveEntrySurvivesCleanup and TestStaleCleanup tests that rely on time.Sleep are the main flakiness risk; replacing with a polling loop would improve CI reliability.

Missing test: injectAuthRateLimiter idempotency and error paths.

---

Summary: Ready to merge with optional follow-ups. The double-rate-limiting comment in generated code, the partial-injection edge case, and a test for injectAuthRateLimiter are the most actionable items. Everything else is minor polish.

🤖 Generated with Claude Code