Bound limit and offset on search and typeahead by matthewberry · Pull Request #25 · moleculemaker/CLEANDB-api

matthewberry · 2026-10-22T15:33:53Z

Problem

/api/v1/search and /api/v1/typeahead accept any integer for limit and offset and put it straight into the SQL text. Postgres rejects a negative value, or one above the bigint maximum (9223372036854775807), when the query runs. The handler's except Exception turns that into a 500, and the raw Postgres error goes back to the client.

I reproduced the negative-value cases against a throwaway Postgres 16.15 container with a minimal schema, running the real app with nothing mocked:

Request	Before
`/search?accession=P51451&limit=-1`	500 "LIMIT must not be negative"
`/search?accession=P51451&limit=1&offset=-5`	500 "OFFSET must not be negative"
`/typeahead?field_name=organism&search=homo&limit=-1`	500 "LIMIT must not be negative"
`/typeahead?field_name=organism&search=homo&offset=-5`	500 "OFFSET must not be negative"

The same requests without a negative value returned 200. For oversized values, I confirmed that they reach the SQL unchanged, and that Postgres 16 rejects LIMIT/OFFSET 99999999999999999999 with "bigint out of range" (while it accepts 9223372036854775807).

limit=0 was also wrong, though it didn't cause a 500:

On /search, it returned a next link pointing back to the same page, so a client following next never stops.
On /typeahead, params.limit or 20 silently turned it into 20.

Fix

New bounds on the Query(...) parameters of parse_query_params and parse_typeahead_params:

Parameter	`/search`	`/typeahead`
`limit`	1 to 100000	1 to 100
`offset`	0 to 2^63 - 1	0 to 2^63 - 1

Values outside these ranges now get a 422 that names the parameter, and no SQL runs.

limit=0 and the caps reject requests that worked before. I checked the latest CLEANDB-frontend main, and none of its requests are affected:

/search always gets 10, 20, or 50 from the table's page-size options.
/typeahead gets 20 or no limit at all, so the backend default of 20 applies.

Other API clients sending limit=0, more than 100000 on /search, or more than 100 on /typeahead will now get a 422.

/ec_lookup doesn't need the fix: parse_ec_lookup_params doesn't read limit from the query string, and get_ec_lookup always sets it to 20.

Tests

tests/test_pagination_bounds.py sends requests to the app with a fake database. It checks:

Each out-of-range value gets a 422 whose detail names that parameter, and the database is never queried. Covered: negative values, limit=0, one past each limit cap, and an offset one past the bigint maximum.
Each boundary value is accepted and reaches the SQL.

Without the router change, the limit=0, over-cap, and over-bigint cases fail.

The fake-database client fixture now lives in tests/conftest.py and is shared with tests/test_search_csv.py.

pytest: 16 passed. ruff check app tests reports the same 5 errors as main, all in lines this PR doesn't touch.

matthewberry

The ge=0 change works: negative limit/offset now get a 422 before any SQL runs. It leaves three gaps that I reproduced, two of them at limit=0, the value this PR keeps allowed. Details are inline.

Reproduced (PR head run against a fake DB with TestClient; finding 3 also checked against a throwaway postgres:16 container):

/search?limit=0 returns a next link to the same page, so a client following next never stops.
/typeahead?limit=0 is quietly treated as limit=20.
An oversized offset/limit still ends in a 500 with the raw Postgres error (bigint out of range), the same failure this PR fixes for negative values.

Found by reading the code, not run:

The PR description says /ec_lookup always uses 10. get_ec_lookup sets params.limit = 20 (search.py:654), so get_ec_suggestions uses params.limit or 10 = 20. The reason for leaving /ec_lookup alone still holds; only the number is wrong.

Not blocking:

Every handler's except Exception still returns str(e) to the client (search.py:316). That's what actually leaks the error text; this PR removes two ways to trigger it.
LIMIT/OFFSET are pasted into the SQL with f-strings (queries.py:167, :170, :364–:418). Passing them as bound parameters would enforce the type in one place. Today every caller goes through the Query(...) checks, so this is about robustness, not a live bug.

Suggested changes: ge=1 on limit, le= bounds on limit and offset, and a test that asserts 422 and covers limit=0. Before switching to ge=1, check whether the frontend ever sends limit=0, since that would start getting a 422.

