From 47674dc3224383b110c36ee1b752c7fd60ee3d7d Mon Sep 17 00:00:00 2001 From: Dave Grochowski Date: Tue, 10 Nov 2026 22:46:43 -0500 Subject: [PATCH] fix(relay): saturate filter limits instead of casting (#9230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Four request paths converted a client-supplied `usize` filter limit with an inline `as` cast before clamping: `(l as i64).min(max)` (REQ query params, bridge feed) and `(l as u32).min(max)` (WS NIP-50 search, bridge channel window). - A limit above `i64::MAX` turns negative (`usize::MAX as i64 == -1`). On REQ and the bridge `/query` general path it reaches Postgres as `LIMIT -1`, which fails the query. On the bridge feed it trips `feed_count >= limit`, so the feed comes back empty. - A limit that is a multiple of 2^32 wraps to `0` through `as u32`. That returns no rows (one row in the window, via `.max(1)`) instead of the page max. Bridge thread and HTTP search already clamped before casting. COUNT drops the row limit, so it was not affected. ## Change - Add `buzz_core::filter::clamp_limit(limit, default, max) -> u32`. It returns `default` when the limit is absent; otherwise it saturates the request to `u32` and then caps it at `max`. - Use it at every filter-limit site: `filter_to_query_params` and WS search in `handlers/req.rs`; channel window, feed, thread and HTTP search in `api/bridge.rs`. The inline casts are deleted, and `BRIDGE_FEED_MAX_LIMIT` is now `u32`. - Each site keeps its default, max and `limit: 0` behaviour. In-range limits are unchanged. ## Testing Failing before the fix (`cargo test -p buzz-relay --lib huge_filter_limit`): ``` assertion `left == right` failed left: Some(-1) right: Some(1000) test result: FAILED. 0 passed; 1 failed ``` After: - `cargo test -p buzz-core --lib clamp_limit` → 1 passed (table test covering absent, 0, in-range, at/over max, the u32 boundary and `usize::MAX`) - `cargo test -p buzz-relay --lib -- huge_filter_limit req:: bridge` → 152 passed, 0 failed (135 Postgres tests ignored) - `cargo clippy -p buzz-core -p buzz-relay --all-targets -- -D warnings` → clean; `cargo fmt --all` → clean - pre-push hooks (incl. rust-tests) passed ## Not covered No Postgres-backed tests were run locally. The bridge call sites rely on the shared helper's table test; they have no per-handler tests. ## Review Every server-side read of `Filter.limit` now goes through `clamp_limit`. The per-site default, max and `limit: 0` behaviour is unchanged. No inline casts are left. Scores: minimalism 9, elegance 9, correctness 9. Blockers: none. Fixed in review: - `PAGE_LIMIT_MAX` had been inserted between `filter_to_query_params`'s doc comment and the function, so the doc comment ended up on the const. Moved the const above it. - Corrected the problem statement. The bridge feed returned an empty feed rather than a Postgres error, COUNT was never affected, and thread and HTTP search were already safe. Optional: - WS search, bridge window and bridge feed have no per-handler regression test. Reverting one of them to an inline cast would not fail any test. - `bridge.rs` now has two functions called `clamp_limit`: the local moderation `clamp_limit(Option)` and `buzz_core::filter::clamp_limit`. They don't collide, but they are easy to confuse. ## Exercised on a local relay **Commit tested:** `88d76f7f1`. Dev profile, isolated harness relay on :3030, authenticated channel member. Human rate limits were raised so seeding wasn't throttled. I seeded 1,100 kind-9 events in one channel. The filters were sent as raw JSON so the large integers arrived exactly. | Path | `limit` 18446744073709551615 | `limit` 4294967296 | `limit` 50 (control) | |---|---|---|---| | WS `REQ` | EOSE after 1000 | EOSE after 1000 | EOSE after 50 | | WS `REQ` with `search` | EOSE after 1000 | EOSE after 1000 | EOSE after 50 | | HTTP `/query` | 200, 1000 events | 200, 1000 | 200, 50 | | HTTP `/query` with `search` | 200, 500 | 200, 500 | 200, 50 | | HTTP `/query` `top_level` (channel window) | 200, 201 (200 + bounds event) | 200, 201 | 200, 51 | | HTTP `/query` `feed_types` | 200, 100 | 200, 100 | 200, 50 | Every huge limit came back as that path's maximum. None of them produced a database error, and none came back empty. The relay log has no error lines, and the access log records the raw `limit` as 18446744073709551615 with `outcome:"served"`. **Postgres-gated tests:** the PR adds or modifies none. As a related check I ran `cargo test -p buzz-relay --lib -- --ignored limit`. Run in parallel, it gave 10 passed and 1 failed: `rate_limited_query_logs_one_denied_line`. That test passes on its own, and the whole set gives 11 passed with `--test-threads=1`. It looks like interference between tests that share the access-log capture, not a problem with this change. **Not exercised:** - Bridge thread path. - A before/after comparison against main. Signed-off-by: David Grochowski --- crates/buzz-core/src/filter.rs | 27 +++++++++++++++++++++++++ crates/buzz-relay/src/api/bridge.rs | 29 +++++++++++++-------------- crates/buzz-relay/src/handlers/req.rs | 26 ++++++++++++++++-------- 3 files changed, 59 insertions(+), 23 deletions(-) diff --git a/crates/buzz-core/src/filter.rs b/crates/buzz-core/src/filter.rs index 73b7d6e56b..6d8b23094c 100644 --- a/crates/buzz-core/src/filter.rs +++ b/crates/buzz-core/src/filter.rs @@ -189,6 +189,15 @@ fn filter_match_one(f: &Filter, ev: &StoredEvent) -> bool { true } +/// Resolves a requested result limit: `default` when absent, otherwise the +/// request saturated to `max`. `Some(0)` stays `0` (NIP-01 "no results"). +/// +/// Saturates before narrowing, so a huge `usize` limit cannot wrap to `0` +/// through `as u32` or turn negative through `as i64`. +pub fn clamp_limit(limit: Option, default: u32, max: u32) -> u32 { + limit.map_or(default, |l| u32::try_from(l).unwrap_or(u32::MAX).min(max)) +} + #[cfg(test)] mod tests { use super::*; @@ -196,6 +205,24 @@ mod tests { use chrono::Utc; use nostr::{EventBuilder, Keys, Kind, Tag, Timestamp}; + #[test] + fn clamp_limit_table() { + let cases: &[(Option, u32, u32, u32)] = &[ + (None, 20, 100, 20), + (Some(0), 20, 100, 0), + (Some(7), 20, 100, 7), + (Some(100), 20, 100, 100), + (Some(101), 20, 100, 100), + (Some(u32::MAX as usize), 20, 100, 100), + (Some(u32::MAX as usize + 1), 20, 100, 100), + (Some(usize::MAX), 20, 100, 100), + (Some(usize::MAX), 1000, u32::MAX, u32::MAX), + ]; + for &(limit, default, max, want) in cases { + assert_eq!(clamp_limit(limit, default, max), want, "{limit:?}"); + } + } + fn stored_with_tag(tag: Tag) -> StoredEvent { let keys = Keys::generate(); let event = EventBuilder::new(Kind::TextNote, "test") diff --git a/crates/buzz-relay/src/api/bridge.rs b/crates/buzz-relay/src/api/bridge.rs index ff7481e810..6204f3d936 100644 --- a/crates/buzz-relay/src/api/bridge.rs +++ b/crates/buzz-relay/src/api/bridge.rs @@ -712,7 +712,7 @@ fn extract_channel_from_filter(filter: &nostr::Filter) -> Option { // Nostr filter JSON. nostr::Filter silently drops unknown fields during // deserialization, so we extract them from the raw JSON Value first. -const BRIDGE_FEED_MAX_LIMIT: i64 = 100; +const BRIDGE_FEED_MAX_LIMIT: u32 = 100; const BRIDGE_THREAD_MAX_LIMIT: u32 = 500; /// The `before_id` extension field, with "present but malformed" kept distinct @@ -1065,11 +1065,12 @@ async fn handle_channel_window_filter( } }; - let limit = filter - .limit - .map(|l| (l as u32).min(BRIDGE_WINDOW_MAX_LIMIT)) - .unwrap_or(BRIDGE_WINDOW_DEFAULT_LIMIT) - .max(1); + let limit = buzz_core::filter::clamp_limit( + filter.limit, + BRIDGE_WINDOW_DEFAULT_LIMIT, + BRIDGE_WINDOW_MAX_LIMIT, + ) + .max(1); let kind_filter: Option> = filter .kinds .as_ref() @@ -1915,10 +1916,11 @@ async fn query_member_events( None => continue, }; - let limit = filter - .limit - .map(|l| (l as i64).min(BRIDGE_FEED_MAX_LIMIT)) - .unwrap_or(20); + let limit = i64::from(buzz_core::filter::clamp_limit( + filter.limit, + 20, + BRIDGE_FEED_MAX_LIMIT, + )); let since = filter .since .and_then(|s| chrono::DateTime::from_timestamp(s.as_secs() as i64, 0)); @@ -2033,10 +2035,7 @@ async fn query_member_events( } } - let limit = filter - .limit - .unwrap_or(100) - .min(BRIDGE_THREAD_MAX_LIMIT as usize) as u32; + let limit = buzz_core::filter::clamp_limit(filter.limit, 100, BRIDGE_THREAD_MAX_LIMIT); let thread_cursor = extract_thread_cursor(raw); let thread_replies = state .db @@ -2732,7 +2731,7 @@ async fn handle_bridge_search( _ => continue, }; - let limit = filter.limit.unwrap_or(100).min(500) as u32; + let limit = buzz_core::filter::clamp_limit(filter.limit, 100, 500); if limit == 0 { continue; } diff --git a/crates/buzz-relay/src/handlers/req.rs b/crates/buzz-relay/src/handlers/req.rs index 39fb6c0fb8..f573a4e851 100644 --- a/crates/buzz-relay/src/handlers/req.rs +++ b/crates/buzz-relay/src/handlers/req.rs @@ -945,10 +945,7 @@ async fn handle_search_req( let accessible_channels = &*super::system_channels::filter_channel_scope(filter, implicit_channels, named_only); - let limit = filter - .limit - .map(|l| (l as u32).min(buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32)) - .unwrap_or(buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32); + let limit = buzz_core::filter::clamp_limit(filter.limit, PAGE_LIMIT_MAX, PAGE_LIMIT_MAX); if limit == 0 { continue; // NIP-01: limit 0 means "no results from this filter" @@ -1322,6 +1319,9 @@ fn extract_channel_id_from_filter(filter: &Filter) -> Option { values.iter().next()?.parse::().ok() } +/// [`buzz_db::DEFAULT_MAX_PAGE_LIMIT`] as the `u32` that [`buzz_core::filter::clamp_limit`] takes. +const PAGE_LIMIT_MAX: u32 = buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32; + /// Convert a single NIP-01 filter into an [`EventQuery`] for the database. /// /// Each filter is queried independently so that per-filter `limit` and time @@ -1349,10 +1349,11 @@ fn filter_to_query_params( let until = filter .until .and_then(|u| chrono::DateTime::from_timestamp(u.as_secs() as i64, 0)); - let limit = filter - .limit - .map(|l| (l as i64).min(buzz_db::DEFAULT_MAX_PAGE_LIMIT)) - .unwrap_or(buzz_db::DEFAULT_MAX_PAGE_LIMIT); + let limit = i64::from(buzz_core::filter::clamp_limit( + filter.limit, + PAGE_LIMIT_MAX, + PAGE_LIMIT_MAX, + )); // Push author filter into SQL. Single-author uses the indexed `pubkey` column; // multi-author uses the `authors` IN-list pushdown added in the pure-nostr PR. @@ -3253,6 +3254,15 @@ mod tests { .expect("max_limit") as i64 } + #[test] + fn huge_filter_limit_is_clamped_to_page_max() { + let community = buzz_core::tenant::CommunityId::from_uuid(uuid::Uuid::new_v4()); + let q = filter_to_query_params(&Filter::new().limit(usize::MAX), None, community); + assert_eq!(q.limit, Some(buzz_db::DEFAULT_MAX_PAGE_LIMIT)); + let q = filter_to_query_params(&Filter::new().limit(0), None, community); + assert_eq!(q.limit, Some(0)); + } + #[test] fn artifact_d_filter_is_pushed_before_limit() { let community = buzz_core::tenant::CommunityId::from_uuid(uuid::Uuid::new_v4());