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

MSC2723: Forwarded message metadata #2723

Open
wants to merge 7 commits into
base: old_master
Choose a base branch
from

Conversation

hummlbach
Copy link

@hummlbach hummlbach commented Aug 12, 2020

Signed-off-by: Johannes Renkl hummlbach@posteo.de

A proposal to properly support marking forwards as what they are.

Thx to @Sorunome and @krille for getting things straight.

@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Aug 12, 2020
@turt2live
Copy link
Member

@hummlbach when you get the chance, please sign off on the changes and line wrap them for review.

@turt2live turt2live changed the title Proposal forward info MSC2723: Forwarded message metadata Aug 12, 2020
@hummlbach
Copy link
Author

Okay, will do.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

(part one of review - github ate my review comments)

1. The forwarder adds `displayname` and `avatar_url` to `m.forwarded`.
2. The receiving client resolves the `displayname` and the `avatar_url` from the user id given by `sender` using `/_matrix/client/r0/profile/{userId}`.

Both solutions have a drawback. In case of 1., changing the display name or the avatar would not be reflected in forwards. And the avatar URL may even become invalid(?). The second solution may cause confusion is the original sender has set different display names and avatars for different rooms. I.e. if the original sender is in the room where the message is forwarded to, it may appear there under a different display name and avatar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the gravest drawback is you can forge the identity of the sender entirely and there isn't much you can do to verify it if you aren't in the room. Given there is huge scope for abuse in writing nasty messages in a private room that you own, and then forwarding them as @alice:example.com, we should be careful there.

The one thing clients could do is highlight the unreliable-ness of the information, but I worry that's a pretty weak defense.

"sender": "@someone:example.org",
"origin_server_ts": 1432735824141
},
"msgtype": "m.location"
Copy link
Contributor

Choose a reason for hiding this comment

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

personal nit: probably would be better to use a simple m.text to explain this, as the m.location format adds additional complexity to the example.

Both solutions have a drawback. In case of 1., changing the display name or the avatar would not be reflected in forwards. And the avatar URL may even become invalid(?). The second solution may cause confusion is the original sender has set different display names and avatars for different rooms. I.e. if the original sender is in the room where the message is forwarded to, it may appear there under a different display name and avatar.

### Clients can fake forwards
Should we care of/can we avoid "fake forwards"? Does it make sense/is it possible at all to only add the original `event_id` when sending a forward and assign the server the responsibility of adding the forward information?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're going to have to accept that fake forwards will exist in the federation and there is little you can do to avoid them, and the onus should be on the recipient to verify what it can. Even if the server is responsible for adding the metadata, the servers can lie just as easily as a client.

### Clients can fake forwards
Should we care of/can we avoid "fake forwards"? Does it make sense/is it possible at all to only add the original `event_id` when sending a forward and assign the server the responsibility of adding the forward information?

## Alternatives
Copy link
Member

@tulir tulir Aug 12, 2020

Choose a reason for hiding this comment

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

Has including the whole federation event data been considered (i.e. include the signatures and everything)? Having the signatures and exact content would allow the recipient servers to validate the event and thus prevent faking forwards.

The only drawbacks I can think of are:

  • it needs server support to send and validate forwards
  • forwarding big events (close to the 64 KiB federation limit) wouldn't be possible

I might be missing something, but even if it's not a viable option, the reasoning should be documented.

Copy link
Author

Choose a reason for hiding this comment

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

No, we haven't considered that yet (at least not me).

Copy link
Contributor

Choose a reason for hiding this comment

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

Then the client would need to verify the federation signatures, though, can it even do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The server could verify that the signatures / content of the included event were signed by the homeserver that originally sent it. The homeserver could include a key in unsigned to notify the client about it's trustworthyness.

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and drafted something for this: #2730

Choose a reason for hiding this comment

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

The problem with forwarding the whole event with signatures is that it becomes impossible to redact data that should not be shared. This means that we are committing to including all data covered by the signature in a forward which may be more information than was intended to be shared.

- fixed typos
- fixed title
- removed dead link
- removed obsolete line regaring event_id
- line wrap
@@ -0,0 +1,116 @@
# MSC2723: Forwardes message metadata

Choose a reason for hiding this comment

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

Suggested change
# MSC2723: Forwardes message metadata
# MSC2723: Forwarded message metadata

"geo_uri": "geo:51.5008,0.1247",
"m.forwarded": {
"event_id": "$123275682943PhrSn:example.org",
"room_id": "!jEsTZKDJdhfrheTzSU:example.org",

Choose a reason for hiding this comment

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

In some cases rooms are "private by knowledge" in this case sharing the room_id may inadvertently let point join. This has to be considered carefully and I think this MSC should at least include a warning that implementing clients should warn users.

### Clients can fake forwards
Should we care of/can we avoid "fake forwards"? Does it make sense/is it possible at all to only add the original `event_id` when sending a forward and assign the server the responsibility of adding the forward information?

## Alternatives

Choose a reason for hiding this comment

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

The problem with forwarding the whole event with signatures is that it becomes impossible to redact data that should not be shared. This means that we are committing to including all data covered by the signature in a forward which may be more information than was intended to be shared.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@shoqvalue
Copy link

I can't tell what the status of this is now, but I have problems with this issue every day. It creates a situation where someone forwards something by someone else, and everyone thinks they said it. Why not at least let the user caption their forwarded message. Or just say "shoq forwarded this" automatically. That doesn't require anything but what you have now with a caption added and solves most of the problem. It doesn't attempt a to find a perfect forward solution, just something that helps stop misattribution and misunderstandings. It's quite disruptive to the chat to have to backtrack and say "hey, I didn't write that, I just forwarded it."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants