From 5a54960cddac751a812adec723206638f17ad613 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 17:35:25 +0100 Subject: [PATCH] R74: recv handler panics, cache TTL race fix --- internal/cache/backends/memory_shard.go | 36 +++++++------ .../cache/backends/race_regression_test.go | 50 +++++++++++++++++++ middleware.go | 14 ++++++ review_r74_test.go | 29 +++++++++++ 4 files changed, 114 insertions(+), 15 deletions(-) create mode 100644 internal/cache/backends/race_regression_test.go create mode 100644 review_r74_test.go diff --git a/internal/cache/backends/memory_shard.go b/internal/cache/backends/memory_shard.go index 8ee9301..a1e3181 100644 --- a/internal/cache/backends/memory_shard.go +++ b/internal/cache/backends/memory_shard.go @@ -33,13 +33,14 @@ func newCacheShard(maxSize, maxMemory int64) *cacheShard { func (s *cacheShard) get(key string) (interface{}, bool, bool) { s.mu.RLock() item, exists := s.items[key] + expired := exists && item.isExpired() s.mu.RUnlock() if !exists { return nil, false, false } - if item.isExpired() { + if expired { return nil, true, true // exists but expired } @@ -47,16 +48,19 @@ func (s *cacheShard) get(key string) (interface{}, bool, bool) { s.mu.Lock() // Re-check item exists (could have been deleted) item, exists = s.items[key] - if exists && !item.isExpired() { - item.accessedAt = time.Now() - item.accessCount++ - if elem, ok := item.element.(*list.Element); ok && elem != nil { - s.lruList.MoveToFront(elem) + if exists { + expired = item.isExpired() + if !expired { + item.accessedAt = time.Now() + item.accessCount++ + if elem, ok := item.element.(*list.Element); ok && elem != nil { + s.lruList.MoveToFront(elem) + } } } s.mu.Unlock() - if !exists || item.isExpired() { + if !exists || expired { return nil, false, false } @@ -139,30 +143,32 @@ func (s *cacheShard) deleteIfExpired(key string) bool { func (s *cacheShard) exists(key string) bool { s.mu.RLock() item, exists := s.items[key] + expired := exists && item.isExpired() s.mu.RUnlock() - if !exists { - return false - } - - return !item.isExpired() + return exists && !expired } // ttl returns the remaining TTL for a key func (s *cacheShard) ttl(key string) (time.Duration, bool) { s.mu.RLock() item, exists := s.items[key] + expired := exists && item.isExpired() + var expiresAt time.Time + if exists { + expiresAt = item.expiresAt + } s.mu.RUnlock() - if !exists || item.isExpired() { + if !exists || expired { return 0, false } - if item.expiresAt.IsZero() { + if expiresAt.IsZero() { return 0, true // No expiration } - remaining := time.Until(item.expiresAt) + remaining := time.Until(expiresAt) if remaining < 0 { return 0, false } diff --git a/internal/cache/backends/race_regression_test.go b/internal/cache/backends/race_regression_test.go new file mode 100644 index 0000000..bdf017e --- /dev/null +++ b/internal/cache/backends/race_regression_test.go @@ -0,0 +1,50 @@ +package backends + +import ( + "context" + "sync" + "testing" + "time" +) + +// TestCache_TTLReadUnderConcurrentExpire exercises get/exists/ttl fast +// paths racing with an in-place Expire that mutates expiresAt. Before +// the fix the fast paths read expiresAt outside the lock, so under -race +// this reports a data race on memoryCacheItem.expiresAt. +func TestCache_TTLReadUnderConcurrentExpire(t *testing.T) { + c := NewMemoryCacheBackend(10000, 1024*1024*64, 0) + defer c.Close() + + ctx := context.Background() + if err := c.Set(ctx, "k", "v", time.Hour); err != nil { + t.Fatalf("set: %v", err) + } + + stop := make(chan struct{}) + var wg sync.WaitGroup + for i := 0; i < 4; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + _, _ = c.Get(ctx, "k") + _, _ = c.Exists(ctx, "k") + _, _ = c.TTL(ctx, "k") + } + }() + } + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 2000; i++ { + _ = c.Expire(ctx, "k", time.Hour) + } + close(stop) + }() + wg.Wait() +} diff --git a/middleware.go b/middleware.go index 0ec8851..0943713 100644 --- a/middleware.go +++ b/middleware.go @@ -7,6 +7,7 @@ import ( "bytes" "fmt" "net/http" + "runtime/debug" "strings" "sync/atomic" "time" @@ -172,6 +173,19 @@ func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) ( // - rw: The HTTP response writer. // - req: The incoming HTTP request. func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + // Recover from any panic in the handler chain (session decode, token + // validation, header injection, authorize-url build) so a bug answers a + // single clean 500 (and logs a stack) instead of escaping to net/http, + // which closes/truncates the connection. WriteHeader is idempotent, so + // this is safe even if a handler already wrote headers; Write appends. + defer func() { + if r := recover(); r != nil { + t.logger.Errorf("OIDC handler panic recovered: %v\n%s", r, debug.Stack()) + rw.WriteHeader(http.StatusInternalServerError) + _, _ = rw.Write([]byte("Internal Server Error")) + } + }() + // Log request entry for debugging routing issues t.logger.Debugf("Incoming request: %s %s", req.Method, req.URL.Path) diff --git a/review_r74_test.go b/review_r74_test.go new file mode 100644 index 0000000..a4a637a --- /dev/null +++ b/review_r74_test.go @@ -0,0 +1,29 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestServeHTTP_RecoversHandlerPanic regresses the request-handler path +// having no panic recovery: a panic in the chain (here, the downstream +// backend) now answers a clean 500 + body instead of escaping to +// net/http (which closes/truncates the connection). +func TestServeHTTP_RecoversHandlerPanic(t *testing.T) { + oidc := &TraefikOidc{ + next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + panic("boom") + }), + logger: NewLogger("error"), + excludedURLs: map[string]struct{}{"/health": {}}, + } + + rw := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/health", nil) + oidc.ServeHTTP(rw, req) // must not panic + + if rw.Code != http.StatusInternalServerError { + t.Fatalf("expected 500 from recovered panic, got %d", rw.Code) + } +}