comment out code that removes trailing slash as per 5823. #11224

src-r-r · 2021-10-31T21:45:44Z

Signed-off-by: Jordan Hewitt 633901+src-r-r@users.noreply.github.com

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Jordan Hewitt <633901+src-r-r@users.noreply.github.com>

callahad · 2021-10-31T21:57:13Z

We actually title newsfragments after the PR, not the issue the fix, so a git mv changelog.d/{5823,11224}.bugfix should get the CI back underway :)

src-r-r · 2021-10-31T23:27:06Z

Great, thank you!

dklimpel · 2021-11-01T09:53:20Z

I am not sure if modify the configuration is the right direction.
IMO it should be some logic in WellKnownBuilder to be sure that there is no trailing slash.

synapse/synapse/rest/well_known.py

Lines 31 to 47 in e320f5d

    
           class WellKnownBuilder: 
        
               def __init__(self, hs: "HomeServer"): 
        
                   self._config = hs.config 
        
               def get_well_known(self) -> Optional[JsonDict]: 
        
                   # if we don't have a public_baseurl, we can't help much here. 
        
                   if self._config.server.public_baseurl is None: 
        
                       return None 
        
                   result = {"m.homeserver": {"base_url": self._config.server.public_baseurl}} 
        
                   if self._config.registration.default_identity_server: 
        
                       result["m.identity_server"] = { 
        
                           "base_url": self._config.registration.default_identity_server 
        
                       } 
        
                   return result

src-r-r · 2021-11-01T13:20:28Z

I am not sure if modify the configuration is the right direction. IMO it should be some logic in WellKnownBuilder to be sure that there is no trailing slash.

synapse/synapse/rest/well_known.py

Lines 31 to 47 in e320f5d

class WellKnownBuilder:

def __init__(self, hs: "HomeServer"):

self._config = hs.config

def get_well_known(self) -> Optional[JsonDict]:

# if we don't have a public_baseurl, we can't help much here.

if self._config.server.public_baseurl is None:

return None

result = {"m.homeserver": {"base_url": self._config.server.public_baseurl}}

if self._config.registration.default_identity_server:

result["m.identity_server"] = {

"base_url": self._config.registration.default_identity_server

}

return result

Gotcha. I'll modify it then. I'm not doubting you; I'm just curious--what's the reasoning?

dklimpel · 2021-11-01T13:58:33Z

The reason is that public_baseurl is used many times in code. With your change the tests fail. You have to verify every position in code where public_baseurl is used if this change has an impact.
Perhaps there are places where the trailing slash is necessary.

synapse/config/server.py

richvdh · 2021-11-01T14:09:00Z

related to #5823

richvdh · 2021-11-01T19:08:36Z

I am not sure if modify the configuration is the right direction.
IMO it should be some logic in WellKnownBuilder to be sure that there is no trailing slash.

I'm not necessarily sure I agree with this: it seems a bit silly to explicitly add a trailing slash in read_config, and then strip it out again in WellKnownBuilder.

But for sure, if we change read_config in this way, we'll have to update the many places in the codebase that expect public_baseurl to have a trailing slash, which sounds like a lot of work.

…WW43_5823_well-known-path-has-errant-trailing-slash

…g-slash-2' into jordan_2021WW43_5823_well-known-path-has-errant-trailing-slash

…WW43_5823_well-known-path-has-errant-trailing-slash

ShadowJonathan · 2021-11-21T13:57:23Z

I think the commit history of this PR is a bit borked, i'd recommend looking at how to rebase your commits until the current main branch.

src-r-r · 2021-11-21T14:36:21Z

I think the commit history of this PR is a bit borked, i'd recommend looking at how to rebase your commits until the current main branch.

I did do a rebase with develop. Should I do a rebase with main?

ShadowJonathan · 2021-11-21T14:41:53Z

No, hold on, something else weird is happening here then

DMRobertson · 2021-11-22T16:04:43Z

This passed most checks, but the sytest integration tests aren't happy. The failures aren't particularly clear, regrettably

From a quick look, I don't think the needs_all call here succeeds.

https://github.com/matrix-org/sytest/blob/3125282b7ac6de20bc8b2bcf07ca663f33250131/tests/10apidoc/13ui-auth.pl#L140-L150

I can't see what the failure is though. Maybe we need more log_if_fail? Perhaps @richvdh could advise?

DMRobertson · 2021-11-23T16:54:26Z

I can't see what the failure is though.

After some more staring I saw this in the results.tap summary:

