From e8b1a89ff69457b5e1aa1137786e4f0c03537d83 Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Tue, 17 Dec 2024 19:18:05 +0100 Subject: [PATCH] Various fixes in `fetchAuthEvents` (#3447) This might fix issues with state events gone missing. --------- Co-authored-by: Neil Alexander [skip ci] --- roomserver/internal/input/input_events.go | 61 +++++++++++++---------- roomserver/types/types.go | 1 + sytest-blacklist | 6 ++- sytest-whitelist | 2 - 4 files changed, 41 insertions(+), 29 deletions(-) diff --git a/roomserver/internal/input/input_events.go b/roomserver/internal/input/input_events.go index e1081afb6..65fd12c86 100644 --- a/roomserver/internal/input/input_events.go +++ b/roomserver/internal/input/input_events.go @@ -205,9 +205,27 @@ func (r *Inputer) processRoomEvent( } } + // Check that the auth events of the event are known. + // If they aren't then we will ask the federation API for them. + authEvents, _ := gomatrixserverlib.NewAuthEvents(nil) + knownEvents := map[string]*types.Event{} + if err = r.fetchAuthEvents(ctx, logger, roomInfo, virtualHost, headered, authEvents, knownEvents, serverRes.ServerNames); err != nil { + return fmt.Errorf("r.fetchAuthEvents: %w", err) + } + isRejected := false var rejectionErr error + // Check if the event is allowed by its auth events. If it isn't then + // we consider the event to be "rejected" — it will still be persisted. + if err = gomatrixserverlib.Allowed(event, authEvents, func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) { + return r.Queryer.QueryUserIDForSender(ctx, roomID, senderID) + }); err != nil { + isRejected = true + rejectionErr = err + logger.WithError(rejectionErr).Warnf("Event %s not allowed by auth events", event.EventID()) + } + // At this point we are checking whether we know all of the prev events, and // if we know the state before the prev events. This is necessary before we // try to do `calculateAndSetState` on the event later, otherwise it will fail @@ -283,24 +301,6 @@ func (r *Inputer) processRoomEvent( } } - // Check that the auth events of the event are known. - // If they aren't then we will ask the federation API for them. - authEvents, _ := gomatrixserverlib.NewAuthEvents(nil) - knownEvents := map[string]*types.Event{} - if err = r.fetchAuthEvents(ctx, logger, roomInfo, virtualHost, headered, authEvents, knownEvents, serverRes.ServerNames); err != nil { - return fmt.Errorf("r.fetchAuthEvents: %w", err) - } - - // Check if the event is allowed by its auth events. If it isn't then - // we consider the event to be "rejected" — it will still be persisted. - if err = gomatrixserverlib.Allowed(event, authEvents, func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) { - return r.Queryer.QueryUserIDForSender(ctx, roomID, senderID) - }); err != nil { - isRejected = true - rejectionErr = err - logger.WithError(rejectionErr).Warnf("Event %s not allowed by auth events", event.EventID()) - } - // Accumulate the auth event NIDs. authEventIDs := event.AuthEventIDs() authEventNIDs := make([]types.EventNID, 0, len(authEventIDs)) @@ -323,7 +323,7 @@ func (r *Inputer) processRoomEvent( ) } } - } else { + } else if !knownEvents[authEventID].Rejected { authEventNIDs = append(authEventNIDs, knownEvents[authEventID].EventNID) } } @@ -704,15 +704,14 @@ func (r *Inputer) fetchAuthEvents( } ev := authEvents[0] - isRejected := false if roomInfo != nil { - isRejected, err = r.DB.IsEventRejected(ctx, roomInfo.RoomNID, ev.EventID()) + ev.Rejected, err = r.DB.IsEventRejected(ctx, roomInfo.RoomNID, ev.EventID()) if err != nil && !errors.Is(err, sql.ErrNoRows) { return fmt.Errorf("r.DB.IsEventRejected failed: %w", err) } } known[authEventID] = &ev // don't take the pointer of the iterated event - if !isRejected { + if !ev.Rejected { if err = auth.AddEvent(ev.PDU); err != nil { return fmt.Errorf("auth.AddEvent: %w", err) } @@ -744,8 +743,13 @@ func (r *Inputer) fetchAuthEvents( return fmt.Errorf("no servers provided event auth for event ID %q, tried servers %v", event.EventID(), servers) } + // Start with a clean state and see if we can auth with what the remote + // server told us. Otherwise earlier topologically sorted events could + // fail to be authed by more recent referenced ones. + auth.Clear() + // Reuse these to reduce allocations. - authEventNIDs := make([]types.EventNID, 0, 5) + _authEventNIDs := [5]types.EventNID{} isRejected := false nextAuthEvent: for _, authEvent := range gomatrixserverlib.ReverseTopologicalOrdering( @@ -755,7 +759,11 @@ nextAuthEvent: // If we already know about this event from the database then we don't // need to store it again or do anything further with it, so just skip // over it rather than wasting cycles. - if ev, ok := known[authEvent.EventID()]; ok && ev != nil { + if ev, ok := known[authEvent.EventID()]; ok && ev != nil && !ev.Rejected { + // Need to add to the auth set for the next event being processed. + if err := auth.AddEvent(authEvent); err != nil { + return fmt.Errorf("auth.AddEvent: %w", err) + } continue nextAuthEvent } @@ -770,11 +778,11 @@ nextAuthEvent: // In order to store the new auth event, we need to know its auth chain // as NIDs for the `auth_event_nids` column. Let's see if we can find those. - authEventNIDs = authEventNIDs[:0] + authEventNIDs := _authEventNIDs[:0] for _, eventID := range authEvent.AuthEventIDs() { knownEvent, ok := known[eventID] if !ok { - continue nextAuthEvent + return fmt.Errorf("auth event ID %s not known but should be", eventID) } authEventNIDs = append(authEventNIDs, knownEvent.EventNID) } @@ -821,6 +829,7 @@ nextAuthEvent: // Now we know about this event, it was stored and the signatures were OK. known[authEvent.EventID()] = &types.Event{ EventNID: eventNID, + Rejected: isRejected, PDU: authEvent, } } diff --git a/roomserver/types/types.go b/roomserver/types/types.go index 759066bc0..b0c393c3f 100644 --- a/roomserver/types/types.go +++ b/roomserver/types/types.go @@ -228,6 +228,7 @@ func (s StateAtEventAndReferences) EventIDs() string { // It is when performing bulk event lookup in the database. type Event struct { EventNID EventNID + Rejected bool gomatrixserverlib.PDU } diff --git a/sytest-blacklist b/sytest-blacklist index d6fadc7e1..f4c08545c 100644 --- a/sytest-blacklist +++ b/sytest-blacklist @@ -17,4 +17,8 @@ If a device list update goes missing, the server resyncs on the next one Leaves are present in non-gapped incremental syncs # We don't have any state to calculate m.room.guest_access when accepting invites -Guest users can accept invites to private rooms over federation \ No newline at end of file +Guest users can accept invites to private rooms over federation + +# Tests Synapse specific behavior +/state returns M_NOT_FOUND for an outlier +/state_ids returns M_NOT_FOUND for an outlier \ No newline at end of file diff --git a/sytest-whitelist b/sytest-whitelist index 35d700d0a..6f38c4ec1 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -774,8 +774,6 @@ Remote user can backfill in a room with version 10 Can reject invites over federation for rooms with version 10 Can receive redactions from regular users over federation in room version 10 New federated private chats get full presence information (SYN-115) -/state returns M_NOT_FOUND for an outlier -/state_ids returns M_NOT_FOUND for an outlier Outbound federation requests missing prev_events and then asks for /state_ids and resolves the state Invited user can reject invite for empty room Invited user can reject local invite after originator leaves