element-hq / element-hq/synapse

Incorrect state calculation on CSAPI endpoints

Open
#18,936 0 comments 0 reactions 0 assignees View on GitHub
A-Membership A-State-Resolution O-Occasional T-Task
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

### Description

There are several CSAPI endpoints like `GET /_matrix/client/v3/rooms/{roomId}/members` which accepts an `?at=` param which:

> The **point in time** (pagination token) to return members for in the room. This token can be obtained from a prev_batch token returned for each room by the sync API. Defaults to the current state of the room, as determined by the server.

These are almost always incorrectly implemented in Synapse. They are usually implemented as:
```py
last_event_id = (
await self.store.get_last_event_id_in_room_before_stream_ordering(
room_id,
end_token=at_token.room_key,
)
)

room_state_events = (
await self._state_storage_controller.get_state_for_events(
[last_event_id], state_filter=state_filter
)
)
```

### Steps to reproduce

This pattern neglects to consider concurrent events in the DAG, making it inappropriate for use to determine what the member list was like at a certain event. As an example:
```
A join
|
B join
| \
C join D join
```

Assume events arrived in alphabetical order. What should `/members` return if you specify `?at=D`?
Currently, Synapse and I suspect other implementations simply calculate the room state after the event
and return that, which would be `{A,B,D}`. In practice however, users live streaming when these events
arrived would have seen `{A,B,C,D}` as the member list after D. This subverts the entire point of the
`?at=` parameter, which is to try to figure out the member list at a particular historical point in time.

In the worst case, this is a footgun as clients may be thinking that by using `?at=` they ensure that
they are protected against race conditions when querying the CSAPI, but in practice they are accidentally
asking for subtly different dataset.

We should _always_ be using the `current_state_delta_stream` to ensure we handle concurrent events.
We should _remove_ `get_last_event_id_in_room_before_stream_ordering` as it always (?) is the wrong thing to do. Even callers which do use it have FIXMEs to say it's wrong for the reasons I have outlined:
```py
# FIXME: This gets the state at the latest event before the stream ordering,
# which might not be the same as the "current state" of the room at the time
# of the stream token if there were multiple forward extremities at the time.
```

The net result of failing to consider this is:
- We may incorrectly include state we should have excluded (if we do not consider a concurrent revocation e.g a change in PLs that invalidates some state)
- We may incorrectly exclude state we should have included (if we do not considder a concurrent addition e.g a room member joining)

### Affected endpoints

- `GET /_matrix/client/v3/rooms/{roomId}/members`
- `/sync` when used without `?state_after=`
- All endpoints which fetch the room state for left/banned users (the leave/ban event is used solely to determine room state at the time), including /sync, sliding sync, /state endpoints
- Sliding sync: calculation of room name, historical rooms

### Synapse Version

v1.138.0

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.