From daa1448442b9e474401a82555a5ae1a3aa316c91 Mon Sep 17 00:00:00 2001 From: Joshua Fenster Date: Wed, 14 Feb 2024 14:37:38 +0100 Subject: [PATCH 1/9] expands function parse_integer with an optional negative value check expands function parse_integer_from_args to raise INVALID_PARAM on a forbidden negative value. --- synapse/http/servlet.py | 63 +++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index b22eb727b15..963e3b6976a 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -72,13 +72,21 @@ def parse_integer(request: Request, name: str, *, required: Literal[True]) -> in @overload def parse_integer( - request: Request, name: str, default: Optional[int] = None, required: bool = False -) -> Optional[int]: + request: Request, + name: str, + default: Optional[int] = None, + required: bool = False, + negative: bool = False, +) -> int: ... def parse_integer( - request: Request, name: str, default: Optional[int] = None, required: bool = False + request: Request, + name: str, + default: Optional[int] = None, + required: bool = False, + negative: bool = False, ) -> Optional[int]: """Parse an integer parameter from the request string @@ -88,16 +96,17 @@ def parse_integer( 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. - + negative: whether to allow negative integers, defaults to True. Returns: An int value or the default. Raises: - SynapseError: if the parameter is absent and required, or if the - parameter is present and not an integer. + SynapseError: if the parameter is absent and required, if the + parameter is present and not an integer, or if the + parameter is illegitimate negative. """ args: Mapping[bytes, Sequence[bytes]] = request.args # type: ignore - return parse_integer_from_args(args, name, default, required) + return parse_integer_from_args(args, name, default, required, negative) @overload @@ -125,6 +134,7 @@ def parse_integer_from_args( name: str, default: Optional[int] = None, required: bool = False, + negative: bool = False, ) -> Optional[int]: ... @@ -134,6 +144,7 @@ def parse_integer_from_args( name: str, default: Optional[int] = None, required: bool = False, + negative: bool = True, ) -> Optional[int]: """Parse an integer parameter from the request string @@ -143,33 +154,37 @@ def parse_integer_from_args( 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. + negative: whether to allow negative integers, defaults to True. Returns: An int value or the default. Raises: - SynapseError: if the parameter is absent and required, or if the - parameter is present and not an integer. + SynapseError: if the parameter is absent and required, if the + parameter is present and not an integer, or if the + parameter is illegitimate negative. """ name_bytes = name.encode("ascii") - if name_bytes in args: - try: - return int(args[name_bytes][0]) - except Exception: - message = "Query parameter %r must be an integer" % (name,) - raise SynapseError( - HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM - ) - else: - if required: - message = "Missing integer query parameter %r" % (name,) - raise SynapseError( - HTTPStatus.BAD_REQUEST, message, errcode=Codes.MISSING_PARAM - ) - else: + 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) + + try: + integer = int(args[name_bytes][0]) + except Exception: + message = f"Query parameter {name} must be an integer" + raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM) + + if not negative and integer < 0: + message = f"Query parameter {name} must be a positive integer." + raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.INVALID_PARAM) + + return integer + @overload def parse_boolean(request: Request, name: str, default: bool) -> bool: From c63ba0aab525b92550aa6f8858413fcc016695c3 Mon Sep 17 00:00:00 2001 From: Joshua Fenster Date: Wed, 14 Feb 2024 14:39:05 +0100 Subject: [PATCH 2/9] Adds limit parameter negative value validation check fixes #16918 500 internal server error on negative limit parameter (with PostgreSQL) --- synapse/rest/client/room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/room.py b/synapse/rest/client/room.py index 65dedb8b921..4eeadf8779a 100644 --- a/synapse/rest/client/room.py +++ b/synapse/rest/client/room.py @@ -499,7 +499,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if server: raise e - limit: Optional[int] = parse_integer(request, "limit", 0) + limit: Optional[int] = parse_integer(request, "limit", 0, negative=False) since_token = parse_string(request, "since") if limit == 0: From 2d18bef96f25548526425d7d76f8d86063d5959e Mon Sep 17 00:00:00 2001 From: Joshua Fenster Date: Wed, 14 Feb 2024 14:43:50 +0100 Subject: [PATCH 3/9] Adds negative value validation to parse_integer functions. Removing duplicate negative value check logics blocks. --- synapse/rest/admin/federation.py | 36 +++------------------ synapse/rest/admin/media.py | 54 +++++--------------------------- synapse/rest/admin/statistics.py | 36 +++------------------ synapse/rest/admin/users.py | 18 ++--------- 4 files changed, 19 insertions(+), 125 deletions(-) diff --git a/synapse/rest/admin/federation.py b/synapse/rest/admin/federation.py index 045153e0cb7..d41ce9aa4c4 100644 --- a/synapse/rest/admin/federation.py +++ b/synapse/rest/admin/federation.py @@ -61,22 +61,8 @@ def __init__(self, hs: "HomeServer"): async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await assert_requester_is_admin(self._auth, request) - start = parse_integer(request, "from", default=0) - limit = parse_integer(request, "limit", default=100) - - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) destination = parse_string(request, "destination") @@ -195,22 +181,8 @@ async def on_GET( if not await self._store.is_destination_known(destination): raise NotFoundError("Unknown destination") - start = parse_integer(request, "from", default=0) - limit = parse_integer(request, "limit", default=100) - - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) direction = parse_enum(request, "dir", Direction, default=Direction.FORWARDS) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 27f0808658e..dbfdaa3bfeb 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -311,29 +311,18 @@ async def on_POST( ) -> Tuple[int, JsonDict]: await assert_requester_is_admin(self.auth, request) - before_ts = parse_integer(request, "before_ts", required=True) - size_gt = parse_integer(request, "size_gt", default=0) + before_ts = parse_integer(request, "before_ts", required=True, negative=False) + size_gt = parse_integer(request, "size_gt", default=0, negative=False) keep_profiles = parse_boolean(request, "keep_profiles", default=True) - if before_ts < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter before_ts must be a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - elif before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds + if before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds raise SynapseError( HTTPStatus.BAD_REQUEST, "Query parameter before_ts you provided is from the year 1970. " + "Double check that you are providing a timestamp in milliseconds.", errcode=Codes.INVALID_PARAM, ) - if size_gt < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter size_gt must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + # This check is useless, we keep it for the legacy endpoint only. if server_name is not None and self.server_name != server_name: @@ -389,22 +378,8 @@ async def on_GET( if user is None: raise NotFoundError("Unknown user") - start = parse_integer(request, "from", default=0) - limit = parse_integer(request, "limit", default=100) - - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) # If neither `order_by` nor `dir` is set, set the default order # to newest media is on top for backward compatibility. @@ -447,22 +422,9 @@ async def on_DELETE( if user is None: raise NotFoundError("Unknown user") - start = parse_integer(request, "from", default=0) - limit = parse_integer(request, "limit", default=100) - - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) # If neither `order_by` nor `dir` is set, set the default order # to newest media is on top for backward compatibility. diff --git a/synapse/rest/admin/statistics.py b/synapse/rest/admin/statistics.py index 832f20402eb..167d486fb6a 100644 --- a/synapse/rest/admin/statistics.py +++ b/synapse/rest/admin/statistics.py @@ -63,38 +63,12 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ), ) - start = parse_integer(request, "from", default=0) - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - limit = parse_integer(request, "limit", default=100) - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - from_ts = parse_integer(request, "from_ts", default=0) - if from_ts < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from_ts must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - until_ts = parse_integer(request, "until_ts") + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) + from_ts = parse_integer(request, "from_ts", default=0, negative=False) + until_ts = parse_integer(request, "until_ts", negative=False) + if until_ts is not None: - if until_ts < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter until_ts must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) if until_ts <= from_ts: raise SynapseError( HTTPStatus.BAD_REQUEST, diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 0229e87f431..e640d427726 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -90,22 +90,8 @@ def __init__(self, hs: "HomeServer"): async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await assert_requester_is_admin(self.auth, request) - start = parse_integer(request, "from", default=0) - limit = parse_integer(request, "limit", default=100) - - if start < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter from must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) - - if limit < 0: - raise SynapseError( - HTTPStatus.BAD_REQUEST, - "Query parameter limit must be a string representing a positive integer.", - errcode=Codes.INVALID_PARAM, - ) + start = parse_integer(request, "from", default=0, negative=False) + limit = parse_integer(request, "limit", default=100, negative=False) user_id = parse_string(request, "user_id") name = parse_string(request, "name", encoding="utf-8") From 00cf8eee265e16fc94071e2f6b861cfc4f914159 Mon Sep 17 00:00:00 2001 From: Joshua Fenster Date: Wed, 14 Feb 2024 14:46:22 +0100 Subject: [PATCH 4/9] adjusts hard-coded INVALID_PARAM error messages in media test cases. --- tests/rest/admin/test_media.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 493e1d1919f..c8476039e9b 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -277,7 +277,7 @@ def test_missing_parameter(self) -> None: self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.MISSING_PARAM, channel.json_body["errcode"]) self.assertEqual( - "Missing integer query parameter 'before_ts'", channel.json_body["error"] + "Missing required integer query parameter before_ts", channel.json_body["error"] ) def test_invalid_parameter(self) -> None: @@ -320,7 +320,7 @@ def test_invalid_parameter(self) -> None: self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) self.assertEqual( - "Query parameter size_gt must be a string representing a positive integer.", + "Query parameter size_gt must be a positive integer.", channel.json_body["error"], ) From f97abbc2f67bf9938b1bb00f95d517e9dc849a08 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Wed, 14 Feb 2024 16:23:40 +0100 Subject: [PATCH 5/9] Newsfile --- changelog.d/16920.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16920.bugfix diff --git a/changelog.d/16920.bugfix b/changelog.d/16920.bugfix new file mode 100644 index 00000000000..12e4bd0dab9 --- /dev/null +++ b/changelog.d/16920.bugfix @@ -0,0 +1 @@ +Adds `limit` parameter negative value validation check to prevent 500 internal server error on publicRooms request. From 40a4fa120d79324191e521fa8faa867934a0d558 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Tue, 19 Mar 2024 17:09:25 +0000 Subject: [PATCH 6/9] Update changelog.d/16920.bugfix --- changelog.d/16920.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/16920.bugfix b/changelog.d/16920.bugfix index 12e4bd0dab9..460f4f71601 100644 --- a/changelog.d/16920.bugfix +++ b/changelog.d/16920.bugfix @@ -1 +1 @@ -Adds `limit` parameter negative value validation check to prevent 500 internal server error on publicRooms request. +Adds validation to ensure that the `limit` parameter on `/publicRooms` is non-negative. From 5c71b504e0f9bc38f1b683ae90fead3362dcdaf1 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Wed, 20 Mar 2024 10:57:32 +0100 Subject: [PATCH 7/9] =?UTF-8?q?Applies=C2=A0black=20Bump=C2=A0from=2023.10?= =?UTF-8?q?.1=20to=2024.2.0=20(#16936)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- synapse/http/servlet.py | 6 ++---- synapse/rest/admin/federation.py | 2 +- synapse/rest/admin/media.py | 2 -- synapse/rest/admin/statistics.py | 2 +- tests/rest/admin/test_media.py | 3 ++- 5 files changed, 6 insertions(+), 9 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 597980256e7..9473fe2fb31 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -75,8 +75,7 @@ def parse_integer( default: Optional[int] = None, required: bool = False, negative: bool = False, -) -> int: - ... +) -> int: ... def parse_integer( @@ -131,8 +130,7 @@ def parse_integer_from_args( default: Optional[int] = None, required: bool = False, negative: bool = False, -) -> Optional[int]: - ... +) -> Optional[int]: ... def parse_integer_from_args( diff --git a/synapse/rest/admin/federation.py b/synapse/rest/admin/federation.py index d41ce9aa4c4..14ab4644cb9 100644 --- a/synapse/rest/admin/federation.py +++ b/synapse/rest/admin/federation.py @@ -23,7 +23,7 @@ from typing import TYPE_CHECKING, Tuple from synapse.api.constants import Direction -from synapse.api.errors import Codes, NotFoundError, SynapseError +from synapse.api.errors import NotFoundError, SynapseError from synapse.federation.transport.server import Authenticator from synapse.http.servlet import RestServlet, parse_enum, parse_integer, parse_string from synapse.http.site import SynapseRequest diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index dbfdaa3bfeb..a05b7252ec0 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -323,7 +323,6 @@ async def on_POST( errcode=Codes.INVALID_PARAM, ) - # This check is useless, we keep it for the legacy endpoint only. if server_name is not None and self.server_name != server_name: raise SynapseError(HTTPStatus.BAD_REQUEST, "Can only delete local media") @@ -425,7 +424,6 @@ async def on_DELETE( start = parse_integer(request, "from", default=0, negative=False) limit = parse_integer(request, "limit", default=100, negative=False) - # If neither `order_by` nor `dir` is set, set the default order # to newest media is on top for backward compatibility. if b"order_by" not in request.args and b"dir" not in request.args: diff --git a/synapse/rest/admin/statistics.py b/synapse/rest/admin/statistics.py index 167d486fb6a..dc27a41dd98 100644 --- a/synapse/rest/admin/statistics.py +++ b/synapse/rest/admin/statistics.py @@ -67,7 +67,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: limit = parse_integer(request, "limit", default=100, negative=False) from_ts = parse_integer(request, "from_ts", default=0, negative=False) until_ts = parse_integer(request, "until_ts", negative=False) - + if until_ts is not None: if until_ts <= from_ts: raise SynapseError( diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index c8476039e9b..f3781655132 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -277,7 +277,8 @@ def test_missing_parameter(self) -> None: self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.MISSING_PARAM, channel.json_body["errcode"]) self.assertEqual( - "Missing required integer query parameter before_ts", channel.json_body["error"] + "Missing required integer query parameter before_ts", + channel.json_body["error"], ) def test_invalid_parameter(self) -> None: From 7a9c5d9e2de38eac70bfafa91edc9663dcfde44b Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Mon, 25 Mar 2024 15:45:37 +0100 Subject: [PATCH 8/9] Implements the correct default parse_integer implementation (mypy) --- synapse/rest/media/preview_url_resource.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/rest/media/preview_url_resource.py b/synapse/rest/media/preview_url_resource.py index 6724986fccc..bfeff2179b8 100644 --- a/synapse/rest/media/preview_url_resource.py +++ b/synapse/rest/media/preview_url_resource.py @@ -72,9 +72,6 @@ async def on_GET(self, request: SynapseRequest) -> None: # XXX: if get_user_by_req fails, what should we do in an async render? requester = await self.auth.get_user_by_req(request) url = parse_string(request, "url", required=True) - ts = parse_integer(request, "ts") - if ts is None: - ts = self.clock.time_msec() - + ts = parse_integer(request, "ts", default=self.clock.time_msec()) og = await self.url_previewer.preview(url, requester.user, ts) respond_with_json_bytes(request, 200, og, send_cors=True) From 046b7d82322bd883bef3305f42e36fc3b17afbb3 Mon Sep 17 00:00:00 2001 From: Gordan Trevis Date: Tue, 26 Mar 2024 11:55:10 +0100 Subject: [PATCH 9/9] Adds Missing overload signatures for different return values variations. (mypy) --- synapse/http/servlet.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 9473fe2fb31..0ca08038f42 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -19,7 +19,8 @@ # # -""" This module contains base REST classes for constructing REST servlets. """ +"""This module contains base REST classes for constructing REST servlets.""" + import enum import logging from http import HTTPStatus @@ -65,7 +66,31 @@ def parse_integer(request: Request, name: str, default: int) -> int: ... @overload -def parse_integer(request: Request, name: str, *, required: Literal[True]) -> int: ... +def parse_integer( + request: Request, name: str, *, default: int, negative: bool +) -> int: ... + + +@overload +def parse_integer( + request: Request, name: str, *, default: int, negative: bool = False +) -> int: ... + + +@overload +def parse_integer( + request: Request, name: str, *, required: Literal[True], negative: bool = False +) -> int: ... + + +@overload +def parse_integer( + request: Request, name: str, *, default: Literal[None], negative: bool = False +) -> None: ... + + +@overload +def parse_integer(request: Request, name: str, *, negative: bool) -> Optional[int]: ... @overload @@ -75,7 +100,7 @@ def parse_integer( default: Optional[int] = None, required: bool = False, negative: bool = False, -) -> int: ... +) -> Optional[int]: ... def parse_integer(