MSC2385: Disable URL Previews, alternative method #2385

Sorunome · 2019-12-08T15:53:25Z

This MSC provides an alternative method to MSC2376

Signed-off-by: Sorunome [email protected]

bmarty · 2019-12-09T13:59:52Z

Other potential issue: naughty clients could add some urls in url_previews that are not in the message, resulting in URL preview displaying bad content not related to the message.
Why not having just a list of booleans to avoid repeating the url in the message?

KitsuneRal · 2019-12-09T15:43:19Z

Other potential issue: naughty clients could add some urls in url_previews that are not in the message, resulting in URL preview displaying bad content not related to the message.
Why not having just a list of booleans to avoid repeating the url in the message?

Further on, please comment on a specific line of text so that discussions could be tracked more easily. As per your concern - as far as I see, the MSC states that URLs in url_previews must be exactly the URLs in the message body, so a boolean would be superfluous.

KitsuneRal

Barring a few wording fixes, I really like this approach.

proposals/2385-no-url-previews.md

Sorunome · 2019-12-09T16:10:51Z

Other potential issue: naughty clients could add some urls in url_previews that are not in the message, resulting in URL preview displaying bad content not related to the message.
Why not having just a list of booleans to avoid repeating the url in the message?

As stated in the MSC, url_previews is basically a whitelist, not a list of URLs to do previews for. So you still do URL previews like currently but additionally check if it is present in url_previews (if present). Thus additional URLs in that list would be just ignored.

dali99 · 2019-12-09T23:23:42Z

Honestly I kinda like the potential to hint clients to render "previews" for links not present in the message.

I believe facebook does this, where you can paste in a link, get a preview, remove the link and then just comment on the link being shared.

essentially turning the preview into more of a "rich link" part of the message.

bmarty · 2019-12-10T00:59:10Z

the MSC states that URLs in url_previews must be exactly the URLs in the message body

Yes, but what if it's not the case?

Anyway, maybe the pb is to avoid rendering useless url preview in the timeline, so maybe the solution is to stop displaying url preview in the timeline and display the preview in a popup when the link is hovered or when the link is clicked (the same way Google docs do it for instance).

Sorunome · 2019-12-10T10:06:39Z

the MSC states that URLs in url_previews must be exactly the URLs in the message body

Yes, but what if it's not the case?

It is supposed to be ignored, as stated in the MSC

If a URL is present in this key that is not present in the body clients SHOULD NOT render a preview for that URL.

KitsuneRal · 2019-12-10T17:59:23Z

Honestly I kinda like the potential to hint clients to render "previews" for links not present in the message.

I believe facebook does this, where you can paste in a link, get a preview, remove the link and then just comment on the link being shared.

essentially turning the preview into more of a "rich link" part of the message.

I would absolutely prefer such behaviour to be in a separate MSC. Matrix operates in a very different realm than Facebook; I could imagine such behaviour as somewhat natural in Mastodon but not in Matrix.

dali99 · 2019-12-10T19:00:51Z

Honestly I kinda like the potential to hint clients to render "previews" for links not present in the message.
I believe facebook does this, where you can paste in a link, get a preview, remove the link and then just comment on the link being shared.
essentially turning the preview into more of a "rich link" part of the message.

I would absolutely prefer such behaviour to be in a separate MSC. Matrix operates in a very different realm than Facebook; I could imagine such behaviour as somewhat natural in Mastodon but not in Matrix.

turt2live · 2020-02-05T12:28:02Z

proposals/2385-no-url-previews.md

+This proposal adds a new, optional key to `m.room.message` messages, named `url_previews`. It is an
+array of strings containing the URLs a preview should be given to. If this key is absent, it is expected
+that the client gives URL previews for all URLs present in the message body. If a URL is present in this key that is *not*
+present in the body clients SHOULD NOT render a preview for that URL.


in the body or formatted body?

(answering this may lead to further questions and concerns)

since all links in the formatted_body should be present in the body and vice-versa (as the body is a plain-text fallback) this should only matter for those messages that actively disobey that.

That being said, it seem to make most sense to use the links in whichever the client uses to render: if it renders formatted stuffs the formatted_body, if it renders plaintext the body

right, so the concern here would be that you can make a very convincing phishing attack: if it uses URLs only from body, your formatted body can be <a href="https://scam.com">https://example.org</a> with url_previews for https://example.org. Likewise, if it only looks at formatted bodies then the same attack can be applied.

Currently at least, clients will (or should) be showing a URL preview for the href rather than the text.

If a client uses body (including URLs in it, naturally) to render the message then formatted_body is irrelevant, whichever URLs it contains, and vice versa. url_previews is only used as a whitelist for URLs in whichever body the client renders. I don't quite see the surface for a phishing attack here: what you see is what you get, with a caveat that an unsuppressed URL preview for a phishy URL in formatted_body may help revealing the attack (at the very least, it won't conceal the attack), whereas if an attacker suppreses the URL preview then it's up to the user to check where that URL leads to before following it.

Sure, it's the responsibility of the user to check links before they click but in practice we shouldn't be designing into the protocol a requirement for people to do that.

The attack is quite literally:

{ "body": "https://example.org", "format": "org.matrix.custom.html", "formatted_body": "<a href="https://phishing.example.org">https://example.org</a>", "url_previews": ["https://example.org"] }

Which clients that don't read the spec to the letter would render as:

https://example.org

Example Domain
This is an example domain.

Instead, we could be better at protecting users from badly written clients and from having to be more responsible than us.

The fix for that is equally as trivial: As the client uses formatted_body to figure out URL previews, it is expected to figure out that the stuff in the href of the <a> tag should give the preview - not its content

Maybe it would be more clear if it was specified like the following:

A client may chose to provide URL previews for the user. For each link the client finds in the message the client must consult the url_previews attribute. If url_previews is absent all URLs are allowed to be previewed, otherwise if the attribute is a list contacting the link URL, the URL is present for previewing.

So it is clear that the client must not read url_previews and search for the URL in the body, but instead if in the process of rendering the body it finds a link it should consult url_previews. In your example the client would never consider https://example.org a link and therefore would never be eligible for a preview whether or not url_previews was absent or ["https://example.org"]

The consequence is that putting arbitrary URLs into url_previews accomplishes nothing unless the URL is actually going to be treated as a link by the client.

proposals/2385-no-url-previews.md

ShadowJonathan · 2021-03-23T11:42:27Z

This could help with clients exempting urls from the array by (for example) surrounding those urls with <>, on other clients, this disables that preview.

kevincox · 2021-03-26T00:26:03Z

proposals/2385-no-url-previews.md

+## Proposal
+This proposal adds a new, optional key to `m.room.message` messages, named `url_previews`. It is an
+array of strings containing the URLs a preview should be given to. If this key is absent, it is expected
+that the client gives URL previews for all URLs present in the message body. If a URL is present in this key that is *not*


We can't expect that clients give URL previews as this could be a security problem. We should say that clients MAY provide previews for all URLs in the body.

Good point. Also FYI url previews are proxied through your homeserver, so you don't actually have any security problem when generating those.

However, there is a problem: WWW URLs may return different things depending on other log-ins and the like, so the sender and the homeserver of the sender may not see the same content.

https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-media-r0-preview-url discussion on that is beyond this MSC. The top comment suggests to use "may" instead of "it is expected", which soru will incorporate into this MSC

But it reveals the URL to the home server. You can imagine a case where a malicious user sends an illegal url so that a user may get in trouble if the honeservers logs are checked. I agree it isn't the worst privacy flaw but I think it is worth letting clients make the choice and provide users with options rather than writing this section like it is expected.

The top comment suggests to use "may" instead of "it is expected", which soru will incorporate into this MSC

use "may" instead of "it is expected"

may means the clients still have a choice.

this MSC does not add any new locations where clients should display url previews, it is about limiting it in some messages. "Disable URL Previews".

Also there is a reason element disables URL previews in e2ee rooms by default.

I like that. I think it makes sense to focus on disallowing certain previews in this MSC and not sound like we are requiring any previews where they weren't required previously.

kevincox · 2021-03-26T00:30:42Z

proposals/2385-no-url-previews.md

+
+Instead of a whitelist there could also be a blacklist of URLs. However, as the likely scenario is a
+bot wanting to surpress all the previews for its URLs sent it seems like a whitelist is more appropriate
+here.


A whitelist also nicely prevents clients from parsing things that were not intended to be URLs and trying to preview them. Should mostly be relevant for text as html has a precise notion of what a link is but still nice to have.

kevincox · 2021-03-26T00:38:11Z

proposals/2385-no-url-previews.md

+This proposal adds a new, optional key to `m.room.message` messages, named `url_previews`. It is an
+array of strings containing the URLs a preview should be given to. If this key is absent, it is expected
+that the client gives URL previews for all URLs present in the message body. If a URL is present in this key that is *not*
+present in the body clients SHOULD NOT render a preview for that URL.


Maybe it would be more clear if it was specified like the following:

A client may chose to provide URL previews for the user. For each link the client finds in the message the client must consult the url_previews attribute. If url_previews is absent all URLs are allowed to be previewed, otherwise if the attribute is a list contacting the link URL, the URL is present for previewing.

So it is clear that the client must not read url_previews and search for the URL in the body, but instead if in the process of rendering the body it finds a link it should consult url_previews. In your example the client would never consider https://example.org a link and therefore would never be eligible for a preview whether or not url_previews was absent or ["https://example.org"]

The consequence is that putting arbitrary URLs into url_previews accomplishes nothing unless the URL is actually going to be treated as a link by the client.

kevincox · 2021-03-26T00:39:06Z

proposals/2385-no-url-previews.md

+## Potential issues
+URLs that should have a preview need to be sent twice - once in the body, once in `url_previews`. As
+this feature is meant for bots, normal human clients would probably omit `url_previews` alltogether,
+resulting in all URLs sent from them having a preview.


I see no reason a client can't display a list of links that it detects and allow the user to disable previews for some or all of them.

I must have missed this paragraph earlier. I agree that I don't see any reason why this should be intended mainly for bots and not for humans. There have been several occasions where I've typed a link and not wanted to have a preview because it causes extra noise (not just for rickrolling).

I, as a human, would have use for this every once in a while. Implementation examples of UI for this have been mentioned earlier (facebook, ...). I don't know if it should be added to the MSC, but since the point is to not send certain URLs to the HS, the sending UX should be constructed in such a way that, if at all, it is the client which generates the preview so that you can then decide to include it or not. The reasoning being that the sending user has (99% likely) already visited that URL and, if privacy is a concern, taken appropriate measures.

uhoreg · 2021-03-26T14:26:05Z

proposals/2385-no-url-previews.md

+
+## Potential issues
+URLs that should have a preview need to be sent twice - once in the body, once in `url_previews`. As
+this feature is meant for bots, normal human clients would probably omit `url_previews` alltogether,


Suggested change

this feature is meant for bots, normal human clients would probably omit `url_previews` alltogether,

this feature is meant for bots, normal human clients would probably omit `url_previews` altogether,

... because English is weird.

JuniorJPDJ · 2021-09-16T00:06:48Z

I was going to open issue about this then I found this MSC! Thanks for good ideas guys ;)