"https://localhost:8800/_matrix/client/r0/login/cas/ticket?redirectUrl=https%3A%2F%2Fclient%3Fp%3Dhttp%253A%252F%252Fserver%2F"
, expected
"https://localhost:8800/_matrix/client/r0/login/cas/ticket?redirectUrl=https%3A%2F%2Fclient%3Fp%3Dhttp%253A%252F%252Fserver"

which comes from here:

https://github.com/matrix-org/sytest/blob/3125282b7ac6de20bc8b2bcf07ca663f33250131/tests/10apidoc/13ui-auth.pl#L159-L161

Sytest is expecting that Synapse makes a request whoseservice query parameter matches $HS_URI. The later is defined here:

https://github.com/matrix-org/sytest/blob/3125282b7ac6de20bc8b2bcf07ca663f33250131/tests/10apidoc/13ui-auth.pl#L102

It sounds like we want to change either $REDIRECT_URL or $HS_URI to include a trailing slash. Please double check this---I'm not familiar with any of this.

Unfortunately this is all in sytest which is a separate repo. There is some configuration in CI to ensure we test synapse and sydent PRs against branches with the same name, if they exist. But I'm not sure if this is set up to work properly against forks.

DMRobertson

Thanks for bearing with us on this one.

There's quite a few things in these changes that I don't follow, and in particular some config handling code that seems to have been deleted. There might be some confusion between the server and client well-known endpoints too?

Additionally, I think it might be clearer in places to use str.rstrip to remove trailing slashes, rather than . (I sympathise with your comment on #5823 about urljoin --- I'd be fine with that too, but painful to implement it across the whole source tree.)

Taking a step back, the plan as I as I understand it is:

when ingesting config, remove any trailing slashes from the end of the URL.
keep the trimmed publicbase_url around in memory
when serving the .well-known/matrix/server file, use the trimmed version
fix up any remaining places which implicitly were expecting the baseurl config to end in a forward slash
- by adding an explicit '/' to the path we build

DMRobertson · 2021-11-26T15:31:26Z

changelog.d/11224.bugfix

@@ -0,0 +1 @@
+Remove trialing slashes form homeserver url so other services don't take it literally.


A few typos here: trialing -> trailing, form -> from, and I think url -> URL since the latter is an abbreviation.

I also think we can better explain the change from the user's perspective.

Suggested change

Remove trialing slashes form homeserver url so other services don't take it literally.

Ensure the URL served at `/.well-known/matrix/server/` does not end with a trailing slash, to workaround clients which expect this.

DMRobertson · 2021-11-26T15:40:18Z

synapse/config/server.py

-        # Uncomment the following to tell other servers to send federation traffic on
-        # port 443.
-        #
-        # By default, other servers will try to reach our server on port 8448, which can
-        # be inconvenient in some environments.
-        #
-        # Provided 'https://<server_name>/' on port 443 is routed to Synapse, this
-        # option configures Synapse to serve a file at
-        # 'https://<server_name>/.well-known/matrix/server'. This will tell other
-        # servers to send traffic to port 443 instead.
-        #
-        # See https://matrix-org.github.io/synapse/latest/delegate.html for more
-        # information.
-        #
-        # Defaults to 'false'.
-        #
-        #serve_server_wellknown: true
-


Why is this removed? We are trying to tweak the behaviour when serve_server_wellknown is true.

DMRobertson · 2021-11-26T15:41:22Z

synapse/config/server.py

-        # Defaults to 'https://<server_name>/'.
-        #


Why is this removed? At the very least, we should still describe the behaviour that happens if this config option is omitted (or else note and enforce that it is mandatory)

DMRobertson · 2021-11-26T15:42:44Z

synapse/config/server.py

-        # Whether we should serve a "client well-known":
-        #  (a) at .well-known/matrix/client on our client HTTP listener
-        #  (b) in the response to /login
-        #
-        # ... which together help ensure that clients use our public_baseurl instead of
-        # whatever they were told by the user.
-        #
-        # For the sake of backwards compatibility with existing installations, this is
-        # True if public_baseurl is specified explicitly, and otherwise False. (The
-        # reasoning here is that we have no way of knowing that the default
-        # public_baseurl is actually correct for existing installations - many things
-        # will not work correctly, but that's (probably?) better than sending clients
-        # to a completely broken URL.
-        self.serve_client_wellknown = False


Why is this removed? This removes functionality that we're not adjusting here (serving a json document at /.well-known/matrix/client)

DMRobertson · 2021-11-26T15:45:36Z

tests/rest/test_well_known.py

        }
    )
    def test_client_well_known(self):
        channel = self.make_request(
            "GET", "/.well-known/matrix/client", shorthand=False
        )

