From 18c10b8075db235e9b8f8d9babc68db444aa0648 Mon Sep 17 00:00:00 2001 From: Ankit Jha Date: Tue, 29 Sep 2026 00:06:39 +0530 Subject: [PATCH] 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 --- changelog.d/20274.bugfix | 1 + synapse/handlers/auth.py | 8 +++- synapse/rest/client/keys.py | 4 ++ synapse/rest/client/login_token_request.py | 10 ++++- synapse/rest/client/register.py | 5 +++ synapse/types/rest/client/__init__.py | 11 ++++++ tests/rest/client/test_auth.py | 10 +++++ tests/rest/client/test_keys.py | 37 +++++++++++++++++++ tests/rest/client/test_login_token_request.py | 28 ++++++++++++++ tests/rest/client/test_register.py | 17 +++++++++ 10 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 changelog.d/20274.bugfix diff --git a/changelog.d/20274.bugfix b/changelog.d/20274.bugfix new file mode 100644 index 0000000000..88d371cd7a --- /dev/null +++ b/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. diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index fc4373e0a5..47f83d4aad 100644 --- a/synapse/handlers/auth.py +++ b/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"] diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 7ffde7ac04..45e94a25b4 100644 --- a/synapse/rest/client/keys.py +++ b/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, diff --git a/synapse/rest/client/login_token_request.py b/synapse/rest/client/login_token_request.py index f455e9c0b7..17cfc29f17 100644 --- a/synapse/rest/client/login_token_request.py +++ b/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( diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 8355d0c571..6e4bb400af 100644 --- a/synapse/rest/client/register.py +++ b/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 diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index dbd4455b82..cd1bd1f057 100644 --- a/synapse/types/rest/client/__init__.py +++ b/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, diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 4eb918d5c5..38196ca2fc 100644 --- a/tests/rest/client/test_auth.py +++ b/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: """ diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 4f3d6a365f..c957354cfe 100644 --- a/tests/rest/client/test_keys.py +++ b/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 = [ diff --git a/tests/rest/client/test_login_token_request.py b/tests/rest/client/test_login_token_request.py index b387cb98fb..28a0fca9ce 100644 --- a/tests/rest/client/test_login_token_request.py +++ b/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}} ) diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 67f78ee24f..2937bb030c 100644 --- a/tests/rest/client/test_register.py +++ b/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"}