From 7bcb6d469c8f940e56b63d5480af83b8ebb0e86e Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 7 Apr 2026 11:32:28 +0100 Subject: [PATCH] =?UTF-8?q?fix(pr#2):=20address=20CodeRabbit=20round=204?= =?UTF-8?q?=20=E2=80=94=20newCacheStore=20fail-closed=20for=20unbounded=20?= =?UTF-8?q?cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cache.go | 6 ++++++ docs/architecture.md | 7 ++++--- options.go | 20 +++++++++++++++----- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/cache.go b/cache.go index 19d6bea..91526c3 100644 --- a/cache.go +++ b/cache.go @@ -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(), diff --git a/docs/architecture.md b/docs/architecture.md index ec0222b..22a38c9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -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. diff --git a/options.go b/options.go index 6f10798..715a524 100644 --- a/options.go +++ b/options.go @@ -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)) } }