From 5cbd93cbc30830881a4b5f2cf495ff0c84d49f92 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Fri, 29 Nov 2024 21:33:55 +0000 Subject: [PATCH 1/3] Various fixes in `fetchAuthEvents` --- roomserver/internal/input/input_events.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/roomserver/internal/input/input_events.go b/roomserver/internal/input/input_events.go index b84db345..cc1c09b5 100644 --- a/roomserver/internal/input/input_events.go +++ b/roomserver/internal/input/input_events.go @@ -738,6 +738,11 @@ 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) isRejected := false @@ -749,7 +754,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 } @@ -768,7 +777,7 @@ nextAuthEvent: 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) } From 09e96f80e329b5b3a4105ecc8ca6360a75bea77b Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 10 Apr 2024 21:59:01 +0100 Subject: [PATCH 2/3] Track rejections on auth event NIDs --- roomserver/internal/input/input_events.go | 48 +++++++++++------------ roomserver/types/types.go | 1 + 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/roomserver/internal/input/input_events.go b/roomserver/internal/input/input_events.go index cc1c09b5..88420bbf 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) } } @@ -698,15 +698,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,7 +743,7 @@ func (r *Inputer) fetchAuthEvents( 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( @@ -773,7 +772,7 @@ 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 { @@ -824,6 +823,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 759066bc..b0c393c3 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 } From 7edceb45f388028e0aa9a3674c29f6b264a00210 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 17 Dec 2024 13:45:59 +0100 Subject: [PATCH 3/3] Move tests to blacklist --- sytest-blacklist | 6 +++++- sytest-whitelist | 2 -- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/sytest-blacklist b/sytest-blacklist index d6fadc7e..f4c08545 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 35d700d0..6f38c4ec 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