Mitigate a race where /make_join could 403 for restricted rooms

[squahtx]

Previously, when creating a join event in /make_join, we would decide
whether to include additional fields to satisfy restricted room checks
based on the current state of the room. Then, when building the event,
we would capture the forward extremities of the room to use as prev
events.

This is subject to race conditions. For example, when leaving and
rejoining a room, the following sequence of events leads to a misleading
403 response:

1. /make_join reads the current state of the room and sees that the user
   is still in the room. It decides to omit the field required for
   restricted room joins.
2. The leave event is persisted and the room's forward extremities are
   updated.
3. /make_join builds the event, using the post-leave forward extremities.
   The event then fails the restricted room checks.

To mitigate the race, we move the read of the forward extremities closer
to the read of the current state. Ideally, we would compute the state
based off the chosen prev events, but that can involve state resolution,
which is expensive.

Signed-off-by: Sean Quah [email protected]

---

This race occurs in the flakes seen in #14986.
matrix-org/complement#614 fixes the flakes by waiting for the remote homeserver to process the leave event before rejoining.

[squahtx]

I wanted to calculate the state off the prev events, but that looked like it would involve state resolution which is slow.

[DMRobertson]

1. /make_join reads the current state of the room and sees that the user
   is still in the room. It decides to omit the field required for
   restricted room joins.

Is there any harm in always including the field required for restricted room joins? (i.e. would that help mitigate the race too?)

[DMRobertson]

I think this is a good idea, though I would interested to know what you think of #15080 (comment).

[DMRobertson]

Pedantry: there can be at most 20 prev events in a given event, so in the worst case the prev events are a subset of the forward extremities of the room. I don't think we need to say that here (but I couldn't resist pointing it out now).

[squahtx]

That's annoying, since the prev events we feed in won't always be representative of the room state the event is evaluated against.

Though I note that right now we hit an assert if there are more than 10 prev events.

synapse/synapse/handlers/message.py

Lines 1164 to 1169 in 5febf88

	if prev_event_ids is not None:
	assert (
	len(prev_event_ids) <= 10
	), "Attempting to create an event with %i prev_events" % (
	len(prev_event_ids),
	)

[DMRobertson]

Good to know. FWIW The spec says

Must contain less than or equal to 20 events.

[squahtx]

That's annoying, since the prev events we feed in won't always be representative of the room state the event is evaluated against.

It turns out that get_prev_events_for_room only returns up to 10 forward extremities, so the prev events we feed in to the event builder will be the ones used. I hadn't quite realised that get_prev_events_for_room was subtly different to fetching the forward extremities.

synapse/synapse/storage/databases/main/event_federation.py

Lines 1087 to 1099 in d0c713c

	def _get_prev_events_for_room_txn(
	self, txn: LoggingTransaction, room_id: str
	) -> List[str]:
	# we just use the 10 newest events. Older events will become
	# prev_events of future events.
	sql = """
	SELECT e.event_id FROM event_forward_extremities AS f
	INNER JOIN events AS e USING (event_id)
	WHERE f.room_id = ?
	ORDER BY e.depth DESC
	LIMIT 10
	"""

[squahtx]

1. /make_join reads the current state of the room and sees that the user
   is still in the room. It decides to omit the field required for
   restricted room joins.

Is there any harm in always including the field required for restricted room joins? (i.e. would that help mitigate the race too?)

No idea. @clokep might have some thoughts on this?

[clokep]

1. /make_join reads the current state of the room and sees that the user is still in the room. It decides to omit the field required for restricted room joins.

Is there any harm in always including the field required for restricted room joins? (i.e. would that help mitigate the race too?)

No idea. @clokep might have some thoughts on this?

I think you cannot do that (or at least I wouldn't trust implementations to treat that reasonably).

[DMRobertson]

Sounds like this is good to merge then?

[squahtx]

Sounds like this is good to merge then?

Yep. I'm a little sad that there is still a race window, even if it is much smaller than before.