Add some checks that we aren't using state from rejected events (#6330)

* Raise an exception if accessing state for rejected events

Add some sanity checks on accessing state_group etc for
rejected events.

* Skip calculating push actions for rejected events

It didn't actually cause any bugs, because rejected events get filtered out at
various later points, but there's not point in trying to calculate the push
actions for a rejected event.
This commit is contained in:
Richard van der Hoff 2019-11-05 22:13:37 +00:00 committed by GitHub
parent 01ba7b38a7
commit 0e3ab8afdc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 50 additions and 6 deletions

1
changelog.d/6330.misc Normal file
View File

@ -0,0 +1 @@
Add some checks that we aren't using state from rejected events.

View File

@ -33,7 +33,7 @@ class EventContext:
Attributes: Attributes:
rejected: A rejection reason if the event was rejected, else False rejected: A rejection reason if the event was rejected, else False
state_group: The ID of the state group for this event. Note that state events _state_group: The ID of the state group for this event. Note that state events
are persisted with a state group which includes the new event, so this is are persisted with a state group which includes the new event, so this is
effectively the state *after* the event in question. effectively the state *after* the event in question.
@ -45,6 +45,9 @@ class EventContext:
For an outlier, where we don't have the state at the event, this will be For an outlier, where we don't have the state at the event, this will be
None. None.
Note that this is a private attribute: it should be accessed via
the ``state_group`` property.
prev_group: If it is known, ``state_group``'s prev_group. Note that this being prev_group: If it is known, ``state_group``'s prev_group. Note that this being
None does not necessarily mean that ``state_group`` does not have None does not necessarily mean that ``state_group`` does not have
a prev_group! a prev_group!
@ -88,7 +91,7 @@ class EventContext:
""" """
rejected = attr.ib(default=False, type=Union[bool, str]) rejected = attr.ib(default=False, type=Union[bool, str])
state_group = attr.ib(default=None, type=Optional[int]) _state_group = attr.ib(default=None, type=Optional[int])
prev_group = attr.ib(default=None, type=Optional[int]) prev_group = attr.ib(default=None, type=Optional[int])
delta_ids = attr.ib(default=None, type=Optional[Dict[Tuple[str, str], str]]) delta_ids = attr.ib(default=None, type=Optional[Dict[Tuple[str, str], str]])
app_service = attr.ib(default=None, type=Optional[ApplicationService]) app_service = attr.ib(default=None, type=Optional[ApplicationService])
@ -136,7 +139,7 @@ class EventContext:
"prev_state_id": prev_state_id, "prev_state_id": prev_state_id,
"event_type": event.type, "event_type": event.type,
"event_state_key": event.state_key if event.is_state() else None, "event_state_key": event.state_key if event.is_state() else None,
"state_group": self.state_group, "state_group": self._state_group,
"rejected": self.rejected, "rejected": self.rejected,
"prev_group": self.prev_group, "prev_group": self.prev_group,
"delta_ids": _encode_state_dict(self.delta_ids), "delta_ids": _encode_state_dict(self.delta_ids),
@ -173,22 +176,52 @@ class EventContext:
return context return context
@property
def state_group(self) -> Optional[int]:
"""The ID of the state group for this event.
Note that state events are persisted with a state group which includes the new
event, so this is effectively the state *after* the event in question.
For an outlier, where we don't have the state at the event, this will be None.
It is an error to access this for a rejected event, since rejected state should
not make it into the room state. Accessing this property will raise an exception
if ``rejected`` is set.
"""
if self.rejected:
raise RuntimeError("Attempt to access state_group of rejected event")
return self._state_group
@defer.inlineCallbacks @defer.inlineCallbacks
def get_current_state_ids(self, store): def get_current_state_ids(self, store):
"""Gets the current state IDs """
Gets the room state map, including this event - ie, the state in ``state_group``
It is an error to access this for a rejected event, since rejected state should
not make it into the room state. This method will raise an exception if
``rejected`` is set.
Returns: Returns:
Deferred[dict[(str, str), str]|None]: Returns None if state_group Deferred[dict[(str, str), str]|None]: Returns None if state_group
is None, which happens when the associated event is an outlier. is None, which happens when the associated event is an outlier.
Maps a (type, state_key) to the event ID of the state event matching Maps a (type, state_key) to the event ID of the state event matching
this tuple. this tuple.
""" """
if self.rejected:
raise RuntimeError("Attempt to access state_ids of rejected event")
yield self._ensure_fetched(store) yield self._ensure_fetched(store)
return self._current_state_ids return self._current_state_ids
@defer.inlineCallbacks @defer.inlineCallbacks
def get_prev_state_ids(self, store): def get_prev_state_ids(self, store):
"""Gets the prev state IDs """
Gets the room state map, excluding this event.
For a non-state event, this will be the same as get_current_state_ids().
Returns: Returns:
Deferred[dict[(str, str), str]|None]: Returns None if state_group Deferred[dict[(str, str), str]|None]: Returns None if state_group
@ -202,11 +235,17 @@ class EventContext:
def get_cached_current_state_ids(self): def get_cached_current_state_ids(self):
"""Gets the current state IDs if we have them already cached. """Gets the current state IDs if we have them already cached.
It is an error to access this for a rejected event, since rejected state should
not make it into the room state. This method will raise an exception if
``rejected`` is set.
Returns: Returns:
dict[(str, str), str]|None: Returns None if we haven't cached the dict[(str, str), str]|None: Returns None if we haven't cached the
state or if state_group is None, which happens when the associated state or if state_group is None, which happens when the associated
event is an outlier. event is an outlier.
""" """
if self.rejected:
raise RuntimeError("Attempt to access state_ids of rejected event")
return self._current_state_ids return self._current_state_ids

View File

@ -1688,7 +1688,11 @@ class FederationHandler(BaseHandler):
# hack around with a try/finally instead. # hack around with a try/finally instead.
success = False success = False
try: try:
if not event.internal_metadata.is_outlier() and not backfilled: if (
not event.internal_metadata.is_outlier()
and not backfilled
and not context.rejected
):
yield self.action_generator.handle_push_actions_for_event( yield self.action_generator.handle_push_actions_for_event(
event, context event, context
) )