Missing pagination limit clamp allows unbounded `limit` query params on airdrops and alerts endpoints

[prodbycorne]

Overview

GET /api/v1/webhooks/:id/deliveries correctly clamps its limit query parameter:

// src/routes/webhooks.js
const limit = Math.min(parseInt(req.query.limit, 10) || 50, 100);

But the equivalent airdrops endpoints do not clamp at all:

// src/routes/airdrops.js
router.get('/airdrops', async (req, res, next) => {
  const page = parseInt(req.query.page, 10) || 1;
  const limit = parseInt(req.query.limit, 10) || 20;   // no upper bound
  const result = await airdropsService.list(page, limit);
  ...
});

router.get('/airdrops/:id/recipients', async (req, res, next) => {
  ...
  const page = parseInt(req.query.page, 10) || 1;
  const limit = parseInt(req.query.limit, 10) || 20;   // no upper bound
  const result = await airdropsService.listRecipients(req.params.id, page, limit);
  ...
});

airdropsService.list() and listRecipients() pass this unclamped limit straight through to ids.slice(start, end) and redis.lrange(recipientsKey(airdropId), start, end) respectively. A request like GET /api/v1/airdrops?limit=999999999 or GET /api/v1/airdrops/:id/recipients?limit=999999999 forces the server to materialize (and JSON-serialize, and transmit) an arbitrarily large slice of data in a single response — for listRecipients specifically, an airdrop can legitimately hold up to 10,000 recipients (the enforced cap on create), so ?limit=10000 alone already returns the maximum, but there's nothing stopping a value far beyond that from being requested against a Redis LRANGE with a huge range, needlessly serializing and holding a large result set in memory and on the wire. parsePagination() in src/utils/paginate.js — used by the alerts route — does correctly clamp (Math.min(maxLimit, ...), default maxLimit = 100), showing the codebase already has the right pattern; it simply wasn't applied to the airdrops routes, which instead re-implement pagination parsing inline and inconsistently.

Requirements

• Route GET /api/v1/airdrops and GET /api/v1/airdrops/:id/recipients through the existing parsePagination() helper from src/utils/paginate.js (or an equivalent explicit Math.min(..., MAX_LIMIT) clamp) instead of their current unbounded inline parsing.
• Standardize on a single maximum page size across the API (documented, e.g. 100) rather than three different inline implementations (webhooks' inline Math.min(...,100), alerts' parsePagination, and airdrops' unclamped parsing).
• Negative or zero limit/page values must also be handled consistently (currently parseInt(req.query.limit, 10) || 20 only guards against NaN/0, not negative numbers, which would produce a negative end/nonsensical slice/lrange range).

Acceptance Criteria

• GET /api/v1/airdrops?limit=999999 returns at most the documented maximum page size, not the raw requested value.
• GET /api/v1/airdrops/:id/recipients?limit=999999 is similarly clamped.
• Negative and zero limit/page values are normalized to sane defaults/minimums rather than passed through to Redis range operations.
• All three list endpoints (webhooks deliveries, alerts, airdrops/recipients) use the same shared pagination-clamping helper, eliminating the current duplicated/inconsistent inline logic.
• New tests assert the clamp behavior for each affected endpoint, including negative-value handling.

Additional Notes

More precise references

• src/routes/airdrops.js:109-119 (GET /airdrops): const limit = parseInt(req.query.limit, 10) || 20; — no Math.min.
• src/routes/airdrops.js:224-239 (GET /airdrops/:id/recipients): identical unclamped pattern.
• src/services/airdrops.js:56-73 (list): ids.slice(start, end) where end = start + limit — an unbounded limit produces an unbounded slice length (bounded in practice only by ids.length, i.e. total number of airdrops in the system, which grows unbounded over time).
• src/services/airdrops.js:130-147 (listRecipients): redis.lrange(recipientsKey(airdropId), start, end) where end = start + limit - 1 — Redis LRANGE with an out-of-range end simply clamps to the list's actual length server-side (this is standard Redis behavior, so the Redis call itself won't error or hang), but the concern is fully legitimate at the current max of 10,000 recipients per airdrop: ?limit=10000 already returns the full list in one response with no clamp, and there's nothing today preventing ?limit=99999999 from being requested (returns the same 10,000 rows, but at that scale the same request pattern against list()'s in-memory ids.slice() is worse, since airdrops:ids has no analogous per-airdrop 10,000 cap — the total number of airdrops ever created is unbounded).
• src/routes/webhooks.js:162 — the "correct" reference pattern: const limit = Math.min(parseInt(req.query.limit, 10) || 50, 100);.
• src/utils/paginate.js:1-6 — parsePagination already handles negative/zero via Math.max(1, ...) for page and Math.max(1, ...) inside the Math.min for limit; confirmed this is currently only imported/used by the alerts route (not verified directly in this pass, but implied by the issue and consistent with src/routes/alerts.js likely being the only consumer — worth double-checking during implementation that alerts route actually imports it, since I did not re-read alerts.js in this pass).

