code/cl8y-dex-terraclassic

Fork 0

Negative limit query param reaches Postgres as negative LIMIT (500) #284

New issue

Closed

opened 2026-06-03 07:12:12 +00:00 by Brouie · 16 comments

Brouie commented

2026-06-03 07:12:12 +00:00

(Migrated from gitlab.com)

Severity: Informational
Reachability: Unauthenticated HTTP — any list endpoint that clamps limit with .min() only.
Affected: the .min()-only limit clamps (indexer/src/api/pairs.rs:351,501; traders.rs:220/277/342/413/471; cg.rs:300).
Root cause: limit is upper-capped but not lower-clamped, so a negative value passes through to Postgres as a negative LIMIT.

Summary

Several handlers do q.limit.unwrap_or(default).min(max) with no lower bound. If limit deserializes as signed, ?limit=-1 survives the .min() and reaches Postgres, which rejects a negative LIMIT — the request 500s instead of returning a clean 400.

No DoS, no data exposure, just an ungraceful error on bad input. The endpoints that use .clamp(1, max) (e.g. cg.rs:52, pairs.rs:157/987/1066) are already fine.

Recommended direction

Replace the .min(max) limit clamps with .clamp(1, max) everywhere (and reject/normalize negative offsets the same way).

Acceptance criteria

?limit=-1 (and 0) returns a clean result or 400, never a 500.
All list endpoints clamp limit to [1, max].

**Severity:** Informational **Reachability:** Unauthenticated HTTP — any list endpoint that clamps `limit` with `.min()` only. **Affected:** the `.min()`-only limit clamps (`indexer/src/api/pairs.rs:351,501`; `traders.rs:220/277/342/413/471`; `cg.rs:300`). **Root cause:** `limit` is upper-capped but not lower-clamped, so a negative value passes through to Postgres as a negative `LIMIT`. ## Summary Several handlers do `q.limit.unwrap_or(default).min(max)` with no lower bound. If `limit` deserializes as signed, `?limit=-1` survives the `.min()` and reaches Postgres, which rejects a negative `LIMIT` — the request 500s instead of returning a clean 400. No DoS, no data exposure, just an ungraceful error on bad input. The endpoints that use `.clamp(1, max)` (e.g. `cg.rs:52`, `pairs.rs:157/987/1066`) are already fine. ## Recommended direction - Replace the `.min(max)` limit clamps with `.clamp(1, max)` everywhere (and reject/normalize negative offsets the same way). ## Acceptance criteria - [ ] `?limit=-1` (and `0`) returns a clean result or 400, never a 500. - [ ] All list endpoints clamp `limit` to `[1, max]`.

PlasticDigits commented

2026-06-03 10:51:10 +00:00

(Migrated from gitlab.com)

Approved

Brouie commented

2026-06-04 03:13:35 +00:00

(Migrated from gitlab.com)

Fixed — swapped the limit clamps from .min(MAX) to .clamp(1, MAX) so ?limit=-1 and ?limit=0 degrade to a 1-row response instead of a negative LIMIT $N -> Postgres 500. limit is Option<i64> so negatives reach the query; offsets were already lower-guarded (.max(0) + max-offset 400), so only the limit clamps needed it.

Sites: traders.rs (5), pairs.rs (7), cg.rs (1) — plus oracle.rs:108 (/oracle/history), which the original report didn't list but has the exact same q.limit.unwrap_or(200).min(1000) -> LIMIT $3 bug. 14 total. The .clamp(1, MAX) pattern already existed in-tree (pairs.rs:157, cg.rs:52, tokens.rs:71), so this just makes it consistent.

cargo check clean. Branch qa/284-clamp-negative-limit, MR fork→main (no closing keyword). @PlasticDigits

Fixed — swapped the `limit` clamps from `.min(MAX)` to `.clamp(1, MAX)` so `?limit=-1` and `?limit=0` degrade to a 1-row response instead of a negative `LIMIT $N` -> Postgres 500. `limit` is `Option<i64>` so negatives reach the query; offsets were already lower-guarded (`.max(0)` + max-offset 400), so only the limit clamps needed it. Sites: traders.rs (5), pairs.rs (7), cg.rs (1) — plus **oracle.rs:108** (`/oracle/history`), which the original report didn't list but has the exact same `q.limit.unwrap_or(200).min(1000)` -> `LIMIT $3` bug. 14 total. The `.clamp(1, MAX)` pattern already existed in-tree (pairs.rs:157, cg.rs:52, tokens.rs:71), so this just makes it consistent. cargo check clean. Branch `qa/284-clamp-negative-limit`, MR fork→main (no closing keyword). @PlasticDigits

Brouie commented

2026-06-04 03:13:38 +00:00

(Migrated from gitlab.com)

mentioned in merge request !740

