R68: cache expiry TOCTOU, logout open-redirect, session max-age

helpers.go

@@ -13,8 +13,6 @@ import (
 	"net/url"
 	"strings"
 	"time"
-
-	"github.com/lukaszraczylo/traefikoidc/internal/utils"
 )
 
 // newUUIDv4 returns an RFC 4122 v4 UUID string (e.g.
@@ -399,24 +397,30 @@ func (t *TraefikOidc) handleLogout(rw http.ResponseWriter, req *http.Request) {
 		}
 	}
 
-	host := utils.DetermineHost(req)
-	scheme := utils.DetermineScheme(req, t.forceHTTPS)
-	baseURL := fmt.Sprintf("%s://%s", scheme, host)
-
 	postLogoutRedirectURI := t.postLogoutRedirectURI
 	// localRedirect is used when there is no provider end-session endpoint and
 	// the plugin redirects the browser itself. It must never be an absolute URL
 	// derived from the request host (X-Forwarded-Host is client-controllable and
 	// would be an open redirect); use a host-relative path, or the operator's
 	// own configured absolute URL, instead.
+	//
+	// The post_logout_redirect_uri we hand to the IdP has the same constraint:
+	// deriving it from baseURL (scheme://client-controlled host) lets an
+	// attacker's X-Forwarded-Host steer the browser to an arbitrary origin
+	// after logout (open redirect through the IdP). So we only emit it when
+	// the operator configured an explicit absolute URL (a trusted origin);
+	// otherwise BuildLogoutURL omits the parameter entirely.
 	localRedirect := "/"
-	if postLogoutRedirectURI == "" {
-		postLogoutRedirectURI = fmt.Sprintf("%s/", baseURL)
-	} else if !strings.HasPrefix(postLogoutRedirectURI, "http") {
-		localRedirect = normalizeLogoutPath(postLogoutRedirectURI)
-		postLogoutRedirectURI = fmt.Sprintf("%s%s", baseURL, postLogoutRedirectURI)
-	} else {
+	if strings.HasPrefix(postLogoutRedirectURI, "http") {
 		localRedirect = postLogoutRedirectURI
+	} else if postLogoutRedirectURI == "" {
+		// Not configured: no trusted origin available — do not send a
+		// host-derived post_logout_redirect_uri to the IdP.
+	} else {
+		// Relative operator-configured path is trusted for the plugin's own
+		// redirect but still has no trusted origin; omit the IdP param.
+		localRedirect = normalizeLogoutPath(postLogoutRedirectURI)
+		postLogoutRedirectURI = ""
 	}
 
 	// Read endSessionURL with RLock

internal/cache/backends/memory.go

@@ -179,8 +179,10 @@ func (m *MemoryCacheBackend) Get(ctx context.Context, key string) (interface{},
 	value, exists, expired := shard.get(key)
 
 	if expired {
-		// Clean up expired item
-		shard.delete(key)
+		// Clean up expired item. Use deleteIfExpired so a concurrent Set
+		// that refreshed this key between get() and here is not deleted
+		// (which would drop a freshly-written value = lost update).
+		shard.deleteIfExpired(key)
 		m.misses.Add(1)
 		return nil, ErrCacheMiss
 	}

internal/cache/backends/memory_shard.go

@@ -117,6 +117,24 @@ func (s *cacheShard) delete(key string) bool {
 	return true
 }
 
+// deleteIfExpired removes a key only if the entry still present is expired.
+// It is safe to call after observing an expired item via get(): a
+// concurrent set() that refreshed the key between the read and this call
+// leaves the fresh value intact. Unlike delete, it avoids deleting a
+// just-written value (a lost update in Get's expiry cleanup).
+func (s *cacheShard) deleteIfExpired(key string) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	item, exists := s.items[key]
+	if !exists || !item.isExpired() {
+		return false
+	}
+
+	s.deleteItemLocked(item)
+	return true
+}
+
 // exists checks if a key exists (and is not expired)
 func (s *cacheShard) exists(key string) bool {
 	s.mu.RLock()

internal/cache/backends/memory_test.go

@@ -781,3 +781,58 @@ func TestMatchPattern(t *testing.T) {
 		})
 	}
 }
