Fix 500 error when user-interactive auth requests send a null or non-object auth (#20274)

Fixes #15871

### What was broken

Sending `{"auth": null}` to any endpoint that requires user-interactive
auth (for example `POST /keys/device_signing/upload`, `DELETE
/devices/{deviceId}` or `POST /register`) returned a 500 instead of the
usual 401 with the available flows. A non-object `auth` on endpoints
that do not validate the body with a model (such as `/register`) could
also 500, e.g. `{"auth": ["session"]}`.

### Root cause

`AuthHandler.check_ui_auth` did `clientdict.pop("auth", {})`, so the
`{}` default only applies when the key is missing. An explicit `null`
came through as `None` and the following `"session" in authdict` raised
`TypeError`. `get_session_id` had the same assumption that `auth` is a
dict.

### Pull Request Checklist

* [x] Pull request is based on the develop branch
* [x] Pull request includes a [changelog
file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog).
* [x] [Code
style](https://element-hq.github.io/synapse/latest/code_style.html) is
correct (run the
[linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

---------

Signed-off-by: Ankit Jha <ankit.jha@tradomate.one>

changelog.d/20274.bugfix

@@ -0,0 +1 @@
+Fix a bug where sending a `null` or non-object `auth` to an endpoint requiring user-interactive authentication returned a 500 error. Contributed by @ankit373.

synapse/handlers/auth.py

@@ -486,7 +486,11 @@ class AuthHandler:
         """
 
         sid: str | None = None
-        authdict = clientdict.pop("auth", {})
+        # `auth` is optional, so an explicit `null` means the same as `{}`, ie. no
+        # auth stage attempted yet.
+        authdict = clientdict.pop("auth", None)
+        if authdict is None:
+            authdict = {}
         if "session" in authdict:
             sid = authdict["session"]
 
@@ -647,7 +651,7 @@ class AuthHandler:
                 not send a session ID, returns None.
         """
         sid = None
-        if clientdict and "auth" in clientdict:
+        if clientdict and clientdict.get("auth") is not None:
             authdict = clientdict["auth"]
             if "session" in authdict:
                 sid = authdict["session"]

synapse/rest/client/keys.py

@@ -48,6 +48,7 @@ from synapse.logging.opentracing import set_tag
 from synapse.rest.client._base import client_patterns, interactive_auth_handler
 from synapse.types import JsonDict, StreamToken
 from synapse.types.rest import RequestBodyModel
+from synapse.types.rest.client import UserInteractiveAuthBody
 from synapse.util.cancellation import cancellable
 
 if TYPE_CHECKING:
@@ -518,6 +519,9 @@ class SigningKeyUploadServlet(RestServlet):
         requester = await self.auth.get_user_by_req(request)
         user_id = requester.user.to_string()
         body = parse_json_object_from_request(request)
+        # Only the `auth` field is validated here. The raw body is kept because
+        # `check_ui_auth` stores it when it creates a new UIA session.
+        validate_json_object(body, UserInteractiveAuthBody)
 
         (
             is_cross_signing_setup,

synapse/rest/client/login_token_request.py

@@ -25,10 +25,15 @@ from typing import TYPE_CHECKING
 from synapse.api.ratelimiting import Ratelimiter
 from synapse.config.ratelimiting import RatelimitSettings
 from synapse.http.server import HttpServer
-from synapse.http.servlet import RestServlet, parse_json_object_from_request
+from synapse.http.servlet import (
+    RestServlet,
+    parse_json_object_from_request,
+    validate_json_object,
+)
 from synapse.http.site import SynapseRequest
 from synapse.rest.client._base import client_patterns, interactive_auth_handler
 from synapse.types import JsonDict
+from synapse.types.rest.client import UserInteractiveAuthBody
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
@@ -92,6 +97,9 @@ class LoginTokenRequestServlet(RestServlet):
     async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]:
         requester = await self.auth.get_user_by_req(request)
         body = parse_json_object_from_request(request)
+        # Only the `auth` field is validated here. The raw body is kept because
+        # `check_ui_auth` stores it when it creates a new UIA session.
+        validate_json_object(body, UserInteractiveAuthBody)
 
         if self._require_ui_auth:
             await self.auth_handler.validate_user_via_ui_auth(

synapse/rest/client/register.py

@@ -54,11 +54,13 @@ from synapse.http.servlet import (
     assert_params_in_dict,
     parse_json_object_from_request,
     parse_string,
+    validate_json_object,
 )
 from synapse.http.site import SynapseRequest
 from synapse.metrics import SERVER_NAME_LABEL, threepid_send_requests
 from synapse.push.mailer import Mailer
 from synapse.types import JsonDict
+from synapse.types.rest.client import UserInteractiveAuthBody
 from synapse.util.duration import Duration
 from synapse.util.msisdn import phone_number_to_msisdn
 from synapse.util.ratelimitutils import FederationRateLimiter
@@ -469,6 +471,9 @@ class RegisterRestServlet(RestServlet):
     @interactive_auth_handler
     async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]:
         body = parse_json_object_from_request(request)
+        # Only the `auth` field is validated here. The raw body is kept because
+        # `check_ui_auth` stores it when it creates a new UIA session.
+        validate_json_object(body, UserInteractiveAuthBody)
 
         client_addr = request.getClientAddress().host
 

synapse/types/rest/client/__init__.py

@@ -58,6 +58,17 @@ class AuthenticationData(RequestBodyModel):
     type: StrictStr | None = None
 
 
+class UserInteractiveAuthBody(RequestBodyModel):
+    """
+    A request body that may carry user-interactive authentication data. Other keys
+    are endpoint-specific and are left for the endpoint to handle.
+    """
+
+    model_config = ConfigDict(extra="allow")
+
+    auth: AuthenticationData | None = None
+
+
 # See also assert_valid_client_secret()
 ClientSecretStr = Annotated[
     str,

tests/rest/client/test_auth.py

@@ -344,6 +344,16 @@ class UIAuthTests(unittest.HomeserverTestCase):
             },
         )
 
+    def test_ui_auth_null_auth(self) -> None:
+        """
+        A null `auth` value is treated as if it was omitted.
+        """
+        channel = self.delete_device(
+            self.user_tok, self.device_id, HTTPStatus.UNAUTHORIZED, {"auth": None}
+        )
+        self.assertIn("session", channel.json_body)
+        self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
+
     @override_config({"password_config": {"enabled": "only_for_reauth"}})
     def test_ui_auth_with_passwords_for_reauth_only(self) -> None:
         """

tests/rest/client/test_keys.py

@@ -342,6 +342,43 @@ class KeyQueryTestCase(unittest.HomeserverTestCase):
         )
         self.assertEqual(channel.code, HTTPStatus.OK, channel.result)
 
+    def test_device_signing_null_or_non_object_auth(self) -> None:
+        """A null `auth` asks for UIA like an omitted one, and a non-object `auth`
+        is rejected, rather than either causing a 500."""
+        password = "wonderland"
+        device_id = "ABCDEFGHI"
+        alice_id = self.register_user("alice", password)
+        alice_token = self.login("alice", password, device_id=device_id)
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/device_signing/upload",
+            self.make_device_keys(alice_id, device_id),
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.OK, channel.result)
+
+        keys = self.make_device_keys(alice_id, device_id)
+        keys["auth"] = None
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/device_signing/upload",
+            keys,
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, channel.result)
+        self.assertIn("session", channel.json_body)
+
+        keys["auth"] = "m.login.password"
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/device_signing/upload",
+            keys,
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(channel.json_body["errcode"], Codes.BAD_JSON)
+
 
 class SigningKeyUploadServletTestCase(unittest.HomeserverTestCase):
     servlets = [

tests/rest/client/test_login_token_request.py

@@ -19,8 +19,11 @@
 #
 #
 
+from http import HTTPStatus
+
 from twisted.internet.testing import MemoryReactor
 
+from synapse.api.errors import Codes
 from synapse.rest import admin
 from synapse.rest.client import login, login_token_request, versions
 from synapse.server import HomeServer
@@ -112,6 +115,31 @@ class LoginTokenRequestServletTestCase(unittest.HomeserverTestCase):
         self.assertEqual(channel.code, 200, channel.result)
         self.assertEqual(channel.json_body["user_id"], user_id)
 
+    @override_config({"login_via_existing_session": {"enabled": True}})
+    def test_uia_null_auth(self) -> None:
+        """A null `auth` value is treated as if it was omitted."""
+        self.register_user(self.user, self.password)
+        token = self.login(self.user, self.password)
+
+        channel = self.make_request(
+            "POST", GET_TOKEN_ENDPOINT, {"auth": None}, access_token=token
+        )
+        self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, msg=channel.result)
+        self.assertIn("session", channel.json_body)
+        self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
+
+    @override_config({"login_via_existing_session": {"enabled": True}})
+    def test_uia_non_object_auth(self) -> None:
+        """A non-object `auth` value is rejected with a 400."""
+        self.register_user(self.user, self.password)
+        token = self.login(self.user, self.password)
+
+        channel = self.make_request(
+            "POST", GET_TOKEN_ENDPOINT, {"auth": "m.login.password"}, access_token=token
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, msg=channel.result)
+        self.assertEqual(channel.json_body["errcode"], Codes.BAD_JSON)
+
     @override_config(
         {"login_via_existing_session": {"enabled": True, "require_ui_auth": False}}
     )

tests/rest/client/test_register.py

@@ -22,6 +22,7 @@
 import datetime
 import importlib.resources as importlib_resources
 import os
+from http import HTTPStatus
 from typing import Any, cast
 from unittest.mock import AsyncMock
 
@@ -220,6 +221,22 @@ class RegisterRestServletTestCase(unittest.HomeserverTestCase):
         self.assertEqual(channel.code, 200, msg=channel.result)
         self.assertLessEqual(det_data.items(), channel.json_body.items())
 
+    def test_POST_null_auth(self) -> None:
+        """A null `auth` value is treated as if it was omitted."""
+        request_data = {"username": "kermit", "password": "monkey", "auth": None}
+        channel = self.make_request(b"POST", self.url, request_data)
+
+        self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, msg=channel.result)
+        self.assertIn("session", channel.json_body)
+
+    def test_POST_non_object_auth(self) -> None:
+        """A non-object `auth` value is rejected with a 400."""
+        request_data = {"username": "kermit", "password": "monkey", "auth": ["session"]}
+        channel = self.make_request(b"POST", self.url, request_data)
+
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, msg=channel.result)
+        self.assertEqual(channel.json_body["errcode"], Codes.BAD_JSON)
+
     @override_config({"enable_registration": False})
     def test_POST_disabled_registration(self) -> None:
         request_data = {"username": "kermit", "password": "monkey"}