From ca10d5ba9e00b08632a61de757746c950485d51c Mon Sep 17 00:00:00 2001 From: Cortana Date: Mon, 8 Jun 2026 03:46:55 +0000 Subject: [PATCH] Security: enforce API key auth precedence --- docs/CHANGELOG.md | 6 ++ internal/api/csrf.go | 12 +++- internal/api/csrf_test.go | 16 +++++ internal/api/middleware.go | 102 +++++++++++++++----------------- internal/api/middleware_test.go | 55 +++++++++++++++++ 5 files changed, 134 insertions(+), 57 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9e178f4..274c8b1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 This repository does not use an `Unreleased` changelog section. Add a concrete patch or minor version entry for every user-facing change. +## [0.17.4] - 2026-06-07 + +### Security +- API-key bearer credentials now take precedence over session cookies, preventing a browser session from widening a restricted API key's effective permissions. +- CSRF protection now still applies to state-changing requests that carry a session cookie, even if they also include a `cpam_` bearer-style Authorization header. + ## [0.17.3] - 2026-06-03 ### Added diff --git a/internal/api/csrf.go b/internal/api/csrf.go index 4172079..22a5900 100644 --- a/internal/api/csrf.go +++ b/internal/api/csrf.go @@ -37,8 +37,11 @@ func CSRFMiddleware() Middleware { return } - // For state-changing methods: skip CSRF check if using API key auth - if authHeader := r.Header.Get("Authorization"); authHeader != "" && strings.HasPrefix(authHeader, "Bearer cpam_") { + // For state-changing methods: skip CSRF only for API-key-style requests + // that are not also carrying a browser session cookie. Actual API key + // validity is enforced later by auth middleware; a forged bearer header + // must not disable CSRF for a cookie-authenticated browser request. + if authHeader := r.Header.Get("Authorization"); authHeader != "" && strings.HasPrefix(authHeader, "Bearer cpam_") && !hasSessionCookie(r) { next.ServeHTTP(w, r) return } @@ -71,3 +74,8 @@ func generateCSRFToken() string { _, _ = rand.Read(b) return hex.EncodeToString(b) } + +func hasSessionCookie(r *http.Request) bool { + cookie, err := r.Cookie("session") + return err == nil && strings.TrimSpace(cookie.Value) != "" +} diff --git a/internal/api/csrf_test.go b/internal/api/csrf_test.go index 9e79333..82aa88a 100644 --- a/internal/api/csrf_test.go +++ b/internal/api/csrf_test.go @@ -123,6 +123,22 @@ func TestCSRFMiddleware_POSTWithAPIKeyBypassesCSRF(t *testing.T) { } } +func TestCSRFMiddleware_POSTWithAPIKeyAndSessionRequiresCSRF(t *testing.T) { + handler := CSRFMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/pools", nil) + req.Header.Set("Authorization", "Bearer cpam_forgedkey123abc") + req.AddCookie(&http.Cookie{Name: "session", Value: "browser-session"}) + handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusForbidden { + t.Fatalf("expected 403 when a bearer header is mixed with a session cookie and no CSRF token, got %d", rr.Code) + } +} + func TestCSRFMiddleware_POSTToLoginBypassesCSRF(t *testing.T) { handler := CSRFMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 0359cb0..bc83f54 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -588,61 +588,18 @@ func DualAuthMiddleware( return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - // Strategy 1: Check session cookie - if cookie, err := r.Cookie("session"); err == nil && cookie.Value != "" { - session, err := sessionStore.Get(ctx, cookie.Value) - if err == nil && session != nil && session.IsValid() { - // Check if the user's account is still active (cached). - isActive, cacheHit := activeCache.check(session.UserID, activeStatusCacheTTL) - if !cacheHit { - // Cache miss or expired — fetch from DB. - user, _ := userStore.GetByID(ctx, session.UserID) - if user == nil { - writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) - return - } - isActive = user.IsActive - activeCache.set(session.UserID, isActive) - if !isActive { - writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) - return - } - ctx = auth.ContextWithSession(ctx, session) - ctx = auth.ContextWithRole(ctx, session.Role) - ctx = auth.ContextWithUser(ctx, user) - } else { - if !isActive { - writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) - return - } - ctx = auth.ContextWithSession(ctx, session) - ctx = auth.ContextWithRole(ctx, session.Role) - if user, _ := userStore.GetByID(ctx, session.UserID); user != nil { - ctx = auth.ContextWithUser(ctx, user) - } - } - r = r.WithContext(ctx) - next.ServeHTTP(w, r) - return - } - } - - // Strategy 2: Check Authorization header + // Strategy 1: Check Authorization header. When a request supplies an API + // key and a session cookie, the API key is the active credential so its + // scopes cannot be widened by the browser session. authHeader := r.Header.Get("Authorization") if authHeader != "" && strings.HasPrefix(authHeader, "Bearer ") { - token := strings.TrimPrefix(authHeader, "Bearer ") - token = strings.TrimSpace(token) + token := strings.TrimSpace(strings.TrimPrefix(authHeader, "Bearer ")) if strings.HasPrefix(token, "cpam_") { - // Strategy 2: API key authentication prefix, err := auth.ParseAPIKeyPrefix(token) if err != nil { - if required { - logAuthFailure(logger, r, "invalid API key format") - writeJSON(w, http.StatusUnauthorized, apiError{Error: "unauthorized", Detail: "invalid API key format"}) - return - } - next.ServeHTTP(w, r) + logAuthFailure(logger, r, "invalid API key format") + writeJSON(w, http.StatusUnauthorized, apiError{Error: "unauthorized", Detail: "invalid API key format"}) return } @@ -653,12 +610,8 @@ func DualAuthMiddleware( return } if storedKey == nil { - if required { - logAuthFailure(logger, r, "API key not found") - writeJSON(w, http.StatusUnauthorized, apiError{Error: "unauthorized", Detail: "invalid API key"}) - return - } - next.ServeHTTP(w, r) + logAuthFailure(logger, r, "API key not found") + writeJSON(w, http.StatusUnauthorized, apiError{Error: "unauthorized", Detail: "invalid API key"}) return } @@ -699,6 +652,45 @@ func DualAuthMiddleware( } } + // Strategy 2: Check session cookie + if cookie, err := r.Cookie("session"); err == nil && cookie.Value != "" { + session, err := sessionStore.Get(ctx, cookie.Value) + if err == nil && session != nil && session.IsValid() { + // Check if the user's account is still active (cached). + isActive, cacheHit := activeCache.check(session.UserID, activeStatusCacheTTL) + if !cacheHit { + // Cache miss or expired — fetch from DB. + user, _ := userStore.GetByID(ctx, session.UserID) + if user == nil { + writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) + return + } + isActive = user.IsActive + activeCache.set(session.UserID, isActive) + if !isActive { + writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) + return + } + ctx = auth.ContextWithSession(ctx, session) + ctx = auth.ContextWithRole(ctx, session.Role) + ctx = auth.ContextWithUser(ctx, user) + } else { + if !isActive { + writeJSON(w, http.StatusUnauthorized, apiError{Error: "account disabled"}) + return + } + ctx = auth.ContextWithSession(ctx, session) + ctx = auth.ContextWithRole(ctx, session.Role) + if user, _ := userStore.GetByID(ctx, session.UserID); user != nil { + ctx = auth.ContextWithUser(ctx, user) + } + } + r = r.WithContext(ctx) + next.ServeHTTP(w, r) + return + } + } + if required { logAuthFailure(logger, r, "missing authentication") writeJSON(w, http.StatusUnauthorized, apiError{Error: "unauthorized", Detail: "missing authentication"}) diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 099658e..7d5b68a 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -1837,6 +1837,61 @@ func TestDualAuth_ActiveUserAllowed(t *testing.T) { } } +func TestDualAuth_APIKeyTakesPrecedenceOverSessionCookie(t *testing.T) { + keyStore := auth.NewMemoryKeyStore() + sessionStore := auth.NewMemorySessionStore() + userStore := auth.NewMemoryUserStore() + ctx := context.Background() + + user := &auth.User{ + ID: "u-mixed", + Username: "adminuser", + Role: auth.RoleAdmin, + IsActive: true, + } + if err := userStore.Create(ctx, user); err != nil { + t.Fatalf("failed to create user: %v", err) + } + session := &auth.Session{ + ID: "sess-mixed", + UserID: user.ID, + Role: auth.RoleAdmin, + CreatedAt: time.Now(), + ExpiresAt: time.Now().Add(1 * time.Hour), + } + if err := sessionStore.Create(ctx, session); err != nil { + t.Fatalf("failed to create session: %v", err) + } + + plaintext, apiKey, err := auth.GenerateAPIKey(auth.GenerateAPIKeyOptions{ + Name: "read-only key", + Scopes: []string{"pools:read"}, + }) + if err != nil { + t.Fatalf("failed to generate API key: %v", err) + } + if err := keyStore.Create(ctx, apiKey); err != nil { + t.Fatalf("failed to store API key: %v", err) + } + + mw := DualAuthMiddleware(keyStore, sessionStore, userStore, true, newTestLogger()) + wrapped := mw(RequirePermissionMiddleware(auth.ResourcePools, auth.ActionCreate, newTestLogger())( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("handler should not be called when the API key lacks create scope") + }), + )) + + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/pools", nil) + req.Header.Set("Authorization", "Bearer "+plaintext) + req.AddCookie(&http.Cookie{Name: "session", Value: session.ID}) + wrapped.ServeHTTP(rr, req) + + if rr.Code != http.StatusForbidden { + t.Fatalf("expected 403 from read-only API key despite admin session cookie, got %d: %s", rr.Code, rr.Body.String()) + } +} + func TestLoginRateLimitMiddleware_AllowsBelowLimit(t *testing.T) { loginRL := LoginRateLimitMiddleware(LoginRateLimitConfig{ AttemptsPerMinute: 5,