fix: set Secure/HttpOnly/SameSite on session cookies (closes #47) (#48)
All checks were successful
check / check (push) Successful in 5s
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>
This commit was merged in pull request #48.
This commit is contained in:
82
internal/session/session_cookie_attributes_test.go
Normal file
82
internal/session/session_cookie_attributes_test.go
Normal file
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user