Parse Integer negative value validation

changelog.d/16920.bugfix

@@ -0,0 +1 @@
+Adds `limit` parameter negative value validation check to prevent 500 internal server error on publicRooms request.

synapse/http/servlet.py

@@ -70,12 +70,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
 
@@ -85,16 +94,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
@@ -120,14 +130,17 @@ def parse_integer_from_args(
     name: str,
     default: Optional[int] = None,
     required: bool = False,
-) -> Optional[int]: ...
+    negative: bool = False,
+) -> Optional[int]:
+    ...
 
 
 def parse_integer_from_args(
     args: Mapping[bytes, Sequence[bytes]],
     name: str,
     default: Optional[int] = None,
     required: bool = False,
+    negative: bool = True,
 ) -> Optional[int]:
     """Parse an integer parameter from the request string
 
@@ -137,33 +150,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: ...

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)
 

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.

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,

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")

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:

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"],
         )