diff --git a/logout.go b/logout.go index 7a29b51..51c56e9 100644 --- a/logout.go +++ b/logout.go @@ -373,17 +373,27 @@ func (t *TraefikOidc) invalidateSession(sid, sub string) error { now := time.Now().Unix() + // The default 25h invalidation lives longer than the default 24h + // session, but a configured sessionMaxAge longer than 25h would let + // the invalidation expire while a still-valid old cookie survives, so a + // replayed dead cookie is accepted again. Derive the TTL from the + // session's actual max age (with margin) when it exceeds the default. + ttl := sessionInvalidationTTL + if t.sessionManager != nil && t.sessionManager.sessionMaxAge > ttl { + ttl = t.sessionManager.sessionMaxAge + time.Hour + } + // Store by session ID if sid != "" { key := t.buildSessionInvalidationKey("sid", sid) - t.sessionInvalidationCache.Set(key, now, sessionInvalidationTTL) + t.sessionInvalidationCache.Set(key, now, ttl) t.logger.Debugf("Invalidated session by sid: %s", sid) } // Store by subject (invalidates all sessions for this user) if sub != "" { key := t.buildSessionInvalidationKey("sub", sub) - t.sessionInvalidationCache.Set(key, now, sessionInvalidationTTL) + t.sessionInvalidationCache.Set(key, now, ttl) t.logger.Debugf("Invalidated session by sub: %s", sub) } diff --git a/middleware.go b/middleware.go index bf7c657..1c1a43e 100644 --- a/middleware.go +++ b/middleware.go @@ -944,6 +944,14 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques if strings.Contains(headerValue, noValueSentinel) { headerValue = strings.ReplaceAll(headerValue, noValueSentinel, "") } + // Skip an empty render: Setting "" would clobber an identity + // header (X-Forwarded-User / X-Auth-Request-*) already injected + // earlier in this function with the authenticated value, silently + // dropping the user identity at the backend. + if headerValue == "" { + t.logger.Debugf("Skipping templated header %s: rendered value is empty", headerName) + continue + } // Sanitize the rendered output: template inputs are claim-derived // and attacker-influenceable, so reject control chars (header // injection), bidi-override runes, the , ; = delimiters, and an diff --git a/review_r79_test.go b/review_r79_test.go new file mode 100644 index 0000000..bfcb8d0 --- /dev/null +++ b/review_r79_test.go @@ -0,0 +1,99 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" + "text/template" + "time" +) + +// ttlRecordingCache is a CacheInterface that records the TTL passed to Set +// so tests can assert the invalidation TTL tracks session max age. +type ttlRecordingCache struct { + mu sync.Mutex + ttls map[string]time.Duration +} + +func (c *ttlRecordingCache) Set(key string, value any, ttl time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + if c.ttls == nil { + c.ttls = make(map[string]time.Duration) + } + c.ttls[key] = ttl +} +func (c *ttlRecordingCache) Get(key string) (any, bool) { return nil, false } +func (c *ttlRecordingCache) Delete(key string) {} +func (c *ttlRecordingCache) SetMaxSize(int) {} +func (c *ttlRecordingCache) Size() int { return 0 } +func (c *ttlRecordingCache) Clear() {} +func (c *ttlRecordingCache) Cleanup() {} +func (c *ttlRecordingCache) Close() {} +func (c *ttlRecordingCache) GetStats() map[string]any { return nil } + +// TestBackchannelInvalidationTTL_TracksSessionMaxAge is a regression for the +// hardcoded 25h invalidation TTL: when the session max age is configured +// longer than 25h, the invalidation entry must outlive the cookie, or a +// replayed dead cookie would be re-accepted after the entry lapses. The +// fix derives the invalidation TTL from the session manager's max age. +func TestBackchannelInvalidationTTL_TracksSessionMaxAge(t *testing.T) { + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + oidc := makeBearerOIDC(t, next) + if oidc.sessionManager == nil { + t.Fatal("expected a session manager from makeBearerOIDC") + } + oidc.sessionManager.sessionMaxAge = 48 * time.Hour + if oidc.logger == nil { + oidc.logger = NewLogger("debug") + } + rec := &ttlRecordingCache{} + oidc.sessionInvalidationCache = rec + + if err := oidc.invalidateSession("sid-t", "sub-t"); err != nil { + t.Fatalf("invalidateSession: %v", err) + } + + const sidKey = "session_invalidation:sid:sid-t" + rec.mu.Lock() + ttl, ok := rec.ttls[sidKey] + rec.mu.Unlock() + if !ok { + t.Fatalf("sid invalidation not recorded (key %q)", sidKey) + } + if ttl < 48*time.Hour { + t.Fatalf("invalidation TTL %v must track session max age (>= 48h), got a fixed short TTL", ttl) + } +} + +// TestHeaderTemplate_EmptyRender_DoesNotClobberIdentity is a regression for +// an operator-header template that renders an empty value: the old code +// req.Header.Set(name, "") after identity headers were already injected, +// silently blanking X-Forwarded-User with the authenticated identifier. +// The fix skips an empty render so the identity header survives. +func TestHeaderTemplate_EmptyRender_DoesNotClobberIdentity(t *testing.T) { + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + oidc := &TraefikOidc{ + logger: NewLogger("debug"), + next: next, + headerTemplates: map[string]*template.Template{ + "X-Forwarded-User": template.Must(template.New("t").Parse("{{.Claims.email}}")), + }, + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + rw := httptest.NewRecorder() + oidc.forwardAuthorized(rw, req, &principal{ + Identifier: "alice@example.com", + Claims: map[string]interface{}{"sub": "alice"}, // no email -> template renders empty + }) + + if got := req.Header.Get("X-Forwarded-User"); got != "alice@example.com" { + t.Fatalf("identity header clobbered by empty template render: got %q, want %q", got, "alice@example.com") + } +}