matthewberry · 2026-10-22T15:40:12Z

    ),
    limit: Optional[int] = Query(
-        None, description="Maximum number of records to return"
+        None, ge=0, description="Maximum number of records to return"


limit=0 still passes, and on /search it gives a pagination link that never ends. With 5 matching rows:

GET /api/v1/search?accession=P51451&limit=0&offset=0 → 200, next = …?accession=P51451&format=json&offset=0&limit=0 (the same page) GET /api/v1/search?accession=P51451&limit=0&offset=3 → 200, next = …offset=3&limit=0, previous = …offset=3&limit=0

current_offset + current_limit < total_count stays true and next_offset = current_offset + 0, so a client that follows next until it is null loops forever. The description says limit=0's behavior doesn't change, but that behavior is the bug. Suggest ge=1.

Reproduced on 97761de: with 5 matching rows, limit=0&offset=0 returned a next link to the same page. Fixed in 58ec7bf: limit is now ge=1, so limit=0 gets a 422. I checked CLEANDB-frontend main first, and it only sends limit 10, 20, or 50 to /search, so it isn't affected.

matthewberry · 2026-10-22T15:40:12Z

+        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"),


There's no upper bound, so a value that's too large still ends in the 500 this PR is fixing. offset=99999999999999999999 goes into the SQL as written (… LIMIT 5000 OFFSET 99999999999999999999), and Postgres 16 rejects it when it runs:

SELECT 1 LIMIT 5000 OFFSET 99999999999999999999; → ERROR: bigint out of range SELECT 1 LIMIT 99999999999999999999; → ERROR: bigint out of range

The except Exception then returns that as a 500 with the raw message. Adding le= to both limit and offset closes it. On limit, it would also stop a request like limit=100000000 from skipping the 5000-row default and building the whole response in memory. I didn't test that last part.

Reproduced: oversized values reach the SQL unchanged, and Postgres 16 rejects anything above 9223372036854775807 with "bigint out of range" (that value itself is accepted). Fixed in 58ec7bf: offset is capped at 2^63 - 1 on both endpoints, and limit at 100000 on /search and 100 on /typeahead. I didn't test the memory point either; the 100000 cap limits it but doesn't measure it.

matthewberry · 2026-10-22T15:40:12Z

    ),
    limit: Optional[int] = Query(
-        20, description="Maximum number of records to return"
+        20, ge=0, description="Maximum number of records to return"


On typeahead, limit=0 passes this check, but the handler does limit = params.limit or 20 (and queries.py:297 repeats it), so 0 becomes 20:

GET /api/v1/typeahead?field_name=organism&search=homo&limit=0 → SQL: … ORDER BY 1 ASC LIMIT 20 OFFSET 0 → response: "limit": 20

/search treats the same limit=0 as zero rows. ge=1 here makes the two endpoints consistent. The same oversized-value point as on /search applies to this limit and offset.

Reproduced: limit=0 produced LIMIT 20 in the SQL and "limit": 20 in the response. Fixed in 58ec7bf with ge=1, plus a cap of le=100 on typeahead limit. The frontend sends 20 or no limit, so it isn't affected.

matthewberry · 2026-10-22T15:40:12Z

+def test_negative_pagination_is_rejected(db, url):
+    response = TestClient(app).get(url)
+
+    assert 400 <= response.status_code < 500, response.text


The PR promises a 422 that names the bad parameter, but this accepts any 4xx. A 400 from somewhere else, or a 404 from a mistyped route, would still pass. Suggest assert response.status_code == 422 plus a check that detail names the parameter. A limit=0 case would also have caught the two limit=0 issues above.

Agreed. When I wrote it I didn't know whether the check would land in Query(...) (422) or in the model (400 via parse_query_params). Now that it's in Query(...), 58ec7bf asserts == 422 and that detail has exactly one entry, at ["query", <param>]. It also adds limit=0, over-cap, and over-bigint cases, plus inclusive-boundary cases that must reach the SQL. Without the router change, the 6 new rejection cases fail.

matthewberry · 2026-10-22T15:40:12Z

+
+
+@pytest.fixture
+def db():


Nit: this repeats the dependency_overrides[get_db] setup and teardown from the client fixture in tests/test_search_csv.py. One shared fixture in a tests/conftest.py would serve both files.

Done in 58ec7bf: db and client fixtures in tests/conftest.py, used by both test files. The CSV tests now get an AsyncMock db instead of None, and they still patch the query functions, so their behavior is the same.

matthewberry · 2026-10-22T15:50:14Z

Replying to the review summary (inline threads answered separately):

/ec_lookup number: confirmed. get_ec_lookup sets params.limit = 20 (search.py:654). The description now says 20.
str(e) returned to clients and f-string LIMIT/OFFSET: both confirmed by reading. I agree they're out of scope here and left them for a follow-up.
Frontend check before ge=1: I searched CLEANDB-frontend main (739d62b) for every caller of the search and typeahead endpoints. /search gets limit 10, 20, or 50 from the table's page-size options. /typeahead gets 20 or no limit. None of these requests is affected by the new bounds.

Replace the configurable CSV_MAX_ROWS with MAX_SEARCH_LIMIT, the same bound #25 put on limit. An explicit limit above it already gets a 422, so drop the CSV-only 400 for that case and its test. Clamp the cap check's OFFSET to MAX_BIGINT: with offset allowed up to the bigint maximum, offset + cap overflowed and Postgres returned a 500.

reject negative limit and offset on search and typeahead

97761de

matthewberry commented Oct 22, 2026

View reviewed changes

require limit >= 1, cap limit and offset, assert 422 in tests

58ec7bf

matthewberry changed the title ~~Reject negative limit and offset on search and typeahead~~ Bound limit and offset on search and typeahead Oct 22, 2026

matthewberry merged commit f66f3cf into main Oct 22, 2026
1 check passed

matthewberry deleted the mjberry/reject-negative-pagination branch October 22, 2026 15:53

matthewberry mentioned this pull request Oct 22, 2026

Return full CSV search results up to 100,000 rows; cap JSON limit at 5,000 #20

Merged

matthewberry mentioned this pull request Nov 4, 2026

fix: reject out-of-range limit and offset; previous past the end goes to the last records moleculemaker/oed-api#10

Merged

Navigation Menu

Repository navigation

Bound limit and offset on search and typeahead - #25

Bound limit and offset on search and typeahead#25
matthewberry merged 2 commits into
mainfrom
mjberry/reject-negative-pagination

matthewberry commented Oct 22, 2026 •

edited

Loading

Uh oh!

matthewberry left a comment

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry Oct 22, 2026

Uh oh!

matthewberry commented Oct 22, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant



		@pytest.fixture
		def db():

Conversation

matthewberry commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tests

Uh oh!

matthewberry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewberry commented Oct 22, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

matthewberry commented Oct 22, 2026 •

edited

Loading