Security: limit parameter tests cover only huge values, not negative or zero inputs [SEC-F05]

totdking commented

2026-06-29 15:30:19 +00:00

(Migrated from gitlab.com)

No description provided.

totdking commented

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

(Migrated from gitlab.com)

Summary

Seven list routes in the indexer API enforce an upper-bound cap on the limit query parameter. Integration tests in indexer/tests/security.rs confirm the cap by sending limit=99999 and asserting the response count does not exceed the max. No test sends a negative value or limit=0 on any route. The checklist explicitly requires tests covering negative, zero, huge, and valid values. Lower-bound behavior for negative and zero inputs is unverified by any automated test.

What Was Checked

indexer/tests/security.rs lines 152-340: all seven capped-limit tests use limit=99999 or limit=999999. Searched for "negative", "limit=-", "limit=0", "zero" across security.rs -- none present. Confirmed by direct file read.
Routes covered by the upper-bound tests: /api/v1/pairs/{addr}/trades (line 160), /api/v1/pairs/{addr}/candles (line 179), /api/v1/oracle/history (line 237), /api/v1/traders/{addr}/trades (line 279), /api/v1/pairs/{addr}/limit-fills (line 297), /api/v1/pairs/{addr}/limit-placements (line 315), /api/v1/pairs/{addr}/limit-cancellations (line 333).
No separate lower-bound test file found.

Expected (per checklist)

Integration tests must cover negative, zero, huge, and valid values for every list/depth/limit route. A negative or zero limit must return a 400 or be treated as the minimum valid value (1), not silently return all rows or panic.

Actual

Tests confirm the upper cap (huge values are clamped to the max and the response returns HTTP 200), but negative and zero inputs are not tested. The lower-bound behavior is unverified.

Evidence

indexer/tests/security.rs line 160: "/api/v1/pairs/{}/trades?limit=99999" -- no matching negative/zero test exists
indexer/tests/security.rs line 179: "/api/v1/pairs/{}/candles?limit=99999" -- no matching negative/zero test exists
indexer/tests/security.rs line 237: "/api/v1/oracle/history?limit=999999" -- no matching negative/zero test exists
indexer/tests/security.rs lines 279, 297, 315, 333: trader routes -- no matching negative/zero tests

Suggested Fix

Add tests to indexer/tests/security.rs for each of the seven routes: send limit=-1 and assert the response is 400; send limit=0 and assert the response is 400 or returns an empty list (whichever matches the intended lower-bound contract). Document the intended behavior in a comment near each test.

Verification Checklist

security.rs has a test for limit=-1 on all seven capped list routes asserting 400 or defined minimum behavior
security.rs has a test for limit=0 on all seven capped list routes asserting 400 or defined minimum behavior
All seven routes covered: trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations

Labels

security, pre-launch

Cc: @PlasticDigits