+
+// TestMemoryShard_DeleteIfExpiredPreservesConcurrentRefresh regresses a
+// TOCTOU in MemoryCacheBackend.Get's expired-item cleanup. Get reads an
+// item, sees it expired (shard.get), then was deleting it with an
+// unconditional shard.delete in a separate critical section. If a
+// concurrent Set refreshed the key between the read and the delete, the
+// delete removed the freshly-written value (a lost update — the key
+// vanished right after it was refreshed). Get now uses deleteIfExpired,
+// which deletes only if the entry is still expired, leaving a concurrent
+// refresh intact. This test models that interleaving deterministically.
+func TestMemoryShard_DeleteIfExpiredPreservesConcurrentRefresh(t *testing.T) {
+	s := newCacheShard(0, 0)
+	const key = "k"
+
+	// Get's first phase: observe an already-expired entry.
+	s.set(key, "old", time.Now().Add(-time.Second), 1)
+	if _, _, expired := s.get(key); !expired {
+		t.Fatal("precondition: seeded item must be expired")
+	}
+
+	// A concurrent refresh lands a fresh value inside the get->delete window.
+	s.set(key, "fresh", time.Now().Add(time.Hour), 1)
+
+	// Get's cleanup must not remove the fresh value.
+	if s.deleteIfExpired(key) {
+		t.Fatal("deleteIfExpired removed a value a concurrent Set just wrote")
+	}
+	if v, _, _ := s.get(key); v != "fresh" {
+		t.Fatalf("fresh value was lost by expiry cleanup, got %v", v)
+	}
+}
+
+// TestMemoryShard_DeleteClobbersConcurrentRefresh documents the failure
+// mode the fix addresses: the old unconditional delete removes the value
+// a concurrent Set just wrote under the same interleaving. If
+// deleteIfExpired ever regresses to unconditional deletion, the sibling
+// test above fails.
+func TestMemoryShard_DeleteClobbersConcurrentRefresh(t *testing.T) {
+	s := newCacheShard(0, 0)
+	const key = "k"
+
+	s.set(key, "old", time.Now().Add(-time.Second), 1)
+	if _, _, expired := s.get(key); !expired {
+		t.Fatal("precondition: seeded item must be expired")
+	}
+	s.set(key, "fresh", time.Now().Add(time.Hour), 1)
+
+	// Old cleanup (delete) removes the just-written value: lost update.
+	if !s.delete(key) {
+		t.Log("old delete found no item")
+	}
+	if v, _, _ := s.get(key); v == "fresh" {
+		t.Log("note: value happened to survive, but delete is unconditional")
+	}
+}

main_servehttp_test.go

@@ -5,6 +5,8 @@ import (
 	"crypto/rsa"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
+	"strings"
 	"testing"
 	"time"
 )
@@ -1317,3 +1319,70 @@ func TestServeHTTP_EventStream_AppliesRolesGate(t *testing.T) {
 		}
 	})
 }
+
+// TestLogout_NoOpenRedirectViaIdP regresses a post-logout open redirect:
+// handleLogout built the IdP's post_logout_redirect_uri from the
+// client-controllable request host (X-Forwarded-Host), so an attacker
+// could steer the browser to an arbitrary origin after logout. With no
+// configured postLogoutRedirectURI the parameter must be omitted, never
+// host-derived.
+func TestLogout_NoOpenRedirectViaIdP(t *testing.T) {
+	sessionManager := createTestSessionManager(t)
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	idToken, err := createTestJWT(key, "RS256", "test-key-id", map[string]interface{}{"sub": "[email protected]"})
+	if err != nil {
+		t.Fatalf("craft id token: %v", err)
+	}
+
+	oidc := &TraefikOidc{
+		logger:         NewLogger("error"),
+		sessionManager: sessionManager,
+		endSessionURL:  "https://idp.example.com/session/end",
+		tokenBlacklist: NewCache(),
+		tokenCache:     NewTokenCache(),
+	}
+
+	req := httptest.NewRequest("GET", "/logout", nil)
+	req.Header.Set("X-Forwarded-Host", "attacker.example")
+
+	session, err := sessionManager.GetSession(req)
+	if err != nil {
+		t.Fatalf("create session: %v", err)
+	}
+	session.SetUserIdentifier("[email protected]")
+	if err := session.SetAuthenticated(true); err != nil {
+		t.Fatalf("authenticate: %v", err)
+	}
+	session.SetIDToken(idToken)
+	rec := httptest.NewRecorder()
+	if err := session.Save(req, rec); err != nil {
+		t.Fatalf("save session: %v", err)
+	}
+	for _, c := range rec.Result().Cookies() {
+		req.AddCookie(c)
+	}
+
+	rw := httptest.NewRecorder()
+	oidc.handleLogout(rw, req)
+
+	loc := rw.Header().Get("Location")
+	if loc == "" {
+		t.Fatalf("expected logout redirect to the IdP end-session URL")
+	}
+	u, err := url.Parse(loc)
+	if err != nil {
+		t.Fatalf("parse redirect %q: %v", loc, err)
+	}
+	plr := u.Query().Get("post_logout_redirect_uri")
+	if plr == "" {
+		// Correct: no configured target, param omitted.
+		return
+	}
+	if strings.Contains(plr, "attacker.example") {
+		t.Fatalf("open redirect via IdP: post_logout_redirect_uri=%q came from client host", plr)
+	}
+}

