CenterForDigitalHumanities / CenterForDigitalHumanities/rerum_server_nodejs

`limit` and `skip` silently guess at invalid input, and an over-maximum `skip` returns the same page forever

Open
#301 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.