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

MSC2644: matrix.to URI syntax v2 #2644

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

Conversation

jryans
Copy link
Contributor

@jryans jryans commented Jun 19, 2020

@jryans jryans changed the title [WIP] MSCXXXX: matrix.to URI syntax v2 [WIP] MSC2644: matrix.to URI syntax v2 Jun 19, 2020
@turt2live turt2live marked this pull request as draft June 19, 2020 13:57
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal labels Jun 19, 2020
@jryans jryans changed the title [WIP] MSC2644: matrix.to URI syntax v2 MSC2644: matrix.to URI syntax v2 Jun 19, 2020
@jryans jryans marked this pull request as ready for review June 19, 2020 14:25
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

After the first quick read I like the direction but we really need to set the record straight on percent-encoding.

proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
* This parameter allows indicating whether a room exists on a federating
server (assumed to be the default), or if the client must connect via the
server identified by the room ID or event ID (when set to `false`)
* `sharer=<MXID>`
Copy link
Member

Choose a reason for hiding this comment

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

That seems similar to what Origin is in the HTTP world. Maybe name it similarly, too?

More importantly, I think it's an unnecessary restriction to use MXID for that. When a matrix.to link is shared from outside Matrix, the sharers might still want to identify themselves but not with an MXID. Can we allow a wider scope here?

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 reason for an MXID in particular is it would allow matrix.to to request profile info (avatar, display name) for the sharer and display that (assuming the user consents to such requests at all). I'll fold this extra info into the text.

Origin seems a bit technical and hides that it's a person who shared a thing, so I'm inclined to leave the name as-is. I also worry it would be easily confused with the various servers and vias flying around in the syntax.

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 explanation for MXID to the text, hopefully that clarifies.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I still wouldn't mind to have the same feature for non-Matrix sharers but if we want to make some kind of experience barrier between Matrix and non-Matrix then so be it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean... One possibility is to allow it to be more free form and generic, but also suggest MXIDs as the thing to put here if you have one. I'm curious what others thinks about this as well.

proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

After the second reading, I'm a bit less happy with the direction (but I still totally like the part about shortened event URLs). See below.

proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
> feedback on whether to define such a thing here, or in a separate MSC.

The ``<additional arguments>`` and the preceding question mark are optional and
only apply in certain circumstances:
Copy link
Member

Choose a reason for hiding this comment

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

action= is missing from this list. #2312 proposes to standardise action=join; and action=chat is reported as unspecced but used too. Alternatively, they can be left as client-specific; in that case I'd at least document the practice of using action but leave the details of the parameter as client-defined (the downside of this alternative being that different clients may produce action values with conflicting meanings).

proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
event IDs are now permitted in the identifier position, so URIs like
`https://matrix.to/#/%24event%3Aexample.org` are now acceptable
* Clients should prefer creating URIs with room aliases instead of room IDs
where possible, as it's more human-friendly and `via` params are not needed
Copy link
Member

Choose a reason for hiding this comment

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

That recommendation doesn't match the feature you introduced just before. First, the recommendation itself is, unfortunately, not universal - see element-hq/element-web#2925 for details. Second, if you link to an event with $, a random federating server is unlikely to find its room if it doesn't already know that room; so you'll have to pass via in the same way you do it for room ids. I'd probably just drop this recommendation because it's outside of this MSC's scope.

proposals/2644-matrix.to-v2.md Outdated Show resolved Hide resolved
reason, then that could become a burden to maintain over time. The flexibility
of accepting any URL as an identifier (and thus avoiding the need to register a
client in a centralised place) seems preferable and hopefully outweighs this
concern.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe using a rev-DNS naming as practised in many other areas throughout Matrix would still be better. With the level of maturity across the ecosystem, clients' download locations change frequently enough to become stale in permalinks.

A bigger issue though is that client and sharer set an unusual (and questionable) precedent where a Uniform Resource Locator describes not only the target location but also the locator's author. If uncontrolled, it can be a means for massive leaking of data people probably didn't want to share - specifically the connectivity of people across the social graph (imagine a reaction upon receiving a link with the original sharer because the next sharer didn't care to update it: "wait, where does he know this sharer from?"). And rewriting the sharing source every time the link is re-shared is really tedious; as soon as the link is copied as plain text, you can assume rewriting won't happen by default.


The new `sharer` parameter is not authenticated, so you could make it appear as
if someone had shared something they did not. It is currently assumed that this
is a minor concern.
Copy link
Member

Choose a reason for hiding this comment

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

Right, you can (at least in theory) distrust sharer but, as mentioned above, you cannot "be forced to forget" if you received a certain sharer string. If this piece of information leaked, it's leaked.

Comment on lines +110 to +112
* This parameter allows indicating whether a room exists on a federating
server (assumed to be the default), or if the client must connect via the
server identified by the room ID or event ID (when set to `false`)
Copy link
Member

Choose a reason for hiding this comment

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

By the definition of federation, you cannot rely on federating servers to identify a server of a room/event residing outside of federation. Seems that federated=false implies using via=<non-federating server> except quite specific situations. The next issue is with the requirement to connect via that server: worded in almost the same way in #2312, this approach faced controversy. To alleviate that, I propose the following:

Suggested change
* This parameter allows indicating whether a room exists on a federating
server (assumed to be the default), or if the client must connect via the
server identified by the room ID or event ID (when set to `false`)
server (assumed to be the default), or if the room/event is only reachable
through a specific server (when set to `false`). This server is usually
specified in a `via` parameter, unless the target client is expected to
be able to unambiguously resolve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, why would the federated=false case include via=<non-federating server>? I had been assuming for the non-federated case, the client effectively must connect to the only server that knows about the room, which is already present in the room ID, so there's not much purpose to including via as well for such a case. Is "unless the target client is expected to be able to unambiguously resolve it" trying to allow for this somehow...? It's vague enough that I am not sure what it really means... 😅

The goal I see with federated=false is it allows the client to know for certain that it must connect via an account on the only server that knows about that room, rather than attempting to do so using via params that will not work from a federating account on some different server than the room.

Copy link
Member

@KitsuneRal KitsuneRal Jul 7, 2020

Choose a reason for hiding this comment

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

Because a client can be exposed to more than one Matrix network? Actually, it just occurred to me that federated=false is not quite a good name for that reason. If you have one public federation and one closed federation (for testing, e.g.) then you actually have two federations, not one federation and an unfederated server :-\

And continuing to the point about using room IDs to find an unfederated server - with certain confidence this works if you have a single entirely unfederated server and it has never federated. But IIRC, Synapse already looks at the server signified in the room id in addition to the list of "via" servers, and we even have this behaviour specced somewhere - so probably federated=false is just extraneous then?

(Update: using the server from the room id doesn't seem to be specced anywhere but the algorithm of building the via= list will produce a trivial list with an unfederated server as the sole entry.)

The "able to unambiguously resolve" piece is, yes, an attempt to describe the situation when the link is shared with someone who most definitely will use it at the correct server. Feel free to rephrase :-D

@turt2live turt2live self-requested a review June 22, 2020 17:31
jryans and others added 3 commits June 23, 2020 18:20
Apply tweaks from review

Co-authored-by: Kitsune Ral <Kitsune-Ral@users.sf.net>
to a download / install page, such as:
* `foo.com`
* `apps.apple.com/app/foo/id1234`
* `play.google.com/store/apps/details?id=com.foo.client`
Copy link
Member

Choose a reason for hiding this comment

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

M suggested that matrix.to should scrape the Name & Icon from this to show to the user but play.google.com doesn't offer ACAO so wouldn't be possible without some server-side service.

Co-authored-by: Kitsune Ral <Kitsune-Ral@users.sf.net>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

overall this is looking well on the right track. Hopefully either this or the proper URI proposal can get through in a timely manner.

`example.org` when linking to a room or an event since room IDs are
not currently routable
* `client=<client URL>`
* This parameter allows clients to indicate which client shared the URI
Copy link
Member

Choose a reason for hiding this comment

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

would be good to spell out that this also gives users the option of downloading the client, particularly for use in the matrix.to fallback page (which is specifically specced to not exist, ironically).

* `foo.com`
* `apps.apple.com/app/foo/id1234`
* `play.google.com/store/apps/details?id=com.foo.client`
* `federated=false`
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? via indeed appears to communicate the intent, and if a client is using Matrix in a private environment it is going to be better off inventing its own permalink structure (for in-app linking rather than trying to tame matrix.to)

Copy link
Contributor Author

@jryans jryans Jul 6, 2020

Choose a reason for hiding this comment

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

The goal I see with federated=false is it allows the client to know for certain that it must connect via an account on the only server that knows about that room, rather than attempting to do so using via params that will not work from a federating account on some different server than the room.

Copy link
Member

Choose a reason for hiding this comment

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

From a spec perspective I'm not sure we care about that. If the user received a link, they have a reasonable shot of going through to 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 author of the link may or may not supply via parts as necessary, basing on how the room is supposed to be reached. Since unfederated rooms by definition can only have unfederated accounts, the via list will contain exactly one entry - the unfederated server (if the current algorithm of constructing it is followed).

* This parameter allows indicating whether a room exists on a federating
server (assumed to be the default), or if the client must connect via the
server identified by the room ID or event ID (when set to `false`)
* `sharer=<MXID>`
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any abuse mitigation suggestions to provide to clients which handle matrix.to in-app (or to the matrix.to fallback page)? It's far too easy to manually craft this to use non-existent IDs which will easily get clients to show in giant header text the bare mxid. Or worse, a real mxid with all kinds of abuse being included in the profile response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I am not sure what we might offer in that regard... In seems like more of social problem to me, as in you should avoid clicking links from untrusted parties who might abuse things in this way. I can't think of technical solution, other than somehow signing the links, but that's quite heavy weight for this problem and aesthetically unsatisfying.

Comment on lines 121 to 122
If multiple ``<additional arguments>`` are present, they should be joined by `&`
characters, as in `https://matrix.to/!somewhere%3Aexample.org?via=example.org&via=alt.example.org`
Copy link
Member

Choose a reason for hiding this comment

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

Can we just say it's query string format under RFC whatever?

Copy link
Member

Choose a reason for hiding this comment

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

It would at least need the "as-if we were not inside the fragment already" caveat.

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 description of fragment in RFC 3986 seems quite lenient about which characters can be used. Why is caveat needed?

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 have at least added a mention of the RFC here.

proposals/2644-matrix.to-v2.md Show resolved Hide resolved
jryans and others added 5 commits July 23, 2020 17:25
@richvdh
Copy link
Member

richvdh commented Apr 6, 2021

What is the state of play with this MSC? Does it need updating to reflect the decisions made for matrix: URIs in MSC2312 ? Does it in fact want to be parked and superceded by matrix: URIs?

@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
@jryans
Copy link
Contributor Author

jryans commented Jun 10, 2021

What is the state of play with this MSC? Does it need updating to reflect the decisions made for matrix: URIs in MSC2312 ? Does it in fact want to be parked and superceded by matrix: URIs?

My understanding is this got a bit lost in the sands of time when we rewrote matrix.to, which went through a series of several approaches. I'm checking with @bwindels in matrix-org/matrix.to#205 (comment) to see if any of this was ever implemented in the end.

If not, it may be better at this point to discard this and let matrix: URIs lead the way forward.

@anoadragon453
Copy link
Member

Looking at matrix-org/matrix.to#205 (comment), it appears an implementation has not happened. Does that spell the end of the road for this MSC?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec 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