-
Notifications
You must be signed in to change notification settings - Fork 382
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
MSC1700: Improving .well-known discovery #1700
Conversation
This looks very very sane to me. |
Parsing a hostname isn't an insane feat, however that doesn't change the argument regarding port usage within matrix.
I would probably avoid changing what port is used when a port is present in the server_name since this would be something any existing impls would have to go back & change, etc. All for simplifying the other steps though. |
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.
Overall, I think the proposal lacks any substential material/code to illustrate its points and should focus on that part first.
developers, without sacrificing critical functionality. | ||
|
||
In general, the process should be shortened to two steps: get the JSON and use it if possible. Currently the | ||
process employs a lot of verification methods that don't seem to prevent anything. |
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.
Many of those verification steps/methods are actually part of most SDK or libraries, making them available and used by default.
The current process is also having steps in an order which allow for their UX states to be "grouped", ensuring you actually don't need to discern them if your coding language/tools allow you to bundle them up.
Not all languages/tools/SDKs/libraries give you that high-level access and therefore the exact steps need to be written down to ensure it is not language specific. This is the same principle which is used in the S2S spec for state resolution - if you were to actually implement it, many steps would actually be grouped together. But that doesn't mean the algorithm shouldn't be documented or guidance not given for implementations.
I think this strips away the needed algorithm which is agnostic of language, SDK or tools and promotes high-level languages like Javascript at the expense of low(er)-level languages like Java, C(++), Go, etc.
I would suggest to give advises for languages on what can and cannot be "bunched up" instead.
A lot of the validation here doesn't need to be done for the following reasons. In general, the validation steps make | ||
it difficult for libraries to provide the functionality to support their UI counterparts. By reducing the amount | ||
of validation required, an expressive UI is still possible while also making it easier for automated clients and | ||
libraries to use the functionality. |
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 think this should be demonstrated with code, showing both implementations (the current spec one, and this MSC one). At this point, this is mere speculation.
If code was produced following the current spec, with notes/comments on how the various elements are not working, it would give the occasion for the community to actually review the code to ensure the problem is with the specification itself instead of:
- The approach used by the person writting the code
- The framework used (matrix-react-sdk)
- The libraries used (react, etc.)
- The language used (Javascript)
On the other hand, the algorithm is implemented in the Matrix Java SDK and used by at least two clients, Palaver and SimpleMatrix - This means that so far we have conflicting evidences, not allowing us to know if a problem exists and if yes, where.
|
||
The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what | ||
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten | ||
rule where the client should be verifying the information is appropriate for its own purpose should come into |
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 think letting anyone decide what qualifies as a "valid URL" instead of simply clarifying what "valid" means is very extreme in the approach. Clarifying the term valid
would fit the issue and achieve the same goal of being "helpful for the people having to implement this".
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 a hard time defining what would constitute a 'valid URL' in this case. Do you have a suggestion of what a clarified valid
would look like, @maxidor?
I don't really get much further than: ASCII not containing any unsafe characters.
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.
@mvgorcum See the comment just below.
there is an appropriate `m.homeserver` configuration in the response. | ||
|
||
The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what | ||
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten |
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.
rule where the client should be verifying the information is appropriate for its own purpose should come into | ||
effect. This also allows application-specific clients to verify the URL independently of the specification, or | ||
not at all if it chooses to. The risk of an invalid URL here is relatively minimal, as a responsible client would | ||
be communicating to the user what the URL it is about to use is for the user to verify. |
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.
Since there is no concept of showing the URL about to be used in the current spec, is this effectively a strong recommendation, or a requirement?
URL. The URL was given to the client for the sole purpose of assisting the user into the system, and is unlikely | ||
to be wrong. If it were wrong, the administrator would be more than likely interested in fixing it for their users. | ||
In the event of a security breach where the .well-known configuration is changed to point at a malicious party, | ||
the client's responsibility to show which URL it is about to connect to comes into play. |
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.
No such thing as "responsibility" or "responsible client" is ever defined in the specification. What is the expectation here?
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten | ||
rule where the client should be verifying the information is appropriate for its own purpose should come into | ||
effect. This also allows application-specific clients to verify the URL independently of the specification, or | ||
not at all if it chooses to. The risk of an invalid URL here is relatively minimal, as a responsible client would |
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 leeway given in "application-specific" behaviour allowing bypass of the check and requirement for HTTPS?
The first check removed is checking whether the `base_url` is actually a URL. There is no definition as to what | ||
makes a valid URL, which is completely unhelpful for the people having to implement this. Instead, the unwritten | ||
rule where the client should be verifying the information is appropriate for its own purpose should come into | ||
effect. This also allows application-specific clients to verify the URL independently of the specification, or |
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.
If clients can choose to verify a URL independently of the specification, is there a point to put any kind of verification system in the specification?
the client's responsibility to show which URL it is about to connect to comes into play. | ||
|
||
Further to the point of not having to verify if a Matrix server exists on the endpoint, the risk of a conflicting | ||
service is minimal. There are not many services out there that use both "matrix" and "m.homeserver" to do .well-known |
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.
This was never given as a reason for validating the .well-known data.
|
||
Also for reference, here is the last step of the discovery process (the validation): | ||
|
||
> 3. Make a `GET` request to `https://hostname/.well-known/matrix/client`. |
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.
this is illegible in the rendered version.
I'm closing this until I have the motivation to rewrite it. |
@turt2live Just found this spec, thanks for writing it. As someone bitten by the missing 30* support in the spec I'd still very much like to see this proposal happen. |
Rendered
Note: This is a summary of my thoughts on the matter of .well-known discovery validation.