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>
83 lines
2.2 KiB
Go
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)
|
|
}
|
|
})
|
|
}
|
|
}
|