From 15e92075a1f979981ea55b99fe3bb3ce55f319f4 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 20 Feb 2025 17:59:21 +0100 Subject: [PATCH] Fix #767: bound provided limit --- .../src/kinto_remote_settings/changes/utils.py | 13 +++++++++++++ .../src/kinto_remote_settings/changes/views.py | 12 +++++++++--- .../tests/changes/test_changeset.py | 15 +++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/kinto-remote-settings/src/kinto_remote_settings/changes/utils.py b/kinto-remote-settings/src/kinto_remote_settings/changes/utils.py index f6b00101..8c7b68b9 100644 --- a/kinto-remote-settings/src/kinto_remote_settings/changes/utils.py +++ b/kinto-remote-settings/src/kinto_remote_settings/changes/utils.py @@ -1,10 +1,23 @@ import hashlib +from typing import Optional from uuid import UUID from kinto.core import utils as core_utils from pyramid.settings import aslist +def bound_limit(settings: dict, value: Optional[int]) -> int: + """ + ``_limit`` querystring value has to be within what is configured as + pagination size and max storage fetching size (respectively defaults + as `None` and 10000 in `kinto.core.DEFAULT_SETTINGS`) + """ + max_fetch_size = settings["storage_max_fetch_size"] + paginate_by = settings["paginate_by"] or max_fetch_size + max_limit = min(paginate_by, max_fetch_size) + return min(abs(value), max_limit) if value is not None else max_limit + + def monitored_collections(registry): storage = registry.storage resources_uri = aslist(registry.settings.get("changes.resources", "")) diff --git a/kinto-remote-settings/src/kinto_remote_settings/changes/views.py b/kinto-remote-settings/src/kinto_remote_settings/changes/views.py index 95c41546..055a5f01 100644 --- a/kinto-remote-settings/src/kinto_remote_settings/changes/views.py +++ b/kinto-remote-settings/src/kinto_remote_settings/changes/views.py @@ -22,7 +22,11 @@ CHANGESET_PATH, MONITOR_BUCKET, ) -from .utils import changes_object, monitored_collections +from .utils import bound_limit, changes_object, monitored_collections + + +POSTGRESQL_MAX_INTEGER_VALUE = 2**63 +positive_big_integer = colander.Range(min=0, max=POSTGRESQL_MAX_INTEGER_VALUE) class ChangesModel(object): @@ -265,7 +269,9 @@ def deserialize(self, cstruct=colander.null): class ChangeSetQuerystring(colander.MappingSchema): _since = QuotedTimestamp(missing=colander.drop) _expected = colander.SchemaNode(colander.String()) - _limit = colander.SchemaNode(colander.Integer(), missing=colander.drop) + _limit = colander.SchemaNode( + colander.Integer(), missing=colander.drop, validator=positive_big_integer + ) # Query parameters used on monitor/changes endpoint. bucket = colander.SchemaNode(colander.String(), missing=colander.drop) collection = colander.SchemaNode(colander.String(), missing=colander.drop) @@ -285,7 +291,7 @@ def get_changeset(request): storage = request.registry.storage queryparams = request.validated["querystring"] - limit = queryparams.get("_limit") + limit = bound_limit(request.registry.settings, queryparams.get("_limit")) filters = [] include_deleted = False if "_since" in queryparams: diff --git a/kinto-remote-settings/tests/changes/test_changeset.py b/kinto-remote-settings/tests/changes/test_changeset.py index 3e8ba04b..aea3281f 100644 --- a/kinto-remote-settings/tests/changes/test_changeset.py +++ b/kinto-remote-settings/tests/changes/test_changeset.py @@ -125,6 +125,21 @@ def test_limit_is_supported(self): resp = self.app.get(self.changeset_uri + "&_limit=1", headers=self.headers) assert len(resp.json["changes"]) == 1 + def test_limit_has_to_be_positive(self): + self.app.get( + self.changeset_uri + "&_limit=-1", headers=self.headers, status=400 + ) + self.app.get( + self.changeset_uri + f"&_limit={2**65}", headers=self.headers, status=400 + ) + + def test_limit_is_bounded(self): + with mock.patch.object(self.app.app.registry.storage, "list_all") as mocked: + self.app.get(self.changeset_uri + "&_limit=42000", headers=self.headers) + mocked.assert_called() + _, kwargs = mocked.call_args + assert kwargs["limit"] == 10_000 # Default in kinto.core + def test_extra_param_is_allowed(self): self.app.get(self.changeset_uri + "&_extra=abc", headers=self.headers)