bugfix: ensure metadata about space children doesn't leak to active c…

…onnections

If Alice and Bob are in the same space, and Bob creates a child in that space,
Alice would incorrectly receive global metadata about that child room if Alice
was live syncing at that time. This leak did not expose confidential information
as Alice could receive all that metadata via the /rooms/{roomId}/hierarchy endpoint
already. However, it would cause clients to put the space child room into the room
list which would be very confusing, as it would have no timeline and no other data.

sync3/caches/user.go

@@ -26,6 +26,10 @@ type TransactionIDFetcher interface {
 	TransactionIDForEvents(userID, deviceID string, eventIDs []string) (eventIDToTxnID map[string]string)
 }
 
+type JoinChecker interface {
+	IsUserJoined(userID, roomID string) bool
+}
+
 // UserRoomData describes a single room from the perspective of particular user.
 // It is primarily used in two places:
 //   - in the caches.UserCache, to hold the latest version of user-specific data; and
@@ -194,11 +198,12 @@ type UserCache struct {
 	store                     UserCacheStore
 	globalCache               *GlobalCache
 	txnIDs                    TransactionIDFetcher
+	joinChecker               JoinChecker
 	ignoredUsers              map[string]struct{}
 	ignoredUsersMu            *sync.RWMutex
 }
 
