harden(auth/sessions): CSRF rotation on logout closes HIGH-2 fourth call site

Audit 2026-05-11 Fix 13 closure. The HIGH-2 closure on
dev/auth-bundle-2 documented four RotateCSRFTokenForActor call
sites — login completion (fresh by construction), Assign/Revoke
RoleToKey (wired at internal/api/handler/auth.go:498 + 546),
Logout, and an explicit operator endpoint. The 2026-05-11
adversarial review observed only 3 of the 4: Logout did NOT
rotate the actor's sibling sessions post-revoke.

Threat closed: a token captured pre-logout (browser DevTools,
malicious extension, session-storage leak) could be replayed
against the user's other-device/other-browser sessions until
those sessions hit their own idle/absolute expiry. Rotation on
logout defeats this — the captured token is dead the moment
the user clicks 'Sign out' anywhere.

What this changes:

* internal/api/handler/auth_session_oidc.go::SessionMinter
  interface gains RotateCSRFTokenForActor(ctx, actorID,
  actorType string) int. Nil-safe semantics by convention —
  the production wiring is *session.Service which already
  implements the method; rotation NEVER errors (returns int
  count, swallows per-row failures via the underlying
  Service.RotateCSRFToken) so it can't block the surrounding
  Revoke that triggered it.

* internal/api/handler/auth_session_oidc.go::Logout calls
  RotateCSRFTokenForActor after Revoke(sess.ID) succeeds. The
  auth.session_revoked audit row gains a csrf_rotated detail
  key carrying the count so SOC/SIEM can correlate logout
  events with CSRF churn on sibling sessions.

* The no-cookie + invalid-cookie 204 short-circuit paths
  skip rotation. No session row exists to rotate against;
  the caller is already unauthenticated. Rotation on those
  paths would do nothing useful and pollute the audit log.

Test coverage in internal/api/handler/auth_session_oidc_test.go:

* TestLogout_RotatesCSRFForActor — happy path. Mocks
  rotateCSRFReturnCount=2; asserts Revoke fires before
  rotation, rotation fires exactly once with caller's
  (actor_id, actor_type), audit details carry csrf_rotated=2.

* TestLogout_NoCookie_SkipsCSRFRotation — pins the 204
  short-circuit branch when there's no cookie. Rotation count
  stays at 0.

* TestLogout_InvalidCookie_SkipsCSRFRotation — pins the 204
  short-circuit branch when Validate rejects the cookie.
  Same rationale: no session row, no rotation.

The stubSession test fake gains RotateCSRFTokenForActor with
call-recording fields; the phase5StubAudit gains a details
slice append-aligned 1:1 with events so the happy-path test
can index into the latest entry and assert the count.

Spec Phase 3 (explicit operator endpoint) — intentionally
NOT shipped. The three automatic triggers (login + role-
mutation + logout) cover the HIGH-2 threat model; operators
who want a nuclear option can use the existing
RevokeAllForActor flow which forces re-login → fresh session
→ fresh CSRF. Adding a dedicated POST /api/v1/auth/sessions/
rotate-csrf admin endpoint would be defense-in-depth without
new attack-surface coverage. Documented in the audit-doc
annotation.

Verify gate:

* gofmt -l — clean
* go vet ./internal/api/handler/... — clean
* go build ./cmd/server/... ./internal/... — clean (production
  *session.Service satisfies the extended interface
  out of the box)
* go test -short -count=1 ./internal/api/handler/...
  ./internal/auth/session/... — all green; 3 new Logout
  cases + the 2 pre-existing Logout cases all pass.

Audit doc annotation at cowork/auth-bundles-audit-2026-05-10.md
flips the HIGH-2 row from 'CLOSED 2026-05-10 (3/4 call sites
wired)' to 'A-B-3 verified 2026-05-11: HIGH-2 fully closed
across all four documented call sites.'

