appview: stop a UI session outliving the OAuth session behind it

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
ui_sessions row pointing at an oauth_session_id that no longer exists. Get
checked only expiry, so those still read back as usable.

Found on a live appview: four ui_sessions rows, three orphaned, and requesting
/settings/user with an orphaned cookie returned 200 with the account's handle
rendered throughout, where an anonymous request gets a 302. The browser looks
signed in while the credential behind it is gone, so every PDS-backed action
fails against a UI insisting the session is fine. It now fails closed and sends
the user back through login.

Get also never checked ownership. oauth_sessions is unique on
(account_did, session_id), so the existence check is scoped by both; matching
session_id alone would let one account's live OAuth session validate another
account's dangling reference. That has its own test.

An empty oauth_session_id stays valid, since Create makes sessions that never
had one, and a test pins that so the check cannot start rejecting them.

TestSessionStore_CreateWithOAuth referenced an OAuth session it never inserted,
which is an orphan by definition, so it now creates the row. Its intent was
that CreateWithOAuth persists the ID; it relied on the orphan behaviour only
incidentally. Its not-found branch used t.Error and then dereferenced the nil
session, so that is now t.Fatal.

Pre-existing at efabb677 rather than introduced by this range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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")
+	}
+}

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
 }
 

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)