-
-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Inconsistency for edits to encrypted events #1299
Comments
It's not entirely clear how best to fix this. A few suggestions:
|
I like the second approach. I have often argued, that events should be immutable (except redactions). It is really confusing when an event content with an distinct eventId can change. This is not a good representation of the original DAG. This also leads to open problems like deleting the replace event - what happens to the replaced event content. Maybe the new content could be added to the aggregation of the replaced event. |
I'd rather we avoid this, because it would be yet another metadata leak and would add another hurdle to hiding |
How so? In this vision, an edit to an encrypted event would look like this: {
"content": {
"m.new_content": {
"algorithm": "m.megolm.v1.aes-sha2",
"ciphertext": "<blah>",
"session_id": "ZLkXSfLNIPGJyJv+LbjGt1Fxs4lZzfuHOhysZ6vNJ98"
},
"m.relates_to": {
"event_id": "$...",
"rel_type": "m.replace"
}
},
"sender": "@example:matrix.org",
"type": "m.room.encrypted",
"room_id": "!...:matrix.org"
} and the encrypted body would have: {
"room_id": "!...:matrix.org",
"type": "m.room.message",
"content": { "body": "..." }
} I don't see a huge metadata leak there. The downside is that, whilst it makes an edit of an encrypted event closer to an edit of an unencrypted event, it means that an edit to an encrypted event is quite different to an original encrypted event, so any client that doesn't support edits is going to be very confused. |
(I said "event types" above but actually meant "relationship type".) The presence of |
The spec is saying that
How can we validate that it the original content is redacted server side? |
It sounds like there is general support for option 2 in the list above: stop servers performing edits, and put the whole of the edit event into the relationship aggregation. In this scenario, an event which had been edited would look like this, when returned by the server: {
"event_id": "$original_event_id",
"type": "m.room.message",
"sender": "@sender:localhost",
"origin_server_ts": 1649772301230,
"room_id": "!room:example.org",
"content": {
"body": "Original message",
"msgtype": "m.text",
},
"unsigned": {
"m.relations": {
"m.replace": {
"event_id": "$latest_edit_event_id",
"type": "m.room.message",
"sender": "@sender:localhost",
"origin_server_ts": 1649772304313,
"room_id": "!room:example.org",
"content": {
"body": " * Edited message",
"msgtype": "m.text",
"m.new_content": {
"body": "Edited message",
"msgtype": "m.text"
},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$original_event_id"
}
}
}
}
}
} Note that the implication is that all clients must now inspect the bundled aggregation, and extract the An encrypted event with edits would look like: {
"event_id": "$original_event_id",
"type": "m.room.encrypted",
"sender": "@sender:localhost",
"origin_server_ts": 1649772301230,
"room_id": "!room:example.org",
"content": {
"algorithm": "m.megolm.v1.aes-sha2",
"session_id": "<outbound_group_session_id>",
"ciphertext": "<original_encrypted_payload_base_64>"
},
"unsigned": {
"m.relations": {
"m.replace": {
"event_id": "$latest_edit_event_id",
"type": "m.room.encrypted",
"sender": "@sender:localhost",
"origin_server_ts": 1649772304313,
"room_id": "!room:example.org",
"content": {
"algorithm": "m.megolm.v1.aes-sha2",
"session_id": "<outbound_group_session_id>",
"ciphertext": "<encrypted_replacement_payload_base_64>"
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$original_event_id"
}
}
}
}
}
} As before, the {
"type": "m.room.message",
"room_id": "!some_room_id",
"content": {
"body": "* Edited message",
"msgtype": "m.text",
"m.new_content": {
"body": "Edited message",
"msgtype": "m.text"
}
}
} My concern is that this could be a breaking change for clients which now expect servers to apply edits before returning them. I would welcome input from client authors on this point. |
This is how I was expecting edits to work until I implemented them in the Rust SDK. It makes no sense to me that "relation aggregation" would touch the content of an event, or that a homeserver would ever communicates a different event I am happy that doing a breaking change to clean this up is being considered :) |
I dislike the idea that you can't just write a client which looks at Is it not possible to flip the event stack on it's head and have the outer event be the edit and then in
This is surely broken regardless in the federation case, as your server may join the room midway through an edit chain? As there exists no way to fetch the earlier edits other than
As nice as it would be to encrypt all the things, I think this will hurt client performance when trying to extract an edit chain, as you would need potentially the entire room history to calculate it and the server wouldn't be able to help you. This applies to all |
Why would it need to download the entire history, though? The idea is to retain relationship arrows in the clear, but without leaking the type of the relationship. This still allows a server to help by supplying all relationship events which point to a given event, but the aggregation operation is to append (returning a list of all related events), rather than to replace. |
Technically it is not the right thing. It is the body of a different event, that pretends to be the right body. But it is not and that is really confusing and does not follow any good technical standards. It has a lot of reasons, why data should be immutable and why more and more languages or frameworks does follow this pattern.
If you don't care about edits, then you don't necessarily see the "right" thing because "new" edits will be rendered as |
Oh okay, this sounds reasonable then. It's obviously not great if the chain is sprawling with many
I'm not talking about the "technical" right thing, I'm talking about the right thing from a UX perspective. Yes, technically events should be immutable and at a federation level they are, where it makes sense for them to be because events are hashed and signed by servers. At the CSAPI level though, we can and should massage this into a more malleable format which is more useful to clients. "More useful" is subjective, but I feel that having the
Can you explain a bit more of your reasons here please? I don't see how this issue relates to functional programming or frameworks which copy data around to get immutable semantics. Wanting a protocol to have immutable data because X is immutable does not feel like a good argument to me. |
Why is it good UX, when one edited event comes with a
As far as I know Matrix clients are implemented with programming languages and frameworks. At least I did it with the SDK Trixnity 😉 😉 So for me as library developer it is far more consistent to rely on immutable events. My framework aims to be as generic as possible. Features like |
I think you're basically saying: return the edit event when we would currently return the original event (and also, return some extra metadata whenever we return the edit event). It's an interesting idea, but we'd have to think hard about the semantics. For example: if a client calls I think my main concern with that approach is that it's going to involve a fair bit of work to implement on both servers and clients, and I'm not sure it's practical right now. Meanwhile, we could do with a simple fix to get out of the situation where implementing the spec breaks end-to-end encryption. |
Incidentally, I polled a couple of client developers on this:
So it probably would cause breakage that we should avoid if possible. One idea is to make the change in three steps: first servers start returning the entire edit event in |
I've been thinking a bit more about this. I'm still worried that, whilst it sounds attractive on the surface, there's complexity here. For instance: suppose you want to react to an event that has been edited. Reactions are supposed to be applied only to the original event, so as a client you still have to remember to go and dig in the I'm very much inclined to leave this for another day. |
I made my first MSC regarding this: matrix-org/matrix-spec-proposals#3925 |
* init m.replace aggregation with full event Signed-off-by: benkuly <kontakt@folivo.net> * Rename tmp.md to 3925-replace-aggregation-with-full-event.md Signed-off-by: benkuly <kontakt@folivo.net> * Update 3925-replace-aggregation-with-full-event.md typos Signed-off-by: benkuly <kontakt@folivo.net> * Update 3925-replace-aggregation-with-full-event.md added MSC number Signed-off-by: benkuly <kontakt@folivo.net> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Jonas Platte <jplatte+git@posteo.de> Signed-off-by: benkuly <kontakt@folivo.net> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Jonas Platte <jplatte+git@posteo.de> Signed-off-by: benkuly <kontakt@folivo.net> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * redact instead of delete * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * word wrap 80 * remove immutable argument * add alternative from matrix-org/matrix-spec#1299 (comment) * add json example * shorter version of the actual proposal Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * describe the actual reason, why encrypted events cannot be replaced Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * mention discussion * typo * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * clarify inconsistent behavior with replaced contents * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update proposals/3925-replace-aggregation-with-full-event.md Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * clarify inconsistent behavior and put it into introduction * remove superfluous event content field * add some details to potential issues suggested by @richvdh --------- Signed-off-by: benkuly <kontakt@folivo.net> Co-authored-by: Jonas Platte <jplatte+git@posteo.de> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Link to problem area:
https://spec.matrix.org/v1.4/client-server-api/#event-replacements
Issue
https://spec.matrix.org/v1.4/client-server-api/#editing-encrypted-events says:
We then have https://spec.matrix.org/v1.4/client-server-api/#server-side-replacement-of-content, which says:
Of course, the server doesn't have access to
m.new_content
, so that would suggest replacing the original content with... nothing?Arguably, Synapse shouldn't even consider the edit as valid, because https://spec.matrix.org/v1.4/client-server-api/#validity-of-replacement-events says:
However, it's important that edits to encrypted events do get aggregated into the original event, so it's not as simple as just excluding them.
See also matrix-org/synapse#14252 (comment).
The text was updated successfully, but these errors were encountered: