diff --git a/TODO.md b/TODO.md index ca2a88f..29359f7 100644 --- a/TODO.md +++ b/TODO.md @@ -56,9 +56,6 @@ returns 410; logout redirects back to login - P0: implement cache size management and eviction so the disk cannot fill up - P0: validate configuration on startup, fail fast on bad config -- P1: remove the ignored former secure toggle parameter from - session.NewManager and update its call sites (requires touching - existing tests; needs approval per repo rules) - P1: implement blocked networks configuration to extend SSRF protection - P1: rate limit global concurrent upstream fetches to prevent diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 069c458..31f07d5 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -95,9 +95,8 @@ func (s *Handlers) initImageService() error { s.log.Info("image service initialized") // Initialize session manager (signing key is validated at config load - // time). The second argument is ignored: session cookies are always - // Secure/HttpOnly/SameSite=Strict. - sessMgr, err := session.NewManager(s.config.SigningKey, true) + // time). Session cookies are always Secure/HttpOnly/SameSite=Strict. + sessMgr, err := session.NewManager(s.config.SigningKey) if err != nil { return err } diff --git a/internal/session/session.go b/internal/session/session.go index 17e9a1f..bf63543 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -44,10 +44,8 @@ type Manager struct { // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict // attributes; this cannot be configured. Browsers treat http://localhost as a // trustworthy origin and accept Secure cookies there, so local development -// keeps working. The second parameter is the former secure toggle: it is -// ignored and retained only so existing call sites keep compiling; it will be -// removed in a follow-up change. -func NewManager(signingKey string, _ bool) (*Manager, error) { +// keeps working. +func NewManager(signingKey string) (*Manager, error) { masterKey := []byte(signingKey) // Derive separate keys for HMAC (hash) and encryption (block) diff --git a/internal/session/session_cookie_attributes_test.go b/internal/session/session_cookie_attributes_test.go index 8f2ddd6..a0bb4ad 100644 --- a/internal/session/session_cookie_attributes_test.go +++ b/internal/session/session_cookie_attributes_test.go @@ -8,85 +8,75 @@ import ( // TestSessionCookieAttributesAlwaysSecure verifies that every cookie // emitted by the session manager carries HttpOnly, Secure, and a -// SameSite mode of Lax or stricter, regardless of how the manager was -// constructed. Session cookies contain the authentication state and -// must never be exposed to script (HttpOnly), sent over plaintext HTTP -// (Secure), or attached to cross-site requests (SameSite). No -// constructor argument may weaken these attributes. +// SameSite mode of Lax or stricter. Session cookies contain the +// authentication state and must never be exposed to script (HttpOnly), +// sent over plaintext HTTP (Secure), or attached to cross-site +// requests (SameSite). Nothing may weaken these attributes. // // This covers both cookie-writing paths: CreateSession (the login // set-cookie path) and ClearSession (the logout delete-cookie path). func TestSessionCookieAttributesAlwaysSecure(t *testing.T) { - for _, constructorBoolArg := range []bool{false, true} { - mgr, err := NewManager("test-signing-key-12345", constructorBoolArg) - if err != nil { - t.Fatalf("NewManager() error = %v", err) - } + mgr, err := NewManager("test-signing-key-12345") + if err != nil { + t.Fatalf("NewManager() error = %v", err) + } - writePaths := []struct { - name string - setCookie func(t *testing.T, w http.ResponseWriter) - }{ - { - name: "CreateSession", - setCookie: func(t *testing.T, w http.ResponseWriter) { - t.Helper() - if err := mgr.CreateSession(w); err != nil { - t.Fatalf("CreateSession() error = %v", err) - } - }, + writePaths := []struct { + name string + setCookie func(t *testing.T, w http.ResponseWriter) + }{ + { + name: "CreateSession", + setCookie: func(t *testing.T, w http.ResponseWriter) { + t.Helper() + if err := mgr.CreateSession(w); err != nil { + t.Fatalf("CreateSession() error = %v", err) + } }, - { - name: "ClearSession", - setCookie: func(t *testing.T, w http.ResponseWriter) { - t.Helper() - mgr.ClearSession(w) - }, + }, + { + name: "ClearSession", + setCookie: func(t *testing.T, w http.ResponseWriter) { + t.Helper() + mgr.ClearSession(w) }, - } + }, + } - for _, writePath := range writePaths { - testName := writePath.name - if constructorBoolArg { - testName += "/constructorBoolArg=true" - } else { - testName += "/constructorBoolArg=false" + for _, writePath := range writePaths { + t.Run(writePath.name, func(t *testing.T) { + w := httptest.NewRecorder() + writePath.setCookie(t, w) + + var sessionCookie *http.Cookie + for _, c := range w.Result().Cookies() { + if c.Name == CookieName { + sessionCookie = c + + break + } } - t.Run(testName, func(t *testing.T) { - w := httptest.NewRecorder() - writePath.setCookie(t, w) + if sessionCookie == nil { + t.Fatalf("no cookie named %q was set", CookieName) + } - var sessionCookie *http.Cookie - for _, c := range w.Result().Cookies() { - if c.Name == CookieName { - sessionCookie = c + t.Logf("cookie attributes: HttpOnly=%v Secure=%v SameSite=%v", + sessionCookie.HttpOnly, sessionCookie.Secure, sessionCookie.SameSite) - break - } - } + if !sessionCookie.HttpOnly { + t.Error("session cookie must have HttpOnly set") + } - if sessionCookie == nil { - t.Fatalf("no cookie named %q was set", CookieName) - } + if !sessionCookie.Secure { + t.Error("session cookie must have Secure set") + } - t.Logf("cookie attributes: HttpOnly=%v Secure=%v SameSite=%v", - sessionCookie.HttpOnly, sessionCookie.Secure, sessionCookie.SameSite) - - if !sessionCookie.HttpOnly { - t.Error("session cookie must have HttpOnly set") - } - - if !sessionCookie.Secure { - t.Error("session cookie must have Secure set") - } - - if sessionCookie.SameSite != http.SameSiteLaxMode && - sessionCookie.SameSite != http.SameSiteStrictMode { - t.Errorf("session cookie SameSite = %v, want Lax (%v) or Strict (%v)", - sessionCookie.SameSite, http.SameSiteLaxMode, http.SameSiteStrictMode) - } - }) - } + if sessionCookie.SameSite != http.SameSiteLaxMode && + sessionCookie.SameSite != http.SameSiteStrictMode { + t.Errorf("session cookie SameSite = %v, want Lax (%v) or Strict (%v)", + sessionCookie.SameSite, http.SameSiteLaxMode, http.SameSiteStrictMode) + } + }) } } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 20bc55b..c971c27 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -8,7 +8,7 @@ import ( ) func TestManager_CreateAndValidate(t *testing.T) { - mgr, err := NewManager("test-signing-key-12345", false) + mgr, err := NewManager("test-signing-key-12345") if err != nil { t.Fatalf("NewManager() error = %v", err) } @@ -57,7 +57,7 @@ func TestManager_CreateAndValidate(t *testing.T) { } func TestManager_ValidateSession_NoCookie(t *testing.T) { - mgr, _ := NewManager("test-signing-key-12345", false) + mgr, _ := NewManager("test-signing-key-12345") req := httptest.NewRequest(http.MethodGet, "/", nil) @@ -72,7 +72,7 @@ func TestManager_ValidateSession_NoCookie(t *testing.T) { } func TestManager_ValidateSession_TamperedCookie(t *testing.T) { - mgr, _ := NewManager("test-signing-key-12345", false) + mgr, _ := NewManager("test-signing-key-12345") req := httptest.NewRequest(http.MethodGet, "/", nil) req.AddCookie(&http.Cookie{ @@ -91,8 +91,8 @@ func TestManager_ValidateSession_TamperedCookie(t *testing.T) { } func TestManager_ValidateSession_WrongKey(t *testing.T) { - mgr1, _ := NewManager("signing-key-1", false) - mgr2, _ := NewManager("signing-key-2", false) + mgr1, _ := NewManager("signing-key-1") + mgr2, _ := NewManager("signing-key-2") // Create session with mgr1 w := httptest.NewRecorder() @@ -118,7 +118,7 @@ func TestManager_ValidateSession_WrongKey(t *testing.T) { } func TestManager_ClearSession(t *testing.T) { - mgr, _ := NewManager("test-signing-key-12345", false) + mgr, _ := NewManager("test-signing-key-12345") w := httptest.NewRecorder() mgr.ClearSession(w) @@ -144,7 +144,7 @@ func TestManager_ClearSession(t *testing.T) { } func TestManager_IsAuthenticated(t *testing.T) { - mgr, _ := NewManager("test-signing-key-12345", false) + mgr, _ := NewManager("test-signing-key-12345") // No session - should return false req := httptest.NewRequest(http.MethodGet, "/", nil) @@ -175,8 +175,7 @@ func TestManager_IsAuthenticated(t *testing.T) { } func TestManager_CookieAttributes(t *testing.T) { - // Test with secure=true - mgr, _ := NewManager("test-key", true) + mgr, _ := NewManager("test-key") w := httptest.NewRecorder() _ = mgr.CreateSession(w)