diff --git a/pkg/appview/db/session_orphan_test.go b/pkg/appview/db/session_orphan_test.go new file mode 100644 index 0000000..0d783b7 --- /dev/null +++ b/pkg/appview/db/session_orphan_test.go @@ -0,0 +1,104 @@ +package db + +import ( + "testing" + "time" +) + +// A ui_sessions row outlives the OAuth session it points at. Only one +// oauth_sessions row is kept per account, so signing in again — on a second +// device, or just a second time — replaces it and leaves every earlier +// ui_sessions row referencing an oauth_session_id that no longer exists. +// +// Observed on a live appview: four ui_sessions rows, three of them orphaned, +// and requesting /settings/user with an orphaned cookie returned 200 with the +// account's handle rendered throughout, rather than the 302 an anonymous +// request gets. The browser looks signed in while the credential behind it is +// gone, so anything needing the PDS token fails against a UI insisting the +// session is fine. + +func insertOAuthSession(t *testing.T, s *SessionStore, did, sessionID string) { + t.Helper() + _, err := s.db.Exec(` + INSERT INTO oauth_sessions (session_key, account_did, session_id, session_data) + VALUES (?, ?, ?, ?) + `, did+":"+sessionID, did, sessionID, `{"account_did":"`+did+`"}`) + if err != nil { + t.Fatalf("insert oauth session: %v", err) + } +} + +// The bug: an orphaned reference must not read back as a usable session. +func TestSessionStore_Get_RejectsOrphanedOAuthSession(t *testing.T) { + store := setupSessionTestDB(t) + const did = "did:plc:orphan" + createSessionTestUser(t, store, did, "orphan.test") + + id, err := store.CreateWithOAuth(did, "orphan.test", "https://pds.example.com", + "oauth-session-that-is-gone", time.Hour) + if err != nil { + t.Fatalf("create session: %v", err) + } + + if _, ok := store.Get(id); ok { + t.Fatal("Get returned a session whose OAuth session no longer exists; " + + "the browser would render as signed in with no usable credential") + } +} + +// A session created without an OAuth session at all is legitimate — Create +// passes an empty oauthSessionID — and must keep working. +func TestSessionStore_Get_AcceptsSessionWithoutOAuthSession(t *testing.T) { + store := setupSessionTestDB(t) + const did = "did:plc:nooauth" + createSessionTestUser(t, store, did, "nooauth.test") + + id, err := store.Create(did, "nooauth.test", "https://pds.example.com", time.Hour) + if err != nil { + t.Fatalf("create session: %v", err) + } + + if _, ok := store.Get(id); !ok { + t.Fatal("Get rejected a session that never had an OAuth session; " + + "the orphan check must not catch these") + } +} + +// The ordinary case stays working. +func TestSessionStore_Get_AcceptsLiveOAuthSession(t *testing.T) { + store := setupSessionTestDB(t) + const did = "did:plc:live" + createSessionTestUser(t, store, did, "live.test") + insertOAuthSession(t, store, did, "live-oauth-session") + + id, err := store.CreateWithOAuth(did, "live.test", "https://pds.example.com", + "live-oauth-session", time.Hour) + if err != nil { + t.Fatalf("create session: %v", err) + } + + if _, ok := store.Get(id); !ok { + t.Fatal("Get rejected a session backed by a live OAuth session") + } +} + +// The reference is only meaningful for the account that owns it: oauth_sessions +// is unique on (account_did, session_id), so matching on session_id alone would +// let one account's live session validate another's dangling reference. +func TestSessionStore_Get_RejectsOAuthSessionBelongingToAnotherAccount(t *testing.T) { + store := setupSessionTestDB(t) + const owner, other = "did:plc:owner", "did:plc:other" + createSessionTestUser(t, store, owner, "owner.test") + createSessionTestUser(t, store, other, "other.test") + insertOAuthSession(t, store, other, "shared-session-id") + + id, err := store.CreateWithOAuth(owner, "owner.test", "https://pds.example.com", + "shared-session-id", time.Hour) + if err != nil { + t.Fatalf("create session: %v", err) + } + + if _, ok := store.Get(id); ok { + t.Fatal("Get accepted a session validated by another account's OAuth session") + } +} diff --git a/pkg/appview/db/session_store.go b/pkg/appview/db/session_store.go index 67c19c6..61eeba4 100644 --- a/pkg/appview/db/session_store.go +++ b/pkg/appview/db/session_store.go @@ -70,15 +70,38 @@ func (s *SessionStore) CreateWithOAuth(did, handle, pdsEndpoint, oauthSessionID return sessionID, nil } -// Get retrieves a session by ID +// Get retrieves a session by ID. +// +// A ui_sessions row outlives the OAuth session it references. Only one +// oauth_sessions row is kept per account, so signing in again — on a second +// device, or simply a second time — replaces it and leaves every earlier row +// pointing at an oauth_session_id that no longer exists. Those must not read +// back as usable: the browser would render fully signed in while the credential +// behind it is gone, so every PDS-backed action fails against a UI insisting +// the session is fine. Failing closed here sends them back through login. +// +// An empty oauth_session_id is legitimate and stays usable — Create makes +// sessions that never had one. +// +// The lookup is scoped by account_did as well as session_id because +// oauth_sessions is unique on that pair; matching session_id alone would let +// one account's live OAuth session validate another account's dangling +// reference. func (s *SessionStore) Get(id string) (*Session, bool) { var sess Session + var oauthLive bool err := s.db.QueryRow(` - SELECT id, did, handle, pds_endpoint, oauth_session_id, expires_at - FROM ui_sessions - WHERE id = ? - `, id).Scan(&sess.ID, &sess.DID, &sess.Handle, &sess.PDSEndpoint, &sess.OAuthSessionID, &sess.ExpiresAt) + SELECT u.id, u.did, u.handle, u.pds_endpoint, u.oauth_session_id, u.expires_at, + (u.oauth_session_id IS NULL OR u.oauth_session_id = '' + OR EXISTS ( + SELECT 1 FROM oauth_sessions o + WHERE o.account_did = u.did AND o.session_id = u.oauth_session_id + )) AS oauth_live + FROM ui_sessions u + WHERE u.id = ? + `, id).Scan(&sess.ID, &sess.DID, &sess.Handle, &sess.PDSEndpoint, &sess.OAuthSessionID, + &sess.ExpiresAt, &oauthLive) if err == sql.ErrNoRows { return nil, false @@ -93,6 +116,13 @@ func (s *SessionStore) Get(id string) (*Session, bool) { return nil, false } + if !oauthLive { + slog.Info("Rejecting UI session whose OAuth session is gone", + "did", sess.DID, + "oauthSessionID", sess.OAuthSessionID) + return nil, false + } + return &sess, true } diff --git a/pkg/appview/db/session_store_test.go b/pkg/appview/db/session_store_test.go index 2a4ebc7..17d741f 100644 --- a/pkg/appview/db/session_store_test.go +++ b/pkg/appview/db/session_store_test.go @@ -93,6 +93,11 @@ func TestSessionStore_CreateWithOAuth(t *testing.T) { createSessionTestUser(t, store, "did:plc:alice123", "alice.bsky.social") oauthSessionID := "oauth-123" + // Get requires the referenced OAuth session to still exist, so give this + // one a real row. Without it the session is an orphan by definition, which + // is the state Get now refuses (see session_orphan_test.go). + insertOAuthSession(t, store, "did:plc:alice123", oauthSessionID) + sessionID, err := store.CreateWithOAuth("did:plc:alice123", "alice.bsky.social", "https://pds.example.com", oauthSessionID, 1*time.Hour) if err != nil { t.Fatalf("CreateWithOAuth() error = %v", err) @@ -102,10 +107,11 @@ func TestSessionStore_CreateWithOAuth(t *testing.T) { t.Error("CreateWithOAuth() returned empty session ID") } - // Verify session has OAuth session ID + // Verify session has OAuth session ID. Fatal, not Error: continuing past a + // missing session dereferences a nil pointer. sess, found := store.Get(sessionID) if !found { - t.Error("Created session not found") + t.Fatal("Created session not found") } if sess.OAuthSessionID != oauthSessionID { t.Errorf("OAuthSessionID = %v, want %v", sess.OAuthSessionID, oauthSessionID)