From ca15f52dcce8102014f44135a6bf2ae5e72d09f2 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 21:02:07 +0700 Subject: [PATCH 1/3] test: require Secure/HttpOnly/SameSite on all session cookies Add a failing test asserting that every cookie written by the session manager (both the CreateSession set-cookie path and the ClearSession delete-cookie path) carries HttpOnly, Secure, and a SameSite mode of Lax or stricter, regardless of constructor arguments. Session cookies carry authentication state and must never be sent over plaintext HTTP. Currently fails for the constructor secure=false case, which produces cookies without the Secure attribute (gosec G124 at internal/session/session.go:84 and :128). --- .../session/session_cookie_attributes_test.go | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 internal/session/session_cookie_attributes_test.go diff --git a/internal/session/session_cookie_attributes_test.go b/internal/session/session_cookie_attributes_test.go new file mode 100644 index 0000000..8f2ddd6 --- /dev/null +++ b/internal/session/session_cookie_attributes_test.go @@ -0,0 +1,92 @@ +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, 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. +// +// 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) + } + + 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 { + testName := writePath.name + if constructorBoolArg { + testName += "/constructorBoolArg=true" + } else { + testName += "/constructorBoolArg=false" + } + + t.Run(testName, 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) + } + }) + } + } +} -- 2.49.1 From 02ca16a68a519d61f180e4abeb94be66b562f37c Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 21:14:16 +0700 Subject: [PATCH 2/3] fix: always set Secure/HttpOnly/SameSite on session cookies (closes #47) Resolve the two remaining gosec G124 findings (internal/session/ session.go:84 and :128): session cookies are now unconditionally Secure, HttpOnly, and SameSite=Strict on both the CreateSession set-cookie path and the ClearSession delete-cookie path. gosec requires these attributes to be constant, and there is no legitimate configuration in which the authentication cookie should be weaker, so the former secure toggle (wired to !config.Debug) is removed rather than kept as a variable. The toggle parameter on NewManager is retained as an ignored blank parameter so existing call sites (including tests) keep compiling; removing it is tracked as a Future Step in TODO.md. Local development over http://localhost keeps working because browsers treat localhost as a trustworthy origin and accept Secure cookies there. Update TODO.md per its Workflow section: record this step as completed, promote the manual auth/URL-flow test pass to Next Step, and correct the stale Status text (make check is now green). --- TODO.md | 33 ++++++++++++++++++--------------- internal/handlers/handlers.go | 6 ++++-- internal/session/session.go | 26 ++++++++++++++------------ 3 files changed, 36 insertions(+), 29 deletions(-) diff --git a/TODO.md b/TODO.md index 9ee4d18..ca2a88f 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,14 +53,12 @@ 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 +- 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 f47d165..069c458 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -94,8 +94,10 @@ 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). The second argument is ignored: session cookies are always + // Secure/HttpOnly/SameSite=Strict. + sessMgr, err := session.NewManager(s.config.SigningKey, true) if err != nil { return err } diff --git a/internal/session/session.go b/internal/session/session.go index 1a0c5c9..17e9a1f 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -36,14 +36,18 @@ 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. 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) { masterKey := []byte(signingKey) // Derive separate keys for HMAC (hash) and encryption (block) @@ -61,9 +65,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 +89,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 +133,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, }) } -- 2.49.1 From cb9e14eee7e2af512d91218451ee14af6430e02e Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 22:24:15 +0700 Subject: [PATCH 3/3] refactor: drop the ignored secure toggle parameter from session.NewManager 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). --- TODO.md | 3 - internal/handlers/handlers.go | 5 +- internal/session/session.go | 6 +- .../session/session_cookie_attributes_test.go | 122 ++++++++---------- internal/session/session_test.go | 17 ++- 5 files changed, 68 insertions(+), 85 deletions(-) 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) -- 2.49.1