+        # import ipdb; ipdb.set_trace()


I think this needs removing.

DMRobertson · 2021-11-26T16:43:50Z

synapse/config/sso.py

+        _b_url = self.root.server.public_baseurl or "/"
+        # if _b_url.endswith("/"):
+        #     _b_url = _b_url[:-1]
+        login_fallback_url = _b_url + "_matrix/static/client/login"


What's going on here? self.root.server.public_baseurl doesn't end in /, so why are we falling back to "/"?

I would have expected to see _matrix replaced with /_matrix here --- it seems that the previous code assumped this ended with a slash.

DMRobertson · 2021-11-26T16:46:08Z

synapse/config/saml2.py

@@ -204,6 +204,8 @@ def _default_saml_config_dict(
        optional_attributes -= required_attributes

        public_baseurl = self.root.server.public_baseurl
+        if not public_baseurl.endswith("/"):
+            public_baseurl = public_baseurl + "/"
        metadata_url = public_baseurl + "_synapse/client/saml2/metadata.xml"
        response_url = public_baseurl + "_synapse/client/saml2/authn_response"
        return {


I think it'd be clearer if things called public_baseurl consistently did not end in a slash, and we changed path building to explicitly add in the slash. After all, the slash is part of a URL's path.

Also, I think the if is redundant because the public_baseurl is never going to end in a / with the proposed changes?

DMRobertson · 2021-11-26T16:46:53Z

synapse/config/oidc.py

        public_baseurl = self.root.server.public_baseurl
+        if not public_baseurl.endswith("/"):
+            public_baseurl = public_baseurl + "/"
        self.oidc_callback_url = public_baseurl + "_synapse/client/oidc/callback"


Same comment as in saml2 applies

DMRobertson · 2021-11-26T16:47:09Z

synapse/config/cas.py

            public_baseurl = self.root.server.public_baseurl
+            if not public_baseurl.endswith("/"):
+                public_baseurl = public_baseurl + "/"
            self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket"


Same comment as on saml2.py

DMRobertson · 2021-11-26T16:55:48Z

docs/sample_config.yaml

-# Defaults to 'https://<server_name>/'.
-#
 #public_baseurl: https://example.com/

-# Uncomment the following to tell other servers to send federation traffic on
-# port 443.
-#
-# By default, other servers will try to reach our server on port 8448, which can
-# be inconvenient in some environments.
-#
-# Provided 'https://<server_name>/' on port 443 is routed to Synapse, this
-# option configures Synapse to serve a file at
-# 'https://<server_name>/.well-known/matrix/server'. This will tell other
-# servers to send traffic to port 443 instead.
-#
-# See https://matrix-org.github.io/synapse/latest/delegate.html for more
-# information.
-#
-# Defaults to 'false'.
-#
-#serve_server_wellknown: true
-


I've commented below where this is generated, but I don't think it's correct to remove this. Don't forget to re-run the script that generates this config once you've made changes.

callahad · 2021-11-26T21:38:40Z

I think we're going around in circles a bit because #5823 superficially appeared simple, but in actually attempting to resolve it, @src-r-r has unearthed a bunch of conflicting / contradictory / undefined behavior.

I suspect it would be a worthwhile effort for someone on the Synapse team to write a new issue to replace #5823, which more completely enumerates the problem, solution, and consequences thereof.

Regardless, it seems clear to me that clients should (must?) be robust against base_url's both with and without trailing slashes, doubly so since matrix.org is inconsistent with Synapse's default behavior.

src-r-r · 2021-11-30T15:06:53Z

Yes, @callahad , thank you! 😆
I'm more than willing to hop on an element call with someone and discuss a resolution to this problem.
I still think a good approach would be to use liburl's constructor in conjunction with pathlib.Path.

src-r-r · 2022-02-15T16:19:56Z

@DMRobertson, what's your take on this issue?

callahad · 2022-02-24T15:59:42Z

@src-r-r 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 pull request moot.

I'm terribly sorry that the outcome of your hard work is not ✅ Merged!, but in many ways this is a much more valuable result: your wrangling with this issue catalyzed improvements to the Matrix Spec in a way that will make the ecosystem itself much more robust. We wouldn't have gotten there without your involvement in this pull request. Thank you.

src-r-r · 2022-02-24T23:39:20Z

Thank you @callahad .

comment out code that removes trailing slash as per 3808.

6148061

Signed-off-by: Jordan Hewitt <633901+src-r-r@users.noreply.github.com>

src-r-r requested a review from a team as a code owner October 31, 2021 21:45

add 5823 to changelog.

76469e7

src-r-r changed the title ~~comment out code that removes trailing slash as per 3808.~~ comment out code that removes trailing slash as per 5823. Oct 31, 2021

callahad linked an issue Oct 31, 2021 that may be closed by this pull request

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

Closed

Change bug PR to news fragments.

de870ed

fix unit test to match issue.

1d1c6b7

richvdh reviewed Nov 1, 2021

View reviewed changes

synapse/config/server.py Show resolved Hide resolved

reivilibre added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Nov 5, 2021

src-r-r added 9 commits November 19, 2021 13:08

fix last remaining slash cases.

be1fa1e

Merge branch 'develop' of github.com:src-r-r/synapse into jordan_2021…

9443680

…WW43_5823_well-known-path-has-errant-trailing-slash

Merge branch 'jordan_2021WW43_5823_well-known-path-has-errant-trailin…

b4e29f0

…g-slash-2' into jordan_2021WW43_5823_well-known-path-has-errant-trailing-slash

merge from develop and fix merges.

e0fa9e1

Merge branch 'develop' of github.com:src-r-r/synapse into jordan_2021…

86902da

…WW43_5823_well-known-path-has-errant-trailing-slash

fix path slash issue.

68fa11b

remove whitespace. complete testing.

6ce40ce

add sample_config

20887b8

add linter.

e071ef2

src-r-r added 2 commits November 21, 2021 11:33

pass codestyle check.

02790c3

fix linter issues.

2d5db3c

DMRobertson self-assigned this Nov 26, 2021

DMRobertson suggested changes Nov 26, 2021

View reviewed changes

DMRobertson removed their assignment Nov 26, 2021

callahad closed this Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

comment out code that removes trailing slash as per 5823. #11224

comment out code that removes trailing slash as per 5823. #11224

src-r-r commented Oct 31, 2021 •

edited

Loading

callahad commented Oct 31, 2021

src-r-r commented Oct 31, 2021

dklimpel commented Nov 1, 2021

src-r-r commented Nov 1, 2021

dklimpel commented Nov 1, 2021

richvdh commented Nov 1, 2021

richvdh commented Nov 1, 2021

ShadowJonathan commented Nov 21, 2021

src-r-r commented Nov 21, 2021 •

edited

Loading

ShadowJonathan commented Nov 21, 2021

DMRobertson commented Nov 22, 2021

DMRobertson commented Nov 23, 2021

DMRobertson left a comment

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

DMRobertson Nov 26, 2021

callahad commented Nov 26, 2021

src-r-r commented Nov 30, 2021 •

edited

Loading

src-r-r commented Feb 15, 2022

callahad commented Feb 24, 2022

src-r-r commented Feb 24, 2022

		@@ -0,0 +1 @@
		Remove trialing slashes form homeserver url so other services don't take it literally.

	Remove trialing slashes form homeserver url so other services don't take it literally.
	Ensure the URL served at `/.well-known/matrix/server/` does not end with a trailing slash, to workaround clients which expect this.

comment out code that removes trailing slash as per 5823. #11224

comment out code that removes trailing slash as per 5823. #11224

Conversation

src-r-r commented Oct 31, 2021 • edited Loading

Pull Request Checklist

callahad commented Oct 31, 2021

src-r-r commented Oct 31, 2021

dklimpel commented Nov 1, 2021

src-r-r commented Nov 1, 2021

dklimpel commented Nov 1, 2021

richvdh commented Nov 1, 2021

richvdh commented Nov 1, 2021

ShadowJonathan commented Nov 21, 2021

src-r-r commented Nov 21, 2021 • edited Loading

ShadowJonathan commented Nov 21, 2021

DMRobertson commented Nov 22, 2021

DMRobertson commented Nov 23, 2021

DMRobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callahad commented Nov 26, 2021

src-r-r commented Nov 30, 2021 • edited Loading

src-r-r commented Feb 15, 2022

callahad commented Feb 24, 2022

src-r-r commented Feb 24, 2022

src-r-r commented Oct 31, 2021 •

edited

Loading

src-r-r commented Nov 21, 2021 •

edited

Loading

src-r-r commented Nov 30, 2021 •

edited

Loading