diff --git a/TODO.md b/TODO.md index 9ee4d18..29359f7 100644 --- a/TODO.md +++ b/TODO.md @@ -10,22 +10,27 @@ # Status -pre-1.0. No git tags exist. main (6b4a1d7, 2026-04-07) is current with -origin; recent work extracted the internal/magic and internal/allowlist -packages. The 10 gosec findings from the 2026-07-06 morning survey are -NOT fixed on main: the newest gosec/lint fix commits are from -2026-02-25 (85729d9, ce6db76) and nothing since touches gosec, so those -findings remain open. +pre-1.0. No git tags exist. Recent work extracted the internal/magic, +internal/allowlist, internal/httpfetcher, and internal/signature +packages. The gosec findings from the 2026-07-06 survey are resolved: +the last two open findings (G124, session cookie attributes in +internal/session) are fixed as of this change, so `make check` is green +on main. # Next Step -Fix the 10 open gosec lint findings so make check passes on main again -(main must always be green; no gosec fix commits have landed since -2026-02-25). No blanket suppressions; fix or justify each finding -individually. +P0: manual test pass of the auth and encrypted URL flows, then commit +the checked-off results to TODO.md: visit / and see the login form; +wrong key shows an error; correct signing key shows the generator form; +a generated encrypted URL serves the image; an expired URL (short TTL) +returns 410; logout redirects back to login # Completed Steps +- 2026-08-07 fix the two remaining gosec findings (G124 in + internal/session): session cookies now always carry + Secure/HttpOnly/SameSite=Strict on both the set and clear paths; + `make check` green (closes #47) - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile shims, README Entrypoints section - 2026-04-07 extract magic byte detection into internal/magic (#42) @@ -48,11 +53,6 @@ individually. # Future Steps -- P0: manual test pass of the auth and encrypted URL flows, then - commit the checked-off results to TODO.md: visit / and see the login - form; wrong key shows an error; correct signing key shows the - generator form; a generated encrypted URL serves the image; an - expired URL (short TTL) 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 diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index f47d165..31f07d5 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -94,8 +94,9 @@ func (s *Handlers) initImageService() error { s.imgSvc = svc s.log.Info("image service initialized") - // Initialize session manager (signing key is validated at config load time) - sessMgr, err := session.NewManager(s.config.SigningKey, !s.config.Debug) + // Initialize session manager (signing key is validated at config load + // 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 1a0c5c9..bf63543 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -36,14 +36,16 @@ type Data struct { // Manager handles session creation and validation using encrypted cookies. type Manager struct { - sc *securecookie.SecureCookie - secure bool // Set Secure flag on cookies (should be true in production) - sameSite http.SameSite + sc *securecookie.SecureCookie } // NewManager creates a session manager with keys derived from the signing key. -// Set secure=true in production to require HTTPS for cookies. -func NewManager(signingKey string, secure bool) (*Manager, error) { +// +// 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. +func NewManager(signingKey string) (*Manager, error) { masterKey := []byte(signingKey) // Derive separate keys for HMAC (hash) and encryption (block) @@ -61,9 +63,7 @@ func NewManager(signingKey string, secure bool) (*Manager, error) { sc.MaxAge(int(SessionTTL.Seconds())) return &Manager{ - sc: sc, - secure: secure, - sameSite: http.SameSiteStrictMode, + sc: sc, }, nil } @@ -87,8 +87,8 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error { Path: "/", MaxAge: int(SessionTTL.Seconds()), HttpOnly: true, - Secure: m.secure, - SameSite: m.sameSite, + Secure: true, + SameSite: http.SameSiteStrictMode, }) return nil @@ -131,8 +131,8 @@ func (m *Manager) ClearSession(w http.ResponseWriter) { Path: "/", MaxAge: -1, // Delete immediately HttpOnly: true, - Secure: m.secure, - SameSite: m.sameSite, + Secure: true, + SameSite: http.SameSiteStrictMode, }) } diff --git a/internal/session/session_cookie_attributes_test.go b/internal/session/session_cookie_attributes_test.go new file mode 100644 index 0000000..a0bb4ad --- /dev/null +++ b/internal/session/session_cookie_attributes_test.go @@ -0,0 +1,82 @@ +package session + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestSessionCookieAttributesAlwaysSecure verifies that every cookie +// emitted by the session manager carries HttpOnly, Secure, and a +// 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) { + 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) + } + }, + }, + { + name: "ClearSession", + setCookie: func(t *testing.T, w http.ResponseWriter) { + t.Helper() + mgr.ClearSession(w) + }, + }, + } + + 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 + } + } + + if sessionCookie == nil { + t.Fatalf("no cookie named %q was set", CookieName) + } + + 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) + } + }) + } +} 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)