Files
pixa/internal/session/session_cookie_attributes_test.go
clawbot 275e145a6d
All checks were successful
check / check (push) Successful in 5s
fix: set Secure/HttpOnly/SameSite on session cookies (closes #47) (#48)
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>
2026-08-07 17:41:03 +02:00

83 lines
2.2 KiB
Go

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