Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

synapse accepted an event with no prev_events #9799

Open
richvdh opened this issue Apr 12, 2021 · 13 comments
Open

synapse accepted an event with no prev_events #9799

richvdh opened this issue Apr 12, 2021 · 13 comments
Labels
A-DAG Directed acyclic graph of events (events connected by prev_events) S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@richvdh
Copy link
Member

richvdh commented Apr 12, 2021

$39VZNdyICR1_8rHvEnuOU9-J6rZ0J4aWEsHpOggBK8o has no prev_events but was accepted on matrix.org

this sounds bogus.

@ShadowJonathan
Copy link
Contributor

What room is this from?

@richvdh
Copy link
Member Author

richvdh commented Apr 12, 2021

not really sure that's relevant

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Apr 12, 2021

It's relevant for the context of this issue, what's the event contents? Which server did it come from? How does it look like? Under what circumstances was it inserted/accepted into the database? Is it a m.room.create event?

@timokoesters
Copy link

It is a member event and was created on an older conduit version

@anoadragon453 anoadragon453 added S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Needs-Info This issue is blocked awaiting information from the reporter labels Apr 20, 2021
@timokoesters
Copy link

Does synapse currently have any check that should prevent this?

@richvdh richvdh removed the X-Needs-Info This issue is blocked awaiting information from the reporter label Apr 23, 2021
@richvdh
Copy link
Member Author

richvdh commented Apr 23, 2021

my logic goes:

  • an event should be rejected unless the state in the room at that point in the DAG (as determined by the prev_events) allows it
  • if there are no prev_events, the state at that point in the DAG is empty
  • the only permissible event if there is no state is m.room.create
  • therefore, an event (other than m.room.create) with no prev_events should be rejected by the event auth algorithm

@timokoesters
Copy link

That makes sense, but it is not in the spec. The spec just says "If type is m.room.create: If it has any previous events, reject." but not the other way around.

My theory (without looking at the synapse code) is that synapse wants to get the state at the event, but can't find it (as there are no prev events), so it uses /state_ids, which works and then the event with no prev events gets into the DAG

@richvdh
Copy link
Member Author

richvdh commented Apr 23, 2021

That makes sense, but it is not in the spec. The spec just says "If type is m.room.create: If it has any previous events, reject." but not the other way around.

The same bit of the spec also says:

If the sender’s current membership state is not join, reject.

If you don't have any state, the sender doesn't have a current membership.

@timokoesters
Copy link

As I said in my last message, Synapse will probably still get the state by calling /state_ids on the event. That other server might pretend to know the state at the event even though there are no prev events

@richvdh
Copy link
Member Author

richvdh commented Apr 23, 2021

Synapse will probably still get the state by calling /state_ids on the event

well, that would be the bug, then.

@richvdh richvdh closed this as completed Apr 23, 2021
@richvdh richvdh reopened this Apr 23, 2021
@timokoesters
Copy link

timokoesters commented Apr 23, 2021

Well servers have to rely on /state_ids sometimes. You can't expect them to fetch ALL prev events, that would take far too long (because you need to fetch the prev events of the prev events too to get their state etc)

@MadLittleMods
Copy link
Contributor

We're currently using this empty prev_events behavior with MSC2716 to hang the historical state and message chains off on their own (outside of the normal DAG). We still have to specify auth_events though.

@MadLittleMods
Copy link
Contributor

Related to #8094

@MadLittleMods MadLittleMods added the A-DAG Directed acyclic graph of events (events connected by prev_events) label May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-DAG Directed acyclic graph of events (events connected by prev_events) S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

5 participants