Move support for application service query parameter authorization behind a configuration option

changelog.d/16017.misc

@@ -0,0 +1 @@
+Move support for application service query parameter authorization behind a configuration option.

docs/upgrade.md

@@ -88,6 +88,21 @@ process, for example:
     dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb
     ```
 
+# Upgrading to v1.90.0
+
+## App service query parameter authorization is now a configuration option
+
+Synapse v1.81.0 deprecated application service authorization via query parameters as this is
+considered insecure - and from Synapse v1.71.0 forwards the application service token has also been sent via 
+[the `Authorization` header](https://spec.matrix.org/v1.6/application-service-api/#authorization)], making the insecure
+query parameter authorization redundant. Since removing the ability to continue to use query parameters could break 
+backwards compatibility it has now been put behind a configuration option, `use_appservice_legacy_authorization`.  
+This option defaults to false, but can be activated by adding 
+```yaml
+"use_appservice_legacy_authorization": True 
+```
+to your configuration.
+
 # Upgrading to v1.89.0
 
 ## Removal of unspecced `user` property for `/register`
@@ -97,7 +112,6 @@ The standard `username` property should be used instead. See the
 [Application Service specification](https://spec.matrix.org/v1.7/application-service-api/#server-admin-style-permissions)
 for more information.
 
-
 # Upgrading to v1.88.0
 
 ## Minimum supported Python version

docs/usage/configuration/config_documentation.md

@@ -2837,6 +2837,17 @@ Example configuration:
 ```yaml
 track_appservice_user_ips: true
 ```
+---
+### `use_appservice_legacy_authorization`
+
+Whether to send the application service access token via the `access_token` query parameter.
+Defaults to false. This option is considered insecure and is not recommended. 
+
+Example configuration:
+```yaml
+"use_appservice_legacy_authorization": True 
+```
+
 ---
 ### `macaroon_secret_key`
 

synapse/appservice/api.py

@@ -20,7 +20,7 @@
     Dict,
     Iterable,
     List,
-    Mapping,
+    MutableMapping,
     Optional,
     Sequence,
     Tuple,
@@ -119,6 +119,7 @@ class ApplicationServiceApi(SimpleHttpClient):
     def __init__(self, hs: "HomeServer"):
         super().__init__(hs)
         self.clock = hs.get_clock()
+        self.config = hs.config.appservice
 
         self.protocol_meta_cache: ResponseCache[Tuple[str, str]] = ResponseCache(
             hs.get_clock(), "as_protocol_meta", timeout_ms=HOUR_IN_MS
@@ -132,11 +133,16 @@ async def query_user(self, service: "ApplicationService", user_id: str) -> bool:
         assert service.hs_token is not None
 
         try:
-            response = await self.get_json(
-                f"{service.url}{APP_SERVICE_PREFIX}/users/{urllib.parse.quote(user_id)}",
-                {"access_token": service.hs_token},
-                headers={"Authorization": [f"Bearer {service.hs_token}"]},
-            )
+            if self.config.use_appservice_legacy_authorization:
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/users/{urllib.parse.quote(user_id)}",
+                    {"access_token": service.hs_token},
+                )
+            else:
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/users/{urllib.parse.quote(user_id)}",
+                    headers={"Authorization": [f"Bearer {service.hs_token}"]},
+                )
             if response is not None:  # just an empty json object
                 return True
         except CodeMessageException as e:
@@ -155,11 +161,16 @@ async def query_alias(self, service: "ApplicationService", alias: str) -> bool:
         assert service.hs_token is not None
 
         try:
-            response = await self.get_json(
-                f"{service.url}{APP_SERVICE_PREFIX}/rooms/{urllib.parse.quote(alias)}",
-                {"access_token": service.hs_token},
-                headers={"Authorization": [f"Bearer {service.hs_token}"]},
-            )
+            if self.config.use_appservice_legacy_authorization:
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/rooms/{urllib.parse.quote(alias)}",
+                    {"access_token": service.hs_token},
+                )
+            else:
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/rooms/{urllib.parse.quote(alias)}",
+                    headers={"Authorization": [f"Bearer {service.hs_token}"]},
+                )
             if response is not None:  # just an empty json object
                 return True
         except CodeMessageException as e:
@@ -190,15 +201,22 @@ async def query_3pe(
         assert service.hs_token is not None
 
         try:
-            args: Mapping[Any, Any] = {
+            args: MutableMapping[Any, Any] = {
                 **fields,
                 b"access_token": service.hs_token,
             }
-            response = await self.get_json(
-                f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
-                args=args,
-                headers={"Authorization": [f"Bearer {service.hs_token}"]},
-            )
+            if self.config.use_appservice_legacy_authorization:
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
+                    args=args,
+                )
+            else:
+                args.pop(b"access_token", None)
+                response = await self.get_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/{kind}/{urllib.parse.quote(protocol)}",
+                    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",
@@ -231,11 +249,16 @@ async def _get() -> Optional[JsonDict]:
             # This is required by the configuration.
             assert service.hs_token is not None
             try:
-                info = await self.get_json(
-                    f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/protocol/{urllib.parse.quote(protocol)}",
-                    {"access_token": service.hs_token},
-                    headers={"Authorization": [f"Bearer {service.hs_token}"]},
-                )
+                if self.config.use_appservice_legacy_authorization:
+                    info = await self.get_json(
+                        f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/protocol/{urllib.parse.quote(protocol)}",
+                        {"access_token": service.hs_token},
+                    )
+                else:
+                    info = await self.get_json(
+                        f"{service.url}{APP_SERVICE_PREFIX}/thirdparty/protocol/{urllib.parse.quote(protocol)}",
+                        headers={"Authorization": [f"Bearer {service.hs_token}"]},
+                    )
 
                 if not _is_valid_3pe_metadata(info):
                     logger.warning(
@@ -344,12 +367,18 @@ async def push_bulk(
                 }
 
         try:
-            await self.put_json(
-                f"{service.url}{APP_SERVICE_PREFIX}/transactions/{urllib.parse.quote(str(txn_id))}",
-                json_body=body,
-                args={"access_token": service.hs_token},
-                headers={"Authorization": [f"Bearer {service.hs_token}"]},
-            )
+            if self.config.use_appservice_legacy_authorization:
+                await self.put_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/transactions/{urllib.parse.quote(str(txn_id))}",
+                    json_body=body,
+                    args={"access_token": service.hs_token},
+                )
+            else:
+                await self.put_json(
+                    f"{service.url}{APP_SERVICE_PREFIX}/transactions/{urllib.parse.quote(str(txn_id))}",
+                    json_body=body,
+                    headers={"Authorization": [f"Bearer {service.hs_token}"]},
+                )
             if logger.isEnabledFor(logging.DEBUG):
                 logger.debug(
                     "push_bulk to %s succeeded! events=%s",

synapse/config/appservice.py

@@ -43,6 +43,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
             )
 
         self.track_appservice_user_ips = config.get("track_appservice_user_ips", False)
+        self.use_appservice_legacy_authorization = config.get(
+            "use_appservice_legacy_authorization", False
+        )
+        if self.use_appservice_legacy_authorization:
+            logger.warning(
+                "The use of appservice legacy authorization via query params is deprecated"
+                " and should be considered insecure."
+            )
 
 
 def load_appservices(

tests/appservice/test_api.py

@@ -11,7 +11,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from typing import Any, List, Mapping, Sequence, Union
+from typing import Any, List, Mapping, Optional, Sequence, Union
 from unittest.mock import Mock
 
 from twisted.test.proto_helpers import MemoryReactor
@@ -22,6 +22,7 @@
 from synapse.util import Clock
 
 from tests import unittest
+from tests.unittest import override_config
 
 PROTOCOL = "myproto"
 TOKEN = "myastoken"
@@ -39,7 +40,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
             hs_token=TOKEN,
         )
 
-    def test_query_3pe_authenticates_token(self) -> None:
+    def test_query_3pe_authenticates_token_via_header(self) -> None:
         """
         Tests that 3pe queries to the appservice are authenticated
         with the appservice's token.
@@ -74,11 +75,79 @@ async def get_json(
             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"):
+            # Ensure the access token is passed as a header.
+            if not headers.get("Authorization"):
                 raise RuntimeError("Access token not provided")
 
             self.assertEqual(headers.get("Authorization"), [f"Bearer {TOKEN}"])
+            self.request_url = url
+            if url == URL_USER:
+                return SUCCESS_RESULT_USER
+            elif url == URL_LOCATION:
+                return SUCCESS_RESULT_LOCATION
+            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.assertEqual(self.request_url, URL_USER)
+        self.assertEqual(result, SUCCESS_RESULT_USER)
+        result = self.get_success(
+            self.api.query_3pe(
+                self.service, "location", PROTOCOL, {b"some": [b"field"]}
+            )
+        )
+        self.assertEqual(self.request_url, URL_LOCATION)
+        self.assertEqual(result, SUCCESS_RESULT_LOCATION)
+
+    @override_config({"use_appservice_legacy_authorization": True})
+    def test_query_3pe_authenticates_token_via_param(self) -> None:
+        """
+        Tests that 3pe queries to the appservice are authenticated
+        with the appservice's token.
+        """
+
+        SUCCESS_RESULT_USER = [
+            {
+                "protocol": PROTOCOL,
+                "userid": "@a:user",
+                "fields": {
+                    "more": "fields",
+                },
+            }
+        ]
+        SUCCESS_RESULT_LOCATION = [
+            {
+                "protocol": PROTOCOL,
+                "alias": "#a:room",
+                "fields": {
+                    "more": "fields",
+                },
+            }
+        ]
+
+        URL_USER = f"{URL}/_matrix/app/v1/thirdparty/user/{PROTOCOL}"
+        URL_LOCATION = f"{URL}/_matrix/app/v1/thirdparty/location/{PROTOCOL}"
+
+        self.request_url = None
+
+        async def get_json(
+            url: str,
+            args: Mapping[Any, Any],
+            headers: Optional[
+                Mapping[Union[str, bytes], Sequence[Union[str, bytes]]]
+            ] = None,
+        ) -> List[JsonDict]:
+            # Ensure the access token is passed as a query param.
+            if not args.get(b"access_token"):
+                raise RuntimeError("Access token not provided")
+
             self.assertEqual(args.get(b"access_token"), TOKEN)
             self.request_url = url
             if url == URL_USER: