diff --git a/changelog.d/15317.bugfix b/changelog.d/15317.bugfix
new file mode 100644
index 0000000000..194e4c46c6
--- /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).
diff --git a/docs/upgrade.md b/docs/upgrade.md
index f14444a400..1ddfc31ff6 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* sent as the `access_token`
+query parameter. This is insecure and should be considered deprecated.
+
+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
## Reporting events error code change
diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py
index b27eedef99..86ddb1bb28 100644
--- a/synapse/appservice/api.py
+++ b/synapse/appservice/api.py
@@ -17,6 +17,8 @@ import urllib.parse
from typing import (
TYPE_CHECKING,
Any,
+ Awaitable,
+ Callable,
Dict,
Iterable,
List,
@@ -24,10 +26,11 @@ from typing import (
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, HttpResponseException
@@ -78,7 +81,11 @@ sent_todevice_counter = Counter(
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,47 @@ class ApplicationServiceApi(SimpleHttpClient):
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.
+ """
+ for i, prefix in enumerate(prefixes, start=1):
+ uri = f"{service.url}{prefix}{path}"
+ try:
+ return await func(uri, *args, **kwargs)
+ 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 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.")
+
async def query_user(self, service: "ApplicationService", user_id: str) -> bool:
if service.url is None:
return False
@@ -128,10 +176,12 @@ class ApplicationServiceApi(SimpleHttpClient):
# 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 +190,9 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +202,23 @@ class ApplicationServiceApi(SimpleHttpClient):
# 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 +240,24 @@ class ApplicationServiceApi(SimpleHttpClient):
# 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 +267,12 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +284,20 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +310,9 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +326,7 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +370,6 @@ class ApplicationServiceApi(SimpleHttpClient):
)
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 +401,11 @@ class ApplicationServiceApi(SimpleHttpClient):
}
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 +413,7 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +424,7 @@ class ApplicationServiceApi(SimpleHttpClient):
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 +432,7 @@ class ApplicationServiceApi(SimpleHttpClient):
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/synapse/http/client.py b/synapse/http/client.py
index 5ee55981d9..b5cf8123ce 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
)
diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py
index 0dd02b7d58..7deb923a28 100644
--- a/tests/appservice/test_api.py
+++ b/tests/appservice/test_api.py
@@ -16,6 +16,7 @@ from unittest.mock import Mock
from twisted.test.proto_helpers import MemoryReactor
+from synapse.api.errors import HttpResponseException
from synapse.appservice import ApplicationService
from synapse.server import HomeServer
from synapse.types import JsonDict
@@ -64,8 +65,8 @@ class ApplicationServiceApiTestCase(unittest.HomeserverTestCase):
}
]
- 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
@@ -106,6 +107,58 @@ class ApplicationServiceApiTestCase(unittest.HomeserverTestCase):
self.assertEqual(self.request_url, URL_LOCATION)
self.assertEqual(result, SUCCESS_RESULT_LOCATION)
+ def test_fallback(self) -> None:
+ """
+ Tests that the fallback to legacy URLs works.
+ """
+ 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 HttpResponseException(404, "NOT_FOUND", b"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)
+
def test_claim_keys(self) -> None:
"""
Tests that the /keys/claim response is properly parsed for missing
|
