Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #2644base: old_master
Are you sure you want to change the base?
MSC2644:
matrix.to
URI syntax v2 #2644Changes from 9 commits
383e6a2
57041b5
6d8c4cf
9c93c40
cccdcd5
b18ab99
bb13132
dd7b714
200c4f1
6c98dfe
4a7babe
700e5da
88c7716
686de48
3a2fa9f
f8c4f51
b5d179f
a0861d6
af54537
23494ca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 standardiseaction=join
; andaction=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 usingaction
but leave the details of the parameter as client-defined (the downside of this alternative being that different clients may produceaction
values with conflicting meanings).There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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 usingvia
params that will not work from a federating account on some different server than the room.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, thevia
list will contain exactly one entry - the unfederated server (if the current algorithm of constructing it is followed).There was a problem hiding this comment.
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 usingvia=<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:There was a problem hiding this comment.
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 includevia=<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 includingvia
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 usingvia
params that will not work from a federating account on some different server than the room.There was a problem hiding this comment.
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 probablyfederated=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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 passvia
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.There was a problem hiding this comment.
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
andsharer
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.There was a problem hiding this comment.
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 certainsharer
string. If this piece of information leaked, it's leaked.