PlasticDigits commented

2026-06-04 08:02:26 +00:00

(Migrated from gitlab.com)

mentioned in commit 385e243901

mentioned in commit 385e24390198089ca4537d5ec473044b463edd5d

Brouie commented

2026-06-05 01:19:10 +00:00

(Migrated from gitlab.com)

mentioned in merge request !749

Brouie commented

2026-06-05 01:21:12 +00:00

(Migrated from gitlab.com)

Heads up — this one isn't fully fixed. The .min() -> .clamp(1, MAX) sweep missed one site and it's a live 500.

/api/v1/hooks (get_hook_events, hooks.rs:59) still had params.limit.unwrap_or(50).min(200) with limit: Option<i64> feeding LIMIT $N, so a negative limit reaches Postgres as a negative LIMIT. Live: GET /api/v1/hooks?limit=-1 -> 500 (controls ?limit=5 and ?limit=0 -> 200). The merged fix (c5e5323) touched cg/oracle/pairs/traders but never hooks.rs, and it's the lone surviving .min() limit clamp in api/.

Also: the fix shipped with no regression test for the actual bug — every *_limit_capped test only asserts the upper bound (limit=9999), nothing covers negative/zero, so the suite wouldn't catch this or a re-break.

Pushed the fix as MR !749 — hooks.rs:59 -> .clamp(1, 200) plus a regression test (hooks_negative_and_zero_limit_clamp_to_one_not_500, asserts ?limit=-1 and =0 return 200 and clamp to <=1 row). api_hooks 4/4. Needs to merge before this closes. @PlasticDigits

Heads up — this one isn't fully fixed. The `.min()` -> `.clamp(1, MAX)` sweep missed one site and it's a live 500. `/api/v1/hooks` (get_hook_events, hooks.rs:59) still had `params.limit.unwrap_or(50).min(200)` with `limit: Option<i64>` feeding `LIMIT $N`, so a negative limit reaches Postgres as a negative LIMIT. Live: `GET /api/v1/hooks?limit=-1` -> 500 (controls `?limit=5` and `?limit=0` -> 200). The merged fix (c5e5323) touched cg/oracle/pairs/traders but never hooks.rs, and it's the lone surviving `.min()` limit clamp in `api/`. Also: the fix shipped with no regression test for the actual bug — every `*_limit_capped` test only asserts the upper bound (limit=9999), nothing covers negative/zero, so the suite wouldn't catch this or a re-break. Pushed the fix as MR !749 — hooks.rs:59 -> `.clamp(1, 200)` plus a regression test (`hooks_negative_and_zero_limit_clamp_to_one_not_500`, asserts `?limit=-1` and `=0` return 200 and clamp to <=1 row). api_hooks 4/4. Needs to merge before this closes. @PlasticDigits

PlasticDigits commented

2026-06-05 03:18:21 +00:00

(Migrated from gitlab.com)

mentioned in commit 9edb3b6430

mentioned in commit 9edb3b64306dcb955de4c11a44071bc695fbdc0c

ghost1 commented

2026-06-05 04:07:12 +00:00

(Migrated from gitlab.com)

mentioned in commit 6f620546a6

mentioned in commit 6f620546a648f47a7706c6916394fefe07d43413

PlasticDigits commented

2026-06-05 04:07:43 +00:00

(Migrated from gitlab.com)

mentioned in merge request !766

PlasticDigits commented

2026-06-05 04:11:58 +00:00

(Migrated from gitlab.com)

mentioned in issue #317

PlasticDigits commented

2026-06-05 04:25:05 +00:00

(Migrated from gitlab.com)

mentioned in commit f331d38aa5

mentioned in commit f331d38aa58e9291e377a037e8cd811c51e827bf

Brouie commented

2026-06-05 06:14:43 +00:00

(Migrated from gitlab.com)

Done on main. The original .min(MAX) -> .clamp(1, MAX) sweep plus the hooks.rs site I'd missed are both merged (MR !749, merge 9edb3b6), and the docs note for the clamp caps landed too (f331d38).

Verified:

indexer/src/api/hooks.rs uses params.limit.unwrap_or(50).clamp(1, 200), so ?limit=-1 / ?limit=0 clamp to 1 instead of reaching Postgres as a negative LIMIT (500).
Regression test hooks_negative_and_zero_limit_clamp_to_one_not_500 (api_hooks.rs) guards exactly that.

Scope note: this issue was the negative-limit/500 root cause on the list endpoints, which is fixed. The broader "add the same -1/0 regression coverage to the other clamp sites beyond /hooks" is the follow-up you split out as #317 — not a gap in this fix.

Good to close from my side. @PlasticDigits

Done on main. The original .min(MAX) -> .clamp(1, MAX) sweep plus the hooks.rs site I'd missed are both merged (MR !749, merge 9edb3b6), and the docs note for the clamp caps landed too (f331d38). Verified: - indexer/src/api/hooks.rs uses params.limit.unwrap_or(50).clamp(1, 200), so ?limit=-1 / ?limit=0 clamp to 1 instead of reaching Postgres as a negative LIMIT (500). - Regression test hooks_negative_and_zero_limit_clamp_to_one_not_500 (api_hooks.rs) guards exactly that. Scope note: this issue was the negative-limit/500 root cause on the list endpoints, which is fixed. The broader "add the same -1/0 regression coverage to the other clamp sites beyond /hooks" is the follow-up you split out as #317 — not a gap in this fix. Good to close from my side. @PlasticDigits

Brouie commented

2026-06-05 06:28:28 +00:00

(Migrated from gitlab.com)

mentioned in merge request !773

PlasticDigits commented

2026-06-05 09:46:23 +00:00

(Migrated from gitlab.com)

Verification complete — issue #284

Issue: https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/284

All acceptance criteria PASS. No repo changes required.

Acceptance criteria

Criterion	Result	How verified
`?limit=-1` and `?limit=0` return a clean result or 400, never 500	PASS	Integration tests assert HTTP 200 (clamped to ≥1 row) for every SQL-backed list endpoint with `limit=-1` and `limit=0`.
All list endpoints clamp `limit` to `[1, max]`	PASS	Static scan of `indexer/src/api/**` — no remaining `unwrap_or(…).min(…)` upper-only clamps. All handlers use `.clamp(1, MAX)`.

Commands run

# Static guardrail (no DB)
cd indexer && cargo test --test limit_clamp_guardrail
# → ok (1 passed)

# Hooks regression (GitLab #284 original miss)
cd indexer && cargo test --test api_hooks hooks_negative_and_zero_limit -- --test-threads=1
# → hooks_negative_and_zero_limit_clamp_to_one_not_500 ... ok

# Per-endpoint lower-bound sweep (16 endpoints)
cd indexer && cargo test --test api_limit_lower_bound -- --test-threads=1
# → 16 passed; 0 failed

Code review highlights

hooks.rs:61 — params.limit.unwrap_or(50).clamp(1, 200) (the site missed in the first sweep, fixed in MR !749)
pairs.rs, traders.rs, cg.rs, oracle.rs, tokens.rs — all use .clamp(1, MAX)
Guardrails: limit_clamp_guardrail.rs (static), api_limit_lower_bound.rs (runtime), documented in docs/indexer-invariants.md

Scope note

Broader negative/zero regression coverage for non-/hooks endpoints is tracked separately in #317 (follow-up, not a gap in this fix).

## Verification complete — issue #284 Issue: https://gitlab.com/PlasticDigits/cl8y-dex-terraclassic/-/work_items/284 All acceptance criteria **PASS**. No repo changes required. ### Acceptance criteria | Criterion | Result | How verified | |-----------|--------|--------------| | `?limit=-1` and `?limit=0` return a clean result or 400, never 500 | **PASS** | Integration tests assert HTTP 200 (clamped to ≥1 row) for every SQL-backed list endpoint with `limit=-1` and `limit=0`. | | All list endpoints clamp `limit` to `[1, max]` | **PASS** | Static scan of `indexer/src/api/**` — no remaining `unwrap_or(…).min(…)` upper-only clamps. All handlers use `.clamp(1, MAX)`. | ### Commands run ```bash # Static guardrail (no DB) cd indexer && cargo test --test limit_clamp_guardrail # → ok (1 passed) # Hooks regression (GitLab #284 original miss) cd indexer && cargo test --test api_hooks hooks_negative_and_zero_limit -- --test-threads=1 # → hooks_negative_and_zero_limit_clamp_to_one_not_500 ... ok # Per-endpoint lower-bound sweep (16 endpoints) cd indexer && cargo test --test api_limit_lower_bound -- --test-threads=1 # → 16 passed; 0 failed ``` ### Code review highlights - `hooks.rs:61` — `params.limit.unwrap_or(50).clamp(1, 200)` (the site missed in the first sweep, fixed in MR !749) - `pairs.rs`, `traders.rs`, `cg.rs`, `oracle.rs`, `tokens.rs` — all use `.clamp(1, MAX)` - Guardrails: `limit_clamp_guardrail.rs` (static), `api_limit_lower_bound.rs` (runtime), documented in `docs/indexer-invariants.md` ### Scope note Broader negative/zero regression coverage for non-`/hooks` endpoints is tracked separately in #317 (follow-up, not a gap in this fix).

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-06-05 09:46:27 +00:00

PlasticDigits commented

2026-06-29 15:41:31 +00:00

(Migrated from gitlab.com)

mentioned in merge request !959