main_test.go

@@ -1862,7 +1862,7 @@ func TestHandleLogout(t *testing.T) {
 			},
 			endSessionURL:  "https://provider/end-session",
 			expectedStatus: http.StatusFound,
-			expectedURL:    "https://provider/end-session?id_token_hint=" + url.QueryEscape(ValidIDToken) + "&post_logout_redirect_uri=http%3A%2F%2Fexample.com%2F",
+			expectedURL:    "https://provider/end-session?id_token_hint=" + url.QueryEscape(ValidIDToken),
 			host:           "test-host",
 		},
 		{

review_r68_created_at_test.go

@@ -0,0 +1,47 @@
+package traefikoidc
+
+import (
+	"net/http/httptest"
+	"testing"
+	"time"
+)
+
+// TestSession_RefreshDoesNotResetCreatedAt regresses the absolute
+// session-max-age being made sliding: SetAuthenticated(true) was called on
+// every token refresh and unconditionally reset created_at to now, so an
+// active (or captured-but-refreshable) session never reached the
+// configured maximum age. The creation anchor must be set only on the
+// unauthenticated->authenticated transition, not on refresh of an
+// already-authenticated session.
+func TestSession_RefreshDoesNotResetCreatedAt(t *testing.T) {
+	sessionManager := createTestSessionManager(t)
+	req := httptest.NewRequest("GET", "/", nil)
+
+	session, err := sessionManager.GetSession(req)
+	if err != nil {
+		t.Fatalf("create session: %v", err)
+	}
+	session.SetUserIdentifier("[email protected]")
+	if err := session.SetAuthenticated(true); err != nil {
+		t.Fatalf("initial login: %v", err)
+	}
+
+	// Simulate a session that is still within its maximum age but was
+	// created a while ago (e.g. 2h ago, well under the 24h default
+	// maximum). getAuthenticatedUnsafe still considers it valid.
+	oldCreated := time.Now().Add(-2 * time.Hour).Unix()
+	session.mainSession.Values["created_at"] = oldCreated
+
+	// This is what the refresh path does on success.
+	if err := session.SetAuthenticated(true); err != nil {
+		t.Fatalf("refresh SetAuthenticated: %v", err)
+	}
+
+	got, ok := session.mainSession.Values["created_at"].(int64)
+	if !ok {
+		t.Fatalf("created_at missing or not int64")
+	}
+	if got != oldCreated {
+		t.Fatalf("refresh advanced created_at: got %d, want %d (old anchor must be preserved)", got, oldCreated)
+	}
+}

session.go

@@ -1736,11 +1736,20 @@ func (sd *SessionData) SetAuthenticated(value bool) error {
 			changed = true
 		}
 		sd.mainSession.ID = id
-		newCreationTime := time.Now().Unix()
-		if oldTime, ok := sd.mainSession.Values["created_at"].(int64); !ok || oldTime != newCreationTime {
-			changed = true
+		// Anchor the creation timestamp only when first authenticating
+		// (transition from unauthenticated). Re-asserting an already-
+		// authenticated session — e.g. on every token refresh — must not
+		// advance the absolute session-max-age clock, otherwise an active
+		// client's session (and a captured cookie that keeps refreshing)
+		// never reaches the configured maximum (sliding window defeats the
+		// absolute timeout).
+		if currentAuth != value {
+			newCreationTime := time.Now().Unix()
+			if oldTime, ok := sd.mainSession.Values["created_at"].(int64); !ok || oldTime != newCreationTime {
+				changed = true
+			}
+			sd.mainSession.Values["created_at"] = newCreationTime
 		}
-		sd.mainSession.Values["created_at"] = newCreationTime
 		if oldAuth, ok := sd.mainSession.Values["authenticated"].(bool); !ok || oldAuth != value {
 			changed = true
 		}