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

MSC2781: Remove the reply fallbacks from the specification #2781

Merged
merged 27 commits into from
Nov 5, 2024
+279 −0
Merged

MSC2781: Remove the reply fallbacks from the specification #2781

Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6868738
MSC2781: Down with the fallbacks
deepbluev7 Sep 18, 2020
8d8b38d
Add a note about dropping the html requirement
deepbluev7 Sep 18, 2020
94cffdc
Add an unstable prefix for removed fallbacks.
deepbluev7 Sep 18, 2020
18a2732
Add a section about fallbacks not being properly specified.
deepbluev7 Sep 18, 2020
8e7c44b
Add appendix about which clients do not support replies (and why, if …
deepbluev7 Oct 1, 2020
ff76870
Correct weechat status
deepbluev7 Oct 1, 2020
74dc8d8
Add another alternative
deepbluev7 May 10, 2021
c8e4377
Document a few more issues with fallbacks
deepbluev7 May 10, 2021
7caeb62
Update client status, remove proposal for edits and try to turn down …
deepbluev7 Jan 14, 2022
1539dcd
Remove mistaken reference to the Qt renderer
deepbluev7 Jan 14, 2022
98df79d
Try to make motivation a bit clearer in the proposal
deepbluev7 Jan 14, 2022
b05e0a2
How do anchors work?
deepbluev7 Jan 14, 2022
60c33c3
Drop reference to issues with edit fallbacks
deepbluev7 Jan 14, 2022
838f0ce
Typos
deepbluev7 Jan 14, 2022
2380162
Address review comments
deepbluev7 Jan 19, 2022
c604999
More edits
deepbluev7 Jan 19, 2022
2a55bc1
Implementation traps
deepbluev7 Jan 19, 2022
892664b
Apply suggestions from code review
deepbluev7 Jan 20, 2022
52fd929
Add dates to client status list
deepbluev7 Jan 20, 2022
6983fde
Mention pushrules proposal in the alternatives section
deepbluev7 Jan 21, 2022
1d18857
Update proposal to 2024
deepbluev7 Jun 2, 2024
cbeb4e0
Be explicit about removal
deepbluev7 Jun 2, 2024
a66f24d
Apply suggestions from code review
deepbluev7 Jun 25, 2024
23dd267
Apply suggestions from code review
deepbluev7 Jun 25, 2024
1ad224e
Update proposals/2781-down-with-the-fallbacks.md
deepbluev7 Jun 25, 2024
2da1ac7
Apply suggestions from code review
deepbluev7 Oct 1, 2024
5c9de13
Simplify wording around invalid html and potential issues
deepbluev7 Oct 1, 2024
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
265 changes: 265 additions & 0 deletions proposals/2781-down-with-the-fallbacks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
# MSC2781: Remove reply fallbacks from the specification

Currently replies require clients to send and parse a fallback representation
of the replied to message. While at least in theory fallbacks should make it
simpler to start supporting replies in a new client, they actually introduce a
lot of complexity and implementation issues and block a few valuable features.
This MSC proposes to deprecate and eventually remove those fallbacks. It was an
alternative to [MSC2589](https://github.com/matrix-org/matrix-doc/pull/2589),
which chose a different representation of the reply fallback, but was
ultimately closed in favor of this proposal.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

## Proposal

Remove the [rich reply fallback from the
Copy link
Member

Choose a reason for hiding this comment

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

Blocking concern for FCP finish: I don't think we can jump to removal, but we can deprecate and say "Clients SHOULD NOT send fallback", for example. A later MSC would be capable of fully removing the fallback.

The spec would read something like:

Clients SHOULD NOT send fallback representation, but SHOULD attempt to parse it intelligently when provided (detected by the presence of <mx-reply />). The fallback representation is as follows: [...]

Later, when removed and clients have had a chance to fully see the transition, an MSC to remove the fallback completely can be FCP'd. The spec after that change would likely just be a note block saying "There used to be a fallback representation, but not anymore. Click [here] to see it.".

This is all as required under the deprecation policy: https://spec.matrix.org/v1.10/#deprecation-policy

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 disagree, that a separate deprecation period is necessary. The behaviour was already made optional since version 1.3. As such clients should already be able to deal with not receiving a fallback. For that reason I think just mentioning the change in an INFO box should be sufficient.

For receiving clients there is no difference between a client, that stops sending a fallback because of a "SHOULD NOT" or the spec not mentioning that clients should or should not send a fallback at all. For the sending client this only adds confusion without any substantial benefit imo.

Also on the receiving end missing fallbacks need to be supported already today (since 1.3). As such there isn't really a change in behaviour, just the frequency of such events. Again, pointing out the change in an INFO box would be helpful, but a deprecation period would just add additional process overhead.

So to summarize, I don't think the deprecation policy applies here and to respect the time of everyone involved, I deem an additional MSC to then remove the behaviour as unnecessary. The actual implementation in the specification might be a bit more sophisticated, but I do believe sending reply fallbacks should be removed now.

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 you may be talking about two slightly different things here: it's a given that clients need to support them not being there. The question is whether the text of the next few versions of the spec needs to describe a thing that was previously in it but is now no longer recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbkr I think the spec should stop suggesting that reply fallbacks are used in replies. I think that should just be historical information as we do for historical mxids, m.room.aliases rX.Y.Z spec versions and unamespaced tags for example, since those can still appear and client authors need to be aware of that. I don't think any of those had a deprecation period either and some of those are arguably higher impact.

Copy link
Member

Choose a reason for hiding this comment

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

All of those examples are from a time before the deprecation policy was enacted in Matrix 1.1, fwiw. This MSC introduces a technically breaking change to clients, which necessitates deprecation first at a minimum. The spec instructs clients which do support rich replies on how to handle the fallback (strip it), and implies that the module is optional. By removing the fallback, we're saying that clients must support rich replies in order to maintain conversation context - something which is currently optional (and not even a SHOULD optional).

The MSC addresses the context piece a bit, but doesn't address the breaking change. As such, this needs to go through deprecation first to manage the breaking change, then removal at least 1 version later as per the policy. This does add additional overhead - feedback regarding that is best placed in a dedicated MSC which aims to change the deprecation policy.

Copy link
Member

Choose a reason for hiding this comment

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

This MSC introduces a technically breaking change to clients

..but it doesn't. Clients have to handle the fallback not being there. It does not meet the bar for a "breaking change".

I would strongly advocate a nuanced approach here, on a case-by-case basis (which honestly isn't hard given the amount of attention MSCs get). In this particular case, because the functionality was optional, there is little benefit to dragging this process out any longer than needed, particularly given the security impacts of reply fallbacks. Having them in the ecosystem any longer than necessary is just untenable, especially if it's just due to bureaucracy rather than any technical/security/social reasons.

Copy link
Member

Choose a reason for hiding this comment

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

This feels like it's become a circular argument with no conclusion. Other members of the SCT are encouraged to weigh in on this, as I'm unconvinced that this MSC can go through as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Since clients already need to handle the fallback not being there then I think it is fine to remove it with a historical reference to strip the content.

Copy link
Member

Choose a reason for hiding this comment

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

The question in my mind is: what are the benefits to the ecosystem by having a deprecation period? What would materially change if we marked these APIs as deprecated?

Naively I would have thought that the only action client authors would need to take was to ensure that they handled the absence of the fallback correctly. Given its has been optional since 1.3 this should be a no-op for client implementers.

However, having a deprecation policy will flag two things to client develops:

  1. They really do need to make sure they correctly handle replies without fallback. There is a difference between clients theoretically supporting lack of fallback and them actually supporting it.
  2. They may want to ensure that the UX without fallback is still good. This is a transition from being able to mostly be able to rely on there being a fallback most of the time, to there never being one.

Personally, I think if we're relatively confident that most clients already deal correctly with the above its OK to skip the deprecation period. I'm in no position to judge the client situation though.

Otherwise, the safest option would be to do a short deprecation period, though I would advocate for not requiring a new MSC and instead simply have this MSC say "fallback will be deprecated in the next release, and removed the release after (unless the SCT hear enough negative feedback to delay)". If push comes to shove we can always do a one-line MSC to change that.

In terms of getting this over the line, I'll try and poke internally to get a conclusion on this.

TL;DR: My instinct is let's skip if we're confident that the vast majority of the client ecosystem would not benefit from a deprecation period, but if not let's just do a quick deprecation period and slate the removal in this MSC.

Copy link
Member

Choose a reason for hiding this comment

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

Having talked this through: the SCT believes that we should skip the deprecation period for this one. This is mainly due to the fact that reply fallbacks are already optional and we believe all the clients that are going to implement handling of proper replies have already done so, so we there is no benefit to doing a deprecation period.

specification](https://spec.matrix.org/v1.1/client-server-api/#fallbacks-for-rich-replies).
Clients should stop sending them and should consider treating `<mx-reply>` parts
as either something to be unconditionally stripped or as something to be escaped
as invalid html. Clients may send replies without a formatted_body now using
arbitrary message events (not state events), which is currently still disallowed
by the
[specification](https://spec.matrix.org/v1.1/client-server-api/#rich-replies).

As a result of this, you would be able to reply with an image. New clients
would also be able to implement edits and replies more easily, as they can
sidestep a lot of pitfalls.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

Given clients have had enough time to implement replies completely, the
overall look & feel of replies should be unchanged or even improved by this
proposal.

An extended motivation is provided at [the end of this document](#user-content-appendix-b-issues-with-the-current-fallbacks).

## Potential issues

Obviously you can't remove the fallback from old events. As such clients would
still need to do something with them in the near future. I'd say just not
handling them in a special way should be okay after some unspecified period of
time.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

Clients not implementing rich replies or edits may show some slightly more
confusing messages to users as well. I'd argue though that in most cases, the
reply is close enough to the replied to message, that you would be able to guess
the correct context. Replies would also be somewhat easier to implement and
worst case, a client could very easily implement a little "this is a reply"
marker to at least mark replies visually. Not having a reply fallback might also
prompt some clients to implement support for rich replies sooner. Users will now
complain about no context. Previously there was some context for replies to text
messages, which made it easy to accept the solution as good enough. However the
experience with replies to images was not acceptable. Motivating clients to
implement rich replies is a good thing in the long run and will improve the
Matrix experience overall.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

You might not get notifications anymore for replies to your messages. This was
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
a feature of the reply fallback, because it included the username, but users had
no control over it. A follow-up MSC will propose a push rule for related events,
which will allow users control over getting notified by replies (and other
relations) or not.
richvdh marked this conversation as resolved.
Show resolved Hide resolved

## Alternatives

[MSC2589](https://github.com/matrix-org/matrix-doc/pull/2589): This adds the
reply text as an addional key. While this solves the parsing issues, it
doesn't address the other issues with fallbacks.

One could also just stick with the current fallbacks and make all clients pay
the cost for a small number of clients actually benefitting from them.

Lastly one could introduce an alternative relation type for replies without
fallback and deprecate the current relation type (since it does not fit the
[new format for relations](https://github.com/matrix-org/matrix-doc/pull/2674)
anyway). We could specify, that the server is supposed to send the replied_to
event in unsigned to the client, so that clients just need to stitch those two
events together, but don't need to fetch the replied_to event from the server.
It would make replies slightly harder to implement for clients, but it would be
simpler than what this MSC proposes.

## Security considerations

Removing the fallback from the spec may lead to issues, when clients experience
the fallback in old events. This should not add any security issues the
client didn't already have from interpreting untrusted html, though. In all
other cases this should **reduce** security issues (see
https://github.com/vector-im/element-web/releases/tag/v1.7.3 or the appendix for
examples).
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

## Appendix A: Support for rich replies in different clients

### Clients without rendering support for rich replies

Of the 23 clients listed in the [matrix client matrix](https://matrix.org/clients-matrix)
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
16 are listed as not supporting replies:

- Element Android: Relies on the reply fallback.
Copy link

Choose a reason for hiding this comment

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

To add to this, Element Android actually seems to completely blank the message if the rich reply is there, but not the reply fallback (new Beeper Android and some mautrix bridges have done this for example)

It also seems a little unlikely that this will be fixed element-hq/element-android#8460

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of my hopes with this MSC was to encourage clients to implement proper reply support. Element Android in particular has been a problem for me, since it sends invalid fallbacks in many cases. While I do agree, that it is unlikely this will actually be fixed in Element Android, considering how problematic the reply implementation in Element Android is, I don't know if that should really be a blocker on this MSC. Stripping out its current reply handling and implement this MSC is probably easier than making Element Android actually compliant with the current specification for replies (from before this MSC).

- Element iOS: [Does not support rich replies](https://github.com/vector-im/element-ios/issues/3517)
Copy link
Member

Choose a reason for hiding this comment

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

... and therefore relies on the reply fallback? So is the same as Element Android? or different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When that was written, yes. Although I didn't investigate more since I don't own any iOS device. But it seems Element iOS might have some support now based on element-hq/element-ios#6155 ?

- weechat-matrix: Actually has an [implementation](https://github.com/poljar/weechat-matrix/issues/86) to send replies although it seems to be [broken](https://github.com/poljar/weechat-matrix/issues/233). Doesn't render rich replies. Hard to implement because of the single socket implementation, but may be easier in the Rust version.
- Quaternion: [Blocked because of fallbacks](https://github.com/quotient-im/libQuotient/issues/245).
turt2live marked this conversation as resolved.
Show resolved Hide resolved
- matrixcli: [Doesn't support formatted messages](https://github.com/ahmedsaadxyzz/matrixcli/issues/10).
- Ditto Chat: [Seems to rely on the fallback](https://gitlab.com/ditto-chat/ditto/-/blob/main/mobile/scenes/chat/components/Html.tsx#L38)
- Mirage: Supports rich replies, but [doesn't strip the fallback correctly](https://github.com/mirukana/mirage/issues/89) and uses the fallback to render them.
- Nio: [Unsupported](https://github.com/niochat/nio/issues/85).
- Pattle: Client is not being developed anymore.
- Seaglass: Doesn't support rich replies, but is [unhappy with how the fallback looks](https://github.com/neilalexander/seaglass/issues/51)?
- Miitrix: Somewhat unlikely to support it, I guess?
- matrix-commander: No idea, but doesn't look like it.
- gotktrix: [Seems to rely on the reply fallback](https://github.com/diamondburned/gotktrix/blob/5f2783d633560421746a82aab71d4f7421e4b99c/internal/app/messageview/message/mcontent/text/html.go#L437)
- Hydrogen: [Seems to use the reply fallback](https://github.com/vector-im/hydrogen-web/blob/c3177b06bf9f760aac2bfd5039342422b7ec8bb4/doc/impl-thoughts/PENDING_REPLIES.md)
- kazv: Doesn't seem to support replies at all
- Syphon: [Uses the reply fallback in body](https://github.com/syphon-org/syphon/blob/fa44c5abe37bdd256a9cb61cbc8552e0e539cdce/lib/views/widgets/messages/message.dart#L368)

So in summary, 3/4 of the listed clients don't support replies. At least one
client doesn't support it because of the fallback (Quaternion). 3 of the command
line clients probably won't support replies, since they don't support formatted
messages and replies require html support for at least sending.

Only one client implemented rich replies in the last 1.5 years. Other clients
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
are either new in my list or didn't change their reply rendering. I would
appreciate to hear, why those client developers decided not to support rich
reply rendering and if dropping the reply fallback would be an issue for them.

Changes from 1.5 years ago:
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

- Fractal: [Seems to support replies now!](https://gitlab.gnome.org/GNOME/fractal/-/merge_requests/941)
- Commune: Seems to support rich reply rendering and style them very nicely.
- NeoChat: [Supports rich replies](https://invent.kde.org/network/neochat/-/blob/master/src/utils.h#L21)
- Cinny: [Seems to support rich replies](https://github.com/ajbura/cinny/blob/6ff339b552e242f6233abd86768bb2373b150f77/src/app/molecules/message/Message.jsx#L111)
- gomuks: [Strips the reply fallback](https://github.com/tulir/gomuks/blob/3510d223b2d765572bf2e97222f2f55d099119f0/ui/messages/html/parser.go#L361)
- Lots of other new clients!


### Testresults without fallback
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

So far I haven't found a client that completely breaks without the fallback.
All clients that support rendering rich replies don't break, when there is no
fallback according to my tests (at least Nheko, Element/Web, FluffyChat and
NeoChat were tested and some events without fallback are in #nheko:nheko.im and
I haven't heard of any breakage). Those clients just show the reply as normal
and otherwise seem to work completely fine as well. Element Android and Element
iOS just don't show what message was replied to. Other clients haven't been
tested by the author, but since the `content` of an event is untrusted, a client
should not break if there is no reply fallback. Otherwise this would be a
trivial abuse vector.


## Appendix B: Issues with the current fallbacks

This section was moved to the back of this MSC, because it is fairly long and
exhaustive. It lists all the issues the proposal author personally experienced
with fallbacks in their client and its interactions with the ecosystem.

### Stripping the fallback

To reply to a reply, a client needs to strip the existing fallback of the first
reply. Otherwise replies will just infinitely nest replies.
[While the spec doesn't necessarily require stripping the fallback in replies to replies (only for rendering)](https://spec.matrix.org/v1.1/client-server-api/#fallback-for-mtext-mnotice-and-unrecognised-message-types),
not doing so risks running into the event size limit, but more importantly, it
just leads to a bad experience for clients actually relying on the fallback.

Stripping the fallback is not trivial. Multiple implementations had bugs in
their fallback stripping logic. The edge cases are not covered in the
specification in detail and some clients have interpreted them differently.
Common mistakes include:

- Not stripping the fallback in body, which leads to a very long nested chain.
- Not dealing with mismatched `<mx-reply>` tags, which can look like you were
impersonating someone.

For the `body` extra attention needs to be paid to only strip lines starting
with `>` until the first empty line. Implementations either only stripped the
first line, stripped all lines starting with `>` until the first non empty line,
that does not start with `>` or stripped only the `formatted_body`. While those
are implementation bugs, they can't happen if you don't need to strip a
fallback.

### Creating a new fallback

To create a new fallback, a client needs to add untrusted html to its own
events. This is an easy attack vector to inject your own content into someone
elses reply. While this can be prevented with enough care, since Riot basically
had to fix this issue twice, it can be expected that other clients can also be
affected by this.

### Requirement of html for replies

The spec requires rich replies to have a fallback using html:

> Rich replies MUST have a format of org.matrix.custom.html and therefore a formatted_body alongside the body and appropriate msgtype.

This means you can't reply using only a `body` and you can't reply with an
image, since those don't have a `formatted_body` property currently. This means
a text only client, that doesn't want to display html, still needs to support
html anyway and that new features are blocked, because of fallbacks.

### Format is unreliable

While the spec says how a fallback "should" look, there are variations in use
which further complicates stripping the fallback or are common mistakes, when
emitting the fallback. Some variations include localizing the fallback,
missing suggested links or tags, using the body in replies to files or images
or using the display name instead of the matrix id.

As a result the experience in clients relying on the fallback or stripping the
fallback varies depending on the sending client.

### Replies leak history

A reply includes the `body` of another event. This means a reply to an event can
leak data to users, that joined this room at a later point, but shouldn't be
able to see the event because of visibility rules or encryption. While this
isn't a big issue, there is still an issue about it: https://github.com/matrix-org/matrix-doc/issues/1654

deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
Historically clients have also sometimes localized the fallbacks. In those cases
they leak the users language selection for their client, which may be personal
information.

### Using the unmodified fallback in clients and bridges

The above issues are minor, if reply fallbacks added sufficient value to
clients. Bridges usually try to bridge to native replies, so they need to
strip the reply fallback
(https://github.com/matrix-org/matrix-doc/issues/1541). Even the IRC bridge
seems to send a custom fallback, because the default fallback is not that
welcome to the IRC crowd, although the use cases for simple, text only bridges
is often touted as a good usecase for the fallback (sometimes even explicitly
mentioning bridging to IRC). As a result there are very few bridges, that
benefit from the fallback being present.

Some clients do choose not to implement rich reply rendering, but the experience
tends to not be ideal, especially in cases where you reply to an image and now
the user needs to guess, what image was being replied to.

As a result the fallbacks provide value to only a subset of the Matrix
ecosystem.

### Fallback increase integration work with new features
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

- [Edits explicitly mention](https://github.com/matrix-org/matrix-doc/pull/2676)
that a reply fallback should not be sent in the `m.new_content`. This causes
issues for clients relying on the fallback, because they won't show replies
once a message has been edited (see Element Android as a current example)
and similar edge cases.
- [Extensible events](https://github.com/matrix-org/matrix-doc/pull/1767)
require an update to the specification for fallbacks (because there is no
`body` or `formatted_body` anymore after the transition period).
[The current proposal](https://github.com/matrix-org/matrix-doc/pull/3644)
also intends to just drop the fallbacks in extensible events.

### Localization

Since the fallback is added as normal text into the message, it needs to be
localized for the receiving party to understand it. This however proves to be a
turt2live marked this conversation as resolved.
Show resolved Hide resolved
challenge, since users may switch languages freely in a room and it is not easy
to guess, which language was used in a short message. One could also use the
client's language, but that leaks the user's localization settings, which can be a
privacy concern and the other party may not speak that language. Alternatively a
client can just send english fallbacks, but that significantly worsens the
experience for casual users in non-english speaking countries. The specification
currently requires them to not be translated (although some clients don't follow
that), but not sending a fallback at all completely sidesteps the need for the
spec to specify that and clients relying on an english only fallback.

## Unstable prefix

Clients should use the prefix `im.nheko.msc2781.` for all their event types, if
they implement this MSC in a publicly available release or events may otherwise
bleed into public rooms.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved