diff --git a/CHANGELOG.md b/CHANGELOG.md index d16cc56..bb25c73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/internal/api/handler/auth_session_oidc.go b/internal/api/handler/auth_session_oidc.go index c1b9c78..f865279 100644 --- a/internal/api/handler/auth_session_oidc.go +++ b/internal/api/handler/auth_session_oidc.go @@ -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) } diff --git a/internal/api/handler/auth_session_oidc_test.go b/internal/api/handler/auth_session_oidc_test.go index 975d558..e20259e 100644 --- a/internal/api/handler/auth_session_oidc_test.go +++ b/internal/api/handler/auth_session_oidc_test.go @@ -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. // =============================================================================