HarHarLinks

With all the remaining comments incorporated, I'd love to see this.

anoadragon453 · 2021-11-30T16:49:41Z

@Sorunome hello! If you don't have the time to respond to feedback on this MSC, you be willing to allow me to shepherd it?

anoadragon453 · 2021-11-30T17:04:03Z

This was added to the "Needs idea feedback" column on the SCT backlog board as I thought it could use a more recent review from a SCT member.

But in hindsight, this is mostly ready, and is just blocked on closing the existing threads of discussion.

So... taking off the board again until that's done.

anoadragon453 · 2021-11-30T18:24:44Z

If you don't have the time to respond to feedback on this MSC, you be willing to allow me to shepherd it?

I have received a "yes" out of band.

tulir · 2024-01-31T21:42:15Z

#4095 is an alternative for this, with the extra functionality of being able to bundle preview data (the bundling isn't mandatory though)

add no url preview MSC

58a72fa

Sorunome mentioned this pull request Dec 8, 2019

MSC2376: Disable URL Previews #2376

Closed

KitsuneRal approved these changes Dec 9, 2019

View reviewed changes

proposals/2385-no-url-previews.md Outdated Show resolved Hide resolved

proposals/2385-no-url-previews.md Outdated Show resolved Hide resolved

proposals/2385-no-url-previews.md Outdated Show resolved Hide resolved

