diff --git a/application/database/db.py b/application/database/db.py index 522612161..1b2e89a8d 100644 --- a/application/database/db.py +++ b/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)) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index 3d183e1b8..59b78572f 100644 --- a/application/tests/web_main_test.py +++ b/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() diff --git a/application/web/web_main.py b/application/web/web_main.py index a40b5fa7d..a05ca67dd 100644 --- a/application/web/web_main.py +++ b/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)