From 97761dedf9fc2330e127063a5c3c3190ffcbe8ee Mon Sep 17 00:00:00 2001 From: matthewberry Date: Thu, 22 Oct 2026 10:33:47 -0500 Subject: [PATCH 1/2] reject negative limit and offset on search and typeahead --- app/routers/search.py | 8 ++++---- tests/test_pagination_bounds.py | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 tests/test_pagination_bounds.py diff --git a/app/routers/search.py b/app/routers/search.py index d0b0ff9..fcc3188 100644 --- a/app/routers/search.py +++ b/app/routers/search.py @@ -61,9 +61,9 @@ def parse_query_params( default=ResponseFormat.JSON, description="Response format (json or csv)" ), limit: Optional[int] = Query( - None, description="Maximum number of records to return" + None, ge=0, description="Maximum number of records to return" ), - offset: Optional[int] = Query(0, description="Number of records to skip"), + offset: Optional[int] = Query(0, ge=0, description="Number of records to skip"), ordering: Optional[str] = Query( None, description="Column to sort by. Prefix with '-' for descending order. " @@ -326,10 +326,10 @@ def parse_typeahead_params( description="Search term for typeahead suggestions (minimum 3 characters)" ), limit: Optional[int] = Query( - 20, description="Maximum number of records to return" + 20, ge=0, description="Maximum number of records to return" ), offset: Optional[int] = Query( - 0, description="Number of records to skip" + 0, ge=0, description="Number of records to skip" ), # Search context filters accession: Optional[List[str]] = Query( diff --git a/tests/test_pagination_bounds.py b/tests/test_pagination_bounds.py new file mode 100644 index 0000000..3741c95 --- /dev/null +++ b/tests/test_pagination_bounds.py @@ -0,0 +1,35 @@ +from unittest.mock import AsyncMock + +import pytest +from fastapi.testclient import TestClient + +from app.db.database import get_db +from app.main import app + +# Postgres rejects a negative LIMIT or OFFSET at execution time, which these +# endpoints turn into a 500. They should be rejected as bad input instead. +URLS = [ + "/api/v1/search?accession=P51451&limit=-1", + "/api/v1/search?accession=P51451&offset=-5", + "/api/v1/typeahead?field_name=organism&search=homo&limit=-1", + "/api/v1/typeahead?field_name=organism&search=homo&offset=-5", +] + + +@pytest.fixture +def db(): + db = AsyncMock() + db.fetch.return_value = [] + db.fetchval.return_value = 0 + app.dependency_overrides[get_db] = lambda: db + yield db + app.dependency_overrides.clear() + + +@pytest.mark.parametrize("url", URLS) +def test_negative_pagination_is_rejected(db, url): + response = TestClient(app).get(url) + + assert 400 <= response.status_code < 500, response.text + db.fetch.assert_not_called() + db.fetchval.assert_not_called() From 58ec7bfbfad2637f32912306e62bf38f1e6c1c5b Mon Sep 17 00:00:00 2001 From: matthewberry Date: Thu, 22 Oct 2026 10:47:08 -0500 Subject: [PATCH 2/2] require limit >= 1, cap limit and offset, assert 422 in tests --- app/routers/search.py | 11 +++--- tests/conftest.py | 24 +++++++++++++ tests/test_pagination_bounds.py | 64 +++++++++++++++++++-------------- tests/test_search_csv.py | 12 ------- 4 files changed, 68 insertions(+), 43 deletions(-) diff --git a/app/routers/search.py b/app/routers/search.py index fcc3188..bcaa61a 100644 --- a/app/routers/search.py +++ b/app/routers/search.py @@ -15,6 +15,9 @@ router = APIRouter(tags=["Search"]) +# LIMIT and OFFSET are bigint in Postgres; larger values fail when the query runs. +MAX_BIGINT = 2**63 - 1 + def parse_query_params( # String filters @@ -61,9 +64,9 @@ def parse_query_params( default=ResponseFormat.JSON, description="Response format (json or csv)" ), limit: Optional[int] = Query( - None, ge=0, description="Maximum number of records to return" + None, ge=1, le=100000, description="Maximum number of records to return" ), - offset: Optional[int] = Query(0, ge=0, description="Number of records to skip"), + offset: Optional[int] = Query(0, ge=0, le=MAX_BIGINT, description="Number of records to skip"), ordering: Optional[str] = Query( None, description="Column to sort by. Prefix with '-' for descending order. " @@ -326,10 +329,10 @@ def parse_typeahead_params( description="Search term for typeahead suggestions (minimum 3 characters)" ), limit: Optional[int] = Query( - 20, ge=0, description="Maximum number of records to return" + 20, ge=1, le=100, description="Maximum number of records to return" ), offset: Optional[int] = Query( - 0, ge=0, description="Number of records to skip" + 0, ge=0, le=MAX_BIGINT, description="Number of records to skip" ), # Search context filters accession: Optional[List[str]] = Query( diff --git a/tests/conftest.py b/tests/conftest.py index f8dcf79..82a4eb2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,29 @@ import os +from unittest.mock import AsyncMock + +import pytest # Settings require DB credentials at import time; tests mock the DB calls. for key in ("CLEAN_DB_HOST", "CLEAN_DB_USER", "CLEAN_DB_PASSWORD", "CLEAN_DB_NAME"): os.environ.setdefault(key, "test") + + +@pytest.fixture +def db(): + db = AsyncMock() + db.fetch.return_value = [] + db.fetchval.return_value = 0 + return db + + +@pytest.fixture +def client(db): + # Imported here so the environment above is set before settings load. + from fastapi.testclient import TestClient + + from app.db.database import get_db + from app.main import app + + app.dependency_overrides[get_db] = lambda: db + yield TestClient(app) + app.dependency_overrides.clear() diff --git a/tests/test_pagination_bounds.py b/tests/test_pagination_bounds.py index 3741c95..f51d167 100644 --- a/tests/test_pagination_bounds.py +++ b/tests/test_pagination_bounds.py @@ -1,35 +1,45 @@ -from unittest.mock import AsyncMock - import pytest -from fastapi.testclient import TestClient - -from app.db.database import get_db -from app.main import app - -# Postgres rejects a negative LIMIT or OFFSET at execution time, which these -# endpoints turn into a 500. They should be rejected as bad input instead. -URLS = [ - "/api/v1/search?accession=P51451&limit=-1", - "/api/v1/search?accession=P51451&offset=-5", - "/api/v1/typeahead?field_name=organism&search=homo&limit=-1", - "/api/v1/typeahead?field_name=organism&search=homo&offset=-5", -] +SEARCH = "/api/v1/search?accession=P51451" +TYPEAHEAD = "/api/v1/typeahead?field_name=organism&search=homo" +TOO_BIG = 2**63 # one past Postgres bigint + +# Out-of-range values used to reach the SQL: negative or oversized ones made +# Postgres fail (a 500), and limit=0 gave /search a `next` link to the same page. +REJECTED = [ + (SEARCH, "limit", -1), + (SEARCH, "limit", 0), + (SEARCH, "limit", 100001), + (SEARCH, "offset", -5), + (SEARCH, "offset", TOO_BIG), + (TYPEAHEAD, "limit", -1), + (TYPEAHEAD, "limit", 0), + (TYPEAHEAD, "limit", 101), + (TYPEAHEAD, "offset", -5), + (TYPEAHEAD, "offset", TOO_BIG), +] -@pytest.fixture -def db(): - db = AsyncMock() - db.fetch.return_value = [] - db.fetchval.return_value = 0 - app.dependency_overrides[get_db] = lambda: db - yield db - app.dependency_overrides.clear() +ACCEPTED = [ + (SEARCH, "limit", 100000), + (SEARCH, "offset", TOO_BIG - 1), + (TYPEAHEAD, "limit", 100), + (TYPEAHEAD, "offset", TOO_BIG - 1), +] -@pytest.mark.parametrize("url", URLS) -def test_negative_pagination_is_rejected(db, url): - response = TestClient(app).get(url) +@pytest.mark.parametrize("url, param, value", REJECTED) +def test_out_of_range_pagination_is_rejected(client, db, url, param, value): + response = client.get(f"{url}&{param}={value}") - assert 400 <= response.status_code < 500, response.text + assert response.status_code == 422, response.text + assert [e["loc"] for e in response.json()["detail"]] == [["query", param]] db.fetch.assert_not_called() db.fetchval.assert_not_called() + + +@pytest.mark.parametrize("url, param, value", ACCEPTED) +def test_pagination_bounds_are_inclusive(client, db, url, param, value): + response = client.get(f"{url}&{param}={value}") + + assert response.status_code == 200, response.text + assert f"{param.upper()} {value}" in db.fetch.call_args.args[0] diff --git a/tests/test_search_csv.py b/tests/test_search_csv.py index 8fd75ae..12d9cad 100644 --- a/tests/test_search_csv.py +++ b/tests/test_search_csv.py @@ -2,24 +2,12 @@ from io import StringIO from unittest.mock import AsyncMock, patch -import pytest -from fastapi.testclient import TestClient - import app.routers.search as search -from app.db.database import get_db -from app.main import app from app.models.clean_data import CLEANColumn HEADER = [c.value for c in CLEANColumn] -@pytest.fixture -def client(): - app.dependency_overrides[get_db] = lambda: None - yield TestClient(app) - app.dependency_overrides.clear() - - def get_csv(client, rows): with ( patch.object(search, "get_total_count", AsyncMock(return_value=len(rows))),