### Summary Seven list routes in the indexer API enforce an upper-bound cap on the `limit` query parameter. Integration tests in `indexer/tests/security.rs` confirm the cap by sending `limit=99999` and asserting the response count does not exceed the max. No test sends a negative value or `limit=0` on any route. The checklist explicitly requires tests covering negative, zero, huge, and valid values. Lower-bound behavior for negative and zero inputs is unverified by any automated test. --- ### What Was Checked - `indexer/tests/security.rs` lines 152-340: all seven capped-limit tests use `limit=99999` or `limit=999999`. Searched for "negative", "limit=-", "limit=0", "zero" across security.rs -- none present. Confirmed by direct file read. - Routes covered by the upper-bound tests: `/api/v1/pairs/{addr}/trades` (line 160), `/api/v1/pairs/{addr}/candles` (line 179), `/api/v1/oracle/history` (line 237), `/api/v1/traders/{addr}/trades` (line 279), `/api/v1/pairs/{addr}/limit-fills` (line 297), `/api/v1/pairs/{addr}/limit-placements` (line 315), `/api/v1/pairs/{addr}/limit-cancellations` (line 333). - No separate lower-bound test file found. --- ### Expected (per checklist) Integration tests must cover negative, zero, huge, and valid values for every list/depth/limit route. A negative or zero `limit` must return a 400 or be treated as the minimum valid value (1), not silently return all rows or panic. --- ### Actual Tests confirm the upper cap (huge values are clamped to the max and the response returns HTTP 200), but negative and zero inputs are not tested. The lower-bound behavior is unverified. --- ### Evidence - `indexer/tests/security.rs` line 160: `"/api/v1/pairs/{}/trades?limit=99999"` -- no matching negative/zero test exists - `indexer/tests/security.rs` line 179: `"/api/v1/pairs/{}/candles?limit=99999"` -- no matching negative/zero test exists - `indexer/tests/security.rs` line 237: `"/api/v1/oracle/history?limit=999999"` -- no matching negative/zero test exists - `indexer/tests/security.rs` lines 279, 297, 315, 333: trader routes -- no matching negative/zero tests --- ### Suggested Fix Add tests to `indexer/tests/security.rs` for each of the seven routes: send `limit=-1` and assert the response is 400; send `limit=0` and assert the response is 400 or returns an empty list (whichever matches the intended lower-bound contract). Document the intended behavior in a comment near each test. --- ### Verification Checklist - [ ] `security.rs` has a test for `limit=-1` on all seven capped list routes asserting 400 or defined minimum behavior - [ ] `security.rs` has a test for `limit=0` on all seven capped list routes asserting 400 or defined minimum behavior - [ ] All seven routes covered: trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations --- ### Labels `security`, `pre-launch` Cc: @PlasticDigits

totdking commented

2026-06-29 15:39:07 +00:00

(Migrated from gitlab.com)

mentioned in issue #381

PlasticDigits commented

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

(Migrated from gitlab.com)

mentioned in commit 536ff00cc1

mentioned in commit 536ff00cc15846244262c2982094c555df448e21

PlasticDigits commented

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

(Migrated from gitlab.com)

mentioned in merge request !959

PlasticDigits commented

2026-06-29 15:49:23 +00:00

(Migrated from gitlab.com)

mentioned in commit 55cf80af2e

mentioned in commit 55cf80af2e6f2f4b6e562b106fe9d99df6e5168a

PlasticDigits commented

2026-06-29 15:53:43 +00:00

(Migrated from gitlab.com)

Verification — GitLab #431 (SEC-F05)

Result: PASS — negative and zero limit coverage is present on all seven capped list routes in indexer/tests/security.rs; no repo changes required.

Checklist

Acceptance item	Result	How verified
`limit=-1` test on all seven capped list routes (400 or defined minimum)	PASS	Shared helper `assert_negative_or_zero_limit_clamps_to_one` (lines 342–371) sends `limit=-1`; contract documented as clamp to 1 via `.clamp(1, max)` → HTTP 200, ≤1 row. Seven route-specific tests at lines 373–461.
`limit=0` test on all seven capped list routes (400 or defined minimum)	PASS	Same helper also sends `limit=0` with identical assertions.
Routes: trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations	PASS	`pair_trades_negative_and_zero_limit_clamp_to_one`, `pair_candles_negative_and_zero_limit_clamp_to_one`, `oracle_history_negative_and_zero_limit_clamp_to_one`, `trader_trades_negative_and_zero_limit_clamp_to_one`, `limit_fills_negative_and_zero_limit_clamp_to_one`, `limit_placements_negative_and_zero_limit_clamp_to_one`, `limit_cancellations_negative_and_zero_limit_clamp_to_one`.
Upper-bound (huge value) tests still present	PASS	Existing cap tests (`trades_limit_capped_at_200`, etc.) unchanged.

Commands run

make setup-indexer-postgres
cd indexer && cargo test --test security -- --test-threads=1 \
  pair_trades_negative pair_candles_negative oracle_history_negative \
  trader_trades_negative limit_fills_negative limit_placements_negative \
  limit_cancellations_negative
# → 7 passed

cd indexer && cargo test --test security -- --test-threads=1 \
  trades_limit candles_limit oracle_history_limit trader_trades_limit \
  limit_fills_limit limit_placements_limit limit_cancellations_limit
# → 7 passed

Notes