proposals/2385-no-url-previews.md Outdated Show resolved Hide resolved

turt2live added proposal A matrix spec change proposal proposal-in-review labels Dec 9, 2019

turt2live self-requested a review December 9, 2019 15:58

anoadragon453 approved these changes Dec 9, 2019

View reviewed changes

proposals/2385-no-url-previews.md Outdated Show resolved Hide resolved

address small fixes

0a989d5

turt2live reviewed Feb 5, 2020

View reviewed changes

turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020

hungrymonkey mentioned this pull request Jul 30, 2020

Support request-side hint for disabling url preview for API's m.room.message matrix-org/matrix-spec#1588

Closed

tulir mentioned this pull request Dec 7, 2020

Url preview element-hq/element-android#2490

Merged

2 tasks

tulir mentioned this pull request Dec 26, 2020

Allow message sender to suggest disabling URL previews element-hq/element-android#2584

Open

erkinalp suggested changes Mar 22, 2021

View reviewed changes

proposals/2385-no-url-previews.md Show resolved Hide resolved

ShadowJonathan approved these changes Mar 23, 2021

View reviewed changes

kevincox approved these changes Mar 26, 2021

View reviewed changes

uhoreg reviewed Mar 26, 2021

View reviewed changes

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

turt2live force-pushed the old_master branch from e895827 to dca99ee Compare August 30, 2021 22:34

HarHarLinks approved these changes Nov 17, 2021

View reviewed changes

uhoreg mentioned this pull request Feb 5, 2021

Ability to suggest whether inline previews (url or thumbnails?) are intended to be shown for a given event matrix-org/matrix-spec#221

Open

turt2live removed the proposal-in-review label May 5, 2022

	this feature is meant for bots, normal human clients would probably omit `url_previews` alltogether,
	this feature is meant for bots, normal human clients would probably omit `url_previews` altogether,

MSC2385: Disable URL Previews, alternative method #2385

Are you sure you want to change the base?

MSC2385: Disable URL Previews, alternative method #2385

Conversation

Sorunome commented Dec 8, 2019 • edited by anoadragon453 Loading

bmarty commented Dec 9, 2019

KitsuneRal commented Dec 9, 2019

KitsuneRal left a comment

Choose a reason for hiding this comment

Sorunome commented Dec 9, 2019

dali99 commented Dec 9, 2019

bmarty commented Dec 10, 2019

Sorunome commented Dec 10, 2019

KitsuneRal commented Dec 10, 2019

dali99 commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowJonathan commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevincox Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuniorJPDJ commented Sep 16, 2021

HarHarLinks left a comment

Choose a reason for hiding this comment

anoadragon453 commented Nov 30, 2021

anoadragon453 commented Nov 30, 2021

anoadragon453 commented Nov 30, 2021

tulir commented Jan 31, 2024

Sorunome commented Dec 8, 2019 •

edited by anoadragon453

Loading

kevincox Mar 27, 2021 •

edited

Loading