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.

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

MSC2676: Message editing #2676

Merged
merged 28 commits into from
Jul 18, 2022
Merged
Changes from 19 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
8b95ffc
initial version of message editing proposal
uhoreg Jul 7, 2020
7f42c8b
fix MSC numbers
uhoreg Jul 7, 2020
5473009
Fix JSON in example
uhoreg Nov 24, 2020
e44f566
clarifications
uhoreg Jun 2, 2021
634dc2e
remove obsolete "XXX:", and fix a typo
uhoreg Oct 28, 2021
f9768e7
Initial cleanup and restructuring
richvdh Apr 12, 2022
1de6c11
Clarify algorithm for replacing content
richvdh Apr 12, 2022
adcdddc
background
richvdh Apr 12, 2022
fc2a690
More clarifications on applying edits
richvdh Apr 12, 2022
80c467b
Clarify behaviour of redactions
richvdh Apr 12, 2022
6cea76a
Minor grammar fixes
richvdh May 17, 2022
dac2399
Move the section on `msgtype` down
richvdh May 17, 2022
b8a7745
Clarify how edits are ordered
richvdh May 17, 2022
79a362e
Spec the behaviour for encrypted events
richvdh May 17, 2022
bb96694
Requirements for an edit event to be considered valid
richvdh May 17, 2022
dd1ca71
Collect "client behaviour" and "sever behaviour" together
richvdh May 17, 2022
eba4753
Clarify permalinks section
richvdh May 17, 2022
78550a2
Notes on edits of replies
richvdh May 17, 2022
e58715c
Clarify that `m.relates_to` within `m.new_content` is ignored
richvdh May 19, 2022
4c77c01
Clarifications from review
richvdh Jun 28, 2022
1850fb5
event ids are sorted lexicographically
richvdh Jun 28, 2022
c9e6652
Clarify aggregation section
richvdh Jun 28, 2022
1e63094
minor clarifications
richvdh Jul 12, 2022
2373873
Clarify which endpoints support edits
richvdh Jul 12, 2022
8ec4cf3
move definition of latest edit
richvdh Jul 12, 2022
08489e8
Apply suggestions from code review
richvdh Jul 13, 2022
f3d328d
fix typo
richvdh Jul 13, 2022
b245761
Attempt to clarify encrypted events
richvdh Jul 13, 2022
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
380 changes: 380 additions & 0 deletions proposals/2676-message-editing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,380 @@
# MSC2676: Message editing
richvdh marked this conversation as resolved.
Show resolved Hide resolved
richvdh marked this conversation as resolved.
Show resolved Hide resolved
turt2live marked this conversation as resolved.
Show resolved Hide resolved

Users may wish to edit previously sent messages, for example to correct typos.
This can be done by sending a new message with an indication that it replaces
the previously sent message.

