fix(pr#2): address CodeRabbit round 4 — newCacheStore fail-closed for unbounded cache

Prevent silent unbounded cache creation when both maxEntries and maxBytes
are non-positive: newCacheStore now returns nil, WithCacheLimits skips
middleware registration, and WithCache defaults to 1 000-entry LRU cap
when called with only a TTL argument.

Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
Snider 2026-04-07 11:32:28 +01:00
parent e27006de30
commit 7bcb6d469c
3 changed files with 25 additions and 8 deletions

View file

@ -35,7 +35,13 @@ type cacheStore struct {
}
// newCacheStore creates an empty cache store.
// At least one of maxEntries or maxBytes must be positive; if both are
// non-positive the store would be unbounded and newCacheStore returns nil so
// callers can skip registering the middleware.
func newCacheStore(maxEntries, maxBytes int) *cacheStore {
if maxEntries <= 0 && maxBytes <= 0 {
return nil
}
return &cacheStore{
entries: make(map[string]*cacheEntry),
order: list.New(),

View file

@ -176,7 +176,7 @@ They execute after `gin.Recovery()` but before any route handler. The `Option` t
| `WithBrotli(level...)` | Brotli response compression | Writer pool for efficiency; default compression if level omitted |
| `WithSlog(logger)` | Structured request logging | Falls back to `slog.Default()` if nil |
| `WithTimeout(d)` | Per-request deadline | 504 with standard error envelope on timeout |
| `WithCache(ttl)` | In-memory GET response caching | Compatibility wrapper for `WithCacheLimits(ttl, 0, 0)`; `X-Cache: HIT` header on cache hits; 2xx only |
| `WithCache(ttl)` | In-memory GET response caching | Defaults to 1 000-entry LRU cap when no explicit bounds given; `X-Cache: HIT` header on cache hits; 2xx only |
| `WithCacheLimits(ttl, maxEntries, maxBytes)` | In-memory GET response caching with explicit bounds | Clearer cache configuration when eviction policy should be self-documenting |
| `WithSessions(name, secret)` | Cookie-backed server sessions | gin-contrib/sessions with cookie store |
| `WithAuthz(enforcer)` | Casbin policy-based authorisation | Subject from HTTP Basic Auth; 403 on deny |
@ -395,8 +395,9 @@ engine, _ := api.New(api.WithCacheLimits(5*time.Minute, 100, 10<<20))
- Cached responses are served with an `X-Cache: HIT` header.
- Expired entries are evicted lazily on the next access for the same key.
- The cache is not shared across `Engine` instances.
- `WithCache(ttl)` remains available as a compatibility wrapper for callers that do not need to spell out the bounds.
- Passing non-positive values to `WithCacheLimits` leaves that limit unbounded.
- `WithCache(ttl)` defaults to a 1 000-entry LRU cap when called with only a TTL; pass explicit limits via `WithCacheLimits` for production tuning.
- Both `maxEntries` and `maxBytes` being non-positive causes `WithCacheLimits` to skip registration; at least one must be positive.
- Setting only one limit to a non-positive value leaves that dimension unbounded while the other limit controls eviction.
The implementation uses a `cacheWriter` that wraps `gin.ResponseWriter` to intercept and
capture the response body and status code for storage.

View file

@ -489,6 +489,11 @@ func timeoutResponse(c *gin.Context) {
c.JSON(http.StatusGatewayTimeout, Fail("timeout", "Request timed out"))
}
// cacheDefaultMaxEntries is the entry cap applied by WithCache when the caller
// does not supply explicit limits. Prevents unbounded growth when WithCache is
// called with only a TTL argument.
const cacheDefaultMaxEntries = 1_000
// WithCache adds in-memory response caching middleware for GET requests.
// Successful (2xx) GET responses are cached for the given TTL and served
// with an X-Cache: HIT header on subsequent requests. Non-GET methods
@ -498,15 +503,15 @@ func timeoutResponse(c *gin.Context) {
// - maxEntries limits the number of cached responses
// - maxBytes limits the approximate total cached payload size
//
// Pass a non-positive value to either limit to leave that dimension
// unbounded for backward compatibility. A non-positive TTL disables the
// middleware entirely.
// At least one limit must be positive; when called with only a TTL the entry
// cap defaults to cacheDefaultMaxEntries (1 000) to prevent unbounded growth.
// A non-positive TTL disables the middleware entirely.
//
// Example:
//
// engine, _ := api.New(api.WithCache(5*time.Minute, 100, 10<<20))
func WithCache(ttl time.Duration, maxEntries ...int) Option {
entryLimit := 0
entryLimit := cacheDefaultMaxEntries
byteLimit := 0
if len(maxEntries) > 0 {
entryLimit = maxEntries[0]
@ -531,10 +536,15 @@ func WithCacheLimits(ttl time.Duration, maxEntries, maxBytes int) Option {
if ttl <= 0 {
return
}
// newCacheStore returns nil when both limits are non-positive (unbounded),
// which is a footgun; skip middleware registration in that case.
store := newCacheStore(maxEntries, maxBytes)
if store == nil {
return
}
e.cacheTTL = ttl
e.cacheMaxEntries = maxEntries
e.cacheMaxBytes = maxBytes
store := newCacheStore(maxEntries, maxBytes)
e.middlewares = append(e.middlewares, cacheMiddleware(store, ttl))
}
}