hooks (#317) #773

Merged

Brouie merged 1 commit from qa/317-negative-limit-coverage into main

2026-06-05 07:03:53 +00:00

Brouie commented

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

(Migrated from gitlab.com)

Follow-up to #284 (the missing lower-bound test coverage I flagged in that thread).

#284 regressed on /api/v1/hooks because the .min(MAX) → .clamp(1, MAX) sweep had no lower-bound test coverage — every *_limit_capped test only asserts the UPPER cap (limit=99999 → len <= MAX). A future refactor reintroducing .min(MAX) on any route would pass CI and 500 on limit=-1. This adds the lower-bound half (tests only, no handler changes):

api_limit_lower_bound.rs — limit=-1 and limit=0 → 200 (never 500) + clamp to ≤1 row, for every SQL-backed list endpoint: pairs list/candles/trades/liquidity-events/limit-fills/placements/cancellations, tokens, traders leaderboard/trades/limit-*, oracle history, cg pairs/historical_trades (16 tests).
api_orderbook_lcd_mock.rs — depth=-1/0 on /cg/orderbook + /cmc/orderbook/{pair} must not 500. depth is usize, so -1 is a 400 at deserialization (handler never runs) and 0 clamps to 1 — either way never 500 (2 tests).
limit_clamp_guardrail.rs — static test (no DB) that fails if any src/api/**.rs line reintroduces the upper-only unwrap_or(…).min(…) idiom. Would have caught #284 directly. The 3 legit bare .min( uses (capacity hint, page-slice bound) lack unwrap_or on the same line and are not flagged.
docs/indexer-invariants.md — numeric-caps row lists the new coverage.

Verified: 16 + 2 + 1 new tests pass; existing upper-bound *_limit_capped suite (security 22, hooks 4, traders 15, cg 10) unchanged.

Follow-up to #284 (the missing lower-bound test coverage I flagged in that thread). #284 regressed on `/api/v1/hooks` because the `.min(MAX)` → `.clamp(1, MAX)` sweep had no lower-bound test coverage — every `*_limit_capped` test only asserts the UPPER cap (`limit=99999` → `len <= MAX`). A future refactor reintroducing `.min(MAX)` on any route would pass CI and 500 on `limit=-1`. This adds the lower-bound half (tests only, no handler changes): - **`api_limit_lower_bound.rs`** — `limit=-1` and `limit=0` → 200 (never 500) + clamp to ≤1 row, for every SQL-backed list endpoint: pairs list/candles/trades/liquidity-events/limit-fills/placements/cancellations, tokens, traders leaderboard/trades/limit-*, oracle history, cg pairs/historical_trades (16 tests). - **`api_orderbook_lcd_mock.rs`** — `depth=-1`/`0` on `/cg/orderbook` + `/cmc/orderbook/{pair}` must not 500. `depth` is `usize`, so `-1` is a 400 at deserialization (handler never runs) and `0` clamps to 1 — either way never 500 (2 tests). - **`limit_clamp_guardrail.rs`** — static test (no DB) that fails if any `src/api/**.rs` line reintroduces the upper-only `unwrap_or(…).min(…)` idiom. Would have caught #284 directly. The 3 legit bare `.min(` uses (capacity hint, page-slice bound) lack `unwrap_or` on the same line and are not flagged. - **`docs/indexer-invariants.md`** — numeric-caps row lists the new coverage. Verified: 16 + 2 + 1 new tests pass; existing upper-bound `*_limit_capped` suite (security 22, hooks 4, traders 15, cg 10) unchanged.

Brouie commented

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

(Migrated from gitlab.com)

mentioned in issue #317

PlasticDigits commented

2026-06-05 06:49:19 +00:00

(Migrated from gitlab.com)

Security review

Commit reviewed: 971253b3a6a575ffc1bde25e6b5d4d1628bdf4cb
Scope: MR !773 — tests-only lower-bound regression coverage for indexer limit/depth query params (#317 follow-up to #284).

Files reviewed

indexer/tests/api_limit_lower_bound.rs (new)
indexer/tests/api_orderbook_lcd_mock.rs (2 new tests)
indexer/tests/limit_clamp_guardrail.rs (new)
docs/indexer-invariants.md (doc pointer update)

Method

Traced attacker-controlled inputs (limit, depth) through the exercised HTTP test paths. Confirmed this diff adds no handler, routing, auth, or SQL changes — only integration/static tests and documentation. Existing production controls (.clamp(1, MAX) on list limits; usize deserialization + cap_orderbook_depth for orderbook depth) are unchanged.

Outcome

FINDINGS: 0 medium+

Security review: no medium+ findings on this diff.

No inline threads — nothing to flag at specific diff lines.

Notes (informational, not findings)

The static limit_clamp_guardrail same-line heuristic is a useful CI guardrail but cannot catch multi-line refactors; runtime coverage in api_limit_lower_bound.rs compensates for SQL-backed routes.
Lower-bound tests for wrapped JSON bodies (pairs list, oracle history, CG historical_trades) assert status-only (documented race-safety trade-off); this is test coverage scope, not a production exposure.

## Security review **Commit reviewed:** `971253b3a6a575ffc1bde25e6b5d4d1628bdf4cb` **Scope:** MR !773 — tests-only lower-bound regression coverage for indexer `limit`/`depth` query params (#317 follow-up to #284). ### Files reviewed - `indexer/tests/api_limit_lower_bound.rs` (new) - `indexer/tests/api_orderbook_lcd_mock.rs` (2 new tests) - `indexer/tests/limit_clamp_guardrail.rs` (new) - `docs/indexer-invariants.md` (doc pointer update) ### Method Traced attacker-controlled inputs (`limit`, `depth`) through the exercised HTTP test paths. Confirmed this diff adds **no handler, routing, auth, or SQL changes** — only integration/static tests and documentation. Existing production controls (`.clamp(1, MAX)` on list limits; `usize` deserialization + `cap_orderbook_depth` for orderbook depth) are unchanged. ### Outcome **FINDINGS: 0** medium+ **Security review: no medium+ findings on this diff.** No inline threads — nothing to flag at specific diff lines. ### Notes (informational, not findings) - The static `limit_clamp_guardrail` same-line heuristic is a useful CI guardrail but cannot catch multi-line refactors; runtime coverage in `api_limit_lower_bound.rs` compensates for SQL-backed routes. - Lower-bound tests for wrapped JSON bodies (pairs list, oracle history, CG historical_trades) assert status-only (documented race-safety trade-off); this is test coverage scope, not a production exposure.