Inconsistency for edits to encrypted events

[richvdh]

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:

[for encrypted events], m.new_content is placed in the content of the encrypted payload.

We then have https://spec.matrix.org/v1.4/client-server-api/#server-side-replacement-of-content, which says:

Whenever an m.replace is to be bundled with an event as above, the server should also modify the content of the original event according to the m.new_content of the most recent replacement event.

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:

The replacement event (once decrypted, if appropriate) must have an m.new_content property.

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).

[richvdh]

It's not entirely clear how best to fix this. A few suggestions:

1. The proximate and easiest fix is just to special-case m.room.encrypted in https://spec.matrix.org/v1.4/client-server-api/#server-side-replacement-of-content, to say that servers should not update the content. (We should also update https://spec.matrix.org/v1.4/client-server-api/#validity-of-replacement-events to say that edits to m.room.encrypted should be aggregated by the server even though they have no m.new_content).

   However, that makes the behaviour very inconsistent with the behaviour of unencrypted events.

2. Reconsider server-side replacement of content. Having the content automatically be replaced is useful in some situations (eg, if you're paginating and don't have the edit event), but the current implementation has downsides:

   • it doesn't work for edits to encrypted events; at present clients have to go and request the edit event separately, and do their own replacement.
   • if you want the full edit history for an event, it's currently hard to get hold of the original event.

   One suggestion is to leave the original event alone, and bundle the complete edit event into unsigned (instead of just the basic information as at present). I think this would be largely backwards compatible, modulo a few edge-cases that probably don't work very well today.

3. A more radical proposal: put the encrypted content inside m.new_content, rather than the other way around. This was discussed during the MSC process, but at that time the goal was to land a spec for edits without blocking it on changing a bunch of implementations.

   This would make encrypted events much more consistent with unencrypted events, but it would, of course, be a breaking change for all clients. It's unclear that anyone is in a position to contribute changes to client implementations to make this a reality.

[benkuly]

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.

[dkasak]

put the encrypted content inside m.new_content, rather than the other way around.

I'd rather we avoid this, because it would be yet another metadata leak and would add another hurdle to hiding event relationship types (of encrypted events) in the future.

[richvdh]

I'd rather we avoid this, because it would be yet another metadata leak and would add another hurdle to hiding event types (of encrypted events) in the future.

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.

[dkasak]

(I said "event types" above but actually meant "relationship type".)

The presence of m.new_content leaks the fact that an event is an edit. In today's world this doesn't seem too terrible, since we're currently leaking relationship types in the clear anyway, but it's a move in the wrong direction since we'd ideally want to be able to encrypt relationship types too (as talked about in #660).

[BillCarsonFr]

The spec is saying that

The replacement and original events must have the same type (i.e. you cannot change the original event’s type).

How can we validate that it the original content is redacted server side?

[richvdh]

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 m.new_content, otherwise they may incorrectly show the original event when back-paginating.

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 encrypted_replacement_payload would be simply:

{
  "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.

[jplatte]

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.

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 content over federation vs. CS-API.

I am happy that doing a breaking change to clean this up is being considered :)

[kegsay]

I dislike the idea that you can't just write a client which looks at content.body and it gets the right thing, as now it is the original event and not the most recent edit. Breaking this assumption may break other areas, such as indexing textual content.

Is it not possible to flip the event stack on it's head and have the outer event be the edit and then in unsigned have the base event being edited? Currently it is the inverse where we have the original event and then in unsigned the newer event sits. That way, if you don't care about edits (because you don't implement them) then you see the right thing, and when you do care about them you can just inspect unsigned to present the "edits" dialog.

Having the content automatically be replaced is useful in some situations (eg, if you're paginating and don't have the edit event)

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 /backfill, we basically don't bother.

but it's a move in the wrong direction since we'd ideally want to be able to encrypt relationship types too

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 rel_types, so reactions and threads too. My concern is that encryping the relationships will end up making them basically unusable in large E2EE rooms without forcing clients to download the entire history of the room, which may be infeasible.

[dkasak]

My concern is that encryping the relationships will end up making them basically unusable in large E2EE rooms without forcing clients to download the entire history of the room, which may be infeasible.

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.

[benkuly]

I dislike the idea that you can't just write a client which looks at content.body and it gets the right thing

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.

That way, if you don't care about edits (because you don't implement them) then you see the right thing

If you don't care about edits, then you don't necessarily see the "right" thing because "new" edits will be rendered as * new body. Therefore I don't think this is a good reason. It is more an argument against it, because it is inconsistent.

[kegsay]

The idea is to retain relationship arrows in the clear

Oh okay, this sounds reasonable then. It's obviously not great if the chain is sprawling with many rel_types as you lose the ability to filter to just the ones you are interested in, but I think that's a fair trade-off for the reduced metadata leakage.

Technically it is not the right thing.

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 content of an edited event being the latest iteration of that event is more useful than having clients to also check for unsigned["m.relations"]["m.replace"].content.

It has a lot of reasons, why data should be immutable and why more and more languages or frameworks does follow this pattern.

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.

[benkuly]

I'm talking about the right thing from a UX perspective.

Why is it good UX, when one edited event comes with a * and the other does not?

Can you explain a bit more of your reasons here please?

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 replace are added on top of basic rules of the spec (aggregation and relations). This can be tested sperately and does not change or touch existing code. The current spec breaks this rules regarding replace-relations. We have aggregations for this. And this is a terrible DX (developer experience).

[richvdh]

Is it not possible to flip the event stack on it's head and have the outer event be the edit and then in unsigned have the base event being edited?

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 /_matrix/client/rooms/{roomId}/context/{originalEvent}, won't they be surprised when $originalEvent is not returned?

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.

[richvdh]

[have servers not apply edits]
My concern is that this could be a breaking change for clients which now expect servers to apply edits before returning them.

Incidentally, I polled a couple of client developers on this:

• @BillCarsonFr said that Element-Android will apply any edits it happens to receive in its timeline, and otherwise just displays any content it receives from the server. This is pretty broken as it is, because (a) it doesn't work at all for encrypted rooms, and (b) if the client sees edit events, there is no guarantee that it can see the most recent edit event - so it may actually be overwriting the "latest" content as returned by the server with a less-recent edit.
• @t3chguy said that Element-Web relies on the server to return the modified content, and not doing so will produce incorrect results for permalink views, etc. The exception is encrypted rooms, because EW special-cases encrypted rooms/events.

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 unsigned, then clients switch over to it, and finally servers stop replacing content.

[richvdh]

@kegsay:

Is it not possible to flip the event stack on it's head and have the outer event be the edit and then in unsigned have the base event being edited? Currently it is the inverse where we have the original event and then in unsigned the newer event sits. That way, if you don't care about edits (because you don't implement them) then you see the right thing, and when you do care about them you can just inspect unsigned to present the "edits" dialog.

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 unsigned object to figure out how to build the reaction.

I'm very much inclined to leave this for another day.

[benkuly]

I made my first MSC regarding this: matrix-org/matrix-spec-proposals#3925