From 369f70c0d73761bb6b01d3144bdce09c74562eb4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 24 Mar 2023 09:14:25 -0400 Subject: [PATCH 1/8] Call appservices on modern paths, falling back to legacy paths. --- synapse/appservice/api.py | 131 ++++++++++++++++++++++++----------- tests/appservice/test_api.py | 57 ++++++++++++++- 2 files changed, 146 insertions(+), 42 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 4812fb44961f..42dcf1e7581c 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -17,6 +17,8 @@ from typing import ( TYPE_CHECKING, Any, + Awaitable, + Callable, Dict, Iterable, List, @@ -24,10 +26,11 @@ Optional, Sequence, Tuple, + TypeVar, ) from prometheus_client import Counter -from typing_extensions import TypeGuard +from typing_extensions import Concatenate, ParamSpec, TypeGuard from synapse.api.constants import EventTypes, Membership, ThirdPartyEntityKind from synapse.api.errors import CodeMessageException @@ -78,7 +81,11 @@ HOUR_IN_MS = 60 * 60 * 1000 -APP_SERVICE_PREFIX = "/_matrix/app/unstable" +APP_SERVICE_PREFIX = "/_matrix/app/v1" +APP_SERVICE_UNSTABLE_PREFIX = "/_matrix/app/unstable" + +P = ParamSpec("P") +R = TypeVar("R") def _is_valid_3pe_metadata(info: JsonDict) -> bool: @@ -121,6 +128,45 @@ def __init__(self, hs: "HomeServer"): hs.get_clock(), "as_protocol_meta", timeout_ms=HOUR_IN_MS ) + async def _send_with_fallbacks( + self, + service: "ApplicationService", + prefixes: List[str], + path: str, + func: Callable[Concatenate[str, P], Awaitable[R]], + *args: P.args, + **kwargs: P.kwargs, + ) -> R: + """ + Attempt to call an application service with multiple paths, falling back + until one succeeds. + + Args: + service: The appliacation service, this provides the base URL. + prefixes: A last of paths to try in order for the requests. + path: A suffix to append to each prefix. + func: The function to call, the first argument will be the full + endpoint to fetch. Other arguments are provided by args/kwargs. + + Returns: + The return value of func. + """ + while prefixes: + prefix = prefixes.pop(0) + uri = f"{service.url}{prefix}{path}" + try: + return await func(uri, *args, **kwargs) + except CodeMessageException as e: + # If an error is received that is due to an unrecognised path, + # fallback to next path (if one exists). Otherwise, consider it + # a legitimate error and raise. + if prefixes and (e.code == 404 or e.code == 405): + continue + raise + + # The function should always exit via the return or raise above this. + raise RuntimeError("Unexpected fallback behaviour. This should never be seen.") + async def query_user(self, service: "ApplicationService", user_id: str) -> bool: if service.url is None: return False @@ -128,10 +174,12 @@ async def query_user(self, service: "ApplicationService", user_id: str) -> bool: # This is required by the configuration. assert service.hs_token is not None - uri = service.url + ("/users/%s" % urllib.parse.quote(user_id)) try: - response = await self.get_json( - uri, + response = await self._send_with_fallbacks( + service, + [APP_SERVICE_PREFIX, ""], + f"/users/{urllib.parse.quote(user_id)}", + self.get_json, {"access_token": service.hs_token}, headers={"Authorization": [f"Bearer {service.hs_token}"]}, ) @@ -140,9 +188,9 @@ async def query_user(self, service: "ApplicationService", user_id: str) -> bool: except CodeMessageException as e: if e.code == 404: return False - logger.warning("query_user to %s received %s", uri, e.code) + logger.warning("query_user to %s received %s", service.url, e.code) except Exception as ex: - logger.warning("query_user to %s threw exception %s", uri, ex) + logger.warning("query_user to %s threw exception %s", service.url, ex) return False async def query_alias(self, service: "ApplicationService", alias: str) -> bool: @@ -152,21 +200,23 @@ async def query_alias(self, service: "ApplicationService", alias: str) -> bool: # This is required by the configuration. assert service.hs_token is not None - uri = service.url + ("/rooms/%s" % urllib.parse.quote(alias)) try: - response = await self.get_json( - uri, + response = await self._send_with_fallbacks( + service, + [APP_SERVICE_PREFIX, ""], + f"/rooms/{urllib.parse.quote(alias)}", + self.get_json, {"access_token": service.hs_token}, headers={"Authorization": [f"Bearer {service.hs_token}"]}, ) if response is not None: # just an empty json object return True except CodeMessageException as e: - logger.warning("query_alias to %s received %s", uri, e.code) + logger.warning("query_alias to %s received %s", service.url, e.code) if e.code == 404: return False except Exception as ex: - logger.warning("query_alias to %s threw exception %s", uri, ex) + logger.warning("query_alias to %s threw exception %s", service.url, ex) return False async def query_3pe( @@ -188,25 +238,24 @@ async def query_3pe( # This is required by the configuration. assert service.hs_token is not None - uri = "%s%s/thirdparty/%s/%s" % ( - service.url, - APP_SERVICE_PREFIX, - kind, - urllib.parse.quote(protocol), - ) try: args: Mapping[Any, Any] = { **fields, b"access_token": service.hs_token, } - response = await self.get_json( - uri, + response = await self._send_with_fallbacks( + service, + [APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX], + f"/thirdparty/{kind}/{urllib.parse.quote(protocol)}", + self.get_json, args=args, headers={"Authorization": [f"Bearer {service.hs_token}"]}, ) if not isinstance(response, list): logger.warning( - "query_3pe to %s returned an invalid response %r", uri, response + "query_3pe to %s returned an invalid response %r", + service.url, + response, ) return [] @@ -216,12 +265,12 @@ async def query_3pe( ret.append(r) else: logger.warning( - "query_3pe to %s returned an invalid result %r", uri, r + "query_3pe to %s returned an invalid result %r", service.url, r ) return ret except Exception as ex: - logger.warning("query_3pe to %s threw exception %s", uri, ex) + logger.warning("query_3pe to %s threw exception %s", service.url, ex) return [] async def get_3pe_protocol( @@ -233,21 +282,20 @@ async def get_3pe_protocol( async def _get() -> Optional[JsonDict]: # This is required by the configuration. assert service.hs_token is not None - uri = "%s%s/thirdparty/protocol/%s" % ( - service.url, - APP_SERVICE_PREFIX, - urllib.parse.quote(protocol), - ) try: - info = await self.get_json( - uri, + info = await self._send_with_fallbacks( + service, + [APP_SERVICE_PREFIX, APP_SERVICE_UNSTABLE_PREFIX], + f"/thirdparty/protocol/{urllib.parse.quote(protocol)}", + self.get_json, {"access_token": service.hs_token}, headers={"Authorization": [f"Bearer {service.hs_token}"]}, ) if not _is_valid_3pe_metadata(info): logger.warning( - "query_3pe_protocol to %s did not return a valid result", uri + "query_3pe_protocol to %s did not return a valid result", + service.url, ) return None @@ -260,7 +308,9 @@ async def _get() -> Optional[JsonDict]: return info except Exception as ex: - logger.warning("query_3pe_protocol to %s threw exception %s", uri, ex) + logger.warning( + "query_3pe_protocol to %s threw exception %s", service.url, ex + ) return None key = (service.id, protocol) @@ -274,7 +324,7 @@ async def ping(self, service: "ApplicationService", txn_id: Optional[str]) -> No assert service.hs_token is not None await self.post_json_get_json( - uri=service.url + "/_matrix/app/unstable/fi.mau.msc2659/ping", + uri=f"{service.url}{APP_SERVICE_UNSTABLE_PREFIX}/fi.mau.msc2659/ping", post_json={"transaction_id": txn_id}, headers={"Authorization": [f"Bearer {service.hs_token}"]}, ) @@ -318,8 +368,6 @@ async def push_bulk( ) txn_id = 0 - uri = service.url + ("/transactions/%s" % urllib.parse.quote(str(txn_id))) - # Never send ephemeral events to appservices that do not support it body: JsonDict = {"events": serialized_events} if service.supports_ephemeral: @@ -351,8 +399,11 @@ async def push_bulk( } try: - await self.put_json( - uri=uri, + await self._send_with_fallbacks( + service, + [APP_SERVICE_PREFIX, ""], + f"/transactions/{urllib.parse.quote(str(txn_id))}", + self.put_json, json_body=body, args={"access_token": service.hs_token}, headers={"Authorization": [f"Bearer {service.hs_token}"]}, @@ -360,7 +411,7 @@ async def push_bulk( if logger.isEnabledFor(logging.DEBUG): logger.debug( "push_bulk to %s succeeded! events=%s", - uri, + service.url, [event.get("event_id") for event in events], ) sent_transactions_counter.labels(service.id).inc() @@ -371,7 +422,7 @@ async def push_bulk( except CodeMessageException as e: logger.warning( "push_bulk to %s received code=%s msg=%s", - uri, + service.url, e.code, e.msg, exc_info=logger.isEnabledFor(logging.DEBUG), @@ -379,7 +430,7 @@ async def push_bulk( except Exception as ex: logger.warning( "push_bulk to %s threw exception(%s) %s args=%s", - uri, + service.url, type(ex).__name__, ex, ex.args, diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index 9d183b733ec2..8ca99c1b1cea 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -16,6 +16,7 @@ from twisted.test.proto_helpers import MemoryReactor +from synapse.api.errors import CodeMessageException from synapse.appservice import ApplicationService from synapse.server import HomeServer from synapse.types import JsonDict @@ -64,8 +65,8 @@ def test_query_3pe_authenticates_token(self) -> None: } ] - URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}" - URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}" + URL_USER = f"{URL}/_matrix/app/v1/thirdparty/user/{PROTOCOL}" + URL_LOCATION = f"{URL}/_matrix/app/v1/thirdparty/location/{PROTOCOL}" self.request_url = None @@ -105,3 +106,55 @@ async def get_json( ) self.assertEqual(self.request_url, URL_LOCATION) self.assertEqual(result, SUCCESS_RESULT_LOCATION) + + def test_fallback(self) -> None: + """ + Tests that Synapse fallsback to legacy URLs. + """ + SUCCESS_RESULT_USER = [ + { + "protocol": PROTOCOL, + "userid": "@a:user", + "fields": { + "more": "fields", + }, + } + ] + + URL_USER = f"{URL}/_matrix/app/v1/thirdparty/user/{PROTOCOL}" + FALLBACK_URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}" + + self.request_url = None + self.v1_seen = False + + async def get_json( + url: str, + args: Mapping[Any, Any], + headers: Mapping[Union[str, bytes], Sequence[Union[str, bytes]]], + ) -> List[JsonDict]: + # Ensure the access token is passed as both a header and query arg. + if not headers.get("Authorization") or not args.get(b"access_token"): + raise RuntimeError("Access token not provided") + + self.assertEqual(headers.get("Authorization"), [f"Bearer {TOKEN}"]) + self.assertEqual(args.get(b"access_token"), TOKEN) + self.request_url = url + if url == URL_USER: + self.v1_seen = True + raise CodeMessageException(404, "NOT_FOUND") + elif url == FALLBACK_URL_USER: + return SUCCESS_RESULT_USER + else: + raise RuntimeError( + "URL provided was invalid. This should never be seen." + ) + + # We assign to a method, which mypy doesn't like. + self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] + + result = self.get_success( + self.api.query_3pe(self.service, "user", PROTOCOL, {b"some": [b"field"]}) + ) + self.assertTrue(self.v1_seen) + self.assertEqual(self.request_url, FALLBACK_URL_USER) + self.assertEqual(result, SUCCESS_RESULT_USER) From 5a96a3ce81c5482d88342e61961543abea80c9dd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 24 Mar 2023 09:36:17 -0400 Subject: [PATCH 2/8] Newsfragment --- changelog.d/15317.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15317.bugfix diff --git a/changelog.d/15317.bugfix b/changelog.d/15317.bugfix new file mode 100644 index 000000000000..194e4c46c64c --- /dev/null +++ b/changelog.d/15317.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug that Synpase only used the [legacy appservice routes](https://spec.matrix.org/v1.6/application-service-api/#legacy-routes). From 87b4f16f01bcd783642751c0cf0e2231973c2568 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 24 Mar 2023 09:45:54 -0400 Subject: [PATCH 3/8] Add upgrade note. --- docs/upgrade.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/upgrade.md b/docs/upgrade.md index f14444a400ea..e0ce70224fa2 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -88,6 +88,22 @@ process, for example: dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb ``` +# Upgrading to v1.81.0 + +## Application service path & authentication deprecations + +Synapse now attempts the versioned appservice paths before falling back to the +[legacy paths](https://spec.matrix.org/v1.6/application-service-api/#legacy-routes). +Usage of the legacy routes should be considered deprecated. + +Additionally, Synapse has supported sending the application service access token +via [the `Authorization` header](https://spec.matrix.org/v1.6/application-service-api/#authorization) +since v1.70.0. For backwards compatibility it is *also* send as the `access_token` +query parameter. This is insecure and should be considered deprecated. + +A future version of Synapse (v1.85.0 or later) will remove support for legacy +application service routes and query parameter authorization. + # Upgrading to v1.80.0 ## Reporting events error code change From 17bfb217e1f959c2ea5d6e997f6227c2a5dd13b9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 24 Mar 2023 14:17:59 -0400 Subject: [PATCH 4/8] Fix typo. Co-authored-by: Travis Ralston --- docs/upgrade.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index e0ce70224fa2..fe83cf9d3204 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -98,7 +98,7 @@ Usage of the legacy routes should be considered deprecated. Additionally, Synapse has supported sending the application service access token via [the `Authorization` header](https://spec.matrix.org/v1.6/application-service-api/#authorization) -since v1.70.0. For backwards compatibility it is *also* send as the `access_token` +since v1.70.0. For backwards compatibility it is *also* sent as the `access_token` query parameter. This is insecure and should be considered deprecated. A future version of Synapse (v1.85.0 or later) will remove support for legacy From 2dd943b365717df448ab1c990d2e0bafa284971f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 28 Mar 2023 12:29:28 -0400 Subject: [PATCH 5/8] Bump version. --- docs/upgrade.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index fe83cf9d3204..1ddfc31ff603 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -101,7 +101,7 @@ via [the `Authorization` header](https://spec.matrix.org/v1.6/application-servic since v1.70.0. For backwards compatibility it is *also* sent as the `access_token` query parameter. This is insecure and should be considered deprecated. -A future version of Synapse (v1.85.0 or later) will remove support for legacy +A future version of Synapse (v1.88.0 or later) will remove support for legacy application service routes and query parameter authorization. # Upgrading to v1.80.0 From 9e5a0c7ca4f0b80f04a37d21eed33750f8c4585b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Mar 2023 08:56:43 -0400 Subject: [PATCH 6/8] Re-use is_unknown_endpoint. --- synapse/appservice/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index d4bc43565507..5e158e7b8d6e 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -156,11 +156,11 @@ async def _send_with_fallbacks( uri = f"{service.url}{prefix}{path}" try: return await func(uri, *args, **kwargs) - except CodeMessageException as e: + except HttpResponseException as e: # If an error is received that is due to an unrecognised path, # fallback to next path (if one exists). Otherwise, consider it # a legitimate error and raise. - if prefixes and (e.code == 404 or e.code == 405): + if prefixes and is_unknown_endpoint(e): continue raise From 87d55765256e0af4e1ea8f8938940858de689058 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Mar 2023 09:33:16 -0400 Subject: [PATCH 7/8] Broaden text-content check and update comments. --- synapse/http/client.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 5ee55981d9f8..b5cf8123ce84 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -982,20 +982,21 @@ def is_unknown_endpoint( """ if synapse_error is None: synapse_error = e.to_synapse_error() - # MSC3743 specifies that servers should return a 404 or 405 with an errcode + + # Matrix v1.6 specifies that servers should return a 404 or 405 with an errcode # of M_UNRECOGNIZED when they receive a request to an unknown endpoint or # to an unknown method, respectively. # - # Older versions of servers don't properly handle this. This needs to be - # rather specific as some endpoints truly do return 404 errors. + # Older versions of servers don't return proper errors, so be graceful. But, + # also handle that some endpoints truly do return 404 errors. return ( # 404 is an unknown endpoint, 405 is a known endpoint, but unknown method. (e.code == 404 or e.code == 405) and ( - # Older Dendrites returned a text body or empty body. - # Older Conduit returned an empty body. + # Consider empty body or non-JSON bodies to be unrecognised (matches + # older Dendrites & Conduits). not e.response - or e.response == b"404 page not found" + or not e.response.startswith(b"{") # The proper response JSON with M_UNRECOGNIZED errcode. or synapse_error.errcode == Codes.UNRECOGNIZED ) From bdd57dda8c4fdf22ccdd7ed37f76174bb1ff120c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Mar 2023 14:31:45 -0400 Subject: [PATCH 8/8] Don't mutate inputs. --- synapse/appservice/api.py | 8 +++++--- tests/appservice/test_api.py | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 5e158e7b8d6e..86ddb1bb289e 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -151,8 +151,7 @@ async def _send_with_fallbacks( Returns: The return value of func. """ - while prefixes: - prefix = prefixes.pop(0) + for i, prefix in enumerate(prefixes, start=1): uri = f"{service.url}{prefix}{path}" try: return await func(uri, *args, **kwargs) @@ -160,9 +159,12 @@ async def _send_with_fallbacks( # If an error is received that is due to an unrecognised path, # fallback to next path (if one exists). Otherwise, consider it # a legitimate error and raise. - if prefixes and is_unknown_endpoint(e): + if i < len(prefixes) and is_unknown_endpoint(e): continue raise + except Exception: + # Unexpected exceptions get sent to the caller. + raise # The function should always exit via the return or raise above this. raise RuntimeError("Unexpected fallback behaviour. This should never be seen.") diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index 1a152075b682..7deb923a280d 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -16,7 +16,7 @@ from twisted.test.proto_helpers import MemoryReactor -from synapse.api.errors import CodeMessageException +from synapse.api.errors import HttpResponseException from synapse.appservice import ApplicationService from synapse.server import HomeServer from synapse.types import JsonDict @@ -109,7 +109,7 @@ async def get_json( def test_fallback(self) -> None: """ - Tests that Synapse fallsback to legacy URLs. + Tests that the fallback to legacy URLs works. """ SUCCESS_RESULT_USER = [ { @@ -141,7 +141,7 @@ async def get_json( self.request_url = url if url == URL_USER: self.v1_seen = True - raise CodeMessageException(404, "NOT_FOUND") + raise HttpResponseException(404, "NOT_FOUND", b"NOT_FOUND") elif url == FALLBACK_URL_USER: return SUCCESS_RESULT_USER else: