From f739bde962daa9bc425c8343f35993ae889dbc67 Mon Sep 17 00:00:00 2001
From: Quentin Gliech <quenting@element.io>
Date: Tue, 23 May 2023 16:59:53 +0200
Subject: [PATCH] Reject tokens with multiple device scopes

---
 synapse/api/auth/msc3861_delegated.py   | 30 ++++++++++++++++++++-----
 tests/handlers/test_oauth_delegation.py | 29 +++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py
index 5b0e678c0..e4b16c0b5 100644
--- a/synapse/api/auth/msc3861_delegated.py
+++ b/synapse/api/auth/msc3861_delegated.py
@@ -303,13 +303,31 @@ class MSC3861DelegatedAuth(BaseAuth):
         else:
             user_id = UserID.from_string(user_id_str)
 
-        # Find device_id in scope
-        device_id = None
-        for tok in scope:
-            if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX):
-                device_id = tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :]
+        # Find device_ids in scope
+        # We only allow a single device_id in the scope, so we find them all in the
+        # scope list, and raise if there are more than one. The OIDC server should be
+        # the one enforcing valid scopes, so we raise a 500 if we find an invalid scope.
+        device_ids = [
+            tok[len(SCOPE_MATRIX_DEVICE_PREFIX) :]
+            for tok in scope
+            if tok.startswith(SCOPE_MATRIX_DEVICE_PREFIX)
+        ]
+
+        if len(device_ids) > 1:
+            raise AuthError(
+                500,
+                "Multiple device IDs in scope",
+            )
+
+        device_id = device_ids[0] if device_ids else None
+        if device_id is not None:
+            # Sanity check the device_id
+            if len(device_id) > 255 or len(device_id) < 1:
+                raise AuthError(
+                    500,
+                    "Invalid device ID in scope",
+                )
 
-        if device_id:
             # Create the device on the fly if it does not exist
             try:
                 await self.store.get_device(
diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py
index 16ce2c069..064153551 100644
--- a/tests/handlers/test_oauth_delegation.py
+++ b/tests/handlers/test_oauth_delegation.py
@@ -27,6 +27,7 @@ from signedjson.sign import sign_json
 from twisted.test.proto_helpers import MemoryReactor
 
 from synapse.api.errors import (
+    AuthError,
     Codes,
     InvalidClientTokenError,
     OAuthInsufficientScopeError,
@@ -68,8 +69,9 @@ INTROSPECTION_ENDPOINT = ISSUER + "introspect"
 SYNAPSE_ADMIN_SCOPE = "urn:synapse:admin:*"
 MATRIX_USER_SCOPE = "urn:matrix:org.matrix.msc2967.client:api:*"
 MATRIX_GUEST_SCOPE = "urn:matrix:org.matrix.msc2967.client:api:guest"
+MATRIX_DEVICE_SCOPE_PREFIX = "urn:matrix:org.matrix.msc2967.client:device:"
 DEVICE = "AABBCCDD"
-MATRIX_DEVICE_SCOPE = "urn:matrix:org.matrix.msc2967.client:device:" + DEVICE
+MATRIX_DEVICE_SCOPE = MATRIX_DEVICE_SCOPE_PREFIX + DEVICE
 SUBJECT = "abc-def-ghi"
 USERNAME = "test-user"
 USER_ID = "@" + USERNAME + ":" + SERVER_NAME
@@ -344,6 +346,31 @@ class MSC3861OAuthDelegation(HomeserverTestCase):
         )
         self.assertEqual(requester.device_id, DEVICE)
 
+    def test_multiple_devices(self) -> None:
+        """The handler should raise an error if multiple devices are found in the scope."""
+
+        self.http_client.request = simple_async_mock(
+            return_value=FakeResponse.json(
+                code=200,
+                payload={
+                    "active": True,
+                    "sub": SUBJECT,
+                    "scope": " ".join(
+                        [
+                            MATRIX_USER_SCOPE,
+                            f"{MATRIX_DEVICE_SCOPE_PREFIX}AABBCC",
+                            f"{MATRIX_DEVICE_SCOPE_PREFIX}DDEEFF",
+                        ]
+                    ),
+                    "username": USERNAME,
+                },
+            )
+        )
+        request = Mock(args={})
+        request.args[b"access_token"] = [b"mockAccessToken"]
+        request.requestHeaders.getRawHeaders = mock_getRawHeaders()
+        self.get_failure(self.auth.get_user_by_req(request), AuthError)
+
     def test_active_guest_not_allowed(self) -> None:
         """The handler should return an insufficient scope error."""