Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MSC3946: Dynamic room predecessor #3946

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

MSC3946: Dynamic room predecessor #3946

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
37ad074
Add MSC for dynamic predecessor
MadLittleMods Dec 9, 2022
16bd625
Specify MSC number
MadLittleMods Dec 9, 2022
e9ff87b
Small langauge tweak
MadLittleMods Dec 9, 2022
af21937
Fix presence typo
MadLittleMods Dec 9, 2022
0322361
Document Gitter use case
MadLittleMods Jan 10, 2023
945c26d
Use "retroactively" because action is being taken
MadLittleMods Jan 10, 2023
44ab2ae
Add via_servers field to make the predecessor room ID routable
MadLittleMods Feb 9, 2023
e47c86d
Comment on current state of via servers in m.room.create
MadLittleMods Feb 9, 2023
3e2713b
Add more context for predecessor in create event should be phased out
MadLittleMods Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions proposals/3946-dynamic-predecessor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# MSC3946: Dynamic room predecessor

Currently, the only way to specify the predecessor of a room is during room creation in
the `m.room.create` state event which can't be changed after the room is created
(the `m.room.create` event is immutable and can't be changed retrospectively).
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

This is in contrast to specifying the successor of a room via the `m.room.tombstone`
state event in the `replacement_room` content field which can be sent at any time to
update the state.

Traditionally, the predecessor use case has only been necessary for room upgrades where
one room is replaced by another and since the old room exists at the time of the new
room being created, it can easily be specified in the `m.room.create` event of the new
room.

But in history import cases like
[MSC2716](https://github.com/matrix-org/matrix-spec-proposals/pull/2716), specifying the
predecessor after the fact becomes important because the new historical room wasn't
available to reference when the current room was originally created. It's important to
note that history import is more than just
[MSC2716](https://github.com/matrix-org/matrix-spec-proposals/pull/2716) since people
can use [MSC3316 timestamp
massaging](https://github.com/matrix-org/matrix-spec-proposals/pull/3316) `?ts=123`
query parameter to send messages back in time, in general servers can craft events at
any time which are accepted over federation.

Replacing the predecessor of a room is also nice if you make a mistake in the import
process and only notice after the fact. You can easily create a new historical room with
the mistake corrected and splice it into the predecessor/successor chain as necessary.


## Proposal

Add a `m.room.predecessor` state event type where you can specify the room predecessor.
This allows the predecessor state to be updated as necessary.

Event type: State event
State key: A zero-length string.

**`m.room.predecessor` event `content` field definitions:**
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

key | type | value | description | required
--- | --- | --- | --- | ---
`predecessor_room_id` | string | Room ID | A reference to the room that came before and was replaced by this room | yes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

predecessor_room_id is a bit wordy but plainly describes what it is and what the value should be 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MadLittleMods did you consider including event_id like in the create event? Some code in matrix-react-sdk expects to be able to have that. I'm not sure yet what the implications would be of not providing it when looking for a room's predecessor. (I imagine we can work around it, but I haven't looked into it yet)

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you consider including event_id like in the create event?

Didn't know about it so no consideration was given to it so far. For my own reference: https://spec.matrix.org/v1.5/client-server-api/#mroomcreate

event_id (Required): The event ID of the last known event in the old room.
room_id (Required): The ID of the old room.

Adding an optional last_known_event_id field seems fine 👍

I assume it's not related to the upgrade verification part at all since we should be looking at the latest state in the room. But this sort of info could be nice as one option for a continuation point to jump to in the old room.

It terms of workaround fallbacks, we can just go to where the latest tombstone in the old room is or just the latest in the old room. One more complicated option is to use the /timestamp_to_event endpoint to find the latest event from when the predecessor was sent but doesn't seem necessary or the right thing to do exactly.

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I understand the use cases, are my assumptions above correct? Are there other use cases? Is this useful?


The presense of `m.room.predecessor` state in the room should take priority over the
`predecessor` specified in the `m.room.create` event.


## Potential issues

*None surmised so far*


## Alternatives

One possible alternative is to allow sending the `m.room.create` multiple times but this
would require extensive [authorization
rule](https://spec.matrix.org/v1.5/rooms/v10/#authorization-rules) changes and
considerations.

It would also be good to consider whether `predecessor` should be part of the
`m.room.create` at all. Maybe we should consider removing it in favor of this new event
type.


## Security considerations
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@turt2live turt2live Jan 5, 2023 •
edited by ara4n
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried this MSC introduces higher risk of security issues in clients regarding room takeovers. Currently, the relationship between the previous and current room can be verified (barring state resets), giving the client confidence that room B is in fact the most recent edition of room A. With the relationship being mutable, clients are likely to not check the predecessor correctly and therefore allow someone to hide/take over non-tombstoned rooms.

The current relationship of rooms is very linear, and though this MSC says the mutable event takes precedence, not all clients will have updated to consider that point. This can lead to inconsistencies in what users see, or abusive scenarios where rooms are hidden from some people and not others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think we race conditioned a bit and share more-or-less the same concern, just worded a bit differently.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly more concerned about clients getting the implementation wrong, despite warnings in the spec, though yes: the root issue is "mutable is scary", I think.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the relationship between the previous and current room can be verified (baring state resets), giving the client confidence that room B is in fact the most recent edition of room A.

There is nothing "verifiable" about the m.room.create predecessor value besides that it came from someone with proper power levels. A Matrix room can be created with whatever predecessor desired (even multiple rooms with the same predecessor). And same for the tombstone from some old room pointing to a new room. And as a note in case onlookers aren't as familiar, tombstones are already mutable.

To me, the chain of tombstone and predecessor are mere suggestions of where you can continue looking at history in the room. I'm not quite following the line of thought to get the severity with danger in any of this.

All of these problems like selectively sharing the state only to certain servers or state resets seem completely separate to this MSC. If we can't trust the state in a room, then everything falls apart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presence of a tombstone pointing to the new room and a predecessor pointing to the tombstoned room forms a verifiable relationship - this was specifically designed in for room upgrades.

With this MSC, we'd be breaking that relationship we previously went out of our way to make verifiable - this is where the concern is coming from. If we're breaking the verifiable nature of the relationship, it needs to be for justifiable/secure reasons.

We already acknowledge that state resets and similar can cause the relationship to be broken, however the expectation in those cases is that clients actually split the room into two logical rooms again, even if half the relationship still exists. With both halves of the relationship, the client can merge the rooms together safely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this a takeover? Above all, this is just a client display thing. And second, it looks like one room needs the tombstone and other one needs a predecessor for things to look replaced. Both sides acknowledged each other for this to happen. Anything other than this seems like a bad assumption on the clients part.

The case I'm concerned about isn't when both rooms point at each other, but rather when a client developer inevitably makes a mistake in the implementation of this MSC. There's already significant risk of clients getting the implementation wrong, and making the predecessor dynamic opens the door wider for mistakes (despite warnings in the spec, which often go unheeded). This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would result in a malicious room being allowed to overwrite/hide another room it shouldn't have right to, thus takeover.

This seems like it would be a client doing something very wrong. We can't help a client that doesn't look at both tombstone and predecessor to see that they match. That's a bare minimum even with the current state of tombstone/predecessor if you're making rooms look replaced. A client can show the pointer forward/backward without this verification though.

Even a naive client implementation that only looks at the create event for the predecessor is safe in the new world that this MSC creates.

If a client implementation updates to look at m.room.predecessor, then that is also pretty fool-proof regardless of which era of state they look at. The only tricky thing I could think of is if we also lived in a MSC3779 world where someone doesn't check for the canoncial version with state_key: "" but that's not a concern yet (and is a general problem).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the client would be doing something wrong, however the risk of a client doing something wrong is higher with an MSC like this (in its current form) that overall leaves an impression that the protocol is insecure, even if the blame is actually with the clients.

I don't think this MSC increases the risk level. We have the exact same risk with the current tombstone/predecessor state of things.

And to address that one aspect, it sounds we should consider another MSC to verify this server-side so clients can more easily rely on eating whatever we feed it.

Unfortunately I don't recall all of the details, but there was also a reason why we embedded the predecessor into the create event instead of a dedicated state event: a dedicated state event would have been cleaner and "more proper" as far as Matrix is concerned, but we landed on putting it in the create event for a reason. Digging up that rationale would likely help this conversation, as it keeps coming to mind, though I sadly do not have the bandwidth to do that research.

This would be interesting to find. We should start a new thread if we come up with anything here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One datapoint around verifiability: I found a bug in matrix-js-sdk's code for verifying links: matrix-org/matrix-js-sdk#3089 . The fact that this has remained undiscovered for a long time may imply that people are not paying a great deal of attention to verifiability.


`m.room.predecessor` is a state event that falls under the same power-level state
restrictions/permissions as other state events so there shouldn't be any additional
considerations here.

Room upgrades are a known hairy area of Matrix and how clients deal with them (or lack
of) but this doesn't change any of the existing semantics of predecessors. This is just
a way to change the predecessor of where people should point to instead.


## Unstable prefix

While this feature is in development, the `m.room.predecessor` state event can be used
as `org.matrix.msc3946.room_predecessor`.

Clients can choose to honor the unstable event type at their discretion.