fix(api): validate and cap REST pagination parameters

application/database/db.py

@@ -2164,8 +2164,14 @@ def export(self, dir: str = None, dry_run: bool = False) -> List[cre_defs.Docume
     def all_cres_with_pagination(
         self, page: int = 1, per_page: int = 10
     ) -> List[cre_defs.CRE]:
-        cres = self.session.query(CRE).paginate(
-            page=int(page), per_page=per_page, error_out=False
+        cres = (
+            self.session.query(CRE)
+            # Without an explicit order the database is free to return rows in
+            # any order, so page boundaries can move between requests and a CRE
+            # can appear on two pages while another is never returned.
+            .order_by(CRE.external_id, CRE.id).paginate(
+                page=int(page), per_page=per_page, error_out=False
+            )
         )
         total_pages = cres.pages
         result = self._hydrate_cres_batch(list(cres.items))

application/tests/web_main_test.py

@@ -1541,6 +1541,125 @@ def test_all_cres_per_page_cap_integration(self) -> None:
             self.assertEqual(web_main.ITEMS_PER_PAGE, len(body["data"]))
             self.assertEqual(6, body["total_pages"])
 
+    def test_all_cres_rejects_malformed_pagination(self) -> None:
+        """A non-integer page or per_page is a client error. Live today:
+        GET https://opencre.org/rest/v1/all_cres?page=abc returns 500."""
+        collection = db.Node_collection().with_graph()
+        collection.add_cre(defs.CRE(name="cre", id="111-111"))
+        collection.session.commit()
+
+        with self.app.test_client() as client:
+            for query in ("page=abc", "per_page=abc", "page=1.5"):
+                response = client.get(
+                    f"/rest/v1/all_cres?{query}",
+                    headers={"Content-Type": "application/json"},
+                )
+                self.assertEqual(400, response.status_code, query)
+
+    def test_all_cres_pages_deterministically(self) -> None:
+        """Paging twice returns the same rows in the same places. Without an
+        ORDER BY the database may return rows in any order, which lets a CRE
+        appear on two pages while another is never returned at all."""
+        collection = db.Node_collection().with_graph()
+        # Insert in an order that is deliberately not the expected one. SQLite
+        # hands back a small table in insertion order, so a fixture inserted
+        # already-sorted would pass even with the ORDER BY removed.
+        external_ids = [f"{i:03d}-{i:03d}" for i in range(30)]
+        for external_id in reversed(external_ids):
+            collection.add_cre(defs.CRE(name=f"cre-{external_id}", id=external_id))
+        collection.session.commit()
+
+        with self.app.test_client() as client:
+
+            def ids(page: int) -> list:
+                response = client.get(
+                    f"/rest/v1/all_cres?per_page=10&page={page}",
+                    headers={"Content-Type": "application/json"},
+                )
+                return [doc["id"] for doc in json.loads(response.data)["data"]]
+
+            first = [ids(p) for p in (1, 2, 3)]
+            self.assertEqual(first, [ids(p) for p in (1, 2, 3)])
+
+            flat = [cre_id for page in first for cre_id in page]
+            self.assertEqual(30, len(set(flat)), "a CRE appeared on two pages")
+            # Inserted descending, returned ascending: this is what fails if the
+            # ORDER BY goes away.
+            self.assertEqual(external_ids, flat)
+
+    def test_all_cres_orders_duplicate_external_ids_by_primary_key(self) -> None:
+        """external_id is not unique on its own: the constraint is on
+        (name, external_id), so two CREs may carry the same one. Ordering by
+        external_id alone would leave those ties undefined, which is what the
+        CRE.id half of the sort is for. The primary keys are fixed here rather
+        than generated, so the expected order cannot drift between runs."""
+        collection = db.Node_collection().with_graph()
+        # Inserted in the opposite order to their primary keys.
+        for pk, name in (
+            ("bbbbbbbb-0000-0000-0000-000000000000", "tie-second"),
+            ("aaaaaaaa-0000-0000-0000-000000000000", "tie-first"),
+        ):
+            collection.session.add(
+                db.CRE(id=pk, external_id="500-500", name=name, description="")
+            )
+        collection.session.commit()
+
+        with self.app.test_client() as client:
+            response = client.get(
+                "/rest/v1/all_cres?per_page=10&page=1",
+                headers={"Content-Type": "application/json"},
+            )
+            self.assertEqual(200, response.status_code)
+            names = [doc["name"] for doc in json.loads(response.data)["data"]]
+
+        self.assertEqual(["tie-first", "tie-second"], names)
+
+    def test_find_node_by_name_rejects_malformed_pagination(self) -> None:
+        """The node endpoints share find_node_by_name, so all of
+        /standard/{name}, /{ntype}/{name} and the section variants have the
+        same defect. Live today: GET
+        https://opencre.org/rest/v1/standard/ASVS?page=abc returns 500."""
+        collection = db.Node_collection().with_graph()
+        collection.add_node(defs.Standard(name="ASVS", section="S1"))
+        collection.session.commit()
+
+        with self.app.test_client() as client:
+            for url in (
+                "/rest/v1/standard/ASVS?page=abc",
+                "/rest/v1/standard/ASVS?items_per_page=abc",
+                "/rest/v1/standard/ASVS/section/S1?page=abc",
+                "/rest/v1/standard/ASVS/sectionid/S1?items_per_page=abc",
+            ):
+                response = client.get(url, headers={"Content-Type": "application/json"})
+                self.assertEqual(400, response.status_code, url)
+
+    def test_find_node_by_name_caps_items_per_page(self) -> None:
+        """items_per_page was passed straight through with no upper bound, so
+        one request could pull the whole table -- the unbounded response #847
+        describes, on the node endpoints rather than the CRE ones."""
+        collection = db.Node_collection().with_graph()
+        for i in range(150):
+            collection.add_node(defs.Standard(name="ASVS", section=f"S{i:04d}"))
+        collection.session.commit()
+
+        with self.app.test_client() as client:
+            response = client.get(
+                "/rest/v1/standard/ASVS?items_per_page=999999",
+                headers={"Content-Type": "application/json"},
+            )
+            self.assertEqual(200, response.status_code)
+            self.assertEqual(
+                web_main.MAX_ITEMS_PER_PAGE,
+                len(json.loads(response.data)["standards"]),
+            )
+
+            # Below the cap the request is honoured unchanged.
+            response = client.get(
+                "/rest/v1/standard/ASVS?items_per_page=10",
+                headers={"Content-Type": "application/json"},
+            )
+            self.assertEqual(10, len(json.loads(response.data)["standards"]))
+
     def test_import_from_cre_csv(self) -> None:
         input_data, _ = data_gen.export_format_data()
         workspace = tempfile.mkdtemp()

application/web/web_main.py

@@ -231,10 +231,26 @@ def find_node_by_name(
     if typ:
         ntype = typ[0]
 
+    # Parse once: int() on a non-integer raises ValueError with nothing to
+    # handle it, so a client typo became a 500. items_per_page was also passed
+    # straight through with no upper bound, so a single request could pull the
+    # whole table; cap it the way all_cres does. A value of zero or less falls
+    # back to the default.
     page = 1
-    if request.args.get("page") is not None and int(request.args.get("page")) > 0:
-        page = request.args.get("page")
-    items_per_page = request.args.get("items_per_page") or ITEMS_PER_PAGE
+    items_per_page = ITEMS_PER_PAGE
+    try:
+        if request.args.get("page") is not None:
+            requested_page = int(request.args.get("page"))
+            if requested_page > 0:
+                page = requested_page
+
+        if request.args.get("items_per_page") is not None:
+            requested_items_per_page = int(request.args.get("items_per_page"))
+            if requested_items_per_page > 0:
+                items_per_page = requested_items_per_page
+    except ValueError:
+        abort(400, "page and items_per_page must be integers")
+    items_per_page = min(items_per_page, MAX_ITEMS_PER_PAGE)
 
     include_only = request.args.getlist("include_only")
     total_pages, nodes = None, None
@@ -1474,14 +1490,21 @@ def all_cres() -> Any:
 
     page = 1
     per_page = ITEMS_PER_PAGE
-    if request.args.get("page") is not None and int(request.args.get("page")) > 0:
-        page = int(request.args.get("page"))
-
-    if (
-        request.args.get("per_page") is not None
-        and int(request.args.get("per_page")) > 0
-    ):
-        per_page = int(request.args.get("per_page"))
+    # int() on a non-integer raises ValueError, which has no handler and
+    # surfaced as a 500 for what is a client mistake. A value of zero or less
+    # still falls back to the default.
+    try:
+        if request.args.get("page") is not None:
+            requested_page = int(request.args.get("page"))
+            if requested_page > 0:
+                page = requested_page
+
+        if request.args.get("per_page") is not None:
+            requested_per_page = int(request.args.get("per_page"))
+            if requested_per_page > 0:
+                per_page = requested_per_page
+    except ValueError:
+        abort(400, "page and per_page must be integers")
     per_page = min(per_page, MAX_ITEMS_PER_PAGE)
 
     documents, page, total_pages = database.all_cres_with_pagination(page, per_page)