Intended lower-bound contract is clamp to 1 (not 400), documented in the helper comment referencing #284/#431. This matches the issue’s “400 or minimum valid value (1)” acceptance.
Broader negative/zero coverage also exists in indexer/tests/api_limit_lower_bound.rs and indexer/tests/limit_clamp_guardrail.rs (static guard against .min(MAX) regressions).

Closing as verified on main.

## Verification — GitLab #431 (SEC-F05) **Result: PASS** — negative and zero `limit` coverage is present on all seven capped list routes in `indexer/tests/security.rs`; no repo changes required. ### Checklist | Acceptance item | Result | How verified | |-----------------|--------|--------------| | `limit=-1` test on all seven capped list routes (400 or defined minimum) | **PASS** | Shared helper `assert_negative_or_zero_limit_clamps_to_one` (lines 342–371) sends `limit=-1`; contract documented as clamp to **1** via `.clamp(1, max)` → HTTP **200**, ≤1 row. Seven route-specific tests at lines 373–461. | | `limit=0` test on all seven capped list routes (400 or defined minimum) | **PASS** | Same helper also sends `limit=0` with identical assertions. | | Routes: trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations | **PASS** | `pair_trades_negative_and_zero_limit_clamp_to_one`, `pair_candles_negative_and_zero_limit_clamp_to_one`, `oracle_history_negative_and_zero_limit_clamp_to_one`, `trader_trades_negative_and_zero_limit_clamp_to_one`, `limit_fills_negative_and_zero_limit_clamp_to_one`, `limit_placements_negative_and_zero_limit_clamp_to_one`, `limit_cancellations_negative_and_zero_limit_clamp_to_one`. | | Upper-bound (huge value) tests still present | **PASS** | Existing cap tests (`trades_limit_capped_at_200`, etc.) unchanged. | ### Commands run ```bash make setup-indexer-postgres cd indexer && cargo test --test security -- --test-threads=1 \ pair_trades_negative pair_candles_negative oracle_history_negative \ trader_trades_negative limit_fills_negative limit_placements_negative \ limit_cancellations_negative # → 7 passed cd indexer && cargo test --test security -- --test-threads=1 \ trades_limit candles_limit oracle_history_limit trader_trades_limit \ limit_fills_limit limit_placements_limit limit_cancellations_limit # → 7 passed ``` ### Notes - Intended lower-bound contract is **clamp to 1** (not 400), documented in the helper comment referencing #284/#431. This matches the issue’s “400 **or** minimum valid value (1)” acceptance. - Broader negative/zero coverage also exists in `indexer/tests/api_limit_lower_bound.rs` and `indexer/tests/limit_clamp_guardrail.rs` (static guard against `.min(MAX)` regressions). Closing as verified on `main`.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-06-29 15:53:45 +00:00

Brouie commented

2026-06-29 15:58:56 +00:00

(Migrated from gitlab.com)

Independently verified the merged !959 on main. The lower-bound tests in security.rs cover all seven capped routes — trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations — and each exercises BOTH limit=-1 and limit=0 via the assert_negative_or_zero_limit_clamps_to_one helper, asserting 200 + at most one row (the clamp-to-1 contract, which keeps a negative/zero value from reaching Postgres as a negative SQL LIMIT — the old #284 HTTP-500 path).

Ran the full security suite serial here: 35/35 (28 existing + the 7 new), 0 failed. clamp-to-1 is a sound lower-bound contract (the checklist allowed 400 or a defined minimum). Confirmed good — saw it was already closed.

Independently verified the merged !959 on main. The lower-bound tests in security.rs cover all seven capped routes — trades, candles, oracle/history, trader trades, limit-fills, limit-placements, limit-cancellations — and each exercises BOTH limit=-1 and limit=0 via the assert_negative_or_zero_limit_clamps_to_one helper, asserting 200 + at most one row (the clamp-to-1 contract, which keeps a negative/zero value from reaching Postgres as a negative SQL LIMIT — the old #284 HTTP-500 path). Ran the full security suite serial here: 35/35 (28 existing + the 7 new), 0 failed. clamp-to-1 is a sound lower-bound contract (the checklist allowed 400 or a defined minimum). Confirmed good — saw it was already closed.

Brouie commented

2026-06-30 03:15:16 +00:00

(Migrated from gitlab.com)

mentioned in issue #337