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

MSC4027: Propose method of specifying custom images in reactions #4027

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
111 changes: 111 additions & 0 deletions proposals/4027-custom-images-in-reactions.md
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

@mscbot concern Needs to be reviewed against other custom emoji/sticker MSCs

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 in particular, this MSC sets up grammar for emoji shortcodes in Matrix, which we'll want to double-check aligns with whatever MSC defines sharable image packs.

@turt2live did you have anything else in mind? The rest is fairly reaction-specific IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Shortcodes and feature as a whole are conflicting, imo. Specific impact to be determined once the remainder of the stack is looked at :)

(I plan to do this over the next 2ish weeks, barring other commitments - will hand over ~next week if things go poorly on my side)

Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
# MSC4027 Custom Images in Reactions

One of the most desired features within the Matrix ecosystem is the ability to
react to messages with custom images. This feature is especially requested by
users who come from Slack and Discord where this functionality is one of the
main ways that the culture of a chat rooms develops.

There is an existing proposal to
[render image data in reactions (MSC3746)](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/),
but it has had little attention recently and has the flaw of not being conducive
to deduplication (either on the client or server). Sorunome proposed a
modification to that MSC to
[use the MXC URI as the key](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/files#r866285147)
which this proposal adopts.

This proposal is meant to replace
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/) and is
additionally intended to document the behaviour of existing clients and bridges.

Like
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/), this
MSC does not propose a mechanism for providing a list of available images.

## Proposal

This proposal suggests two changes to events with the `m.annotation` relation.

1. If the `key` of an `m.annotation` relation is an MXC URI of an image, clients
should render the referenced image instead of the key text.

Detecting if the `key` is an MXC URI can be as sophisticated as the client
wants, but this proposal recommends checking if the string starts with
`mxc://`.
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved

2. When the annotation's key is an MXC URI, a new (optional) `shortcode` key can
be added to the content of the event with a textual name for the image.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

This shortcode should be shown by clients in situations such as hovering over
the annotation, as alt-text, or if the client does not support rendering
images.

Example custom image reaction event content

```json
"content": {
"m.relates_to": {
"rel_type": "m.annotation",
"event_id": "$abcdefg",
"key": "mxc://matrix.org/VOczFYqjdGaUKNwkKsTjDwUa"
},
"shortcode": ":partyparrot:"
Copy link
Member

Choose a reason for hiding this comment

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

Should we enforce that shortcode's must begin and end with :? I think it'd be confusing to users if some clients started picking some other character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a "should" for that. It's not enforceable other than by convention.

Copy link
Member

Choose a reason for hiding this comment

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

A "must" snuck back in on line 33.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of the field is fairly easily enforceable by servers. We are already saying that they should verify the length. (Obviously if a rouge server doesn't validate these fields, we could have "bad" events come across federation.)

I think the question here is what part of the verification is "should" vs "must"? Right now, the servers "must" verify type and length of the shortcode, but the leading and trailing : is "should".

Maybe everything should be "must" and force servers to verify all three things, but leave it up to the server implementation if they want to locally redact invalid events that come across federation?

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 it would eliminate confusion in implementations for all three to be a MUST on the Client-Server API.

leave it up to the server implementation if they want to locally redact invalid events that come across federation?

I would say yes, though I imagine most implementations would not redact such events - instead just allowing any UI bugs in the client that would surface.

}
```

## Potential issues
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved

### Clients rendering the MXC URI as text

The biggest disadvantage is that clients that do not support rendering custom
reactions will render the MXC URI as text. However, this is already problematic
because many bridges and clients already support this MSC, and users likely
already encounter this.

### Un-renderable image referenced in the `key`

The MXC URI could specify an asset that either does not exist, or is not a
renderable image. Clients can opt to render the `shortcode` in these situations,
or some placeholder/error image, or just opt to render the full key.

## Alternatives

### Use the shortcode as the `key`

This is what was proposed by
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/). The
problem with this is that there could possibly be multiple distinct images with
the same shortcode. Reactions are only deduplicated based on `key`, so clients
and servers would group these distinct reactions together which is undesirable.

### Put the `shortcode` as a key within `m.relates_to`

Instead of being at the root of the `content` dictionary, the `shortcode` value
could be included within `m.relates_to`. This is the wrong place to put this
value because `m.relates_to` is meant to only contain information pertaining to
the relationship between events, not information about the reaction event
itself.

## Security considerations

### Image is unencrypted

Reaction events are not encrypted, and so the MXC URI referenced by the key
would have to be an unencrypted image. However, this is probably not a problem
for the following reasons:

- Custom reactions are most likely not sensitive information.

- Users are already able to upload unencrypted content into encrypted rooms, so
this does not introduce any leakage that was not previously possible.

- Clients can add UX to indicate to users that the reaction images are not
encrypted.

## Unstable prefix

Until this proposal is merged into the spec, the `shortcode` key should be
prefixed with `com.beeper.msc4027.`.

An unstable prefix for the `key` in `m.relates_to` is not necessary as the spec
already allows arbitrary data to be used as the `key`. This MSC merely adds
extra meaning to a specific class of key.