From 9a14f568c5f04b9aa140e7ae9173aac1c67db9ef Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 10:13:26 +0530 Subject: [PATCH 1/4] fix(api): return 400 for non-integer all_cres pagination, order before paging /rest/v1/all_cres?page=abc is a 500 on opencre.org today. int() on the query parameter raises ValueError and nothing handles it, so a client typo becomes a server error. Parse each value once and return 400, the same way text_search reports a bad text parameter since #867. A value of zero or less still falls back to the default, as before. all_cres_with_pagination also paginated a query with no ORDER BY. The database is then 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 external_id with the primary key breaking ties, so the order is total. The ordering does not reproduce on SQLite, which returns a small table in insertion order, so that test is a regression guard rather than a reproduction. It matters on Postgres, which is what production runs. --- application/database/db.py | 10 ++++++-- application/tests/web_main_test.py | 39 ++++++++++++++++++++++++++++++ application/web/web_main.py | 23 ++++++++++++------ 3 files changed, 62 insertions(+), 10 deletions(-) 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..aae313180 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -1541,6 +1541,45 @@ 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() + for i in range(30): + collection.add_cre(defs.CRE(name=f"cre-{i}", id=f"{i:03d}-{i:03d}")) + 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") + 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..2cbbb9be0 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -1474,14 +1474,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) From fc11923783c33be9d8211d6f49de1bcc306b8a33 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 12:06:27 +0530 Subject: [PATCH 2/4] fix(api): validate and cap pagination on the node endpoints too find_node_by_name serves /standard/{name}, /{ntype}/{name} and the four section and sectionid variants, so both defects fixed for all_cres exist on six more documented endpoints. https://opencre.org/rest/v1/standard/ASVS?page=abc -> 500 Same cause: int() on the query parameter raises ValueError with nothing to handle it. Return 400 instead. items_per_page was worse. It was read straight from the query string with no upper bound and passed to paginate(), so one request could pull the whole table -- with 250 nodes in the database, ?items_per_page=999999 returned all 250. That is the unbounded response #847 is about, on the node endpoints rather than the CRE ones. It is now capped at MAX_ITEMS_PER_PAGE like all_cres, and a value of zero or less still falls back to the default. --- application/tests/web_main_test.py | 46 ++++++++++++++++++++++++++++++ application/web/web_main.py | 22 ++++++++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index aae313180..17d1077f3 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -1580,6 +1580,52 @@ def ids(page: int) -> list: flat = [cre_id for page in first for cre_id in page] self.assertEqual(30, len(set(flat)), "a CRE appeared on two pages") + 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 2cbbb9be0..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 From 697bfae0bb0a2e703ec57e8b590c115723bc33a9 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 12:16:44 +0530 Subject: [PATCH 3/4] test(api): make the ordering test fail when the ORDER BY is removed The fixture inserted CREs already in external_id order, and SQLite hands back a small table in insertion order, so the test passed with or without the ordering it was meant to protect. Insert them descending and assert the ascending sequence: removing the ORDER BY now fails the test with the rows in insertion order. Raised in review on this PR. --- application/tests/web_main_test.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index 17d1077f3..8cfcd49e6 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -1561,8 +1561,12 @@ def test_all_cres_pages_deterministically(self) -> None: 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() - for i in range(30): - collection.add_cre(defs.CRE(name=f"cre-{i}", id=f"{i:03d}-{i:03d}")) + # 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: @@ -1579,6 +1583,9 @@ def ids(page: int) -> list: 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_find_node_by_name_rejects_malformed_pagination(self) -> None: """The node endpoints share find_node_by_name, so all of From 5d08ac01113c91836657d2dd687ad4497828695b Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 12:28:44 +0530 Subject: [PATCH 4/4] test(api): cover the CRE.id tie-break in the pagination ordering The ordering fixtures all used a distinct external_id, so the test passed whether or not the sort carried its CRE.id half. external_id is not unique on its own -- the constraint is on (name, external_id) -- so ties are possible and the tie-break is what makes the order total. Two CREs share an external_id here and are inserted in the opposite order to their primary keys, which are fixed rather than generated so the expectation cannot drift. Dropping CRE.id from the sort now fails the test with the rows the other way round. Raised in review on this PR. --- application/tests/web_main_test.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index 8cfcd49e6..59b78572f 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -1587,6 +1587,33 @@ def ids(page: int) -> list: # 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