Add parse_int to handle all cases of BadRequest from ints passed in (#…

…1858)

* Another should be BadRequest found in the wild

* add changelog

* add parse_int util

* use parse_int for navigation also

* improve bad int message

* change log

* fix flake8/black

* use parse_int for comment_ids

* fix black

* Update news/1858.bugfix

---------

Co-authored-by: David Glick <[email protected]>

news/1858.bugfix

@@ -0,0 +1 @@
+Add parse_int to handle all cases of BadRequests from ints passed in. @djay

src/plone/restapi/batching.py

@@ -1,5 +1,6 @@
 from plone.batching.batch import Batch
 from plone.restapi.deserializer import json_body
+from plone.restapi.deserializer import parse_int
 from plone.restapi.exceptions import DeserializationError
 from urllib.parse import parse_qsl
 from urllib.parse import urlencode
@@ -14,14 +15,15 @@ def __init__(self, request, results):
         self.request = request
 
         try:
-            self.b_start = int(json_body(self.request).get("b_start", False)) or int(
-                self.request.form.get("b_start", 0)
-            )
-            self.b_size = int(json_body(self.request).get("b_size", False)) or int(
-                self.request.form.get("b_size", DEFAULT_BATCH_SIZE)
-            )
-        except (ValueError, DeserializationError) as e:
+            data = json_body(request)
+        except DeserializationError as e:
             raise BadRequest(e)
+        self.b_start = parse_int(data, "b_start", False) or parse_int(
+            self.request.form, "b_start", 0
+        )
+        self.b_size = parse_int(data, "b_size", False) or parse_int(
+            self.request.form, "b_size", DEFAULT_BATCH_SIZE
+        )
         self.batch = Batch(results, self.b_size, self.b_start)
 
     def __iter__(self):

src/plone/restapi/deserializer/__init__.py

@@ -1,4 +1,5 @@
 from plone.restapi.exceptions import DeserializationError
+from zExceptions import BadRequest
 
 import json
 
