Merge Fix 13 (HIGH-2 fourth call site): CSRF rotation on Logout

# Conflicts: # CHANGELOG.md
2026-06-07 12:31:29 +00:00 · 2026-05-11 13:01:56 +00:00
parent 9f617add29 9a8130de32
commit a4b2919f59
3 changed files with 175 additions and 2 deletions
@@ -35,6 +35,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.**
+
 - **Demo-mode residual-grants detector + cleanup endpoint + CI guard (Audit 2026-05-11 A-8).**
  HIGH-12 (closure `b81588e`) added a fail-closed bind-address guard
  that refuses startup when `CERTCTL_AUTH_TYPE=none` binds non-loopback
@@ -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)
 }
@@ -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.
 // =============================================================================