diff --git a/AGENTS.md b/AGENTS.md index a380a55..849296d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,7 @@ read the file that matches your task: | Upgrading a consumer past the 2026-08-18 breaking changes | [docs/agents/migration-service-user.md](docs/agents/migration-service-user.md), [docs/agents/migration-modular-gorm-stores.md](docs/agents/migration-modular-gorm-stores.md) | | Upgrading a consumer of the removed `preset` package (2026-08-23) | [docs/agents/migration-remove-preset.md](docs/agents/migration-remove-preset.md) | | Upgrading a consumer of the removed `loginid` package (2026-08-23) | [docs/agents/migration-loginid-into-user.md](docs/agents/migration-loginid-into-user.md) | +| Session cookies tightened to `SameSite=Lax` (2026-08-23) | [docs/agents/migration-cookie-samesite.md](docs/agents/migration-cookie-samesite.md) | | Anything under `flow/login/` (engine, policies, methods, attempt stores, JSON transport) | [docs/agents/loginflow.md](docs/agents/loginflow.md) | | "Is X implemented?" — feature status and gaps | [docs/agents/features.md](docs/agents/features.md) | | Writing/running tests, `make verify`, coverage, lint | [docs/agents/testing.md](docs/agents/testing.md) | diff --git a/TODO.md b/TODO.md index a1fdc71..0cfec27 100644 --- a/TODO.md +++ b/TODO.md @@ -50,9 +50,9 @@ Notes for editors: # Security - [ ] No CSRF protection - Left entirely to the consumer, with no guidance or helpers. -- [ ] Session cookies use SameSite=None - Both the registry session-id cookie (auth/cookieauth/sidcookie.go) and NewCookieStore (auth/cookieauth/cookiestore.go) set SameSite=None; combined with the profile example's bodyless POST to sessionsRevokeOthers and no CSRF, any site can log a victim out everywhere. Intent is to move both to Lax in the next minor; the //nolint:gosec suppressions stay until then. + Left entirely to the consumer, with no guidance or helpers. SameSite=Lax (auth/cookieauth/cookieattrs.go) is now the only mitigation the library provides; it withholds the cookie from cross-site POSTs but is not a substitute for tokens. +- [x] Session cookies use SameSite=None + Fixed: auth/cookieauth/cookieattrs.go is now the single source of truth for Path/HttpOnly/Secure/SameSite, applied by NewCookieStore, NewFsStore (previously gorilla's bare defaults) and the registry session-id cookie alike. Default SameSite=Lax; Cfg.SameSite opts the registry flavour back into None. One //nolint:gosec remains, on the single cookie literal, because G124 cannot prove a non-constant value. See docs/agents/migration-cookie-samesite.md. # Error handling & observability diff --git a/auth/cookieauth/cookie.go b/auth/cookieauth/cookie.go index 005ae5a..5c42084 100644 --- a/auth/cookieauth/cookie.go +++ b/auth/cookieauth/cookie.go @@ -49,7 +49,20 @@ type Cfg struct { MinWriteSpace time.Duration // CookieName is the session cookie name. Defaults to DefaultCookieName. CookieName string - Logger *slog.Logger + + // SameSite sets the SameSite attribute of the session-identifier cookie. + // The zero value defaults to http.SameSiteLaxMode. + // + // It belongs to the registry flavour, which builds that cookie itself; in + // the stateless flavour the cookie is the Store's, so set it on the Store's + // own Options and leave this unset. + // + // Set http.SameSiteNoneMode only for a deliberate cross-site deployment: it + // makes the cookie travel on cross-site POSTs, and this library ships no + // CSRF protection, so that protection becomes yours to add. + SameSite http.SameSite + + Logger *slog.Logger } // Manager manages session storage and validation. It implements chain.AuthHandler. @@ -62,6 +75,7 @@ type Manager struct { minWriteSpace time.Duration maxSessionDur time.Duration cookieName string + sameSite http.SameSite logger *slog.Logger } @@ -90,6 +104,9 @@ func New(cfg Cfg) (*Manager, error) { } else if len(cfg.HashKey) > 0 || len(cfg.BlockKey) > 0 { return nil, fmt.Errorf("cookieauth: HashKey and BlockKey belong to the Registry flavour; " + "the Store flavour's keys are the Store's own") + } else if cfg.SameSite != 0 { + return nil, fmt.Errorf("cookieauth: SameSite belongs to the Registry flavour; " + + "the Store flavour's cookie is the Store's, so set store.Options.SameSite instead") } if cfg.SessionDur == 0 { cfg.SessionDur = time.Hour @@ -113,6 +130,7 @@ func New(cfg Cfg) (*Manager, error) { minWriteSpace: cfg.MinWriteSpace, maxSessionDur: cfg.MaxSessionDur, cookieName: cfg.CookieName, + sameSite: resolveSameSite(cfg.SameSite), store: cfg.Store, registry: cfg.Registry, logger: cfg.Logger.With("auth-handler", SessionMngrName), @@ -147,7 +165,7 @@ func checkRegistryCfg(cfg Cfg) error { if l := len(cfg.BlockKey); l != 0 && l != 16 && l != 24 && l != 32 { return fmt.Errorf("cookieauth: BlockKey length should be 16, 24 or 32 bytes, got %d", l) } - return nil + return checkSameSite(cfg.SameSite) } // Name implements chain.AuthHandler. diff --git a/auth/cookieauth/cookieattrs.go b/auth/cookieauth/cookieattrs.go new file mode 100644 index 0000000..5348196 --- /dev/null +++ b/auth/cookieauth/cookieattrs.go @@ -0,0 +1,69 @@ +package cookieauth + +import ( + "fmt" + "net/http" + + "github.com/gorilla/sessions" +) + +// This file is the single source of truth for the attributes of every session +// cookie this package issues: the two convenience store constructors +// (NewCookieStore, NewFsStore) for the stateless flavour, and the +// session-identifier cookie the Manager builds by hand for the registry +// flavour. Both have to behave identically in a browser, which only holds if +// there is one place that says what "identically" means. +// +// SameSite=Lax is the default, and it is a security decision rather than a +// preference: this library ships no CSRF protection, and its own logout and +// session-revocation endpoints are state-changing requests with no body to +// validate. A cookie that travels on cross-site POSTs therefore lets any site +// act on a visitor's session — logging them out everywhere, at minimum. Lax +// still sends the cookie on top-level cross-site navigation, so following a +// link into the application keeps the user logged in. +// +// A deliberate cross-site deployment can opt back into +// http.SameSiteNoneMode (Cfg.SameSite, or the Store's own Options), and owns +// the CSRF protection that choice requires. +const defaultSameSite = http.SameSiteLaxMode + +// defaultStoreMaxAge is the cookie lifetime the store constructors apply: the +// callers pass it to the store's MaxAge, so it bounds the securecookie timestamp +// validity as well as the browser's Max-Age. The registry flavour sets Max-Age +// per session instead, from the record's absolute deadline, but is subject to +// the same 30-day codec ceiling (see sidcookie.go). +const defaultStoreMaxAge = 86400 * 30 + +// storeOptions returns the cookie policy for a stateless-flavour session store. +// The caller may still adjust the returned Options: these are defaults, not a +// cage. +func storeOptions() *sessions.Options { + return &sessions.Options{ + Path: "/", + MaxAge: defaultStoreMaxAge, + HttpOnly: true, + Secure: true, + SameSite: defaultSameSite, + } +} + +// resolveSameSite maps the zero value onto the package default. http.SameSite +// has no "unset" constant — its zero value is not SameSiteDefaultMode and makes +// net/http omit the attribute entirely — so zero is what an unconfigured Cfg +// looks like, and it must mean Lax rather than "no policy". +func resolveSameSite(s http.SameSite) http.SameSite { + if s == 0 { + return defaultSameSite + } + return s +} + +// checkSameSite rejects a value net/http would silently drop, which would +// downgrade the cookie to no SameSite attribute at all. +func checkSameSite(s http.SameSite) error { + switch s { + case 0, http.SameSiteDefaultMode, http.SameSiteLaxMode, http.SameSiteStrictMode, http.SameSiteNoneMode: + return nil + } + return fmt.Errorf("cookieauth: SameSite value %d is not one of http.SameSite{Default,Lax,Strict,None}Mode", s) +} diff --git a/auth/cookieauth/cookieattrs_test.go b/auth/cookieauth/cookieattrs_test.go new file mode 100644 index 0000000..d90da7b --- /dev/null +++ b/auth/cookieauth/cookieattrs_test.go @@ -0,0 +1,226 @@ +package cookieauth_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-bumbu/userauth/auth/cookieauth" + "github.com/go-bumbu/userauth/service/session" + sessionmem "github.com/go-bumbu/userauth/service/session/store/memory" + "github.com/google/go-cmp/cmp" + "github.com/gorilla/securecookie" + "github.com/gorilla/sessions" +) + +// attrs is the browser-visible part of a session cookie: the security policy, +// without the value or the lifetime. Both flavours and both store constructors +// must agree on it. +type attrs struct { + Path string + HttpOnly bool + Secure bool + SameSite http.SameSite +} + +// wantAttrs is the policy this package issues. SameSite=Lax is the point of +// these tests: the library ships no CSRF helpers, and its own logout and +// session-revocation endpoints are bodyless POSTs, so a cookie that travels on +// cross-site POSTs lets any site log a victim out everywhere. +var wantAttrs = attrs{ + Path: "/", + HttpOnly: true, + Secure: true, + SameSite: http.SameSiteLaxMode, +} + +func cookieAttrs(c *http.Cookie) attrs { + return attrs{Path: c.Path, HttpOnly: c.HttpOnly, Secure: c.Secure, SameSite: c.SameSite} +} + +func optionAttrs(o *sessions.Options) attrs { + return attrs{Path: o.Path, HttpOnly: o.HttpOnly, Secure: o.Secure, SameSite: o.SameSite} +} + +// newSameSiteManager builds a registry-flavour Manager with an explicit +// SameSite, so the override can be exercised end to end. +func newSameSiteManager(t *testing.T, sameSite http.SameSite) *cookieauth.Manager { + t.Helper() + svc, err := session.NewService(sessionmem.New(), session.Opts{}) + if err != nil { + t.Fatalf("NewService: %v", err) + } + mgr, err := cookieauth.New(cookieauth.Cfg{ + Registry: svc, + HashKey: securecookie.GenerateRandomKey(64), + BlockKey: securecookie.GenerateRandomKey(32), + SameSite: sameSite, + }) + if err != nil { + t.Fatalf("cookieauth.New: %v", err) + } + return mgr +} + +// TestStoreConstructorsApplyTheCookiePolicy pins the attributes of the two +// convenience store constructors. NewFsStore is the one that used to inherit +// gorilla's bare defaults, i.e. no HttpOnly, no Secure and no SameSite at all. +func TestStoreConstructorsApplyTheCookiePolicy(t *testing.T) { + hashKey := securecookie.GenerateRandomKey(64) + blockKey := securecookie.GenerateRandomKey(32) + + t.Run("NewCookieStore", func(t *testing.T) { + store, err := cookieauth.NewCookieStore(hashKey, blockKey) + if err != nil { + t.Fatalf("NewCookieStore: %v", err) + } + if diff := cmp.Diff(wantAttrs, optionAttrs(store.Options)); diff != "" { + t.Errorf("cookie policy mismatch (-want +got):\n%s", diff) + } + }) + + t.Run("NewFsStore", func(t *testing.T) { + store, err := cookieauth.NewFsStore(t.TempDir(), hashKey, blockKey) + if err != nil { + t.Fatalf("NewFsStore: %v", err) + } + if diff := cmp.Diff(wantAttrs, optionAttrs(store.Options)); diff != "" { + t.Errorf("cookie policy mismatch (-want +got):\n%s", diff) + } + }) +} + +// TestBothFlavoursIssueTheSameCookiePolicy pins the parity the package claims: +// the registry flavour builds its session-identifier cookie by hand, and it has +// to end up indistinguishable from the stateless flavour's store-issued one. +func TestBothFlavoursIssueTheSameCookiePolicy(t *testing.T) { + registry := cookieAttrs(login(t, newSameSiteManager(t, 0), "user1", false)[0]) + stateless := cookieAttrs(login(t, newManager(t, cookieauth.Cfg{}), "user1", false)[0]) + + if diff := cmp.Diff(wantAttrs, registry); diff != "" { + t.Errorf("registry flavour cookie policy mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(wantAttrs, stateless); diff != "" { + t.Errorf("stateless flavour cookie policy mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(registry, stateless); diff != "" { + t.Errorf("the two flavours disagree (-registry +stateless):\n%s", diff) + } +} + +// TestLogoutCookieMatchesTheLoginCookie matters mechanically, not just for +// tidiness: a browser only overwrites a cookie when the deletion cookie carries +// the same name, path and domain, so a mismatch leaves the session cookie in +// place after logout. +func TestLogoutCookieMatchesTheLoginCookie(t *testing.T) { + mgr := newSameSiteManager(t, 0) + loginCookies := login(t, mgr, "user1", false) + + r := httptest.NewRequest(http.MethodPost, "/logout", nil) + for _, c := range loginCookies { + r.AddCookie(c) + } + w := httptest.NewRecorder() + if err := mgr.LogoutUser(r, w); err != nil { + t.Fatalf("LogoutUser: %v", err) + } + res := w.Result() + defer func() { _ = res.Body.Close() }() + got := res.Cookies() + if len(got) == 0 { + t.Fatal("LogoutUser set no cookie") + } + + if diff := cmp.Diff(cookieAttrs(loginCookies[0]), cookieAttrs(got[0])); diff != "" { + t.Errorf("deletion cookie will not overwrite the session cookie (-login +logout):\n%s", diff) + } + if got[0].MaxAge != -1 { + t.Errorf("deletion cookie MaxAge = %d, want -1", got[0].MaxAge) + } +} + +// TestCfgSameSiteOverridesTheDefault covers the registry flavour's only knob +// for the cookie attributes: the stateless flavour can already reach them +// through its Store's Options, this is the equivalent seam. +func TestCfgSameSiteOverridesTheDefault(t *testing.T) { + tcs := []struct { + name string + cfg http.SameSite + want http.SameSite + }{ + {name: "zero value defaults to Lax", cfg: 0, want: http.SameSiteLaxMode}, + {name: "explicit Lax", cfg: http.SameSiteLaxMode, want: http.SameSiteLaxMode}, + {name: "Strict", cfg: http.SameSiteStrictMode, want: http.SameSiteStrictMode}, + {name: "None for a deliberate cross-site deployment", cfg: http.SameSiteNoneMode, want: http.SameSiteNoneMode}, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + mgr := newSameSiteManager(t, tc.cfg) + c := login(t, mgr, "user1", false)[0] + if c.SameSite != tc.want { + t.Errorf("login cookie SameSite = %v, want %v", c.SameSite, tc.want) + } + if !c.Secure { + // SameSite=None is ignored by browsers without Secure, and the + // other modes have no reason to drop it either. + t.Error("login cookie is not Secure") + } + + // The deletion cookie has to carry the override too, or it will not + // overwrite the cookie the override produced. + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/logout", nil) + r.AddCookie(c) + if err := mgr.LogoutUser(r, w); err != nil { + t.Fatalf("LogoutUser: %v", err) + } + res := w.Result() + defer func() { _ = res.Body.Close() }() + deletion := res.Cookies() + if len(deletion) == 0 { + t.Fatal("LogoutUser set no cookie") + } + if deletion[0].SameSite != tc.want { + t.Errorf("deletion cookie SameSite = %v, want %v", deletion[0].SameSite, tc.want) + } + }) + } +} + +// TestNewRejectsSameSiteWithoutRegistry keeps the flavour boundary honest: in +// the stateless flavour the cookie attributes belong to the Store, so a +// SameSite set here would be silently ignored. +func TestNewRejectsSameSiteWithoutRegistry(t *testing.T) { + store, err := cookieauth.NewCookieStore(securecookie.GenerateRandomKey(64), securecookie.GenerateRandomKey(32)) + if err != nil { + t.Fatalf("NewCookieStore: %v", err) + } + _, err = cookieauth.New(cookieauth.Cfg{Store: store, SameSite: http.SameSiteStrictMode}) + if err == nil { + t.Fatal("expected an error when SameSite is set in the stateless flavour") + } + if !strings.Contains(err.Error(), "SameSite") { + t.Errorf("error should name the offending field, got: %v", err) + } +} + +// TestNewRejectsInvalidSameSite guards against a garbage value silently +// producing a cookie with no SameSite attribute at all. +func TestNewRejectsInvalidSameSite(t *testing.T) { + svc, err := session.NewService(sessionmem.New(), session.Opts{}) + if err != nil { + t.Fatalf("NewService: %v", err) + } + _, err = cookieauth.New(cookieauth.Cfg{ + Registry: svc, + HashKey: securecookie.GenerateRandomKey(64), + SameSite: http.SameSite(99), + }) + if err == nil { + t.Fatal("expected an error for an out-of-range SameSite value") + } + if !strings.Contains(err.Error(), "SameSite") { + t.Errorf("error should name the offending field, got: %v", err) + } +} diff --git a/auth/cookieauth/cookiestore.go b/auth/cookieauth/cookiestore.go index fbf1616..3aed9aa 100644 --- a/auth/cookieauth/cookiestore.go +++ b/auth/cookieauth/cookiestore.go @@ -2,7 +2,6 @@ package cookieauth import ( "fmt" - "net/http" "github.com/gorilla/securecookie" "github.com/gorilla/sessions" @@ -10,6 +9,9 @@ import ( // NewFsStore is a convenience function to generate a new session store that stores data on filesystem. // It uses a secure cookie to keep the session id. +// +// It overrides gorilla's cookie defaults with this package's policy +// (cookieattrs.go), which gorilla leaves without HttpOnly, Secure or SameSite. func NewFsStore(path string, HashKey, BlockKey []byte) (*sessions.FilesystemStore, error) { hashL := len(HashKey) if hashL != 32 && hashL != 64 { @@ -20,10 +22,14 @@ func NewFsStore(path string, HashKey, BlockKey []byte) (*sessions.FilesystemStor return nil, fmt.Errorf("blockKey length should be 16, 24 or 32 bytes") } fsStore := sessions.NewFilesystemStore(path, HashKey, BlockKey) + fsStore.Options = storeOptions() + fsStore.MaxAge(fsStore.Options.MaxAge) return fsStore, nil } -// NewCookieStore creates a new cookie store with the given keys. +// NewCookieStore creates a new cookie store with the given keys. Its cookies +// carry this package's attribute policy (cookieattrs.go); adjust the returned +// store's Options to deviate from it. func NewCookieStore(HashKey, BlockKey []byte) (*sessions.CookieStore, error) { hashL := len(HashKey) if hashL != 32 && hashL != 64 { @@ -35,14 +41,8 @@ func NewCookieStore(HashKey, BlockKey []byte) (*sessions.CookieStore, error) { } cs := &sessions.CookieStore{ - Codecs: securecookie.CodecsFromPairs(HashKey, BlockKey), - Options: &sessions.Options{ - Path: "/", - MaxAge: 86400 * 30, - SameSite: http.SameSiteNoneMode, - HttpOnly: true, - Secure: true, - }, + Codecs: securecookie.CodecsFromPairs(HashKey, BlockKey), + Options: storeOptions(), } cs.MaxAge(cs.Options.MaxAge) return cs, nil diff --git a/auth/cookieauth/sidcookie.go b/auth/cookieauth/sidcookie.go index 6aa0f6c..d538073 100644 --- a/auth/cookieauth/sidcookie.go +++ b/auth/cookieauth/sidcookie.go @@ -11,10 +11,22 @@ import ( // would silently stop decoding its own cookies after that ceiling. This matches // NewCookieStore's behaviour, so parity holds. -// sidCookie builds the session-identifier cookie. Its attributes match what -// NewCookieStore sets, so both flavours behave the same in a browser; expires -// is the session's absolute deadline, after which the client can stop sending -// it unprompted. +// sidCookie is the only place in the package that builds a session-identifier +// cookie, live or deletion. Its attributes mirror what the stateless flavour's +// store applies (see cookieattrs.go), so both flavours behave the same in a +// browser — and since a browser only replaces a cookie when the replacement +// agrees on name, path and domain, one builder is what makes logout reliably +// clear the cookie login set. +// +// Expires is the session's absolute deadline, after which the client can stop +// sending the cookie unprompted; an expiry in the past yields the deletion +// cookie. +// +// G124 cannot see through resolveSameSite to prove the attribute is safe. What +// it returns is SameSite=Lax unless an explicit Cfg.SameSite weakens it, and +// that field documents the CSRF obligation which comes with doing so. +// +//nolint:gosec // G124: SameSite is Cfg-configurable and defaults to Lax; see cookieattrs.go func (m *Manager) sidCookie(value string, expires time.Time) *http.Cookie { maxAge := int(time.Until(expires).Seconds()) if maxAge < 0 { @@ -25,12 +37,6 @@ func (m *Manager) sidCookie(value string, expires time.Time) *http.Cookie { // Reachable only with absurd sub-second AbsoluteTimeout. maxAge = 1 } - // G124 is correct that SameSite=None is the weakest setting; it is set - // deliberately, to match the attributes NewCookieStore already applies - // (cookiestore.go:42) so both flavours behave identically in a browser. - // Tightening it is a change to both flavours, tracked in TODO.md, not - // something to decide here. - //nolint:gosec // G124: SameSite=None is deliberate parity with NewCookieStore; see TODO.md return &http.Cookie{ Name: m.cookieName, Value: value, @@ -40,7 +46,7 @@ func (m *Manager) sidCookie(value string, expires time.Time) *http.Cookie { MaxAge: maxAge, HttpOnly: true, Secure: true, - SameSite: http.SameSiteNoneMode, + SameSite: resolveSameSite(m.sameSite), } } @@ -54,20 +60,11 @@ func (m *Manager) writeSID(w http.ResponseWriter, sessionID string, expires time return nil } -// clearSID expires the session-identifier cookie. +// clearSID expires the session-identifier cookie. It goes through sidCookie +// with a long-past expiry so the deletion cookie is attribute-for-attribute the +// one login set, which is what a browser requires to overwrite it. func (m *Manager) clearSID(w http.ResponseWriter) { - //nolint:gosec // G124: SameSite=None is deliberate parity with NewCookieStore; see TODO.md - http.SetCookie(w, &http.Cookie{ - Name: m.cookieName, - Value: "", - Path: "/", - Domain: "", - Expires: time.Unix(1, 0), - MaxAge: -1, - HttpOnly: true, - Secure: true, - SameSite: http.SameSiteNoneMode, - }) + http.SetCookie(w, m.sidCookie("", time.Unix(1, 0))) } // readSID returns the session identifier the client presented. An absent or diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index ffc5404..00c40f9 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -484,6 +484,37 @@ is silent config drift. On success the manager puts `UserData` into the request context. Stateless stores: `NewFsStore` (filesystem) or `NewCookieStore` (client-side), both gorilla/sessions. Registry stores: see **Sessions** above. +### Cookie attribute policy (`auth/cookieauth/cookieattrs.go`) + +Changed 2026-08-23. One file owns the browser-visible attributes of every +session cookie the package issues — `Path: "/"`, `HttpOnly`, `Secure`, +`SameSite=Lax` — because the two flavours must be indistinguishable in a +browser, and that only holds if a single place defines what that means. It feeds +`NewCookieStore`, `NewFsStore` and the registry flavour's hand-built +session-identifier cookie alike. + +- **`SameSite=Lax`, not `None`.** The old `None` was a security bug, not a + preference: the library ships no CSRF protection, and its own logout and + session-revocation endpoints are bodyless POSTs, so a cookie that travels on + cross-site POSTs let any site log a visitor out everywhere. `Lax` still + arrives on top-level cross-site navigation, so following a link into the app + keeps the user logged in. The rest of the library (`attemptstore/cookie`, + `pendingstore/cookie`) was already on `Lax`; `cookieauth` was the outlier. +- **`Cfg.SameSite` is the registry flavour's escape hatch**, zero value → `Lax`. + It exists for parity of *configurability*: a stateless-flavour consumer can + already reach these attributes through its `Store.Options`, whereas the + registry flavour's cookie is built by the Manager and had no knob at all. + Setting it in the stateless flavour is a construction error rather than a + silently ignored value, matching how `HashKey`/`BlockKey` are handled. +- **`NewFsStore` no longer inherits gorilla's bare defaults**, which carried no + `HttpOnly`, no `Secure` and no `SameSite` — weaker than either of the two + sites the TODO named. +- **One builder, one literal.** `clearSID` routes through `sidCookie` with a + past expiry instead of writing its own literal: a browser only replaces a + cookie when the replacement agrees on name, path and domain, so deriving the + deletion cookie from the builder is what keeps logout working. It also leaves + exactly one `//nolint:gosec` (G124 cannot prove a non-constant `SameSite`). + ## Dependencies - `gorilla/mux`, `gorilla/sessions`, `gorilla/securecookie` — HTTP + sessions @@ -494,7 +525,8 @@ context. Stateless stores: `NewFsStore` (filesystem) or `NewCookieStore` ## Known debt -The 2026-08-18 modular-store split closed the schema-coupling item, +The 2026-08-23 cookie-attribute change closed the `SameSite=None` item. The +2026-08-18 modular-store split closed the schema-coupling item, `service/user` + `service/password` the same day closed the store-owns-policy, password-policy and cost-migration items, and `service/session` closed session listing and revocation. Still open: `AttemptStore` leaking HTTP concerns, coarse diff --git a/docs/agents/features.md b/docs/agents/features.md index 9aa8117..d9e0c23 100644 --- a/docs/agents/features.md +++ b/docs/agents/features.md @@ -8,6 +8,7 @@ check here and in TODO.md — several gaps are known and have a chosen direction | Feature | Status | Where | |---|---|---| | Cookie/session auth | Implemented | `cookieauth.Manager` — two flavours: stateless (entire session in cookie, unchanged) and registry (server-side records, enumerable, revocable); see architecture.md | +| Cookie attribute policy | Implemented | `auth/cookieauth/cookieattrs.go` — one source of truth for `Path`/`HttpOnly`/`Secure`/`SameSite` across `NewCookieStore`, `NewFsStore` and the registry flavour's own cookie. `SameSite=Lax` by default; `Cfg.SameSite` overrides it in the registry flavour (stateless: use `Store.Options`) | | Session listing/revocation | Implemented | `service/session` — list, revoke, revoke-all, revoke-all-except, purge-user, purge-expired; registry flavour only | | Session metadata | Not implemented | Deferred to v2 — IP, user-agent, device name. `UserData.DeviceID` is the unfilled hook | | HTTP Basic Auth | Implemented | `basicauth` — optional enforce mode (`WWW-Authenticate`), no sessions, per-request verify | @@ -85,7 +86,9 @@ policy: it reports which of totp/email/sms a user has enabled. Compose a ## Not implemented (catalogued in TODO.md) -CSRF helpers, session metadata (v2 in the session spec), session HTTP handlers +CSRF helpers (`SameSite=Lax` is the only mitigation the library provides — see +the cookie attribute policy in architecture.md), session metadata (v2 in the +session spec), session HTTP handlers (v3, placement unresolved), email-based password reset, audit/event hooks, JWT/OAuth2/OIDC. Rate limiting is covered by `service/throttle` for per-credential backoff; per-IP/volumetric limiting stays the caller's job. diff --git a/docs/agents/migration-cookie-samesite.md b/docs/agents/migration-cookie-samesite.md new file mode 100644 index 0000000..f8a10c5 --- /dev/null +++ b/docs/agents/migration-cookie-samesite.md @@ -0,0 +1,56 @@ +# Cookie attributes tighten to `SameSite=Lax` + +Behaviour change, 2026-08-23 (pre-1.0). Every session cookie `auth/cookieauth` +issues now carries `SameSite=Lax` instead of `SameSite=None`, and `NewFsStore` +stops inheriting gorilla's bare defaults. **No API is removed and no data +migration is needed** — nothing stored changes, and code compiles unchanged. +What changes is what the browser does with the cookie. + +Why: `SameSite=None` was a security bug rather than a preference. This library +ships no CSRF protection, and its own logout and session-revocation endpoints +are bodyless POSTs, so a cookie that travels on cross-site POSTs let any site +act on a visitor's session — logging them out everywhere, at minimum. The rest +of the library (`flow/login/attemptstore/cookie`, +`flow/register/pendingstore/cookie`) was already on `Lax`; `cookieauth` was the +outlier. + +## What changed + +| Site | Before | After | +|---|---|---| +| `NewCookieStore` | `SameSite=None`, `HttpOnly`, `Secure` | `SameSite=Lax`, `HttpOnly`, `Secure` | +| `NewFsStore` | gorilla defaults: **no** `SameSite`, `HttpOnly` or `Secure` | `SameSite=Lax`, `HttpOnly`, `Secure` | +| registry session-id cookie (`sidcookie.go`) | `SameSite=None`, `HttpOnly`, `Secure` | `SameSite=Lax`, `HttpOnly`, `Secure` | + +The policy now lives in one place, `auth/cookieauth/cookieattrs.go`, which all +three read from. + +## New: `Cfg.SameSite` + +```go +mgr, err := cookieauth.New(cookieauth.Cfg{ + Registry: sessionSvc, + HashKey: hashKey, + SameSite: http.SameSiteNoneMode, // opt back into cross-site, CSRF is yours +}) +``` + +Registry flavour only, zero value → `Lax`. The stateless flavour's cookie +belongs to its `Store`, so set `store.Options.SameSite` there; passing +`Cfg.SameSite` alongside `Cfg.Store` is now a **construction error**, in line +with how `HashKey`/`BlockKey` are rejected in that flavour. An out-of-range +value is rejected too, rather than silently producing a cookie with no +`SameSite` attribute at all. + +## Do I need to do anything? + +- **Same-site deployment** (app and API on one site): no. This is a pure + hardening; a link from another site into your app still keeps the session. +- **Cross-site deployment** (SPA on a different site than the API, third-party + embed, iframe): yes — the cookie will no longer be sent, and users appear + logged out. Set `Cfg.SameSite: http.SameSiteNoneMode` (registry flavour) or + `store.Options.SameSite = http.SameSiteNoneMode` (stateless), and add CSRF + protection, which the library still does not provide. +- **`NewFsStore` over plain HTTP**: yes — its cookies are now `Secure`, so a + browser will not send them over `http://` (localhost excepted). Terminate TLS, + or override `store.Options.Secure` for a local-only setup.