All checks were successful
check / check (push) Successful in 5s
closes #47 Fixes the two remaining `gosec` findings on `main`, both `G124` (http.Cookie missing or has insecure `Secure`, `HttpOnly`, or `SameSite` attribute): - `internal/session/session.go:84` (`CreateSession`, the login set-cookie path) - `internal/session/session.go:128` (`ClearSession`, the logout delete-cookie path) ## What changed - Both cookie-writing paths now unconditionally set `Secure: true`, `HttpOnly: true`, and `SameSite: http.SameSiteStrictMode`. - The `secure` field (previously wired to `!config.Debug`) and the `sameSite` field are removed from `session.Manager`, and the dead secure-toggle parameter is removed from `session.NewManager`, which now takes only the signing key (reviewer-directed; the mechanical call-shape updates in `session_test.go` leave every assertion untouched). - TDD per repo rules: the first commit adds `TestSessionCookieAttributesAlwaysSecure` (failing), asserting that every cookie emitted by the session manager carries `HttpOnly`, `Secure`, and `SameSite` of Lax or stricter, for both write paths. The second commit makes it pass. - `TODO.md` updated per its Workflow section (Next Step completed, next Future Step promoted, stale "10 open findings" Status text corrected). ## Attribute choices and reasoning - `Secure: true` always: the `G124` analyzer only accepts a constant `true` store, and there is no legitimate configuration in which the authentication cookie should be sent over plaintext HTTP. The old behavior disabled `Secure` whenever `debug` was on. Local development over `http://localhost` keeps working: browsers treat `localhost` as a trustworthy origin and accept `Secure` cookies there. Any plain-HTTP flow on a non-localhost host will no longer keep a session, which is the point of the fix. - `SameSite: Strict` (unchanged from current production behavior, and stricter than the Lax minimum): the login form is a same-origin POST to `/` followed by a same-site redirect, so `Strict` breaks nothing. - `HttpOnly: true` (unchanged). ## Verification `make check` (tests, golangci-lint, fmt-check) is fully green on the branch head `cb9e14e`: all tests pass and the linter reports 0 issues, independently confirmed by the reviewer in a fresh worktree. Commit history: `ca15f52` (failing test) → `02ca16a` (fix + TODO.md, closes #47) → `cb9e14e` (drop the dead `NewManager` parameter). Co-authored-by: sneak <sneak@sneak.berlin> Reviewed-on: #48 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org>
204 lines
4.5 KiB
Go
204 lines
4.5 KiB
Go
package session
|
|
|
|
import (
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
func TestManager_CreateAndValidate(t *testing.T) {
|
|
mgr, err := NewManager("test-signing-key-12345")
|
|
if err != nil {
|
|
t.Fatalf("NewManager() error = %v", err)
|
|
}
|
|
|
|
// Create a session
|
|
w := httptest.NewRecorder()
|
|
if err := mgr.CreateSession(w); err != nil {
|
|
t.Fatalf("CreateSession() error = %v", err)
|
|
}
|
|
|
|
// Extract the cookie from response
|
|
resp := w.Result()
|
|
cookies := resp.Cookies()
|
|
if len(cookies) == 0 {
|
|
t.Fatal("CreateSession() did not set a cookie")
|
|
}
|
|
|
|
var sessionCookie *http.Cookie
|
|
for _, c := range cookies {
|
|
if c.Name == CookieName {
|
|
sessionCookie = c
|
|
break
|
|
}
|
|
}
|
|
|
|
if sessionCookie == nil {
|
|
t.Fatalf("CreateSession() did not set cookie named %q", CookieName)
|
|
}
|
|
|
|
// Validate the session
|
|
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
req.AddCookie(sessionCookie)
|
|
|
|
data, err := mgr.ValidateSession(req)
|
|
if err != nil {
|
|
t.Fatalf("ValidateSession() error = %v", err)
|
|
}
|
|
|
|
if !data.Authenticated {
|
|
t.Error("ValidateSession() returned unauthenticated session")
|
|
}
|
|
|
|
if data.ExpiresAt.Before(time.Now()) {
|
|
t.Error("ValidateSession() returned already-expired session")
|
|
}
|
|
}
|
|
|
|
func TestManager_ValidateSession_NoCookie(t *testing.T) {
|
|
mgr, _ := NewManager("test-signing-key-12345")
|
|
|
|
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
|
|
_, err := mgr.ValidateSession(req)
|
|
if err == nil {
|
|
t.Error("ValidateSession() should fail with no cookie")
|
|
}
|
|
|
|
if err != ErrNoSession {
|
|
t.Errorf("ValidateSession() error = %v, want %v", err, ErrNoSession)
|
|
}
|
|
}
|
|
|
|
func TestManager_ValidateSession_TamperedCookie(t *testing.T) {
|
|
mgr, _ := NewManager("test-signing-key-12345")
|
|
|
|
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
req.AddCookie(&http.Cookie{
|
|
Name: CookieName,
|
|
Value: "tampered-invalid-cookie-value",
|
|
})
|
|
|
|
_, err := mgr.ValidateSession(req)
|
|
if err == nil {
|
|
t.Error("ValidateSession() should fail with tampered cookie")
|
|
}
|
|
|
|
if err != ErrInvalidSession {
|
|
t.Errorf("ValidateSession() error = %v, want %v", err, ErrInvalidSession)
|
|
}
|
|
}
|
|
|
|
func TestManager_ValidateSession_WrongKey(t *testing.T) {
|
|
mgr1, _ := NewManager("signing-key-1")
|
|
mgr2, _ := NewManager("signing-key-2")
|
|
|
|
// Create session with mgr1
|
|
w := httptest.NewRecorder()
|
|
_ = mgr1.CreateSession(w)
|
|
|
|
resp := w.Result()
|
|
var sessionCookie *http.Cookie
|
|
for _, c := range resp.Cookies() {
|
|
if c.Name == CookieName {
|
|
sessionCookie = c
|
|
break
|
|
}
|
|
}
|
|
|
|
// Try to validate with mgr2 (different key)
|
|
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
req.AddCookie(sessionCookie)
|
|
|
|
_, err := mgr2.ValidateSession(req)
|
|
if err == nil {
|
|
t.Error("ValidateSession() should fail with different signing key")
|
|
}
|
|
}
|
|
|
|
func TestManager_ClearSession(t *testing.T) {
|
|
mgr, _ := NewManager("test-signing-key-12345")
|
|
|
|
w := httptest.NewRecorder()
|
|
mgr.ClearSession(w)
|
|
|
|
resp := w.Result()
|
|
cookies := resp.Cookies()
|
|
|
|
var sessionCookie *http.Cookie
|
|
for _, c := range cookies {
|
|
if c.Name == CookieName {
|
|
sessionCookie = c
|
|
break
|
|
}
|
|
}
|
|
|
|
if sessionCookie == nil {
|
|
t.Fatal("ClearSession() did not set a cookie")
|
|
}
|
|
|
|
if sessionCookie.MaxAge != -1 {
|
|
t.Errorf("ClearSession() cookie MaxAge = %d, want -1", sessionCookie.MaxAge)
|
|
}
|
|
}
|
|
|
|
func TestManager_IsAuthenticated(t *testing.T) {
|
|
mgr, _ := NewManager("test-signing-key-12345")
|
|
|
|
// No session - should return false
|
|
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
if mgr.IsAuthenticated(req) {
|
|
t.Error("IsAuthenticated() should return false with no session")
|
|
}
|
|
|
|
// Create session
|
|
w := httptest.NewRecorder()
|
|
_ = mgr.CreateSession(w)
|
|
|
|
resp := w.Result()
|
|
var sessionCookie *http.Cookie
|
|
for _, c := range resp.Cookies() {
|
|
if c.Name == CookieName {
|
|
sessionCookie = c
|
|
break
|
|
}
|
|
}
|
|
|
|
// With valid session - should return true
|
|
req = httptest.NewRequest(http.MethodGet, "/", nil)
|
|
req.AddCookie(sessionCookie)
|
|
|
|
if !mgr.IsAuthenticated(req) {
|
|
t.Error("IsAuthenticated() should return true with valid session")
|
|
}
|
|
}
|
|
|
|
func TestManager_CookieAttributes(t *testing.T) {
|
|
mgr, _ := NewManager("test-key")
|
|
|
|
w := httptest.NewRecorder()
|
|
_ = mgr.CreateSession(w)
|
|
|
|
resp := w.Result()
|
|
var sessionCookie *http.Cookie
|
|
for _, c := range resp.Cookies() {
|
|
if c.Name == CookieName {
|
|
sessionCookie = c
|
|
break
|
|
}
|
|
}
|
|
|
|
if !sessionCookie.HttpOnly {
|
|
t.Error("Cookie should have HttpOnly flag")
|
|
}
|
|
|
|
if !sessionCookie.Secure {
|
|
t.Error("Cookie should have Secure flag when manager created with secure=true")
|
|
}
|
|
|
|
if sessionCookie.SameSite != http.SameSiteStrictMode {
|
|
t.Errorf("Cookie SameSite = %v, want %v", sessionCookie.SameSite, http.SameSiteStrictMode)
|
|
}
|
|
}
|