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

MSC2499: Fixes for Well-known URIs #2499

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

Conversation

aaronraimist
Copy link
Contributor

@aaronraimist aaronraimist commented Apr 13, 2020

Rendered

Intended to address:
https://github.com/matrix-org/matrix-doc/issues/1919
https://github.com/matrix-org/matrix-doc/issues/1896
https://github.com/matrix-org/matrix-doc/issues/2335

If there are any other issues with the client well-known that I've missed let me know so there doesn't need to be a followup MSC.

@aaronraimist aaronraimist changed the title MSCXXXX: Fixes for Client Well-known URI MSC2499: Fixes for Client Well-known URI Apr 13, 2020
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@uhoreg uhoreg added the proposal A matrix spec change proposal label Apr 14, 2020
@turt2live turt2live added proposal-in-review kind:maintenance MSC which clarifies/updates existing spec labels Apr 14, 2020
@turt2live turt2live self-requested a review April 27, 2020 23:38
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.

Generally ok except one small addition, please.

changed from `FAIL_PROMPT` to `IGNORE` so that a non 200 response is treated in the same
way as 404. This change is intended to fix issues like https://github.com/vector-im/riot-web/issues/7875.

This change does have potential security concerns, see https://github.com/vector-im/riot-web/issues/11136.
Copy link
Member

Choose a reason for hiding this comment

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

In relation to this issue, I think it's worth clarifying in the spec that as long as a fully qualified MXID is supplied and the .well-known lookup fails for whatever reason, the client MUST NOT use whichever default server it might have. IGNORE description in the spec isn't quite clear on that; actually, it almost encourages the behaviour described in the issue by saying that the client can use "default values" to fill in required parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what, actually reading the spec again maybe FAIL_PROMPT is the correct thing to do but Riot just isn't doing that. Riot is doing something closer to a FAIL_ERROR, rather than a FAIL_PROMPT. I'm thinking IGNORE should just be removed from the spec completely and 3a and 3b should be a FAIL_PROMPT.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't be so harsh on IGNORE - it allows the user to go without an unneeded roadbump in 3a in case the client managed to get the clear message from the server that there's no .well-known support here. The IGNORE description is too affording though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it would be nice if there was no extra step but if using defaults values is removed from IGNORE then how will the client find out the server URL? The only way is to prompt the user unless I am missing something. So it would be the same as FAIL_PROMPT.

Copy link
Member

Choose a reason for hiding this comment

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

Probably the spec should be clear about the scope of cases for auto-discovery. Specifically, if the user entered a fully qualified MXID, only then auto-discovery kicks in. Using the default value in a situation when the serverpart is already provided but the server happens to have .well-known not quite ideally configured (i.e. step 3b) is definitely the wrong way out. Asking the user to confirm that the server is correct because auto-discovery fell short is the right way but both IGNORE and FAIL_PROMPT speak about that anyway.

Copy link
Member

Choose a reason for hiding this comment

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

FAIL_PROMPT is probably still correct, I think. Element (then Riot) likely has a bug in this area, though its UX doesn't really lend itself to "prompting" the user. It should probably offer the ability to continue anyways, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll take this out.

@aaronraimist aaronraimist changed the title MSC2499: Fixes for Client Well-known URI MSC2499: Fixes for Well-known URIs Apr 10, 2021
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@turt2live turt2live self-requested a review May 4, 2021 15:59
proposals/2499-client-well-known-fixes.md Outdated Show resolved Hide resolved
changed from `FAIL_PROMPT` to `IGNORE` so that a non 200 response is treated in the same
way as 404. This change is intended to fix issues like https://github.com/vector-im/riot-web/issues/7875.

This change does have potential security concerns, see https://github.com/vector-im/riot-web/issues/11136.
Copy link
Member

Choose a reason for hiding this comment

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

FAIL_PROMPT is probably still correct, I think. Element (then Riot) likely has a bug in this area, though its UX doesn't really lend itself to "prompting" the user. It should probably offer the ability to continue anyways, though.

Comment on lines 37 to 38
1. The maximum size of size of the well-known file is 51200 bytes. A client or server
requesting a well-known file MUST abort and FAIL_PROMPT if the response exceeds 51200 bytes.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a super realistic thing that can be applied by clients: if the server admin has configured the server so badly that a 10gb JSON file is being served then so be it. It's just not in the server admin's interest to attack bandwidth in this way given the costs they would be incurring from their ISP/service provider.

Copy link
Member

Choose a reason for hiding this comment

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

At least for native clients (and servers) it's fairly easy to abort on 51201st byte but FAIL_PROMPT looks a bit dubious here: should the client re-request .well-known or rather just continue (rather than abort) receiving the payload after confirming with the user? And yes, generally there's no incentive to attack in that way of course; but a recommendation to protect receiving sides from such misconfigurations wouldn't hurt, would it? Maybe not as MUST but as SHOULD.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that this endpoint specifically needs this recommendation though: surely we should be applying this to all endpoints like profiles, sync, etc...

It feels like another MSC's problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine, I can leave it to another MSC. The main reason for including it here is because Synapse has already implemented this limit for the server well-known so a well-known bigger than that already isn't going to work with most servers. If it was specced then at least people could expect there is some reasonable limit on the size.

Copy link
Member

Choose a reason for hiding this comment

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

I would be a bit surprised if people expected 1mb json files to work for this use case, honestly. I think it's reasonable for clients/servers to impose their own limits, and iirc Synapse only really limited the JSON file size on server-server because federation has tiny timeouts (typically), so read delays are not ideal.

aaronraimist and others added 3 commits May 6, 2021 13:49
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Co-authored-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@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
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist aaronraimist changed the base branch from old_master to main December 28, 2021 18:15
Co-authored-by: Vladimir Panteleev <CyberShadow@users.noreply.github.com>
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