@@ -28,3 +29,19 @@ def boolean_value(value):
 
     """
     return value not in {False, "false", "False", "0", 0}
+
+
+def parse_int(data, prop, default):
+    """
+    Args:
+        data: dict from a request
+        prop: name of a integer paramater in the dict
+        default: default if not found
+
+    Returns: an integer
+    Raises: BadRequest if not an int
+    """
+    try:
+        return int(data.get(prop, default))
+    except (ValueError, TypeError):
+        raise BadRequest(f"Invalid {prop}: Not an integer")

src/plone/restapi/services/discussion/conversation.py

@@ -2,7 +2,7 @@
 from plone.app.discussion.browser.comment import EditCommentForm
 from plone.app.discussion.browser.comments import CommentForm
 from plone.app.discussion.interfaces import IConversation
-from plone.restapi.deserializer import json_body
+from plone.restapi.deserializer import json_body, parse_int
 from plone.restapi.interfaces import ISerializeToJson
 from plone.restapi.services import Service
 from plone.restapi.services.discussion.utils import can_delete
@@ -38,7 +38,7 @@ class CommentsGet(Service):
 
     def publishTraverse(self, request, name):
         if name:
-            self.comment_id = int(name)
+            self.comment_id = parse_int(dict(comment_id=name), "comment_id", None)
         return self
 
     def reply(self):
@@ -57,7 +57,7 @@ class CommentsAdd(Service):
 
     def publishTraverse(self, request, name):
         if name:
-            self.comment_id = int(name)
+            self.comment_id = parse_int(dict(comment_id=name), "comment_id", None)
             request["form.widgets.in_reply_to"] = name
         return self
 
@@ -96,7 +96,7 @@ class CommentsUpdate(Service):
 
     def publishTraverse(self, request, name):
         if name:
-            self.comment_id = int(name)
+            self.comment_id = parse_int(dict(comment_id=name), "comment_id", None)
             request["form.widgets.comment_id"] = name
         return self
 
@@ -140,7 +140,7 @@ class CommentsDelete(Service):
     comment_id = None
 
     def publishTraverse(self, request, name):
-        self.comment_id = int(name)
+        self.comment_id = parse_int(dict(comment_id=name), "comment_id", None)
         return self
 
     def reply(self):

src/plone/restapi/services/navigation/get.py

@@ -6,6 +6,7 @@
 from plone.registry.interfaces import IRegistry
 from plone.restapi.bbb import INavigationSchema
 from plone.restapi.bbb import safe_text
+from plone.restapi.deserializer import parse_int
 from plone.restapi.interfaces import IExpandableElement
 from plone.restapi.serializer.converters import json_compatible
 from plone.restapi.services import Service
@@ -17,7 +18,6 @@
 from zope.i18n import translate
 from zope.interface import implementer
 from zope.interface import Interface
-from zExceptions import BadRequest
 
 
 @implementer(IExpandableElement)
@@ -29,13 +29,7 @@ def __init__(self, context, request):
         self.portal = getSite()
 
     def __call__(self, expand=False):
-        if self.request.form.get("expand.navigation.depth", False):
-            try:
-                self.depth = int(self.request.form["expand.navigation.depth"])
-            except (ValueError, TypeError) as e:
-                raise BadRequest(e)
-        else:
-            self.depth = 1
+        self.depth = parse_int(self.request.form, "expand.navigation.depth", 1)
 
         result = {"navigation": {"@id": f"{self.context.absolute_url()}/@navigation"}}
         if not expand:

src/plone/restapi/services/querystringsearch/get.py

@@ -2,6 +2,7 @@
 from pkg_resources import parse_version
 from plone.restapi.bbb import IPloneSiteRoot
 from plone.restapi.deserializer import json_body
+from plone.restapi.deserializer import parse_int
 from plone.restapi.exceptions import DeserializationError
 from plone.restapi.interfaces import ISerializeToJson
 from plone.restapi.services import Service
@@ -31,20 +32,12 @@ def __call__(self):
             raise BadRequest(str(err))
 
         query = data.get("query", None)
-        try:
-            b_start = int(data.get("b_start", 0))
-        except (ValueError, TypeError):
-            raise BadRequest("Invalid b_start")
-        try:
-            b_size = int(data.get("b_size", 25))
-        except (ValueError, TypeError):
-            raise BadRequest("Invalid b_size")
+
+        b_start = parse_int(data, "b_start", 0)
+        b_size = parse_int(data, "b_size", 25)
         sort_on = data.get("sort_on", None)
         sort_order = data.get("sort_order", None)
-        try:
-            limit = int(data.get("limit", 1000))
-        except (ValueError, TypeError):
-            raise BadRequest("Invalid limit")
+        limit = parse_int(data, "limit", 1000)
         fullobjects = bool(data.get("fullobjects", False))
 
         if not query:

src/plone/restapi/tests/test_batching.py

@@ -188,7 +188,11 @@ def test_batching_links_omitted_if_resulset_fits_in_single_batch(self):
     def test_batching_badrequests(self):
         response = self.api_session.get("/collection?b_size=php")
         self.assertEqual(response.status_code, 400)
-        self.assertIn("invalid literal for int()", response.json()["message"])
+        self.assertIn("Invalid b_size", response.json()["message"])
+
+        response = self.api_session.get("/collection?b_size:list=1")
+        self.assertEqual(response.status_code, 400)
+        self.assertIn("Invalid b_size", response.json()["message"])
 
 
 class TestBatchingDXFolders(TestBatchingDXBase):

src/plone/restapi/tests/test_services_navigation.py

@@ -246,4 +246,4 @@ def test_navigation_badrequests(self):
         )
 
         self.assertEqual(response.status_code, 400)
-        self.assertIn("invalid literal for int()", response.json()["message"])
+        self.assertIn("Invalid expand.navigation.depth", response.json()["message"])