mirror of
https://github.com/block/buzz.git
synced 2026-11-13 21:01:53 +00:00
fix(relay): saturate filter limits instead of casting (#9230)
## 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<i64>)` 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 <dgrochowski@squareup.com>
This commit is contained in:
@@ -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<usize>, 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<usize>, 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")
|
||||
|
||||
@@ -712,7 +712,7 @@ fn extract_channel_from_filter(filter: &nostr::Filter) -> Option<uuid::Uuid> {
|
||||
// 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<Vec<u32>> = 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;
|
||||
}
|
||||
|
||||
@@ -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<uuid::Uuid> {
|
||||
values.iter().next()?.parse::<uuid::Uuid>().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());
|
||||
|
||||
Reference in New Issue
Block a user