CenterForDigitalHumanities / CenterForDigitalHumanities/rerum_server_nodejs
`limit` and `skip` silently guess at invalid input, and an over-maximum `skip` returns the same page forever
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 6
- Avg merge
- 1h 25m
- Merged PRs (30d)
- 3
Description
## Summary
`getPagination()` (`controllers/utils.js:21`) never rejects anything. `clampNonNegativeInt()` (`controllers/utils.js:15`) substitutes a fallback for any value it cannot parse, and caps anything above the maximum. Every case below returns 200 with a page the client did not ask for and cannot detect:
```text
limit=0 -> 100 limit=abc -> 100 limit=10abc -> 10
limit=-5 -> 100 limit=1e3 -> 1 limit=0x10 -> 100
limit=250.7 -> 250 limit= -> 100
skip=-5 -> 0 skip=abc -> 0 skip=1e3 -> 1
skip=2.9 -> 2 skip= -> 0
```
The most damaging case is `skip` above the maximum. It is clamped rather than rejected, so every request past 100000 returns the identical page, indefinitely. A client that advances `skip` by the number of records it received and stops on an empty page never terminates.
The helper is shared, so this covers `/query`, `HEAD /query`, `/search`, `/search/phrase`, and the two `/gog/*InManuscript` endpoints in one fix site.
**Decision recorded on the parent thread:** an over-maximum `limit` is **clamped and reported**, not rejected. Clamping an over-large page size is conventional server behavior and rejecting it is the more breaking of the two options; the defect is the silence, not the clamp. An over-maximum `skip` is **rejected**, because a repeated page has no valid reading.
## Why this matters
**The non-terminating loop is our own published example.** The `pagedQuery` function at `public/API.html:555` stops only on an empty page and advances by `results.length`. Run verbatim against the local deployment at the real caps, entered at `skip=99800` so the cap is reached cheaply:
```text
req #1: skip=99800 -> 100 docs, first=912509cc
req #2: skip=99900 -> 100 docs, first=4c0eef00
req #3: skip=100000 -> 100 docs, first=4c0ef019
req #4: skip=100100 -> 100 docs, first=4c0ef019
req #5: skip=100200 -> 100 docs, first=4c0ef019
...
GUARD TRIPPED after 8 requests
=> 800 records accumulated, 300 distinct
```
Without the added guard this does not stop and the accumulator grows without bound. The loop is reachable on production today: `{"@type":"oa:Annotation"}` — every IIIF 2.1 annotation — still returns a document at `skip=100000` on `store.rerum.io`, and `{"@type":"Annotation"}` is between 50000 and 100000 and growing toward it.
**`limit=1e3` is a data-loss trap.** `Number.parseInt("1e3", 10)` is `1`, so a client asking for a thousand records receives one. A client that stops on a short page reports a completed walk of a single object. The same parse puts `skip=1e3` at offset 1 rather than 1000.
**A repeated parameter is reduced to a guess.** Express 5's `simple` query parser is the default this app uses (`app.set('query parser')` is never called), so `?limit=100&limit=200` arrives as an array, `Number.parseInt(["100","200"], 10)` coerces it to `"100,200"`, and the server takes `100`. `?limit=200&limit=100` gives 200. Neither is an error today.
**Clamped `limit` is invisible.** A clamped response carries `Allow`, `Content-Type`, `Content-Length`, an `ETag`, the JSON-LD context `Link`, and the CORS headers. No applied `limit` or `skip`, no maximum, no total. A client asking for 1000 and receiving 500 has nothing on the wire that distinguishes truncation from a genuine final page. This is how the problem was found.
## Evidence
Verified 2026-09-02 and re-run 2026-09-03, read-only, on `localhost:3001`, `devstore.rerum.io`, and `store.rerum.io`. Full tables in [the detailed report on #299](https://github.com/CenterForDigitalHumanities/rerum_server_nodejs/issues/299#issuecomment-5529634101).
Clamping is identical on all three deployments and both endpoints — `limit=1000` and `limit=5000` both return 500.
Querying `{"__rerum.APIversion":{"$exists":true}}` with `limit=2`, comparing the first returned id:
| `skip` | local / devstore | store (production) |
|--------|------------------|--------------------|
| 99999 | `…4c0ef016` | `…645fb6b6` |
| 100000 | `…4c0ef019` | `…645fb6b7` |
| 100001 | **`…4c0ef019`** | **`…645fb6b7`** |
| 150000 | **`…4c0ef019`** | **`…645fb6b7`** |
| 500000 | **`…4c0ef019`** | — |
Consecutive pages at `skip=100000` and `skip=100100` with `limit=100` return byte-identical id lists.
`/search` applies the same clamp but slices from its merged in-memory set, so the repeated page only appears for a term with more than 100000 merged matches. No term tried reaches that on the dev collection, so `/search` terminates today by accident of data, not by design. The mechanism is identical.
## Affected lines
| File | Line | Current |
|------|------|---------|
| `controllers/utils.js` | 15-19 | `clampNonNegativeInt()` silently substitutes fallbacks and caps |
| `controllers/utils.js` | 21-28 | `getPagination()` returns clamped values with no report of clamping |
| `controllers/crud.js` | 77 | `/query` caller |
| `controllers/history.js` | 89 | `HEAD /query` caller |
| `controllers/search.js` | 274, 360 | `/search`, `/search/phrase` callers |
| `controllers/gog.js` | 36, 167 | `/gog/*InManuscript` callers, default 50 |
| `public/API.html` | 555 | Published `pagedQuery` is the non-terminating shape |
## Proposed change
Validate the raw parameter before parsing, in `getPagination()`, so all six endpoints change together.
### Reject with 400
- `skip` greater than the maximum. This is the change that converts the infinite loop into an immediate, legible failure:
```json
{
"message": "The skip value 150000 exceeds the maximum of 100000. Follow the Link rel=\"next\" header or use a cursor to page deeper.",
"status": 400
}
```
- Any value that is not a decimal integer string: `abc`, `10abc`, `1e3`, `0x10`, `250.7`, `2.9`, the empty string.
- `limit` of `0` or negative, and negative `skip`.
- A repeated parameter, which arrives as an array rather than a string.
Validating the raw value as a decimal integer string, and rejecting anything that is not a string, covers all of these in one place.
`?limit[a]=5` is the one case that cannot be caught. Under the `simple` parser the server never receives a `limit` key at all, so it is indistinguishable from a request that omitted the parameter. Worth a line in the docs, not a code change.
### Clamp and report
An over-maximum `limit` keeps returning the maximum, and says so. Report the applied values and the maximums on every paged response, not only on clamped ones, so a client can configure itself from any single response:
```text
RERUM-Limit: 500
RERUM-Skip: 0
RERUM-Limit-Max: 500
RERUM-Skip-Max: 100000
```
Header naming is open. The codebase already sets unprefixed custom headers (`Current-Overwritten-Version`, `controllers/gog.js:388`), and RFC 6648 deprecates the `X-` prefix, so a `RERUM-` namespace is the suggestion rather than a convention already in place. `Access-Control-Expose-Headers` is already `*` (`app.js:52`), so browser clients can read whatever is chosen.
## Notes
- **Breaking.** Requests that return 200 today will return 400. Land on dev first and give known client maintainers notice. The `skip` rejection is the part most likely to be hit by a real client, which is the point.
- The `skip` rejection is much easier to defend once #302 and #303 land, because clients then follow a link instead of computing offsets and depth stops being bounded at all. Consider sequencing the rejection after `rel="next"` ships so there is somewhere to send people.
- The 400 message names the configured maximum, so it has to be the real one. `controllers/utils.js:12-13` reads `RERUM_MAX_QUERY_LIMIT` / `RERUM_MAX_QUERY_SKIP` while `.env` sets the unprefixed `MAX_QUERY_LIMIT` / `MAX_QUERY_SKIP`, and `env-loader.js` does no prefixing — so both caps currently fall back to the code defaults of 500 and 100000, and the `skip` cap of 10000 that `.env` asks for has never been in effect on either deployment. Reconcile the names as part of this work, and add a unit test that sets the key and asserts the resulting cap; the tests at `__tests__/utils.test.js:329-354` pass under either name today, which is why the mismatch was never caught.
- The same PR should update `public/API.html` and the OpenAPI contract; see #305 for the full sweep.
## Acceptance criteria
- [ ] `skip` above the maximum returns 400 naming the maximum, rather than a repeated page
- [ ] `limit=abc`, `limit=-5`, `limit=0`, `limit=1e3`, `limit=250.7`, and the empty string return 400
- [ ] `skip=abc`, `skip=-5`, `skip=1e3`, and `skip=2.9` return 400
- [ ] A repeated `limit` or `skip` parameter returns 400 rather than silently taking the first value
- [ ] `limit` above the maximum still returns the maximum, and the applied value and maximum appear in response headers
- [ ] The applied `limit` and `skip` are reported on every paged response, clamped or not
- [ ] `/query`, `HEAD /query`, `/search`, `/search/phrase`, and both `/gog/*InManuscript` endpoints behave identically for every case above
- [ ] Regression tests cover each rejected form and both clamp boundaries
Contributor guide
Research direction
Start with getPagination() and clampNonNegativeInt() in controllers/utils.js, then run the utility tests at __tests__/utils.test.js:329-354, including the environment-key cases. Trace callers in controllers/crud.js, history.js, search.js, and gog.js, plus headers in app.js and the paging example in public/API.html. Done means consistent 400 validation, reported pagination headers, corrected caps, regression coverage, and updated API documentation and OpenAPI contract.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- express, javascript, openapi
- Domain
- api, backend, documentation, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100