From ff1b8a94acb4ec62c2e899194956e1fef7870549 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Tue, 14 Jan 2025 22:31:46 +0700 Subject: [PATCH 01/10] Another should be BadRequest found in the wild --- src/plone/restapi/services/querystringsearch/get.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plone/restapi/services/querystringsearch/get.py b/src/plone/restapi/services/querystringsearch/get.py index acd9f3647b..9b3c0e60c7 100644 --- a/src/plone/restapi/services/querystringsearch/get.py +++ b/src/plone/restapi/services/querystringsearch/get.py @@ -33,17 +33,17 @@ def __call__(self): query = data.get("query", None) try: b_start = int(data.get("b_start", 0)) - except ValueError: + except (ValueError, TypeError): raise BadRequest("Invalid b_start") try: b_size = int(data.get("b_size", 25)) - except ValueError: + except (ValueError, TypeError): raise BadRequest("Invalid b_size") sort_on = data.get("sort_on", None) sort_order = data.get("sort_order", None) try: limit = int(data.get("limit", 1000)) - except ValueError: + except (ValueError, TypeError): raise BadRequest("Invalid limit") fullobjects = bool(data.get("fullobjects", False)) From df464cfb2acfd8c4ffe2579d0c7177409393c565 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Tue, 14 Jan 2025 23:22:34 +0700 Subject: [PATCH 02/10] add changelog --- news/1857.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/1857.bugfix diff --git a/news/1857.bugfix b/news/1857.bugfix new file mode 100644 index 0000000000..b159edf046 --- /dev/null +++ b/news/1857.bugfix @@ -0,0 +1 @@ +Handle ValueError on querystringsearch as BadRequest - djay \ No newline at end of file From de70f10992a6adead0143b40b46c520d28e45c2b Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 09:57:16 +0700 Subject: [PATCH 03/10] add parse_int util --- src/plone/restapi/batching.py | 11 +++++------ src/plone/restapi/deserializer/__init__.py | 17 +++++++++++++++++ .../restapi/services/querystringsearch/get.py | 17 +++++------------ 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/plone/restapi/batching.py b/src/plone/restapi/batching.py index 7b0b814eea..f69ee0b0c7 100644 --- a/src/plone/restapi/batching.py +++ b/src/plone/restapi/batching.py @@ -1,5 +1,5 @@ from plone.batching.batch import Batch -from plone.restapi.deserializer import json_body +from plone.restapi.deserializer import json_body, parse_int from urllib.parse import parse_qsl from urllib.parse import urlencode @@ -11,11 +11,10 @@ class HypermediaBatch: def __init__(self, request, results): self.request = request - 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) + self.b_start = parse_int(json_body(self.request), "b_start", False) or parse_int( + self.request.form, "b_start", 0) + self.b_size = parse_int(json_body(self.request), "b_size", False) or parse_int( + self.request.form, "b_size", DEFAULT_BATCH_SIZE ) self.batch = Batch(results, self.b_size, self.b_start) diff --git a/src/plone/restapi/deserializer/__init__.py b/src/plone/restapi/deserializer/__init__.py index a5112dae93..c9e6130e6a 100644 --- a/src/plone/restapi/deserializer/__init__.py +++ b/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("Invalid %s" % prop) diff --git a/src/plone/restapi/services/querystringsearch/get.py b/src/plone/restapi/services/querystringsearch/get.py index 9b3c0e60c7..5e2bfee45d 100644 --- a/src/plone/restapi/services/querystringsearch/get.py +++ b/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: From d33092b55ab2633db8287cfcb0ed4d810b4c1c31 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:06:56 +0700 Subject: [PATCH 04/10] use parse_int for navigation also --- src/plone/restapi/services/navigation/get.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/plone/restapi/services/navigation/get.py b/src/plone/restapi/services/navigation/get.py index de9f91ed6b..46622c84a0 100644 --- a/src/plone/restapi/services/navigation/get.py +++ b/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 @@ -29,13 +30,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: From 8c6ab203514f556bf79c298adcb52c592b712015 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:15:05 +0700 Subject: [PATCH 05/10] improve bad int message --- src/plone/restapi/deserializer/__init__.py | 2 +- src/plone/restapi/tests/test_batching.py | 6 +++++- src/plone/restapi/tests/test_services_navigation.py | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/plone/restapi/deserializer/__init__.py b/src/plone/restapi/deserializer/__init__.py index c9e6130e6a..0096235c67 100644 --- a/src/plone/restapi/deserializer/__init__.py +++ b/src/plone/restapi/deserializer/__init__.py @@ -44,4 +44,4 @@ def parse_int(data, prop, default): try: return int(data.get(prop, default)) except (ValueError, TypeError): - raise BadRequest("Invalid %s" % prop) + raise BadRequest(f"Invalid {prop}: Not an integer") diff --git a/src/plone/restapi/tests/test_batching.py b/src/plone/restapi/tests/test_batching.py index 4470b2c8f6..a1b4414e38 100644 --- a/src/plone/restapi/tests/test_batching.py +++ b/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): diff --git a/src/plone/restapi/tests/test_services_navigation.py b/src/plone/restapi/tests/test_services_navigation.py index a61009005c..c0bc6149f4 100644 --- a/src/plone/restapi/tests/test_services_navigation.py +++ b/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"]) From 35cc7ee389ab84db16178b7e827be02c7c884edb Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:23:30 +0700 Subject: [PATCH 06/10] change log --- news/1857.bugfix | 2 +- news/1858.bugfix | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 news/1858.bugfix diff --git a/news/1857.bugfix b/news/1857.bugfix index ac95595be3..ea4ba7b398 100644 --- a/news/1857.bugfix +++ b/news/1857.bugfix @@ -1 +1 @@ -Add parse_int to handle all BadRequests from bad ints. @djay +Handle TypeError on querystringsearch as BadRequest. @djay \ No newline at end of file diff --git a/news/1858.bugfix b/news/1858.bugfix new file mode 100644 index 0000000000..ff9d6a9170 --- /dev/null +++ b/news/1858.bugfix @@ -0,0 +1 @@ +add parse_int to handle all cases of BadReqests from ints passed in. @djay \ No newline at end of file From 5c00a04b2cdf07b0f1c0da4428d872c4c4c5d8e5 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:29:35 +0700 Subject: [PATCH 07/10] fix flake8/black --- src/plone/restapi/batching.py | 3 +-- src/plone/restapi/services/navigation/get.py | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/plone/restapi/batching.py b/src/plone/restapi/batching.py index a19f3c489b..822ba14a06 100644 --- a/src/plone/restapi/batching.py +++ b/src/plone/restapi/batching.py @@ -21,8 +21,7 @@ def __init__(self, request, results): 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.request.form, "b_size", DEFAULT_BATCH_SIZE) self.batch = Batch(results, self.b_size, self.b_start) def __iter__(self): diff --git a/src/plone/restapi/services/navigation/get.py b/src/plone/restapi/services/navigation/get.py index 46622c84a0..96748f336f 100644 --- a/src/plone/restapi/services/navigation/get.py +++ b/src/plone/restapi/services/navigation/get.py @@ -18,7 +18,6 @@ from zope.i18n import translate from zope.interface import implementer from zope.interface import Interface -from zExceptions import BadRequest @implementer(IExpandableElement) From 1f15d27c522216c1aa5fd7fd36caf9d870a8b1e6 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:40:17 +0700 Subject: [PATCH 08/10] use parse_int for comment_ids --- src/plone/restapi/services/discussion/conversation.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/plone/restapi/services/discussion/conversation.py b/src/plone/restapi/services/discussion/conversation.py index 071d75b04d..36b0b5b860 100644 --- a/src/plone/restapi/services/discussion/conversation.py +++ b/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): From 0cea526b3a9408cfb9f67baafed3a347f34aac66 Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 10:42:55 +0700 Subject: [PATCH 09/10] fix black --- src/plone/restapi/batching.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plone/restapi/batching.py b/src/plone/restapi/batching.py index 822ba14a06..7d9bec01ba 100644 --- a/src/plone/restapi/batching.py +++ b/src/plone/restapi/batching.py @@ -19,9 +19,11 @@ def __init__(self, request, results): 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.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.request.form, "b_size", DEFAULT_BATCH_SIZE + ) self.batch = Batch(results, self.b_size, self.b_start) def __iter__(self): From 5b5386593255d119c7bb252795adf213284e4b92 Mon Sep 17 00:00:00 2001 From: David Glick Date: Tue, 14 Jan 2025 20:21:31 -0800 Subject: [PATCH 10/10] Update news/1858.bugfix --- news/1858.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/1858.bugfix b/news/1858.bugfix index ff9d6a9170..b275567243 100644 --- a/news/1858.bugfix +++ b/news/1858.bugfix @@ -1 +1 @@ -add parse_int to handle all cases of BadReqests from ints passed in. @djay \ No newline at end of file +Add parse_int to handle all cases of BadRequests from ints passed in. @djay \ No newline at end of file