-func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore, txnIDs TransactionIDFetcher) *UserCache {
+func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore, txnIDs TransactionIDFetcher, joinChecker JoinChecker) *UserCache {
 	// see SyncLiveHandler.userCache for the initialisation proper, which works by
 	// firing off a bunch of OnBlahBlah callbacks.
 	uc := &UserCache{
@@ -210,6 +215,7 @@ func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore,
 		store:          store,
 		globalCache:    globalCache,
 		txnIDs:         txnIDs,
+		joinChecker:    joinChecker,
 		ignoredUsers:   make(map[string]struct{}),
 		ignoredUsersMu: &sync.RWMutex{},
 	}
@@ -572,12 +578,14 @@ func (c *UserCache) OnSpaceUpdate(ctx context.Context, parentRoomID, childRoomID
 	c.roomToDataMu.Unlock()
 
 	// now we need to notify connections for the _child_
-	roomUpdate := &RoomEventUpdate{
-		RoomUpdate: c.newRoomUpdate(ctx, childRoomID),
-		EventData:  eventData,
+	// but only if they are allowed to see the child event (i.e they are joined to the child room)
+	if c.joinChecker.IsUserJoined(c.UserID, childRoomID) {
+		roomUpdate := &RoomEventUpdate{
+			RoomUpdate: c.newRoomUpdate(ctx, childRoomID),
+			EventData:  eventData,
+		}
+		c.emitOnRoomUpdate(ctx, roomUpdate)
 	}
-
-	c.emitOnRoomUpdate(ctx, roomUpdate)
 }
 
 func (c *UserCache) OnNewEvent(ctx context.Context, eventData *EventData) {

sync3/caches/user_test.go

@@ -10,6 +10,12 @@ import (
 	"github.com/matrix-org/sliding-sync/sync3/caches"
 )
 
+type joinChecker struct{}
+
+func (c *joinChecker) IsUserJoined(userID, roomID string) bool {
+	return true
+}
+
 type txnIDFetcher struct {
 	data map[string]string
 }
@@ -82,7 +88,7 @@ func TestAnnotateWithTransactionIDs(t *testing.T) {
 		fetcher := &txnIDFetcher{
 			data: tc.eventIDToTxnIDs,
 		}
-		uc := caches.NewUserCache(userID, nil, nil, fetcher)
+		uc := caches.NewUserCache(userID, nil, nil, fetcher, &joinChecker{})
 		got := uc.AnnotateWithTransactionIDs(context.Background(), userID, "DEVICE", convertIDToEventStub(userID, tc.roomIDToEvents))
 		want := convertIDTxnToEventStub(userID, tc.wantRoomIDToEvents)
 		if !reflect.DeepEqual(got, want) {

sync3/handler/connstate_test.go

@@ -18,6 +18,12 @@ import (
 	"github.com/matrix-org/sliding-sync/testutils"
 )
 
+type joinChecker struct{}
+
+func (c *joinChecker) IsUserJoined(userID, roomID string) bool {
+	return true
+}
+
 type NopExtensionHandler struct{}
 
 func (h *NopExtensionHandler) Handle(ctx context.Context, req extensions.Request, extCtx extensions.Context) (res extensions.Response) {
@@ -106,7 +112,7 @@ func TestConnStateInitial(t *testing.T) {
 				roomC.RoomID: {NID: 780, Timestamp: 789},
 			}, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
 	userCache.LazyLoadTimelinesOverride = func(loadPos int64, roomIDs []string, maxTimelineEvents int) map[string]state.LatestEvents {
@@ -279,7 +285,7 @@ func TestConnStateMultipleRanges(t *testing.T) {
 		}
 		return 1, roomMetadata, joinTimings, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = mockLazyRoomOverride
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
@@ -458,7 +464,7 @@ func TestBumpToOutsideRange(t *testing.T) {
 			}, nil, nil
 
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = mockLazyRoomOverride
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
@@ -561,7 +567,7 @@ func TestConnStateRoomSubscriptions(t *testing.T) {
 				roomD.RoomID: {NID: 4, Timestamp: 4},
 			}, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = func(loadPos int64, roomIDs []string, maxTimelineEvents int) map[string]state.LatestEvents {
 		result := make(map[string]state.LatestEvents)
 		for _, roomID := range roomIDs {

sync3/handler/handler.go

@@ -528,7 +528,7 @@ func (h *SyncLiveHandler) userCache(userID string) (*caches.UserCache, error) {
 	if ok {
 		return c.(*caches.UserCache), nil
 	}
-	uc := caches.NewUserCache(userID, h.GlobalCache, h.Storage, h)
+	uc := caches.NewUserCache(userID, h.GlobalCache, h.Storage, h, h.Dispatcher)
 	// select all non-zero highlight or notif counts and set them, as this is less costly than looping every room/user pair
 	err := h.Storage.UnreadTable.SelectAllNonZeroCountsForUser(userID, func(roomID string, highlightCount, notificationCount int) {
 		uc.OnUnreadCounts(context.Background(), roomID, &highlightCount, &notificationCount)

tests-e2e/security_test.go

@@ -324,3 +324,88 @@ func TestSecuritySpaceMetadataLeak(t *testing.T) {
 	})
 	m.MatchResponse(t, res, m.MatchNoV3Ops(), m.MatchRoomSubscriptionsStrict(nil))
 }
+
+// Test that adding a child room to a space does not leak global room metadata about that
+// child room to users in the parent space. This information isn't strictly confidential as
+// the /rooms/{roomId}/hierarchy endpoint will include such metadata (room name, avatar, join count, etc)
+// because the user is part of the parent space. There isn't an attack vector here, but repro steps:
+//   - Alice and Bob are in a parent space.
+//   - Bob has a poller on SS running.
+//   - Alice is live streaming from SS.
+//   - Bob creates a child room in that space, and sends both the m.space.parent in the child room AND
+//     the m.space.child in the parent space.
+//   - Ensure that no information about the child room comes down Alice's connection.
+func TestSecuritySpaceChildMetadataLeakFromParent(t *testing.T) {
+	alice := registerNewUser(t)
+	bob := registerNewUser(t)
+	parentName := "The Parent Room Name"
+	childName := "The Child Room Name"
+
+	// Alice and Bob are in a parent space.
+	parentSpace := bob.MustCreateRoom(t, map[string]interface{}{
+		"preset": "public_chat",
+		"name":   parentName,
+		"creation_content": map[string]string{
+			"type": "m.space",
+		},
+	})
+	alice.MustJoinRoom(t, parentSpace, []string{"hs1"})
+
+	// Bob has a poller on SS running.
+	bobRes := bob.SlidingSync(t, sync3.Request{})
+
+	// Alice is live streaming from SS.
+	aliceRes := alice.SlidingSync(t, sync3.Request{
+		Lists: map[string]sync3.RequestList{
+			"a": {
+				Ranges: [][2]int64{{0, 20}},
+			},
+		},
+	})
+	m.MatchResponse(t, aliceRes, m.MatchRoomSubscriptionsStrict(map[string][]m.RoomMatcher{
+		parentSpace: {
+			m.MatchJoinCount(2),
+			m.MatchRoomName(parentName),
+		},
+	}))
+
+	// Bob creates a child room in that space, and sends both the m.space.parent in the child room AND
+	// the m.space.child in the parent space.
+	childRoom := bob.MustCreateRoom(t, map[string]interface{}{
+		"preset": "public_chat",
+		"name":   childName,
+		"initial_state": []map[string]interface{}{
+			{
+				"type":      "m.space.parent",
+				"state_key": parentSpace,
+				"content": map[string]interface{}{
+					"canonical": true,
+					"via":       []string{"hs1"},
+				},
+			},
+		},
+	})
+	chlidEventID := bob.SendEventSynced(t, parentSpace, b.Event{
+		Type:     "m.space.child",
+		StateKey: ptr(childRoom),
+		Content: map[string]interface{}{
+			"via": []string{"hs1"},
+		},
+	})
+	// wait for SS to process it
+	bob.SlidingSyncUntilEventID(t, bobRes.Pos, parentSpace, chlidEventID)
+
+	// Ensure that no information about the child room comes down Alice's connection.
+	aliceRes = alice.SlidingSync(t, sync3.Request{}, WithPos(aliceRes.Pos))
+	m.MatchResponse(t, aliceRes, m.MatchRoomSubscriptionsStrict(map[string][]m.RoomMatcher{
+		parentSpace: {
+			MatchRoomTimeline([]Event{{
+				Type:     "m.space.child",
+				StateKey: ptr(childRoom),
+				Content: map[string]interface{}{
+					"via": []interface{}{"hs1"},
+				},
+			}}),
+		},
+	}))
+}