From aeb3ad0ccb550860ed32398a0632b14c0c61561b Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 05:53:17 +0700 Subject: [PATCH 1/2] Handle bad input as BadRequest instead of ValueError (#1855) * Handle bad input as BadRequest instead of ValueError useful so it can be more easily ignored * added basic test * Update src/plone/restapi/services/navigation/get.py Co-authored-by: David Glick * batching int valueerror to badrequests * fix flake8 * add changelog * Update news/1855.bugfix --------- Co-authored-by: David Glick --- news/1855.bugfix | 1 + src/plone/restapi/batching.py | 18 +++++++++++------- src/plone/restapi/services/navigation/get.py | 6 +++++- src/plone/restapi/tests/test_batching.py | 5 +++++ .../restapi/tests/test_services_navigation.py | 8 ++++++++ 5 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 news/1855.bugfix diff --git a/news/1855.bugfix b/news/1855.bugfix new file mode 100644 index 0000000000..b164e9f037 --- /dev/null +++ b/news/1855.bugfix @@ -0,0 +1 @@ +Changed bad int inputs into 500 Exceptions to 400 BadRequest so they can be filtered out of logs more easily. @djay \ No newline at end of file diff --git a/src/plone/restapi/batching.py b/src/plone/restapi/batching.py index 7b0b814eea..30653c43c3 100644 --- a/src/plone/restapi/batching.py +++ b/src/plone/restapi/batching.py @@ -1,7 +1,9 @@ from plone.batching.batch import Batch from plone.restapi.deserializer import json_body +from plone.restapi.exceptions import DeserializationError from urllib.parse import parse_qsl from urllib.parse import urlencode +from zExceptions import BadRequest DEFAULT_BATCH_SIZE = 25 @@ -11,13 +13,15 @@ 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) - ) - + 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: + raise BadRequest(e) 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 a21a52abe4..de9f91ed6b 100644 --- a/src/plone/restapi/services/navigation/get.py +++ b/src/plone/restapi/services/navigation/get.py @@ -17,6 +17,7 @@ from zope.i18n import translate from zope.interface import implementer from zope.interface import Interface +from zExceptions import BadRequest @implementer(IExpandableElement) @@ -29,7 +30,10 @@ def __init__(self, context, request): def __call__(self, expand=False): if self.request.form.get("expand.navigation.depth", False): - self.depth = int(self.request.form["expand.navigation.depth"]) + try: + self.depth = int(self.request.form["expand.navigation.depth"]) + except (ValueError, TypeError) as e: + raise BadRequest(e) else: self.depth = 1 diff --git a/src/plone/restapi/tests/test_batching.py b/src/plone/restapi/tests/test_batching.py index 3e809ebabc..4470b2c8f6 100644 --- a/src/plone/restapi/tests/test_batching.py +++ b/src/plone/restapi/tests/test_batching.py @@ -185,6 +185,11 @@ def test_batching_links_omitted_if_resulset_fits_in_single_batch(self): response = self.api_session.get("/collection?b_size=100") self.assertNotIn("batching", list(response.json())) + 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"]) + class TestBatchingDXFolders(TestBatchingDXBase): diff --git a/src/plone/restapi/tests/test_services_navigation.py b/src/plone/restapi/tests/test_services_navigation.py index 890c5c1220..a61009005c 100644 --- a/src/plone/restapi/tests/test_services_navigation.py +++ b/src/plone/restapi/tests/test_services_navigation.py @@ -239,3 +239,11 @@ def test_use_nav_title_when_available_and_set(self): ) self.assertEqual(response.json()["items"][1]["items"][-1]["title"], nav_title) + + def test_navigation_badrequests(self): + response = self.api_session.get( + "/folder/@navigation", params={"expand.navigation.depth": "php"} + ) + + self.assertEqual(response.status_code, 400) + self.assertIn("invalid literal for int()", response.json()["message"]) From 7af964916aa59cb38b33700198829b8372c1b4ea Mon Sep 17 00:00:00 2001 From: Dylan Jay Date: Wed, 15 Jan 2025 08:48:13 +0700 Subject: [PATCH 2/2] Handle more BadRequests (#1857) * Another should be BadRequest found in the wild * add changelog * Update news/1857.bugfix --------- Co-authored-by: David Glick --- news/1857.bugfix | 1 + src/plone/restapi/services/querystringsearch/get.py | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) create mode 100644 news/1857.bugfix diff --git a/news/1857.bugfix b/news/1857.bugfix new file mode 100644 index 0000000000..ea4ba7b398 --- /dev/null +++ b/news/1857.bugfix @@ -0,0 +1 @@ +Handle TypeError on querystringsearch as BadRequest. @djay \ No newline at end of file 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))