well_known m.homeserver path has errant trailing "/" which breaks discovery in clients such as Pattle

[khimaros]

Description

Matrix clients such as Pattle honor the output of /.well-known/matrix/client literally.

A block in synapse/config/server modifies the value of public_baseurl by appending a / if one is not already present. The value of public_baseurl is returned unmodified by synapse/rest/well_known which causes the errant trailing / to reach the user. By way of example:

{"m.identity_server": {"base_url": "https://myhome.server"}, "m.homeserver": {"base_url": "https://myhome.server/"}}

Steps to reproduce

• configure a homeserver
• set public_baseurl to https://myhome.server or equiv.
• use Pattle to log into that homeserver (configure server in Advanced)

Pattle simply appends /_matrix to the URL returned in m.homeserver which results in requests to the Matrix server of the form:

2019-08-05 17:32:34,571 - synapse.http.server - 79 - INFO - GET-77- <XForwardedForRequest at 0x7f70fdbce240 method='GET' uri='//_matrix/client/r0/register/available?username=myuser' clientproto='HTTP/1.0' site=8008> SynapseError: 400 - Unrecognized request

synapse refuses to handle these requests due to the double //

Removing all three lines in this block appears to have no ill effect on Riot Android or Riot Web and fixes the discovery issue for Pattle.

Version information

All versions, including HEAD.

[turt2live]

It's not specified to include or not include the trailing slash. The reason for riot-web (and probably the other riots) not exploding is https://github.com/matrix-org/matrix-js-sdk/blob/35f1cdf89c26a0a6567e8983a03a178d0b198b49/src/autodiscovery.js#L462-L464

I'd personally recommend that Pattle do the same, as Synapse is not the only producer of well-known files.

[khimaros]

@wilkomanger who is the author of Pattle.

[khimaros]

Making the clients more robust seems reasonable to me.

Also, removing the trailing slash from the returned JSON in synapse still seems like a good move. It's a bit odd that the trailing slash is added to m.homeserver but not m.identityserver.

It is also the case that the canonical Matrix server https://matrix.org/.well-known/matrix/client returns a result without trailing slash for both keys.

[wilkomanger]

Thanks for notifying me, this should be handled correctly in Pattle anyway.

[anoadragon453]

Is this still a concern to anyone?

[richvdh]

I'm minded to think that synapse should be stripping trailing slashes rather than adding them, and ideally this should be mentioned in the spec.

[lub]

Maybe it makes sense to just normalize multiple slashes instead? At least in Linux land it's common practice to handle multiple slashes as single slash, exactly to prevent issues when two parts of a path both specify a trailing and a preceding slash

For example https://example.com/ +/_matrix/client/ + /r0/login is https://example.com//_matrix/client//r0/login and could be normalized to https://example.com/_matrix/client/r0/login before handling the request

[worldwidemv]

This is also an issue for sailtrix, a Sailfish OS Matrix client.

The ticket for this issue on the sailtrix side.

[src-r-r]

I think a better way to do this would be using urllib.parse.urlparse to ensure canonicalization.

[richvdh]

I think the spec should be explicit about expected behaviour here: matrix-org/matrix-spec-proposals#3465

[callahad]

Per discussion on the Matrix Spec matrix-org/matrix-spec-proposals#3465 (comment) we've ultimately decided that mandating either that trailing slashes are stripped or that they're added is fraught with peril, and that the only practically viable path forward is to explicitly instruct clients to handle both cases. This has now become part of the Matrix spec, rendering this issue moot.