Merge pull request #1954 from matrix-org/neilj/Remove-prev_content-fr…

…om-the-essential-keys-list2

MSC1954: Proposal to remove prev_content from the essential keys list

proposals/1954-remove-prev_event-from-essential-keys-list.md

@@ -0,0 +1,77 @@
+# Remove prev_content from the essential keys list
+
+Matrix supports the concept of event redaction. The ability to redact rather
+than delete is necessary because some events e.g. membership events are
+essential to the protocol and _cannot_ be deleted. Therefore we do not delete
+events outright and instead redact them. This involves removing all keys from
+an event that are not required by the protocol. The stripped down event is
+thereafter returned anytime a client or remote server requests it.
+
+
+## Proposal
+
+[The redaction algorithm](https://matrix.org/docs/spec/client_server/r0.4.0.html#redactions)
+defines which keys must be retained through a redaction. Currently it lists
+```prev_content``` as a key to retain, though in practice there is no need to
+do so at the protocol level.
+
+The proposal is simply to remove ```prev_content``` from the essential keys
+list.
+
+Note: the inclusion of ```prev_content``` in the essential keys list was
+unintentional and should be considered a spec bug. Synapse (and other server
+implementations) have not implemented the bug and already omit
+```prev_content``` from redacted events.
+
+
+## Tradeoffs
+
+When sending events over federation the events are [hashed and
+signed](https://matrix.org/docs/spec/server_server/unstable.html#adding-hashes-and-signatures-to-outgoing-events),
+this involves operating not only on the original event but also the redacted
+form of the event. The redacted hash and redacted signed event are necessary if
+the event is ever redacted in future. As a result, any change of the essential
+keys list must be managed carefully. If disparate servers implement different
+versions of the redaction algorithm (for a given event) attempts to send the
+event over federation will fail.
+
+We _could_ manage this change via room versioning and create a new room
+version that implements this MSC. However, because the federation already
+omits the ```prev_content``` key by convention, implementing this MSC only in
+the new room version would mean that the entire existing federation would not
+be spec compliant.
+
+As a result it seems pragmatic to have the spec reflect reality, acknowledge
+that the spec and federation have deviated and instead update the spec
+retrospectively to describe the de-facto redaction algorithm.
+
+## Potential issues
+
+It is theoretically possible that a closed federation could exist whose servers
+do follow the spec as is. This MSC would render those servers non-compliant with
+the spec. On balance this seems unlikely and in the worst case those
+implementors could add the change to a subsequent room version, eventually
+reaching spec consistency as older room versions are deprecated.
+
+Another scenario is that a client may redact events according to the spec as is
+and persist prev_content through the redaction, thereby diverting from that on
+the server(s). Client authors will have to update their code to drop
+```prev_content``` - however, given that prev_content should not be used in
+important calculations and/or visualisations, this ought to be a relatively
+non-invasive change.
+
+
+## Security considerations
+
+A further reason to support removal of ```prev_content``` is the case where a
+malicious user adds illegal or abusive content into a state event and then
+overwrites that state event. The content would then be preserved through the
+redaction.
+
+Additionally, there are plenty of reasons to have security concerns over a
+precedent that the federation can deviate from the spec.
+
+## Conclusions
+Removing ```prev_content``` is pragmatic response to the current situation. It
+alligns the federation and the spec, and does so in a way that removes
+unecessary overhead.