refactor: drop the ignored secure toggle parameter from session.NewManager
All checks were successful
check / check (push) Successful in 1m48s

Per review on PR #48: pre-1.0 there is no installed base to keep
compiling against, so remove the dead parameter in one pass instead of
deferring. NewManager now takes only the signing key. Update the
handlers.go call site and its stale comment, mechanically update the
NewManager call shapes in the session tests (assertions unchanged),
collapse the now-meaningless constructor-argument loop in the cookie
attributes test, and drop the moot P1 Future Step from TODO.md.

Reviewer-directed test call-site updates; cookie behavior is unchanged
from the previous commit (always Secure/HttpOnly/SameSite=Strict).
This commit is contained in:
2026-08-07 22:24:15 +07:00
parent 02ca16a68a
commit cb9e14eee7
5 changed files with 68 additions and 85 deletions

View File

@@ -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
}

View File

@@ -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)

View File

@@ -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)
}
})
}
}

View File

@@ -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)