From a0987ab65b1056df419b2a1f9f14f88dc904f5f8 Mon Sep 17 00:00:00 2001 From: David Grochowski Date: Fri, 6 Nov 2026 22:32:04 -0500 Subject: [PATCH] fix(relay): keep database error text off WebSocket replies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: two WebSocket paths sent the database driver's error text to the client. HTTP already avoids this (`api::db_read_error` logs only `DbError::kind()`, because DB error text can repeat a bound value). - COUNT: `req::db_read_closed_reason` returned `format!("error: {e}")` for any non-timeout failure, so a failed COUNT read closed with the raw sqlx message, e.g. `error: database error: encountered unexpected or invalid data: `. The sibling `history_read_closed_reason` already returned the fixed `DATABASE_ERROR_CLOSED`. - Channel write gate: `ingest::check_channel_membership` returned `Result<(), String>` and built `"error: database error: {e}"` for a failed lookup. Callers mapped every `Err` to `IngestError::Rejected`, whose text is forwarded verbatim and counted as `invalid`. A channel ephemeral event whose membership lookup failed answered `OK false "error: database error: database error: pool timed out while waiting for an open connection"`. Two callers told the cases apart by `starts_with("error:")`. Change: - One `req::db_read_closed_reason(&DbError) -> &'static str` for REQ history and COUNT: a statement cancel keeps `error: query timed out`, anything else is `error: database error`. The four COUNT failure sites go through `count::close_failed_count`, which logs only the error kind. - `check_channel_membership` returns `ChannelWriteRefusal` (`Denied(&'static str)` | `Db(DbError)`). `From for IngestError` maps a denial to `Rejected` with the same text and a failed lookup to `Internal`. The string sniffing in `validate_edit_ownership` and `report::reporter_can_read` is deleted; both now match on the variant. - `IngestError::db(&DbError)` builds the existing `Internal("error: database error: ")` text. It replaces the one existing copy (the event insert) and is used by the gate, edit ownership and the report handler. `validate_edit_ownership` and `handle_report_event` return `IngestError`, so their own lookup failures (which also became `Rejected("…: ")`) are internal too. Why it is safe: every client-facing refusal text stays byte-identical (`restricted: not a channel member`, `restricted: only members can post in this channel`, the edit/report `invalid:` reasons). Only DB failures change: persistent writes now answer `error: internal server error` (HTTP 500) instead of `invalid`/400 with driver text, ephemeral writes answer `error: database error: `, and COUNT closes with the same fixed reason as REQ. A connection-unavailable COUNT read gets `error: database error`, like a REQ history read; a separate WS "unavailable" reason would have to change REQ too. A non-member whose channel lookup fails is still refused as a non-member, and a token-scoped target denial still hides the target on any refusal. Tests: - `handlers::req::tests::db_read_closed_reason_never_carries_driver_text` failed before (`left: "error: database error: encountered unexpected or invalid data: bound value 'secret'"`), passes after. - `handlers::event::tests::channel_ephemeral_membership_lookup_failure_is_internal` drives `publish_channel_ephemeral` against an unreachable database. Failed before (`Err(Rejected("error: database error: database error: pool timed out while waiting for an open connection"))`), passes after with `Internal("error: database error: pool_timed_out")`. - The Postgres-only bridge COUNT test now pins the exact ordinary-error CLOSED frame instead of accepting any `error: ` prefix. Signed-off-by: David Grochowski --- crates/buzz-relay/src/api/bridge.rs | 11 +-- crates/buzz-relay/src/handlers/count.rs | 53 ++++++--------- crates/buzz-relay/src/handlers/event.rs | 42 +++++++++++- crates/buzz-relay/src/handlers/ingest.rs | 86 ++++++++++++++---------- crates/buzz-relay/src/handlers/report.rs | 32 ++++++--- crates/buzz-relay/src/handlers/req.rs | 32 +++++---- 6 files changed, 159 insertions(+), 97 deletions(-) diff --git a/crates/buzz-relay/src/api/bridge.rs b/crates/buzz-relay/src/api/bridge.rs index 0b6c481a887..a44a6e09e82 100644 --- a/crates/buzz-relay/src/api/bridge.rs +++ b/crates/buzz-relay/src/api/bridge.rs @@ -10561,10 +10561,13 @@ pub(crate) mod postgres_tests { ), "HTTP COUNT {arm} ordinary error" ); - let frame = fx.ws_count(&filter).await; - assert!( - frame.starts_with(r#"["CLOSED","c","error: "#) && !frame.contains("query timed out"), - "WS COUNT {arm} ordinary error: {frame}" + assert_eq!( + fx.ws_count(&filter).await, + format!( + r#"["CLOSED","c","{}"]"#, + crate::handlers::req::DATABASE_ERROR_CLOSED + ), + "WS COUNT {arm} ordinary error" ); fx.drop().await; diff --git a/crates/buzz-relay/src/handlers/count.rs b/crates/buzz-relay/src/handlers/count.rs index 3672ce6b300..8168abce970 100644 --- a/crates/buzz-relay/src/handlers/count.rs +++ b/crates/buzz-relay/src/handlers/count.rs @@ -252,14 +252,7 @@ pub async fn handle_count( { match state.db.count_events_routed("count_req", &query).await { Ok(n) => total += n as u64, - Err(e) => { - conn.send(RelayMessage::closed( - &sub_id, - &super::req::db_read_closed_reason(&e), - )); - access.db_failed(&e); - return; - } + Err(e) => return close_failed_count(&conn, &sub_id, &mut access, &e), } } else { // Fallback: query + post-filter for non-pushable constraints. @@ -291,14 +284,7 @@ pub async fn handle_count( total += 1; } } - Err(e) => { - conn.send(RelayMessage::closed( - &sub_id, - &super::req::db_read_closed_reason(&e), - )); - access.db_failed(&e); - return; - } + Err(e) => return close_failed_count(&conn, &sub_id, &mut access, &e), } } } else { @@ -342,14 +328,7 @@ pub async fn handle_count( query.limit = None; // COUNT doesn't need a row limit match state.db.count_events_routed("count_req", &query).await { Ok(n) => total += n as u64, - Err(e) => { - conn.send(RelayMessage::closed( - &sub_id, - &super::req::db_read_closed_reason(&e), - )); - access.db_failed(&e); - return; - } + Err(e) => return close_failed_count(&conn, &sub_id, &mut access, &e), } } else { // Fallback: query a bounded candidate set + post-filter. @@ -380,14 +359,7 @@ pub async fn handle_count( total += 1; } } - Err(e) => { - conn.send(RelayMessage::closed( - &sub_id, - &super::req::db_read_closed_reason(&e), - )); - access.db_failed(&e); - return; - } + Err(e) => return close_failed_count(&conn, &sub_id, &mut access, &e), } } } @@ -396,6 +368,23 @@ pub async fn handle_count( access.counted(total); } +/// Close a COUNT whose DB read failed. The client gets the fixed +/// [`super::req::db_read_closed_reason`]; the log gets only the error kind, +/// since DB error text can repeat a bound value. +fn close_failed_count( + conn: &ConnectionState, + sub_id: &str, + access: &mut crate::access::ReadLog, + e: &buzz_db::DbError, +) { + warn!(sub_id = %sub_id, error_kind = %e.kind(), "COUNT read failed"); + conn.send(RelayMessage::closed( + sub_id, + super::req::db_read_closed_reason(e), + )); + access.db_failed(e); +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/buzz-relay/src/handlers/event.rs b/crates/buzz-relay/src/handlers/event.rs index 7c93733262b..8bd3b6f1551 100644 --- a/crates/buzz-relay/src/handlers/event.rs +++ b/crates/buzz-relay/src/handlers/event.rs @@ -1213,7 +1213,7 @@ async fn publish_channel_ephemeral( pubkey_bytes: &[u8], ) -> Result<(), IngestError> { // Membership refusals are client-input rejections, and the shared - // gate's message text is surfaced verbatim. + // gate's message text is surfaced verbatim; a failed lookup is internal. super::ingest::check_channel_membership( tenant.community(), state, @@ -1222,8 +1222,7 @@ async fn publish_channel_ephemeral( None, super::ingest::ChannelWrite::Post, ) - .await - .map_err(IngestError::Rejected)?; + .await?; // Mark as local before Redis publish to prevent double-delivery when // the event comes back through the Redis subscriber loop. @@ -1630,6 +1629,43 @@ mod tests { ); } + /// A channel ephemeral event whose membership lookup fails is an internal + /// error naming only the error kind, never a client rejection quoting the + /// database error. The ephemeral arm forwards `Internal` text verbatim, + /// so it must stay fixed. Mutation: map the gate's DB arm to `Rejected` + /// or format the error itself → RED. + #[tokio::test] + async fn channel_ephemeral_membership_lookup_failure_is_internal() { + let state = crate::state::tests::test_state_with_unreachable_database_url( + "postgres://127.0.0.1:1/none", + ) + .await; + let community = buzz_core::CommunityId::from_uuid(Uuid::new_v4()); + let tenant = buzz_core::TenantContext::resolved(community, "ephemeral.test"); + let keys = Keys::generate(); + let channel_id = Uuid::new_v4(); + let event = EventBuilder::new(Kind::Custom(20002), "") + .tags([Tag::parse(["h", &channel_id.to_string()]).unwrap()]) + .sign_with_keys(&keys) + .unwrap(); + + let result = super::publish_channel_ephemeral( + &state, + &tenant, + event, + channel_id, + &keys.public_key().to_bytes(), + ) + .await; + + match result { + Err(super::IngestError::Internal(message)) => { + assert_eq!(message, "error: database error: pool_timed_out") + } + other => panic!("a failed membership lookup must be internal, got {other:?}"), + } + } + #[test] fn agent_presence_request_names_distinct_agents_with_a_known_status() { let owner = Keys::generate(); diff --git a/crates/buzz-relay/src/handlers/ingest.rs b/crates/buzz-relay/src/handlers/ingest.rs index 46a1c81f282..8b6cced44e0 100644 --- a/crates/buzz-relay/src/handlers/ingest.rs +++ b/crates/buzz-relay/src/handlers/ingest.rs @@ -542,6 +542,13 @@ impl IngestError { other => other, } } + + /// The internal error for a failed database call. It names only the + /// error kind: DB error text can repeat a bound value, and the ephemeral + /// WebSocket arm forwards `Internal` text verbatim. + pub(crate) fn db(e: &buzz_db::DbError) -> Self { + Self::Internal(format!("error: database error: {}", e.kind())) + } } /// Map the durable community write-fence lookup onto the ingest error taxonomy. @@ -941,6 +948,26 @@ impl ChannelWrite { const NOT_A_CHANNEL_MEMBER: &str = "restricted: not a channel member"; const MEMBERS_ONLY_POSTING: &str = "restricted: only members can post in this channel"; +/// Why [`check_channel_membership`] refused a write. +#[derive(Debug)] +pub(crate) enum ChannelWriteRefusal { + /// The actor may not write to the channel; the text is client-safe. + Denied(&'static str), + /// A lookup the gate needed failed, so no decision was made. + Db(buzz_db::DbError), +} + +impl From for IngestError { + /// A denial is a client rejection; a failed lookup is internal and + /// never carries the database error text. + fn from(refusal: ChannelWriteRefusal) -> Self { + match refusal { + ChannelWriteRefusal::Denied(reason) => Self::Rejected(reason.to_owned()), + ChannelWriteRefusal::Db(e) => Self::db(&e), + } + } +} + /// The channel write gate: member OR open-visibility channel, plus, for /// [`ChannelWrite::Post`], the role-table write rule ([`may_post`]). /// @@ -948,7 +975,7 @@ const MEMBERS_ONLY_POSTING: &str = "restricted: only members can post in this ch /// one (E1 within-request threading; correctness ruling §4.8). Callers without /// a row pass `None` and the gate reads the DB directly when it needs the row. /// -/// Returns `Ok(())` if allowed, `Err(reason)` if denied. +/// Returns `Ok(())` if allowed, otherwise why the write was refused. pub(crate) async fn check_channel_membership( community_id: CommunityId, state: &AppState, @@ -956,11 +983,11 @@ pub(crate) async fn check_channel_membership( pubkey_bytes: &[u8], channel: Option<&buzz_db::channel::ChannelRecord>, write: ChannelWrite, -) -> Result<(), String> { +) -> Result<(), ChannelWriteRefusal> { let is_member = state .is_member_cached(community_id, ch_id, pubkey_bytes) .await - .map_err(|e| format!("error: database error: {e}"))?; + .map_err(ChannelWriteRefusal::Db)?; if is_member && write == ChannelWrite::Membership { return Ok(()); } @@ -976,19 +1003,19 @@ pub(crate) async fn check_channel_membership( fetched = ch; &fetched } - Err(_) if !is_member => return Err(NOT_A_CHANNEL_MEMBER.to_string()), - Err(e) => return Err(format!("error: database error: {e}")), + Err(_) if !is_member => return Err(ChannelWriteRefusal::Denied(NOT_A_CHANNEL_MEMBER)), + Err(e) => return Err(ChannelWriteRefusal::Db(e)), }, }; if !is_member && channel.visibility != "open" { - return Err(NOT_A_CHANNEL_MEMBER.to_string()); + return Err(ChannelWriteRefusal::Denied(NOT_A_CHANNEL_MEMBER)); } if write == ChannelWrite::Post && !may_post(state, community_id, ch_id, pubkey_bytes, channel, is_member) .await - .map_err(|e| format!("error: database error: {e}"))? + .map_err(ChannelWriteRefusal::Db)? { - return Err(MEMBERS_ONLY_POSTING.to_string()); + return Err(ChannelWriteRefusal::Denied(MEMBERS_ONLY_POSTING)); } Ok(()) } @@ -1051,8 +1078,7 @@ pub(crate) async fn check_channel_write( channel.as_ref(), ChannelWrite::Post, ) - .await - .map_err(IngestError::Rejected)?; + .await?; if channel.is_some_and(|ch| ch.archived_at.is_some()) { return Err(IngestError::Rejected("invalid: channel is archived".into())); } @@ -1459,22 +1485,23 @@ async fn validate_edit_ownership( community_id: CommunityId, event: &Event, state: &AppState, -) -> Result<(), String> { +) -> Result<(), IngestError> { + let invalid = |reason: &str| IngestError::Rejected(format!("invalid: {reason}")); let target_bytes = buzz_core::media::first_edit_target(event) - .ok_or_else(|| "missing e tag for edit target".to_string())?; + .ok_or_else(|| invalid("missing e tag for edit target"))?; // Kind 40003 skips the generic membership gate, so this validator is the // only authority: a target outside the edit's `h` channel, unreadable, or // not the actor's to edit is denied exactly like a missing one. let edit_channel_id = - extract_channel_id(event).ok_or_else(|| "missing h tag for edit".to_string())?; + extract_channel_id(event).ok_or_else(|| invalid("missing h tag for edit"))?; let actor = event.pubkey.to_bytes().to_vec(); let target_event = state .db .get_event_by_id_for_event_write(community_id, &target_bytes) .await - .map_err(|e| format!("db error: {e}"))? + .map_err(|e| IngestError::db(&e))? .filter(|target| target_readable_in_channel(target, edit_channel_id, &actor)) - .ok_or_else(|| EDIT_TARGET_DENIED.to_string())?; + .ok_or_else(|| invalid(EDIT_TARGET_DENIED))?; let author = effective_message_author(&target_event.event, &state.relay_keypair.public_key()); if author == actor { @@ -1483,15 +1510,12 @@ async fn validate_edit_ownership( // A guest may not edit what they wrote before they became a guest. if let Some(ch_id) = target_event.channel_id { // A refused write is denied exactly like a missing target; - // database errors keep their own message. + // a failed lookup stays internal. check_channel_membership(community_id, state, ch_id, &actor, None, ChannelWrite::Post) .await - .map_err(|error| { - if error.starts_with("error:") { - error - } else { - EDIT_TARGET_DENIED.to_string() - } + .map_err(|refusal| match refusal { + ChannelWriteRefusal::Denied(_) => invalid(EDIT_TARGET_DENIED), + ChannelWriteRefusal::Db(e) => IngestError::db(&e), })?; } } else { @@ -1500,9 +1524,9 @@ async fn validate_edit_ownership( .db .is_agent_owner(community_id, &author, &actor) .await - .map_err(|e| format!("db error checking agent ownership: {e}"))?; + .map_err(|e| IngestError::db(&e))?; if !is_owner { - return Err(EDIT_TARGET_DENIED.to_string()); + return Err(invalid(EDIT_TARGET_DENIED)); } } Ok(()) @@ -2763,9 +2787,7 @@ async fn ingest_event_inner( // report; that is tolerated because reports are non-actioning signals and // remain visible only to moderators. if kind_u32 == KIND_REPORT { - super::report::handle_report_event(tenant, &event, state, auth.channel_ids()) - .await - .map_err(IngestError::Rejected)?; + super::report::handle_report_event(tenant, &event, state, auth.channel_ids()).await?; return Ok(IngestResult { event_id: event_id_hex, accepted: true, @@ -2975,7 +2997,7 @@ async fn ingest_event_inner( ); auth_result.map_err(|e| match target_denial { Some(reason) => target_denied(reason), - None => IngestError::Rejected(e), + None => e.into(), })?; } } @@ -3180,9 +3202,7 @@ async fn ingest_event_inner( } if kind_u32 == KIND_STREAM_MESSAGE_EDIT { - validate_edit_ownership(tenant.community(), &event, state) - .await - .map_err(|e| IngestError::Rejected(format!("invalid: {e}")))?; + validate_edit_ownership(tenant.community(), &event, state).await?; } if kind_u32 == KIND_FORUM_VOTE { @@ -3767,9 +3787,7 @@ async fn ingest_event_inner( buzz_db::DbError::AuthEventRejected => { IngestError::Rejected("invalid: AUTH events cannot be stored".into()) } - other => { - IngestError::Internal(format!("error: database error: {}", other.kind())) - } + other => IngestError::db(&other), }); } } diff --git a/crates/buzz-relay/src/handlers/report.rs b/crates/buzz-relay/src/handlers/report.rs index 50217f21fc1..1f0614c366f 100644 --- a/crates/buzz-relay/src/handlers/report.rs +++ b/crates/buzz-relay/src/handlers/report.rs @@ -25,6 +25,7 @@ use buzz_db::moderation::{NewReport, ReportTarget}; use nostr::Event; use uuid::Uuid; +use super::ingest::{ChannelWriteRefusal, IngestError}; use crate::state::AppState; /// NIP-56 report types accepted at ingest. @@ -40,7 +41,8 @@ pub const REPORT_TYPES: &[&str] = &[ /// Validate a kind:1984 report and persist it to `moderation_reports`. /// -/// Rejections use client-safe `invalid:`/`restricted:` reasons. On success +/// Rejections use client-safe `invalid:`/`restricted:` reasons; a failed +/// lookup is an internal error naming only the error kind. On success /// the report is queued (idempotently, keyed by the signed event id) and the /// event itself is **not** stored or broadcast as a regular event. /// @@ -50,8 +52,8 @@ pub async fn handle_report_event( event: &Event, state: &Arc, token_channels: Option<&[Uuid]>, -) -> Result<(), String> { - let parsed = parse_report(event)?; +) -> Result<(), IngestError> { + let parsed = parse_report(event).map_err(IngestError::Rejected)?; let reporter_pubkey = event.pubkey.to_bytes(); let (target, channel_id) = match parsed.target { @@ -60,7 +62,7 @@ pub async fn handle_report_event( .db .get_event_by_id(tenant.community(), &event_id) .await - .map_err(|e| format!("error: database error resolving report target: {e}"))?; + .map_err(|e| IngestError::db(&e))?; // A target the reporter cannot read is rejected exactly like a // missing one, so the response never reveals whether it exists. let readable = match &stored { @@ -72,7 +74,11 @@ pub async fn handle_report_event( }; match stored { Some(stored) if readable => (ReportTarget::Event(event_id), stored.channel_id), - _ => return Err("invalid: report target event not found".to_string()), + _ => { + return Err(IngestError::Rejected( + "invalid: report target event not found".to_string(), + )) + } } } ParsedReportTarget::Blob { sha256, .. } => { @@ -84,7 +90,9 @@ pub async fn handle_report_event( .media_storage .get_sidecar(tenant, &sha_hex) .await - .map_err(|_| "invalid: report target blob not found".to_string())?; + .map_err(|_| { + IngestError::Rejected("invalid: report target blob not found".to_string()) + })?; if state.config.media_read_mode == crate::api::media_access::MediaReadMode::Enforce { let base = crate::api::media::media_base_url_for_tenant( &state.config.relay_url, @@ -107,7 +115,9 @@ pub async fn handle_report_event( Ok(buzz_db::media::MediaAccess::Allowed | buzz_db::media::MediaAccess::UploaderPreview) ) { - return Err("invalid: report target blob not found".to_owned()); + return Err(IngestError::Rejected( + "invalid: report target blob not found".to_owned(), + )); } } (ReportTarget::Blob(sha256), None) @@ -129,7 +139,7 @@ pub async fn handle_report_event( }, ) .await - .map_err(|e| format!("error: database error inserting report: {e}"))?; + .map_err(|e| IngestError::db(&e))?; Ok(()) } @@ -143,7 +153,7 @@ async fn reporter_can_read( target: &StoredEvent, reporter: &[u8], token_channels: Option<&[Uuid]>, -) -> Result { +) -> Result { if !super::req::event_visible_to_reader(&target.event, reporter) { return Ok(false); } @@ -166,8 +176,8 @@ async fn reporter_can_read( .await { Ok(()) => Ok(true), - Err(reason) if reason.starts_with("error:") => Err(reason), - Err(_) => Ok(false), + Err(ChannelWriteRefusal::Denied(_)) => Ok(false), + Err(ChannelWriteRefusal::Db(e)) => Err(IngestError::db(&e)), } } diff --git a/crates/buzz-relay/src/handlers/req.rs b/crates/buzz-relay/src/handlers/req.rs index b5e9347941b..ec02fd173d7 100644 --- a/crates/buzz-relay/src/handlers/req.rs +++ b/crates/buzz-relay/src/handlers/req.rs @@ -558,7 +558,7 @@ pub async fn handle_req( }); // CLOSED, never EOSE: EOSE tells the client it has every // stored event, so a resuming client would skip this history. - return Some(history_read_closed_reason(&e)); + return Some(db_read_closed_reason(&e)); } }; @@ -1752,8 +1752,10 @@ pub(crate) const QUERY_TIMED_OUT_CLOSED: &str = "error: query timed out"; /// error stays in the relay log, and clients treat it as retryable. pub(crate) const DATABASE_ERROR_CLOSED: &str = "error: database error"; -/// CLOSED reason for a failed REQ history read. -fn history_read_closed_reason(e: &buzz_db::DbError) -> &'static str { +/// CLOSED reason for a failed DB read (REQ history, COUNT): a statement +/// cancel gets the stable timeout reason; anything else the fixed +/// [`DATABASE_ERROR_CLOSED`], never the error text. +pub(crate) fn db_read_closed_reason(e: &buzz_db::DbError) -> &'static str { if e.is_statement_cancelled() { QUERY_TIMED_OUT_CLOSED } else { @@ -1776,16 +1778,6 @@ fn search_closed_reason(e: &buzz_search::SearchError) -> &'static str { } } -/// CLOSED reason for a failed one-shot DB read (COUNT): a statement cancel gets -/// the stable timeout reason; anything else keeps the raw error. -pub(crate) fn db_read_closed_reason(e: &buzz_db::DbError) -> String { - if e.is_statement_cancelled() { - QUERY_TIMED_OUT_CLOSED.to_string() - } else { - format!("error: {e}") - } -} - fn extract_channel_id_from_filters(filters: &[Filter]) -> Option { let mut found_id: Option = None; for f in filters { @@ -2058,6 +2050,20 @@ mod tests { use super::*; use nostr::{Alphabet, Filter, SingleLetterTag}; + /// A failed COUNT read closes with the same fixed reason as a failed + /// history read: the driver message, which can quote a bound value, + /// stays in the relay log. Mutation: return the error text → RED. + #[test] + fn db_read_closed_reason_never_carries_driver_text() { + for e in [ + buzz_db::DbError::Sqlx(sqlx::Error::Protocol("bound value 'secret'".into())), + buzz_db::DbError::Sqlx(sqlx::Error::PoolTimedOut), + buzz_db::DbError::InvalidData("bound value 'secret'".into()), + ] { + assert_eq!(db_read_closed_reason(&e), DATABASE_ERROR_CLOSED, "{e:?}"); + } + } + fn lifecycle_conn() -> ( ConnectionState, tokio::sync::mpsc::Receiver,