Refs cowork/auth-bundles-fixes-2026-05-11/13-verify-logout-csrf-rotation.md.
This commit is contained in:
shankar0123
2026-05-11 12:24:41 +00:00
parent b8fac59200
commit 9a8130de32
3 changed files with 175 additions and 2 deletions
+25
View File
@@ -4,6 +4,31 @@
### Security
- **CSRF rotation on logout closes HIGH-2 fourth call site (Audit 2026-05-11 Fix 13).**
The HIGH-2 closure (`dev/auth-bundle-2`) documented four
`RotateCSRFTokenForActor` call sites: login completion (fresh by
construction), Assign/RevokeRole on role-mutation (wired), Logout, and
an explicit operator endpoint. The 2026-05-11 review verified only 3
of the 4 — Logout did NOT rotate the actor's sibling sessions
post-revoke, leaving a window where a token captured pre-logout
(browser DevTools, malicious extension, session-storage leak) could
be replayed against the user's other-device/other-browser sessions
until those sessions hit their own idle/absolute expiry.
`SessionMinter` interface extended with `RotateCSRFTokenForActor`;
`Logout` invokes it after `Revoke(sess.ID)` succeeds. The
`auth.session_revoked` audit row gains a `csrf_rotated` detail key
carrying the rotated count so SOC / SIEM can correlate logout events
with CSRF churn. The no-cookie + invalid-cookie 204 short-circuit
paths skip rotation (no session row to rotate against). 3 regression
tests in `internal/api/handler/auth_session_oidc_test.go` pin the
happy path + the two short-circuit branches. The explicit operator
endpoint (4) remains intentionally unbuilt — the three automatic
triggers (login + role-mutation + logout) cover the threat model;
operators who want a nuclear option can use the existing
`RevokeAllForActor` flow which forces re-login → fresh session →
fresh CSRF. **HIGH-2 fully closed across all four documented call
sites.**
- **Scope-aware actor-role revoke (Audit 2026-05-11 A-4).**
HIGH-10 made it possible to grant the same role to the same actor at
multiple scopes (e.g. `r-operator` on `profile=p-acme` AND `profile=p-globex`)
+32 -1
View File
@@ -68,11 +68,31 @@ type OIDCAuthHandshaker interface {
}
// SessionMinter is the slice of *session.Service the OIDC handler uses.
//
// Audit 2026-05-11 Fix 13 closure — adds RotateCSRFTokenForActor so the
// Logout handler can fire the HIGH-2 fourth call site. The HIGH-2 spec
// at cowork/auth-bundles-fixes-2026-05-10/06-high-1-2-revoke-and-rotate.md
// enumerated four CSRF-rotation triggers; three were wired (login mints
// fresh by construction, AssignRoleToKey + RevokeRoleFromKey rotate
// post-success), but Logout was missing. A token captured pre-logout
// (browser DevTools, malicious extension) was reusable on the actor's
// sibling sessions until those sessions hit their own idle/absolute
// expiry. Rotation on logout defeats this. Nil-safe: when the wired
// implementation isn't the production *session.Service (e.g. a future
// minimal-config deployment), the Logout handler skips the rotation
// instead of panic-ing.
type SessionMinter interface {
Create(ctx context.Context, actorID, actorType, ip, userAgent string) (*sessionsvc.CreateResult, error)
Validate(ctx context.Context, in sessionsvc.ValidateInput) (*sessiondomain.Session, error)
Revoke(ctx context.Context, sessionID string) error
RevokeAllForActor(ctx context.Context, actorID, actorType string) error
// RotateCSRFTokenForActor mints a fresh CSRF token across every
// active session for the (actorID, actorType) pair. Returns the
// count rotated. NEVER errors — rotation is defense-in-depth and
// must not block the surrounding mutation that triggered it.
// Matches the signature on *session.Service so the production
// wiring satisfies the interface without an adapter.
RotateCSRFTokenForActor(ctx context.Context, actorID, actorType string) int
}
// BackChannelLogoutVerifier validates an OpenID Connect Back-Channel
@@ -553,8 +573,19 @@ func (h *AuthSessionOIDCHandler) Logout(w http.ResponseWriter, r *http.Request)
Error(w, http.StatusInternalServerError, "could not revoke session")
return
}
// Audit 2026-05-11 Fix 13 — HIGH-2 fourth call site. Rotate the CSRF
// token on the actor's remaining sessions so a token captured in
// this device's browser pre-logout (DevTools, malicious extension,
// session-storage leak) can't be replayed against a sibling session
// (other browser, other device) after the user logged out here.
// The just-revoked session also rotates but its CSRF lookup will
// fail at the sessions table's revoked_at IS NOT NULL filter
// anyway; rotation on the revoked row is harmless. RotateCSRFTokenForActor
// returns the count rotated and NEVER errors — rotation is defense
// in depth and must not block the logout success.
rotated := h.sessionSvc.RotateCSRFTokenForActor(r.Context(), caller.ActorID, string(caller.ActorType))
h.recordAudit(r.Context(), "auth.session_revoked", caller.ActorID, caller.ActorType, sess.ID,
map[string]interface{}{"session_id": sess.ID, "self_initiated": true})
map[string]interface{}{"session_id": sess.ID, "self_initiated": true, "csrf_rotated": rotated})
h.clearSessionCookies(w)
w.WriteHeader(http.StatusNoContent)
}
+118 -1
View File
@@ -72,6 +72,12 @@ type stubSession struct {
revokedIDs []string
revokeAllIDs []string
revokeAllTypes []string
// Audit 2026-05-11 Fix 13 — record RotateCSRFTokenForActor calls so
// the Logout test can assert HIGH-2's fourth call site fires.
rotateCSRFCalls int
rotateCSRFActorIDs []string
rotateCSRFActorTypes []string
rotateCSRFReturnCount int
}
func (s *stubSession) Create(_ context.Context, _, _, _, _ string) (*sessionsvc.CreateResult, error) {
@@ -89,6 +95,12 @@ func (s *stubSession) RevokeAllForActor(_ context.Context, actorID, actorType st
s.revokeAllTypes = append(s.revokeAllTypes, actorType)
return s.revokeAllErr
}
func (s *stubSession) RotateCSRFTokenForActor(_ context.Context, actorID, actorType string) int {
s.rotateCSRFCalls++
s.rotateCSRFActorIDs = append(s.rotateCSRFActorIDs, actorID)
s.rotateCSRFActorTypes = append(s.rotateCSRFActorTypes, actorType)
return s.rotateCSRFReturnCount
}
type stubBCLVerifier struct {
issuer string
@@ -245,10 +257,17 @@ func (s *stubUserRepo) ListAll(_ context.Context, _ string) ([]*userdomain.User,
type phase5StubAudit struct {
events []string
// Audit 2026-05-11 Fix 13 — capture the details map so the
// TestLogout_RotatesCSRFForActor case can assert the rotated
// count carried by the auth.session_revoked row. Existing tests
// only consume `events`; details is append-aligned 1:1 with
// events for easy index-based correlation.
details []map[string]interface{}
}
func (s *phase5StubAudit) RecordEventWithCategory(_ context.Context, _ string, _ domain.ActorType, action, _, _, _ string, _ map[string]interface{}) error {
func (s *phase5StubAudit) RecordEventWithCategory(_ context.Context, _ string, _ domain.ActorType, action, _, _, _ string, details map[string]interface{}) error {
s.events = append(s.events, action)
s.details = append(s.details, details)
return nil
}
@@ -740,6 +759,104 @@ func TestLogout_NoCookie_Returns204(t *testing.T) {
}
}
// TestLogout_RotatesCSRFForActor pins the HIGH-2 fourth call site
// (Audit 2026-05-11 Fix 13). After Revoke succeeds, the handler must
// call RotateCSRFTokenForActor with the caller's (actorID, actorType)
// pair so a token captured pre-logout (browser DevTools, malicious
// extension) can't be replayed against a sibling session after the
// user logged out here. The audit row must record the rotated count
// so SOC / SIEM can correlate logout events with CSRF churn.
func TestLogout_RotatesCSRFForActor(t *testing.T) {
sess := &stubSession{
validateRes: &sessiondomain.Session{ID: "ses-abc", ActorID: "u-x", ActorType: "User"},
rotateCSRFReturnCount: 2, // caller has 2 active sessions before logout
}
h, _, _, _, audit, _ := newPhase5Handler(t, &stubOIDCSvc{}, sess, &stubBCLVerifier{})
req := httptest.NewRequest(http.MethodPost, "/auth/logout", nil)
req = withActor(req, "u-x", "User")
req.AddCookie(&http.Cookie{Name: sessiondomain.PostLoginCookieName, Value: "v1.ses-abc.sk-xyz.mac"})
w := httptest.NewRecorder()
h.Logout(w, req)
if w.Code != http.StatusNoContent {
t.Fatalf("status = %d; want 204", w.Code)
}
// Rotation MUST fire exactly once with the caller's (actor_id, actor_type).
if sess.rotateCSRFCalls != 1 {
t.Errorf("RotateCSRFTokenForActor call count = %d; want 1", sess.rotateCSRFCalls)
}
if len(sess.rotateCSRFActorIDs) != 1 || sess.rotateCSRFActorIDs[0] != "u-x" {
t.Errorf("rotateCSRF actor_ids = %v; want [u-x]", sess.rotateCSRFActorIDs)
}
if len(sess.rotateCSRFActorTypes) != 1 || sess.rotateCSRFActorTypes[0] != "User" {
t.Errorf("rotateCSRF actor_types = %v; want [User]", sess.rotateCSRFActorTypes)
}
// Revoke must still fire BEFORE rotation — pin the ordering by
// asserting the revokedIDs collection has been populated.
if len(sess.revokedIDs) != 1 || sess.revokedIDs[0] != "ses-abc" {
t.Errorf("expected Revoke(ses-abc) to fire; got revokedIDs=%v", sess.revokedIDs)
}
// Audit row carries the rotated count so SOC / SIEM can correlate
// logout events with CSRF churn on sibling sessions.
if !contains(audit.events, "auth.session_revoked") {
t.Fatalf("expected auth.session_revoked audit; got %v", audit.events)
}
last := audit.details[len(audit.details)-1]
if got, _ := last["csrf_rotated"].(int); got != 2 {
t.Errorf("audit details csrf_rotated = %v; want 2", last["csrf_rotated"])
}
}
// TestLogout_NoCookie_SkipsCSRFRotation pins the "no session →
// short-circuit" path. When the caller has no session cookie, Logout
// returns 204 immediately without touching Revoke OR the rotator —
// rotating CSRF for a caller who's already logged out (or never logged
// in) would do nothing useful and pollutes the audit log.
func TestLogout_NoCookie_SkipsCSRFRotation(t *testing.T) {
sess := &stubSession{}
h, _, _, _, _, _ := newPhase5Handler(t, &stubOIDCSvc{}, sess, &stubBCLVerifier{})
req := httptest.NewRequest(http.MethodPost, "/auth/logout", nil)
req = withActor(req, "u-x", "User")
w := httptest.NewRecorder()
h.Logout(w, req)
if w.Code != http.StatusNoContent {
t.Fatalf("status = %d; want 204", w.Code)
}
if sess.rotateCSRFCalls != 0 {
t.Errorf("RotateCSRFTokenForActor called %d times on the no-cookie path; want 0",
sess.rotateCSRFCalls)
}
}
// TestLogout_InvalidCookie_SkipsCSRFRotation pins the "invalid cookie
// → 204 + clear" path. Same rationale as the no-cookie test — there's
// no session row to rotate against, and the caller is already
// unauthenticated.
func TestLogout_InvalidCookie_SkipsCSRFRotation(t *testing.T) {
sess := &stubSession{validateErr: errors.New("invalid session")}
h, _, _, _, _, _ := newPhase5Handler(t, &stubOIDCSvc{}, sess, &stubBCLVerifier{})
req := httptest.NewRequest(http.MethodPost, "/auth/logout", nil)
req = withActor(req, "u-x", "User")
req.AddCookie(&http.Cookie{Name: sessiondomain.PostLoginCookieName, Value: "v1.junk.sk.mac"})
w := httptest.NewRecorder()
h.Logout(w, req)
if w.Code != http.StatusNoContent {
t.Fatalf("status = %d; want 204", w.Code)
}
if sess.rotateCSRFCalls != 0 {
t.Errorf("RotateCSRFTokenForActor called %d times on the invalid-cookie path; want 0",
sess.rotateCSRFCalls)
}
}
// =============================================================================
// 5. /api/v1/auth/sessions — list + revoke.
// =============================================================================