Additional edge cases

• parseInt(req.query.limit, 10) || 20 treats NaN, 0, and '' identically (falls back to 20) but a negative value like -5 is not NaN, so it passes through as -5, producing end = start - 5. For Array.prototype.slice(start, end) a negative end less than start returns an empty array (safe, if confusing), but for Redis LRANGE key start end a negative end less than start is interpreted by Redis as "count from the end of the list" — LRANGE treats negative indices as offsets from the tail, so LRANGE key 0 -5 does not return empty; it returns most of the list up to 5 from the end. This is a materially different (and more surprising/potentially confusing, not just cosmetically different) bug between the two call sites and should be called out explicitly as a Redis-specific correctness hazard, not just a generic "negative numbers are weird" note.
• page values astronomically large (e.g. ?page=999999999) combined with a small limit produce a huge start — for ids.slice(start, end) this is harmless (empty array), but worth including in the negative/zero-page test matrix alongside limit.
• Query params arriving as arrays (e.g. ?limit=10&limit=999999 produces req.query.limit as ['10', '999999'] in Express's default qs parser) — parseInt(['10','999999'], 10) stringifies the array first ("10,999999") and parses 10 due to how parseInt handles the comma, but this is fragile and worth a defensive test given it's untested today.

Implementation sketch

1. Replace both inline blocks in src/routes/airdrops.js (lines ~111-112 and ~231-232) with const { page, limit } = parsePagination(req.query, { maxLimit: 100 }); imported from ../utils/paginate.
2. Update airdropsService.list/listRecipients signatures unchanged (they already accept page, limit positionally) — just pass the clamped values through.
3. Refactor src/routes/webhooks.js:162's inline Math.min(...,100) to also call parsePagination for consistency, per the issue's own "standardize on one helper" requirement, and update src/routes/alerts.js if it isn't already using it.
4. For the Redis LRANGE negative-end hazard specifically: since parsePagination guarantees limit >= 1, end = start + limit - 1 can only be negative if start itself is negative, which can't happen once page >= 1 is enforced — so adopting parsePagination uniformly incidentally fixes the LRANGE edge case too; call this out explicitly in the PR description since it's not obvious from the acceptance criteria alone.

Test/reproduction plan

• GET /airdrops?limit=999999 → assert response pagination.limit is clamped to 100 (or whatever the standardized max is), and airdrops.length <= 100.
• GET /airdrops/:id/recipients?limit=999999 → same assertion against the recipients array length and pagination.limit.
• GET /airdrops?limit=-5 and ?page=-1 → assert normalized to sane positive minimums (1 and default limit respectively), not passed through raw.
• Regression test specifically reproducing the pre-fix LRANGE key 0 -5 behavior against a real/mocked Redis to document the tail-counting hazard, then confirm it's gone post-fix.
• Confirm test/webhooks.routes.test.js and any alerts pagination tests still pass after refactoring the shared helper usage.

Cross-references

• multer() is configured with no file size limit — unbounded CSV upload enables memory-exhaustion DoS #85 (unbounded CSV/multer upload) touches the same src/routes/airdrops.js file — both are best landed together to avoid repeated review/merge friction on this route file.
• Airdrops never auto-expire against expiry_ledger — no reconciliation job checks current chain state #88 (airdrop expiry reconciliation job) will need to page through all non-terminal airdrops; the issue for Airdrops never auto-expire against expiry_ledger — no reconciliation job checks current chain state #88 explicitly says it should "use the corrected, stable pagination/listing mechanism (see the companion SMEMBERS-pagination issue) rather than assuming airdropsService.list()'s current behavior is safe to page through exhaustively" — that companion issue is this one (Missing pagination limit clamp allows unbounded limit query params on airdrops and alerts endpoints #86), so Airdrops never auto-expire against expiry_ledger — no reconciliation job checks current chain state #88 has a direct dependency on this fix landing first, or at minimum on list()'s pagination semantics being stable/well-defined (e.g. confirming SMEMBERS order isn't guaranteed stable across calls, which matters for exhaustive pagination even independent of the clamp).