This proposal is one in a series of proposals that defines a mechanism for
events to relate to each other. Together, these proposals replace
[MSC1849](https://github.com/matrix-org/matrix-doc/pull/1849).

* [MSC2674](https://github.com/matrix-org/matrix-doc/pull/2674) defines a
standard shape for indicating events which relate to other events.
* [MSC2675](https://github.com/matrix-org/matrix-doc/pull/2675) defines APIs to
let the server calculate the aggregations on behalf of the client, and so
bundle the related events with the original event where appropriate.
* This proposal defines how users can edit messages using this mechanism.
* [MSC2677](https://github.com/matrix-org/matrix-doc/pull/2677) defines how
users can annotate events, such as reacting to events with emoji, using this
mechanism.

## Background

Element-Web (then Riot-Web) and Synapse both implemented initial support for
message editing, following the proposals of MSC1849, in May 2019
([matrix-react-sdk](https://github.com/matrix-org/matrix-react-sdk/pull/2952),
[synapse](https://github.com/matrix-org/synapse/pull/5209)). Element-Android
and Element-iOS also added implementations around that time. Unfortunately,
those implementations presented the feature as "production-ready", despite it
not yet having been adopted into the Matrix specification.

The current situation is therefore that client or server implementations hoping
to interact with Element users must simply follow the examples of that
implementation. In other words, message edits form part of the *de-facto* spec
despite not being formalised in the written spec. This is clearly a regrettable
situation. Hopefully, processes have improved over the last three years so that
this situation will not arise again. Nevertheless there is little we can do
now other than formalise the status quo.

This MSC, along with the others mentioned above, therefore seeks primarily to
do that. Although there is plenty of scope for improvement, we consider that
better done in *future* MSCs, based on a shared understaning of the *current*
implementation.

In short, this MSC prefers fidelity to the current implementations over
elegance of design.

## Proposal
richvdh marked this conversation as resolved.
Show resolved Hide resolved

### `m.replace` event relationship type

A new `rel_type` of `m.replace` is defined for use with the `m.relates_to`
field as defined in
[MSC2674](https://github.com/matrix-org/matrix-doc/pull/2674). This is
intended primarily for handling edits, and lets you define an event which
replaces an existing event.

Such an event, with `rel_type: m.replace`, is referred to as a "message edit event".

### `m.new_content` property

richvdh marked this conversation as resolved.
Show resolved Hide resolved
The `content` of a message edit event must contain a `m.new_content` property
which defines the replacement content. (This allows the normal `body` fields to
be used for a fallback for clients who do not understand replacement events.)

For instance, an `m.room.message` which replaces an existing event might look like:

```json
{
"type": "m.room.message",
"content": {
"body": "* Hello! My name is bar",
"msgtype": "m.text",
"m.new_content": {
"body": "Hello! My name is bar",
uhoreg marked this conversation as resolved.
Show resolved Hide resolved
"msgtype": "m.text"
uhoreg marked this conversation as resolved.
Show resolved Hide resolved
},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$some_event_id"
}
}
}
```

The `m.new_content` can include any properties that would normally be found in
an event's `content` property, such as `formatted_body`.

#### Encrypted events

If the original event was encrypted, the replacement should be too. In that
case, `m.new_content` is placed in the `content` of the encrypted payload. For
turt2live marked this conversation as resolved.
Show resolved Hide resolved
example, an encrypted replacement event might look like this:
turt2live marked this conversation as resolved.
Show resolved Hide resolved

```json
{
"type": "m.room.encrypted",
"content": {
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$some_event_id"
},
"algorithm": "m.megolm.v1.aes-sha2",
"sender_key": "<sender_curve25519_key>",
"device_id": "<sender_device_id>",
"session_id": "<outbound_group_session_id>",
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth noting that what we should encrypt with? I.e. think we encrypt with the "next" key as if it was a normal message, rather than reusing the same key as for the edit event? Not 100% sure though

Copy link
Contributor

Choose a reason for hiding this comment

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

You always need to use a new key. Not only for security, but if you didn't the fallback would be useless since clients relying on the fallback would mark the event as a key reuse attack.

Copy link
Member

Choose a reason for hiding this comment

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

hopefully clarified in uhoreg@1e63094.

"ciphertext": "<encrypted_payload_base_64>"
}
}
```

... and, once decrypted, the payload might look like this:


```json
{
"type": "m.room.<event_type>",
"room_id": "!some_room_id",
"content": {
"body": "* Hello! My name is bar",
"msgtype": "m.text",
"m.new_content": {
"body": "Hello! My name is bar",
"msgtype": "m.text"
},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$some_event_id"
}
turt2live marked this conversation as resolved.
Show resolved Hide resolved
}
}
```

#### Applying `m.new_content`

When applying a replacement, the `content` property of the original event is
Copy link
Contributor

@benkuly benkuly Jul 13, 2022

Choose a reason for hiding this comment

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

When the original content is replaced, what happens, when the edit is deleted? Where does the original content come from, when it already has been replaced? Wouldn't it be more consistent to keep the original content and the client just uses the aggregation for the latest content?

Copy link
Member

Choose a reason for hiding this comment

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

  1. please see the background: we are not in the business of making significant changes to the design as part of this MSC.
  2. what do you mean by "when the edit is deleted" ? Why would it be deleted? The matrix protocol does not support deleting events.

Copy link
Contributor

@benkuly benkuly Jul 13, 2022

Choose a reason for hiding this comment

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

  1. I don't think that my opinion will change anything, but to cite this MSC "Hopefully, processes have improved over the last three years". I don't think that merging an immature MSC just because Element does implement it is an "improvement". Element should implement the matrix spec and not the matrix spec should specify Elements implementation ;)
  2. I meant redacted. According to this MSC, events with rel_type of m.replace can be redacted. Therefore you first have event1 with content1, then with an edit you have event1 with content2 (from event2) and with a redaction of event2 you should have event1 with content1.

The section "Applying m.new_content" is from my viewpoint of a SDK developer far more complicated then it needs to be.

replaced entirely by the `m.new_content`, with the exception of `m.relates_to`,
which is left *unchanged*. Any `m.relates_to` property within `m.new_content`
is ignored.

For example, given a pair of events:

```json
{
"event_id": "$original_event",
"type": "m.room.message",
"content": {
"body": "I *really* like cake",
"msgtype": "m.text",
"formatted_body": "I <em>really</em> like cake",
}
}
```

```json
{
"event_id": "$edit_event",
"type": "m.room.message",
"content": {
"body": "* I *really* like *chocolate* cake",
"msgtype": "m.text",
"m.new_content": {
"body": "I *really* like *chocolate* cake",
"msgtype": "m.text",
"com.example.extension_property": "chocolate"
},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$original_event_id"
}
}
}
```

... then the end result is an event as shown below. Note that `formatted_body`
is now absent, because it was absent in the replacement event, but
`m.relates_to` remains unchanged (ie, absent).

```json
{
"event_id": "$original_event",
"type": "m.room.message",
"content": {
"body": "I *really* like *chocolate* cake",
"msgtype": "m.text",
"com.example.extension_property": "chocolate"
}
}
```

Note that the `msgtype` property of `m.room.message` events need not be the
same as in the original event. For example, if a user intended to send a
message beginning with "/me", but their client sends an `m.emote` event
instead, they could edit the message to send be an `m.text` event as they had
originally intended.

### Validity of message edit events
turt2live marked this conversation as resolved.
Show resolved Hide resolved

Some message edit events are defined to be invalid. To be considered valid, all
of the following criteria must be satisfied:

* The replacement and original events must have the same `type`.
* Neither the replacement nor original events can be state events (ie, neither
may have a `state_key`).
* The original event must not, itself, have a `rel_type` of `m.replace`.
* The original event and replacement event must have the same `room_id`.
turt2live marked this conversation as resolved.
Show resolved Hide resolved
* The original event and replacement event must have the same `sender`.
* The replacement event (once decrypted, if appropriate) must have an
`m.new_content` property.

If any of these criteria are not satisfied, implementations should ignore the
replacement event (the content of the original should not be replaced, and the
edit should not be included in the server-side aggregation).

### Server behaviour

Whenever a homeserver would return an event via the Client-Server API, it
turt2live marked this conversation as resolved.
Show resolved Hide resolved
should check for any valid, applicable `m.replace` event, and if one is found, it
should first modify the `content` of the original event according to the
`m.new_content` of the most recent edit (as determined by
`origin_server_ts`, falling back to `event_id`).
turt2live marked this conversation as resolved.
Show resolved Hide resolved

An exception applies to [`GET
/_matrix/client/v3/rooms/{roomId}/event/{eventId}`](https://spec.matrix.org/v1.2/client-server-api/#get_matrixclientv3roomsroomideventeventid),
which should return the *unmodified* event (though the relationship should
still be "bundled", as described
below.

#### Server-side aggregation of `m.replace` relationships

Note that there can be multiple events with an `m.replace` relationship to a
given event (for example, if an event is edited multiple times). Homeservers
should aggregate `m.replace` relationships as in
[MSC2675](https://github.com/matrix-org/matrix-doc/pull/2675). The aggregation
turt2live marked this conversation as resolved.
Show resolved Hide resolved
gives the `event_id`, `origin_server_ts`, and `sender` of the most recent
replacement event (as determined by `origin_server_ts`).

This aggregation is bundled into the `unsigned/m.relations` property of any
event that is the target of an `m.replace` relationship. For example:

```json5

{
"event_id": "$original_event_id",
// ...
"unsigned": {
"m.relations": {
"m.replace": {
"event_id": "$latest_edit_event_id",
"origin_server_ts": 1649772304313,
"sender": "@editing_user:localhost
}
}
}
}
```

If the original event is redacted, any `m.replace` relationship should **not**
be bundled with it (whether or not any subsequent edits are themselves
redacted). Note that this behaviour is specific to the `m.replace`
relationship.

### Client behaviour

Clients can often ignore message edit events, since any events the server
returns via the C-S API will be updated by the server to account for subsequent
edits.

However, clients should apply the replacement themselves (or, alternatively,
request an updated copy of the original via [`GET
+/_matrix/client/v3/rooms/{roomId}/context/{eventId}`](https://spec.matrix.org/v1.2/client-server-api/#get_matrixclientv3roomsroomidcontexteventid)
turt2live marked this conversation as resolved.
Show resolved Hide resolved
or similar) when the server is unable to do so. This happens in the following
situations:

1. The client has already received the original event before the message
edit event arrives.

2. The original event (and hence its replacement) are encrypted.

Client authors are reminded to take note of the requirements for [Validity of
message edit events](#validity-of-message-edit-events), and to ignore any
invalid edit events that may be received.

### Permalinks

Permalinks to edited events should capture the event ID that the creator of the
permalink is viewing at that point (which might be a message edit event).

The client viewing the permalink should resolve this ID to the original event
ID, and then display the most recent version of that event.

### Redactions

When a message using a `rel_type` of `m.replace` is redacted, it removes that
edit revision. This has little effect if there were subsequent edits, however
if it was the most recent edit, the event is in effect reverted to its content
before the redacted edit.

Redacting the original message in effect removes the message, including all
subsequent edits, from the visible timeline. In this situation, homeservers
will return an empty `content` for the original event as with any other
redacted event. It must be noted that, although they are not immediately
visible in Element, subsequent edits remain unredacted and can be seen via API
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems a bit strange to call out Element here specifically. Is this behaviour applicable only to Element and not to other clients? Or is this how most clients are expected to behave?

Copy link
Contributor

Choose a reason for hiding this comment

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

At least a few other clients don't behave that way, since they consider it a privacy violation. :3

Copy link
Member

Choose a reason for hiding this comment

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

This means "I haven't done a comprehensive survey, and I wouldn't necessarily expect other clients to follow suit".

calls. See [Future considerations](#future-considerations).

### Edits of replies

An event which replaces a [reply](https://spec.matrix.org/v1.2/client-server-api/#rich-replies) might look like this:
turt2live marked this conversation as resolved.
Show resolved Hide resolved

```json
{
"type": "m.room.message",
"content": {
"body": "> <@richvdh:sw1v.org> ab\n\n * ef",
"msgtype": "m.text",
"format": "org.matrix.custom.html",
"formatted_body": "<mx-reply><blockquote><a href=\"https://matrix.to/#/!qOZKfwKPirAoSosXrf:matrix.org/$1652807718765kOVDf:sw1v.org?via=matrix.org&amp;via=sw1v.org\">In reply to</a> <a href=\"https://matrix.to/#/@richvdh:sw1v.org\">@richvdh:sw1v.org</a><br>ab</blockquote></mx-reply> * ef",
"m.new_content": {
"body": "ef",
"msgtype": "m.text",
"format": "org.matrix.custom.html",
"formatted_body": "ef"
},
"m.relates_to": {
"rel_type": "m.replace",
"event_id": "$original_reply_event"
}
}
}
```

Note in particular:

* There is no `m.in_reply_to` property in the the `m.relates_to`
object, since it would be redundant (see [Applying
`m.new_content`](#applying-mnew_content) above, which notes that the original
event's `m.relates_to` is preserved), as well as being contrary to the
spirit of
[MSC2674](https://github.com/matrix-org/matrix-spec-proposals/pull/2674)
which expects only one relationship per event.
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

* `m.new_content` does not contain any ["reply
fallback"](https://spec.matrix.org/v1.2/client-server-api/#fallbacks-for-rich-replies),
since it is assumed that any client which can handle edits can also
display replies natively.

## Future considerations

### Ordering of edits

In future we may wish to consider ordering replacements (or relations in
general) via a DAG rather than using `origin_server_ts` to determine ordering -
particularly to mitigate potential abuse of edits applied by moderators.
Whatever, care must be taken by the server to ensure that if there are multiple
replacement events, the server must consistently choose the same one as all
other servers.

### Redaction of edits

It is highly unintuitive that redacting the original event leaves subsequent
edits visible to curious eyes even though they are hidden from the
timeline. This is considered a bug which this MSC makes no attempt to
resolve. See also
Comment on lines +388 to +389
Copy link
Member

Choose a reason for hiding this comment

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

the MSC could at least suggest that clients redacting events (as moderators or as senders) redact the edits too, if it knows about them...

From a moderation perspective it's certainly a good idea to redact the edits too.

Copy link
Member

Choose a reason for hiding this comment

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

the MSC could at least suggest that clients redacting events redact the edits too

Is that what we actually do though? I'm reluctant to spec it if it's not a thing that happens.

Copy link
Member

Choose a reason for hiding this comment

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

it's not what we do, but the rationale for why we should be redacting old versions is much easier to reason about than why we didn't implement the feature in the first place.

it's long-since been considered a security-ish issue aiui that clients (including bots) don't redact old versions properly. What would be future MSC territory though is making it happen magically from the server rather than requiring the client to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, it just feels like a can of worms that I don't particularly want to open right now. What happens if the client doesn't have all the edits? (ok, it's better, but it's still a crappy solution). What happens if there are a million edits and we unexpectedly hit a rate limit?

I'm not really sure what we gain by adding a "hey, we could do this totally untested temporary hack" to an MSC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that what we actually do though? I'm reluctant to spec it if it's not a thing that happens.

Nheko does, but I don't think other clients do.

Link: https://github.com/Nheko-Reborn/nheko/blob/b6bbbdeae7966761ad2de7cdad92363c6926cfb5/src/timeline/TimelineModel.cpp#L1275

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 fairly sure that if we were to pull stats then the average number of edits would be less than a million, well within a rate limit :p

but we're talking about abuse situations, not the "average", aren't we?

Copy link
Member

Choose a reason for hiding this comment

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

there's several degrees of abuse which we can consider. Spamming edits is one form of abuse, but so is redacting a message and not the edits.

Copy link
Member

Choose a reason for hiding this comment

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

Well fine, this isn't a hill I'm going to die on. Can you suggest some words?

Copy link
Member

Choose a reason for hiding this comment

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

The Telegram bridge also redacts all edits when a message is deleted on Telegram. It causes a fun redacted event placeholder spam on Matrix because clients don't know they're edits anymore (#3389), especially if you delete a message sent by some fun telegram bot that edits a message every second for a few hours. But appservices aren't ratelimited so it's fine

Copy link
Contributor

Choose a reason for hiding this comment

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

(A follow up MSC could also remove ratelimit for edit and reaction redactions)

[element-web#11978](https://github.com/vector-im/element-web/issues/11978) and
[synapse#5594](https://github.com/matrix-org/synapse/issues/5594).

### Edits to state events

There are various issues which would need to be resolved before edits to state
events could be supported. In particular, we would need to consider how the
semantically-meaningful fields of the content of a state event relate to
`m.new_content`. Variation between implementations could easily lead to
security problems (See
[element-web#21851](https://github.com/vector-im/element-web/issues/21851) for
example.)

### Editing other users' events

There is a usecase for users with sufficient power-level to edit other peoples'
events. For now, no attempt is made to support this. If it is supported in the
future, we would need to find a way to make it clear in the timeline.