mirror of
https://github.com/block/buzz.git
synced 2026-11-03 07:51:22 +00:00
fix(search): harden FTS query inputs
Co-authored-by: npub1t2tgm7d8f995uqvmnm8h88sg3wnpp9a5xysjf6dg3tjmgt3ltulqdp8ehr <5a968df9a7494b4e019b9ecf739e088ba61097b4312124e9a88ae5b42e3f5f3e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: npub1t2tgm7d8f995uqvmnm8h88sg3wnpp9a5xysjf6dg3tjmgt3ltulqdp8ehr <5a968df9a7494b4e019b9ecf739e088ba61097b4312124e9a88ae5b42e3f5f3e@sprout-oss.stage.blox.sqprod.co>
This commit is contained in:
parent
d5fe805860
commit
4c51a8119f
@@ -402,6 +402,12 @@ pub async fn query_events(
|
||||
.map_err(|e| internal_error(&format!("channel access lookup: {e}")))?;
|
||||
|
||||
if filters.iter().any(|f| f.search.is_some()) {
|
||||
if has_mixed_search_filters(&filters) {
|
||||
return Err(api_error(
|
||||
StatusCode::BAD_REQUEST,
|
||||
"mixed search and non-search filters not supported",
|
||||
));
|
||||
}
|
||||
return handle_bridge_search(
|
||||
&state,
|
||||
&filters,
|
||||
@@ -800,6 +806,10 @@ pub async fn count_events(
|
||||
Ok(Json(serde_json::json!({ "count": total })))
|
||||
}
|
||||
|
||||
fn has_mixed_search_filters(filters: &[nostr::Filter]) -> bool {
|
||||
filters.iter().any(|f| f.search.is_some()) && filters.iter().any(|f| f.search.is_none())
|
||||
}
|
||||
|
||||
/// Decide whether a search hit should be returned to the caller.
|
||||
///
|
||||
/// Mirrors the WS NIP-50 path's post-filter step in `handlers/req.rs`:
|
||||
@@ -1228,6 +1238,36 @@ mod tests {
|
||||
.to_bytes()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bridge_detects_mixed_search_and_non_search_filters() {
|
||||
let filters = vec![
|
||||
nostr::Filter::new().search("hello"),
|
||||
nostr::Filter::new().kind(Kind::TextNote),
|
||||
];
|
||||
|
||||
assert!(has_mixed_search_filters(&filters));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bridge_accepts_all_search_filters() {
|
||||
let filters = vec![
|
||||
nostr::Filter::new().search("hello"),
|
||||
nostr::Filter::new().search("world"),
|
||||
];
|
||||
|
||||
assert!(!has_mixed_search_filters(&filters));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bridge_accepts_all_non_search_filters() {
|
||||
let filters = vec![
|
||||
nostr::Filter::new().kind(Kind::TextNote),
|
||||
nostr::Filter::new().kind(Kind::Metadata),
|
||||
];
|
||||
|
||||
assert!(!has_mixed_search_filters(&filters));
|
||||
}
|
||||
|
||||
/// Attack 3 proof: two stateless relay pods sharing Redis must share one
|
||||
/// community-scoped NIP-98 seen-set. Pod A's first claim succeeds; pod B's
|
||||
/// replay of the same event id in the same community is rejected. The same
|
||||
|
||||
@@ -113,6 +113,33 @@ pub struct SearchResult {
|
||||
|
||||
const PER_PAGE_MAX: u32 = 500;
|
||||
const PER_PAGE_DEFAULT: u32 = 100;
|
||||
/// Hard cap on search text handed to `websearch_to_tsquery`. This keeps a
|
||||
/// single request from spending unbounded parser CPU/memory while still allowing
|
||||
/// far longer queries than the desktop UI normally emits.
|
||||
const SEARCH_TEXT_MAX_CHARS: usize = 4096;
|
||||
/// Search pages are currently server-generated (WS uses 1..=MAX_SEARCH_PAGES,
|
||||
/// bridge uses page 1), but clamp here too so a future caller cannot accidentally
|
||||
/// wire untrusted input into a multi-trillion-row OFFSET.
|
||||
const PAGE_MAX: u32 = 1000;
|
||||
|
||||
fn normalized_search_text(q: &str) -> Option<String> {
|
||||
let trimmed = q.trim();
|
||||
if trimmed.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let mut cleaned = String::with_capacity(trimmed.len().min(SEARCH_TEXT_MAX_CHARS));
|
||||
for ch in trimmed.chars().take(SEARCH_TEXT_MAX_CHARS) {
|
||||
cleaned.push(if ch == '\0' { ' ' } else { ch });
|
||||
}
|
||||
|
||||
let cleaned = cleaned.trim();
|
||||
if cleaned.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(cleaned.to_string())
|
||||
}
|
||||
}
|
||||
|
||||
/// Execute a community-scoped FTS query.
|
||||
///
|
||||
@@ -133,13 +160,12 @@ const PER_PAGE_DEFAULT: u32 = 100;
|
||||
/// `community_id = $ctx` is the first predicate and is non-negotiable. There
|
||||
/// is no code path through this function that omits it.
|
||||
pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result<SearchResult, SearchError> {
|
||||
let trimmed = query.q.trim();
|
||||
if trimmed.is_empty() {
|
||||
let Some(search_text) = normalized_search_text(&query.q) else {
|
||||
return Ok(SearchResult {
|
||||
hits: Vec::new(),
|
||||
page: query.page.max(1),
|
||||
page: query.page.max(1).min(PAGE_MAX),
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
let per_page = query.per_page.clamp(1, PER_PAGE_MAX);
|
||||
let per_page_actual = if query.per_page == 0 {
|
||||
@@ -147,7 +173,7 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result<SearchResult,
|
||||
} else {
|
||||
per_page
|
||||
};
|
||||
let page = query.page.max(1);
|
||||
let page = query.page.max(1).min(PAGE_MAX);
|
||||
let offset = ((page - 1) as i64) * (per_page_actual as i64);
|
||||
|
||||
let mut qb: QueryBuilder<sqlx::Postgres> = QueryBuilder::new(
|
||||
@@ -156,7 +182,7 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result<SearchResult,
|
||||
ts_rank_cd(search_tsv, query) AS rank \
|
||||
FROM events, websearch_to_tsquery('simple', ",
|
||||
);
|
||||
qb.push_bind(trimmed);
|
||||
qb.push_bind(&search_text);
|
||||
qb.push(") AS query WHERE community_id = ");
|
||||
qb.push_bind(*query.community.as_uuid());
|
||||
qb.push(" AND deleted_at IS NULL AND search_tsv @@ query");
|
||||
@@ -241,3 +267,32 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result<SearchResult,
|
||||
|
||||
Ok(SearchResult { hits, page })
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn normalized_search_text_trims_and_rejects_empty() {
|
||||
assert_eq!(
|
||||
normalized_search_text(" hello ").as_deref(),
|
||||
Some("hello")
|
||||
);
|
||||
assert!(normalized_search_text(" ").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalized_search_text_replaces_nul_bytes() {
|
||||
assert_eq!(
|
||||
normalized_search_text("foo\0bar").as_deref(),
|
||||
Some("foo bar")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalized_search_text_caps_length() {
|
||||
let long = "x".repeat(SEARCH_TEXT_MAX_CHARS + 10);
|
||||
let cleaned = normalized_search_text(&long).expect("non-empty");
|
||||
assert_eq!(cleaned.chars().count(), SEARCH_TEXT_MAX_CHARS);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -676,6 +676,119 @@ async fn channel_less_only_excludes_per_channel_events() {
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
/// Search-input hardening: NUL bytes are not valid Postgres text-search input.
|
||||
/// Sanitize before calling `websearch_to_tsquery` so the bridge does not turn
|
||||
/// adversarial search text into HTTP 500s.
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn nul_bytes_in_query_are_sanitized() {
|
||||
let (pool, schema) = setup().await;
|
||||
|
||||
let c = mk_community(&pool, "nul.example").await;
|
||||
let evt_id = rand_bytes32();
|
||||
insert_event(
|
||||
&pool,
|
||||
c,
|
||||
evt_id,
|
||||
rand_bytes32(),
|
||||
9,
|
||||
"foo bar search text",
|
||||
None,
|
||||
1_700_000_000,
|
||||
)
|
||||
.await;
|
||||
|
||||
let svc = SearchService::new(pool.clone());
|
||||
let result = svc
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "foo\0bar".into(),
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
until: None,
|
||||
page: 1,
|
||||
per_page: 10,
|
||||
})
|
||||
.await
|
||||
.expect("NUL-containing search query should not bubble a Postgres error");
|
||||
|
||||
assert_eq!(result.hits.len(), 1);
|
||||
assert_eq!(result.hits[0].event_id, evt_id);
|
||||
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn enormous_page_number_is_clamped() {
|
||||
let (pool, schema) = setup().await;
|
||||
|
||||
let c = mk_community(&pool, "page-clamp.example").await;
|
||||
for i in 0..5 {
|
||||
insert_event(
|
||||
&pool,
|
||||
c,
|
||||
rand_bytes32(),
|
||||
rand_bytes32(),
|
||||
9,
|
||||
"clamp-token",
|
||||
None,
|
||||
1_700_000_000 + i,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
let svc = SearchService::new(pool.clone());
|
||||
let result = svc
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "clamp-token".into(),
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
until: None,
|
||||
page: u32::MAX,
|
||||
per_page: 10,
|
||||
})
|
||||
.await
|
||||
.expect("huge page number should be bounded, not error");
|
||||
|
||||
assert_eq!(result.page, 1000);
|
||||
assert!(result.hits.is_empty());
|
||||
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn very_long_query_is_bounded_before_pg_parse() {
|
||||
let (pool, schema) = setup().await;
|
||||
|
||||
let c = mk_community(&pool, "long-query.example").await;
|
||||
let svc = SearchService::new(pool.clone());
|
||||
let result = svc
|
||||
.search(&SearchQuery {
|
||||
community: c,
|
||||
q: "x".repeat(10_000),
|
||||
channel_scope: ChannelScope::Any,
|
||||
kinds: None,
|
||||
authors: None,
|
||||
since: None,
|
||||
until: None,
|
||||
page: 1,
|
||||
per_page: 10,
|
||||
})
|
||||
.await
|
||||
.expect("long search query should be capped before Postgres parses it");
|
||||
|
||||
assert!(result.hits.is_empty());
|
||||
|
||||
teardown(pool, &schema).await;
|
||||
}
|
||||
|
||||
/// Privacy regression gate: the storage layer MUST NOT make these kinds
|
||||
/// searchable. The migration's `search_tsv` generated column emits NULL
|
||||
/// for excluded kinds, so a `search_tsv @@ query` probe never matches.
|
||||
|
||||
Reference in New Issue
Block a user