From 96083948878a8e1d1cf25576117dbe18aec8b2db Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Thu, 15 Feb 2024 09:31:35 +0100 Subject: [PATCH 01/11] Adds parse_json & parse_json_from_args function. For parsing JSON parameter from the request query string. (Used for Filtering) --- synapse/http/servlet.py | 81 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index b22eb727b15..afa9d293aa6 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -22,6 +22,7 @@ """ This module contains base REST classes for constructing REST servlets. """ import enum import logging +import urllib.parse as urlparse from http import HTTPStatus from typing import ( TYPE_CHECKING, @@ -428,6 +429,86 @@ def parse_string( ) +def parse_json( + request: Request, + name: str, + default: Optional[dict] = None, + required: bool = False, + encoding: str = "ascii", +) -> Optional[JsonDict]: + """ + Parse a JSON parameter from the request query string. + + Args: + request: the twisted HTTP request. + name: the name of the query parameter. + default: value to use if the parameter is absent, + defaults to None. + required: whether to raise a 400 SynapseError if the + parameter is absent, defaults to False. + encoding: The encoding to decode the string content with. + + Returns: + A JSON value. + + Raises: + SynapseError if the parameter is absent or if the + parameter is present and not a JSON object. + """ + args: Mapping[bytes, Sequence[bytes]] = request.args # type: ignore + return parse_json_from_args( + args, + name, + default, + required=required, + encoding=encoding, + ) + + +def parse_json_from_args( + args: Mapping[bytes, Sequence[bytes]], + name: str, + default: Optional[dict] = None, + required: bool = False, + encoding: str = "ascii", +) -> Optional[JsonDict]: + """ + Parse a JSON parameter from the request query string. + + Args: + args: A mapping of request args as bytes to a list of bytes (e.g. request.args). + name: the name of the query parameter. + default: value to use if the parameter is absent, + defaults to None. + required: whether to raise a 400 SynapseError if the + parameter is absent, defaults to False. + encoding: The encoding to decode the string content with. + + Returns: + A JSON Object or the default. + + Raises: + SynapseError if the parameter is absent and required, or if the + parameter is present and not a JSON object. + """ + name_bytes = name.encode("ascii") + + if name_bytes not in args: + if not required: + return default + + message = f"Missing required integer query parameter {name}" + raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.MISSING_PARAM) + + json_str = parse_string_from_args(args, name, required=True, encoding=encoding) + + try: + return json_decoder.decode(urlparse.unquote(json_str)) + except Exception: + message = f"Query parameter {name} must be a valid JSON object" + raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM) + + EnumT = TypeVar("EnumT", bound=enum.Enum) From a040208bfbb318588876fe5683ffcbefbd64279a Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Thu, 15 Feb 2024 09:38:38 +0100 Subject: [PATCH 02/11] Implements parse_json function at filter_json appearances. Validating the JSON query parameter should fix #16922 --- synapse/rest/admin/rooms.py | 34 ++++++++++++---------------------- synapse/rest/client/room.py | 35 ++++++++++++----------------------- 2 files changed, 24 insertions(+), 45 deletions(-) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 4252f98a6c3..ef8e2ccc5ef 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -38,6 +38,7 @@ assert_params_in_dict, parse_enum, parse_integer, + parse_json, parse_json_object_from_request, parse_string, ) @@ -776,14 +777,8 @@ async def on_GET( limit = parse_integer(request, "limit", default=10) # picking the API shape for symmetry with /messages - filter_str = parse_string(request, "filter", encoding="utf-8") - if filter_str: - filter_json = urlparse.unquote(filter_str) - event_filter: Optional[Filter] = Filter( - self._hs, json_decoder.decode(filter_json) - ) - else: - event_filter = None + filter_json = parse_json(request, "filter", encoding="utf-8") + event_filter = Filter(self._hs, filter_json) if filter_json else None event_context = await self.room_context_handler.get_event_context( requester, @@ -914,21 +909,16 @@ async def on_GET( ) # Twisted will have processed the args by now. assert request.args is not None + + filter_json = parse_json(request, "filter", encoding="utf-8") + event_filter = Filter(self._hs, filter_json) if filter_json else None + as_client_event = b"raw" not in request.args - filter_str = parse_string(request, "filter", encoding="utf-8") - if filter_str: - filter_json = urlparse.unquote(filter_str) - event_filter: Optional[Filter] = Filter( - self._hs, json_decoder.decode(filter_json) - ) - if ( - event_filter - and event_filter.filter_json.get("event_format", "client") - == "federation" - ): - as_client_event = False - else: - event_filter = None + if ( + event_filter + and event_filter.filter_json.get("event_format", "client") == "federation" + ): + as_client_event = False msgs = await self._pagination_handler.get_messages( room_id=room_id, diff --git a/synapse/rest/client/room.py b/synapse/rest/client/room.py index 65dedb8b921..849fe999a30 100644 --- a/synapse/rest/client/room.py +++ b/synapse/rest/client/room.py @@ -52,6 +52,7 @@ parse_boolean, parse_enum, parse_integer, + parse_json, parse_json_object_from_request, parse_string, parse_strings_from_args, @@ -65,7 +66,6 @@ from synapse.streams.config import PaginationConfig from synapse.types import JsonDict, Requester, StreamToken, ThirdPartyInstanceID, UserID from synapse.types.state import StateFilter -from synapse.util import json_decoder from synapse.util.cancellation import cancellable from synapse.util.stringutils import parse_and_validate_server_name, random_string @@ -703,21 +703,16 @@ async def on_GET( ) # Twisted will have processed the args by now. assert request.args is not None + + filter_json = parse_json(request, "filter", encoding="utf-8") + event_filter = Filter(self._hs, filter_json) if filter_json else None + as_client_event = b"raw" not in request.args - filter_str = parse_string(request, "filter", encoding="utf-8") - if filter_str: - filter_json = urlparse.unquote(filter_str) - event_filter: Optional[Filter] = Filter( - self._hs, json_decoder.decode(filter_json) - ) - if ( - event_filter - and event_filter.filter_json.get("event_format", "client") - == "federation" - ): - as_client_event = False - else: - event_filter = None + if ( + event_filter + and event_filter.filter_json.get("event_format", "client") == "federation" + ): + as_client_event = False msgs = await self.pagination_handler.get_messages( room_id=room_id, @@ -898,14 +893,8 @@ async def on_GET( limit = parse_integer(request, "limit", default=10) # picking the API shape for symmetry with /messages - filter_str = parse_string(request, "filter", encoding="utf-8") - if filter_str: - filter_json = urlparse.unquote(filter_str) - event_filter: Optional[Filter] = Filter( - self._hs, json_decoder.decode(filter_json) - ) - else: - event_filter = None + filter_json = parse_json(request, "filter", encoding="utf-8") + event_filter = Filter(self._hs, filter_json) if filter_json else None event_context = await self.room_context_handler.get_event_context( requester, room_id, event_id, limit, event_filter From 96a05b23e7a8ceaadce708f660f19abb5154fd74 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Thu, 15 Feb 2024 10:02:09 +0100 Subject: [PATCH 03/11] Newsfile --- changelog.d/16923.bugfix | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/16923.bugfix diff --git a/changelog.d/16923.bugfix b/changelog.d/16923.bugfix new file mode 100644 index 00000000000..f620d091892 --- /dev/null +++ b/changelog.d/16923.bugfix @@ -0,0 +1,3 @@ +Adds parse_json servlet function for standardized JSON parsing from query parameters, ensuring enhanced data validation and error handling. +Introduces INVALID_PARAM error response for invalid JSON objects, improving parameter validation feedback. +Adds validation check to prevent 500 internal server error on invalid Json Filter request. \ No newline at end of file From 3cee2e994c08d88a72d34acab728324f54529260 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 20 Feb 2024 13:45:30 +0100 Subject: [PATCH 04/11] Apply suggestions from code review Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/http/servlet.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index afa9d293aa6..2ebc779ca8e 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -445,14 +445,15 @@ def parse_json( default: value to use if the parameter is absent, defaults to None. required: whether to raise a 400 SynapseError if the - parameter is absent, defaults to False. + parameter is absent, defaults to False. encoding: The encoding to decode the string content with. Returns: - A JSON value. + A JSON value, or `default` if the named query parameter was not found + and `required` was False. Raises: - SynapseError if the parameter is absent or if the + SynapseError if the parameter is absent and required, or if the parameter is present and not a JSON object. """ args: Mapping[bytes, Sequence[bytes]] = request.args # type: ignore @@ -476,16 +477,16 @@ def parse_json_from_args( Parse a JSON parameter from the request query string. Args: - args: A mapping of request args as bytes to a list of bytes (e.g. request.args). + args: a mapping of request args as bytes to a list of bytes (e.g. request.args). name: the name of the query parameter. default: value to use if the parameter is absent, defaults to None. required: whether to raise a 400 SynapseError if the parameter is absent, defaults to False. - encoding: The encoding to decode the string content with. + encoding: the encoding to decode the string content with. - Returns: - A JSON Object or the default. + A JSON value, or `default` if the named query parameter was not found + and `required` was False. Raises: SynapseError if the parameter is absent and required, or if the From 279bf93824f9adeb87f6d1a0dac0425b622cb810 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 20 Feb 2024 17:09:56 +0100 Subject: [PATCH 05/11] Adds client filter_query_validation tests --- tests/rest/client/test_rooms.py | 52 +++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index b11a73e92be..ef70710785d 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -2157,6 +2157,31 @@ def test_room_messages_purge(self) -> None: chunk = channel.json_body["chunk"] self.assertEqual(len(chunk), 0, [event["content"] for event in chunk]) + def test_room_message_filter_query_validation(self) -> None: + # Test json validation in (filter) query parameter. + # Does not test the validity of the filter, only the json validation. + + # Check Get with valid json filter parameter, expect 200. + _valid_filter_str = '{"types": ["m.room.message"]}' + channel = self.make_request( + "GET", + f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={_valid_filter_str}", + ) + + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + _invalid_filter_str = "}}}{}" + channel = self.make_request( + "GET", + f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={_invalid_filter_str}", + ) + + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) + self.assertEqual( + channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + ) + class RoomMessageFilterTestCase(RoomBase): """Tests /rooms/$room_id/messages REST events.""" @@ -3195,6 +3220,33 @@ def test_erased_sender(self) -> None: self.assertDictEqual(events_after[0].get("content"), {}, events_after[0]) self.assertEqual(events_after[1].get("content"), {}, events_after[1]) + def test_room_event_context_filter_query_validation(self) -> None: + # Test json validation in (filter) query parameter. + # Does not test the validity of the filter, only the json validation. + event_id = self.helper.send(self.room_id, "message 7", tok=self.tok)["event_id"] + + # Check Get with valid json filter parameter, expect 200. + _valid_filter_str = '{"types": ["m.room.message"]}' + channel = self.make_request( + "GET", + f"/rooms/{self.room_id}/context/{event_id}?filter={_valid_filter_str}", + access_token=self.tok, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + _invalid_filter_str = "}}}{}" + channel = self.make_request( + "GET", + f"/rooms/{self.room_id}/context/{event_id}?filter={_invalid_filter_str}", + access_token=self.tok, + ) + + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) + self.assertEqual( + channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + ) + class RoomAliasListTestCase(unittest.HomeserverTestCase): servlets = [ From 9c5152a72cf58ab8fdfd77970af34085bde37c92 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 20 Feb 2024 17:10:15 +0100 Subject: [PATCH 06/11] Adds admin filter_query_validation tests --- tests/rest/admin/test_room.py | 61 +++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 0b669b6ee7c..cd04fd04fdf 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -21,6 +21,7 @@ import json import time import urllib.parse +from http import HTTPStatus from typing import List, Optional from unittest.mock import AsyncMock, Mock @@ -2190,6 +2191,33 @@ def test_room_messages_purge(self) -> None: chunk = channel.json_body["chunk"] self.assertEqual(len(chunk), 0, [event["content"] for event in chunk]) + def test_room_message_filter_query_validation(self) -> None: + # Test json validation in (filter) query parameter. + # Does not test the validity of the filter, only the json validation. + + # Check Get with valid json filter parameter, expect 200. + _valid_filter_str = '{"types": ["m.room.message"]}' + channel = self.make_request( + "GET", + f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={_valid_filter_str}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + _invalid_filter_str = "}}}{}" + channel = self.make_request( + "GET", + f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={_invalid_filter_str}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) + self.assertEqual( + channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + ) + class JoinAliasRoomTestCase(unittest.HomeserverTestCase): servlets = [ @@ -2522,6 +2550,39 @@ def test_context_as_admin(self) -> None: else: self.fail("Event %s from events_after not found" % j) + def test_room_event_context_filter_query_validation(self) -> None: + # Test json validation in (filter) query parameter. + # Does not test the validity of the filter, only the json validation. + + # Create a user with room and event_id. + user_id = self.register_user("test", "test") + user_tok = self.login("test", "test") + room_id = self.helper.create_room_as(user_id, tok=user_tok) + event_id = self.helper.send(room_id, "message 1", tok=user_tok)["event_id"] + + # Check Get with valid json filter parameter, expect 200. + _valid_filter_str = '{"types": ["m.room.message"]}' + channel = self.make_request( + "GET", + f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={_valid_filter_str}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + _invalid_filter_str = "}}}{}" + channel = self.make_request( + "GET", + f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={_invalid_filter_str}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) + self.assertEqual( + channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + ) + class MakeRoomAdminTestCase(unittest.HomeserverTestCase): servlets = [ From f44b1bfa09378d763db355e50db59d3d7f5a483c Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Fri, 23 Feb 2024 11:40:06 +0100 Subject: [PATCH 07/11] Changes test helper variable naming. --- tests/rest/admin/test_room.py | 16 ++++++++-------- tests/rest/client/test_rooms.py | 16 ++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index cd04fd04fdf..71dc21be39f 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -2196,20 +2196,20 @@ def test_room_message_filter_query_validation(self) -> None: # Does not test the validity of the filter, only the json validation. # Check Get with valid json filter parameter, expect 200. - _valid_filter_str = '{"types": ["m.room.message"]}' + valid_filter_str = '{"types": ["m.room.message"]}' channel = self.make_request( "GET", - f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={_valid_filter_str}", + f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={valid_filter_str}", access_token=self.admin_user_tok, ) self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. - _invalid_filter_str = "}}}{}" + invalid_filter_str = "}}}{}" channel = self.make_request( "GET", - f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={_invalid_filter_str}", + f"/_synapse/admin/v1/rooms/{self.room_id}/messages?dir=b&filter={invalid_filter_str}", access_token=self.admin_user_tok, ) @@ -2561,20 +2561,20 @@ def test_room_event_context_filter_query_validation(self) -> None: event_id = self.helper.send(room_id, "message 1", tok=user_tok)["event_id"] # Check Get with valid json filter parameter, expect 200. - _valid_filter_str = '{"types": ["m.room.message"]}' + valid_filter_str = '{"types": ["m.room.message"]}' channel = self.make_request( "GET", - f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={_valid_filter_str}", + f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={valid_filter_str}", access_token=self.admin_user_tok, ) self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. - _invalid_filter_str = "}}}{}" + invalid_filter_str = "}}}{}" channel = self.make_request( "GET", - f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={_invalid_filter_str}", + f"/_synapse/admin/v1/rooms/{room_id}/context/{event_id}?filter={invalid_filter_str}", access_token=self.admin_user_tok, ) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index ef70710785d..2ae6960eebb 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -2162,19 +2162,19 @@ def test_room_message_filter_query_validation(self) -> None: # Does not test the validity of the filter, only the json validation. # Check Get with valid json filter parameter, expect 200. - _valid_filter_str = '{"types": ["m.room.message"]}' + valid_filter_str = '{"types": ["m.room.message"]}' channel = self.make_request( "GET", - f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={_valid_filter_str}", + f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={valid_filter_str}", ) self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. - _invalid_filter_str = "}}}{}" + invalid_filter_str = "}}}{}" channel = self.make_request( "GET", - f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={_invalid_filter_str}", + f"/rooms/{self.room_id}/messages?access_token=x&dir=b&filter={invalid_filter_str}", ) self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) @@ -3226,19 +3226,19 @@ def test_room_event_context_filter_query_validation(self) -> None: event_id = self.helper.send(self.room_id, "message 7", tok=self.tok)["event_id"] # Check Get with valid json filter parameter, expect 200. - _valid_filter_str = '{"types": ["m.room.message"]}' + valid_filter_str = '{"types": ["m.room.message"]}' channel = self.make_request( "GET", - f"/rooms/{self.room_id}/context/{event_id}?filter={_valid_filter_str}", + f"/rooms/{self.room_id}/context/{event_id}?filter={valid_filter_str}", access_token=self.tok, ) self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. - _invalid_filter_str = "}}}{}" + invalid_filter_str = "}}}{}" channel = self.make_request( "GET", - f"/rooms/{self.room_id}/context/{event_id}?filter={_invalid_filter_str}", + f"/rooms/{self.room_id}/context/{event_id}?filter={invalid_filter_str}", access_token=self.tok, ) From f5f8864e561b52b4d1a8f6d26d011e0b3c557158 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Wed, 20 Mar 2024 11:14:45 +0100 Subject: [PATCH 08/11] =?UTF-8?q?Applies=C2=A0lint=20changes.=20(removes?= =?UTF-8?q?=20unused=20imports)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- synapse/rest/admin/rooms.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index ef8e2ccc5ef..0d86a4e15f1 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -21,7 +21,6 @@ import logging from http import HTTPStatus from typing import TYPE_CHECKING, List, Optional, Tuple, cast -from urllib import parse as urlparse import attr @@ -52,7 +51,6 @@ from synapse.streams.config import PaginationConfig from synapse.types import JsonDict, RoomID, ScheduledTask, UserID, create_requester from synapse.types.state import StateFilter -from synapse.util import json_decoder if TYPE_CHECKING: from synapse.api.auth import Auth @@ -909,7 +907,7 @@ async def on_GET( ) # Twisted will have processed the args by now. assert request.args is not None - + filter_json = parse_json(request, "filter", encoding="utf-8") event_filter = Filter(self._hs, filter_json) if filter_json else None From 0634aeb8e9725fcc16092dbc966d50ece13f8b92 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 16 Apr 2024 19:12:21 +0200 Subject: [PATCH 09/11] updates changelog suggestion with NOT_JSON --- changelog.d/16923.bugfix | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/changelog.d/16923.bugfix b/changelog.d/16923.bugfix index f620d091892..bd6f24925ee 100644 --- a/changelog.d/16923.bugfix +++ b/changelog.d/16923.bugfix @@ -1,3 +1 @@ -Adds parse_json servlet function for standardized JSON parsing from query parameters, ensuring enhanced data validation and error handling. -Introduces INVALID_PARAM error response for invalid JSON objects, improving parameter validation feedback. -Adds validation check to prevent 500 internal server error on invalid Json Filter request. \ No newline at end of file +Return `400 M_NOT_JSON` upon receiving invalid JSON in query parameters across various client and admin endpoints, rather than an internal server error. \ No newline at end of file From 776dadb174ae641c0f5f44d80ae995f6ab21310c Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 16 Apr 2024 19:13:24 +0200 Subject: [PATCH 10/11] Adds 400 NOT_JSON code instead of INVALID_PARAM --- synapse/http/servlet.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index ea7dd857106..860e9dd87ca 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -489,7 +489,7 @@ def parse_json_from_args( return json_decoder.decode(urlparse.unquote(json_str)) except Exception: message = f"Query parameter {name} must be a valid JSON object" - raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM) + raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.NOT_JSON) EnumT = TypeVar("EnumT", bound=enum.Enum) From fa414ac83f610ac08a0fec4a488b47eb916e1f43 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 16 Apr 2024 19:14:41 +0200 Subject: [PATCH 11/11] Updates new test cases with NOT_JSON error --- tests/rest/admin/test_room.py | 8 ++++---- tests/rest/client/test_rooms.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 71dc21be39f..75627472605 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -2205,7 +2205,7 @@ def test_room_message_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) - # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + # Check Get with invalid json filter parameter, expect 400 NOT_JSON. invalid_filter_str = "}}}{}" channel = self.make_request( "GET", @@ -2215,7 +2215,7 @@ def test_room_message_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) self.assertEqual( - channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + channel.json_body["errcode"], Codes.NOT_JSON, channel.json_body ) @@ -2570,7 +2570,7 @@ def test_room_event_context_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) - # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + # Check Get with invalid json filter parameter, expect 400 NOT_JSON. invalid_filter_str = "}}}{}" channel = self.make_request( "GET", @@ -2580,7 +2580,7 @@ def test_room_event_context_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) self.assertEqual( - channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + channel.json_body["errcode"], Codes.NOT_JSON, channel.json_body ) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index fa62b169a05..b796163dcbb 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -2188,7 +2188,7 @@ def test_room_message_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) - # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + # Check Get with invalid json filter parameter, expect 400 NOT_JSON. invalid_filter_str = "}}}{}" channel = self.make_request( "GET", @@ -2197,7 +2197,7 @@ def test_room_message_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) self.assertEqual( - channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + channel.json_body["errcode"], Codes.NOT_JSON, channel.json_body ) @@ -3252,7 +3252,7 @@ def test_room_event_context_filter_query_validation(self) -> None: ) self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) - # Check Get with invalid json filter parameter, expect 400 INVALID_PARAM. + # Check Get with invalid json filter parameter, expect 400 NOT_JSON. invalid_filter_str = "}}}{}" channel = self.make_request( "GET", @@ -3262,7 +3262,7 @@ def test_room_event_context_filter_query_validation(self) -> None: self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.json_body) self.assertEqual( - channel.json_body["errcode"], Codes.INVALID_PARAM, channel.json_body + channel.json_body["errcode"], Codes.NOT_JSON, channel.json_body )