diff --git a/nostr-sdk/src/local_relay/local/inner.rs b/nostr-sdk/src/local_relay/local/inner.rs index 83371642a..cdce9b058 100644 --- a/nostr-sdk/src/local_relay/local/inner.rs +++ b/nostr-sdk/src/local_relay/local/inner.rs @@ -916,7 +916,6 @@ impl InnerLocalRelay { subscription_id, filter, initial_message, - .. } => { if let RateLimiterResponse::Limited = session.check_query_rate_limit(self.queries_per_minute) diff --git a/nostr-sdk/src/relay/api/sync.rs b/nostr-sdk/src/relay/api/sync.rs index 8a079d200..968b087f3 100644 --- a/nostr-sdk/src/relay/api/sync.rs +++ b/nostr-sdk/src/relay/api/sync.rs @@ -331,7 +331,6 @@ pub(super) async fn sync( let open_msg: ClientMessage = ClientMessage::NegOpen { subscription_id: Cow::Borrowed(&sub_id), filter: Cow::Borrowed(filter), - id_size: None, initial_message: Cow::Owned(faster_hex::hex_string(&initial_message)), }; relay.send_msg(open_msg).await?; @@ -565,9 +564,7 @@ async fn check_negentropy_support( } RelayMessage::Notice(message) => { if message == "ERROR: negentropy error: negentropy query missing elements" { - // The NEG-OPEN message is sent with 4 elements instead of 5 - // If the relay return this error means that is not support new - // negentropy protocol + // The relay expects the deprecated five-element NEG-OPEN format. return Err(negentropy::Error::UnsupportedProtocolVersion.into()); } else if message.contains("bad msg") && (message.contains("unknown cmd") diff --git a/nostr/CHANGELOG.md b/nostr/CHANGELOG.md index e7b500f9e..2b3546acb 100644 --- a/nostr/CHANGELOG.md +++ b/nostr/CHANGELOG.md @@ -47,6 +47,7 @@ - Remove `EventBuilder::sign_with_keys` and `EventBuilder::sign_with_ctx` (https://github.com/nostrdevkit/nostr/pull/1355) - Remove `UnsignedEvent::sign_with_keys`, `UnsignedEvent::sign_with_ctx` and `UnsignedEvent::sign_with_aux_rand` (https://github.com/nostrdevkit/nostr/pull/1355) - Remove `JsonUtil` trait (https://github.com/nostrdevkit/nostr/pull/1365) +- Remove the deprecated `id_size` field from `ClientMessage::NegOpen` https://github.com/nostrdevkit/nostr/pull/1426 ### Changed @@ -117,7 +118,7 @@ - Optimize event serialization by ~73% (https://github.com/nostrdevkit/nostr/pull/1319) - Hardware-accelerate SHA-256 where available (https://github.com/nostrdevkit/nostr/pull/1419) - Reduce allocations and redundant key parsing in the NIP-44 v2 path (https://github.com/nostrdevkit/nostr/pull/1421) -- Serialize and deserialize `RelayMessage` without a `serde_json::Value` tree (https://github.com/nostrdevkit/nostr/pull/1425) +- Serialize and deserialize `RelayMessage` and `ClientMessage` without a `serde_json::Value` tree (https://github.com/nostrdevkit/nostr/pull/1425 and https://github.com/nostrdevkit/nostr/pull/1426) - Avoid allocating a `String` per generic tag key when serializing and deserializing `Filter` (https://github.com/nostrdevkit/nostr/pull/1427) ### Security diff --git a/nostr/src/message/client.rs b/nostr/src/message/client.rs index 4b5b2dca2..2765b8074 100644 --- a/nostr/src/message/client.rs +++ b/nostr/src/message/client.rs @@ -6,17 +6,18 @@ //! Client messages use alloc::borrow::Cow; -use alloc::string::{String, ToString}; +use alloc::string::String; use alloc::vec::Vec; +use core::fmt; +use serde::de::{self, SeqAccess, Visitor}; +use serde::ser::SerializeSeq; use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use serde_json::{Value, json}; -use super::{SubscriptionId, invalid_message_format}; -use crate::error::Error; +use super::SubscriptionId; use crate::event::Event; use crate::filter::Filter; -use crate::util::{impl_json_methods, parse_json, parse_json_from_value}; +use crate::util::impl_json_methods; /// Messages sent by clients, received by relays #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -49,8 +50,6 @@ pub enum ClientMessage<'a> { subscription_id: Cow<'a, SubscriptionId>, /// Filter filter: Cow<'a, Filter>, - /// ID size (deprecated) - id_size: Option, /// Initial message (hex) initial_message: Cow<'a, str>, }, @@ -117,7 +116,6 @@ impl ClientMessage<'_> { Self::NegOpen { subscription_id: Cow::Owned(subscription_id), filter: Cow::Owned(filter), - id_size: None, initial_message: Cow::Owned(initial_message), } } @@ -146,231 +144,200 @@ impl ClientMessage<'_> { matches!(self, ClientMessage::Auth(_)) } - /// Serialize as [`Value`] - pub fn as_value(&self) -> Value { + /// Number of elements in the JSON array form. + /// + /// Matched exhaustively on purpose: a new variant must state its own + /// length rather than inherit a default that may not fit. + fn len(&self) -> usize { + match self { + Self::Event(..) | Self::Close(..) | Self::Auth(..) | Self::NegClose { .. } => 2, + Self::Count { .. } | Self::NegMsg { .. } => 3, + Self::Req { filters, .. } => 2 + filters.len(), + Self::NegOpen { .. } => 4, + } + } +} + +impl Serialize for ClientMessage<'_> { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + // Write the array elements straight to the serializer. Building a + // `serde_json::Value` first would allocate for every element, and for + // an `EVENT` message that means the whole event too. + let mut seq = serializer.serialize_seq(Some(self.len()))?; + match self { - Self::Event(event) => json!(["EVENT", event]), + Self::Event(event) => { + seq.serialize_element("EVENT")?; + seq.serialize_element(event)?; + } Self::Req { subscription_id, filters, } => { - let mut json = json!(["REQ", subscription_id]); - let mut filters = json!(filters); - - if let Some(json) = json.as_array_mut() { - if let Some(filters) = filters.as_array_mut() { - json.append(filters); - } + seq.serialize_element("REQ")?; + seq.serialize_element(subscription_id)?; + for filter in filters { + seq.serialize_element(filter)?; } - - json } Self::Count { subscription_id, filter, } => { - json!(["COUNT", subscription_id, filter]) + seq.serialize_element("COUNT")?; + seq.serialize_element(subscription_id)?; + seq.serialize_element(filter)?; + } + Self::Close(subscription_id) => { + seq.serialize_element("CLOSE")?; + seq.serialize_element(subscription_id)?; + } + Self::Auth(event) => { + seq.serialize_element("AUTH")?; + seq.serialize_element(event)?; } - Self::Close(subscription_id) => json!(["CLOSE", subscription_id]), - Self::Auth(event) => json!(["AUTH", event]), Self::NegOpen { subscription_id, filter, - id_size, initial_message, - } => match id_size { - Some(id_size) => json!([ - "NEG-OPEN", - subscription_id, - filter, - id_size, - initial_message - ]), - None => json!(["NEG-OPEN", subscription_id, filter, initial_message]), - }, + } => { + seq.serialize_element("NEG-OPEN")?; + seq.serialize_element(subscription_id)?; + seq.serialize_element(filter)?; + seq.serialize_element(initial_message)?; + } Self::NegMsg { subscription_id, message, - } => json!(["NEG-MSG", subscription_id, message]), - Self::NegClose { subscription_id } => json!(["NEG-CLOSE", subscription_id]), - } - } - - /// Deserialize from [`Value`] - /// - /// **This method NOT verify the event signature!** - pub fn from_value(msg: Value) -> Result { - let v = msg.as_array().ok_or(invalid_message_format())?; - - if v.is_empty() { - return Err(invalid_message_format()); - } - - let v_len: usize = v.len(); - - // ["EVENT", ] - if v[0] == "EVENT" { - if v_len >= 2 { - let event: Event = parse_json_from_value(v[1].clone())?; - return Ok(Self::event(event)); - } else { - return Err(invalid_message_format()); + } => { + seq.serialize_element("NEG-MSG")?; + seq.serialize_element(subscription_id)?; + seq.serialize_element(message)?; } - } - - // ["REQ", , ] - if v[0] == "REQ" { - if v_len >= 3 { - // Deprecated REQ - // ["REQ", , , , ...] - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - let filters: Vec> = - parse_json_from_value(Value::Array(v[2..].to_vec()))?; - return Ok(Self::Req { - subscription_id: Cow::Owned(subscription_id), - filters, - }); - } else { - return Err(invalid_message_format()); + Self::NegClose { subscription_id } => { + seq.serialize_element("NEG-CLOSE")?; + seq.serialize_element(subscription_id)?; } } - // ["COUNT", , ] - if v[0] == "COUNT" { - if v_len >= 3 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - let filter: Filter = parse_json_from_value(v[2].clone())?; - return Ok(Self::count(subscription_id, filter)); - } else { - return Err(invalid_message_format()); - } - } + seq.end() + } +} - // Close - // ["CLOSE", ] - if v[0] == "CLOSE" { - if v_len >= 2 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - return Ok(Self::close(subscription_id)); - } else { - return Err(invalid_message_format()); - } - } +impl<'de> Deserialize<'de> for ClientMessage<'_> { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + deserializer.deserialize_seq(ClientMessageVisitor) + } +} - // Auth - // ["AUTH", ] - if v[0] == "AUTH" { - if v_len >= 2 { - let event: Event = parse_json_from_value(v[1].clone())?; - return Ok(Self::auth(event)); - } else { - return Err(invalid_message_format()); - } - } +struct ClientMessageVisitor; - // Negentropy Open - // New: ["NEG-OPEN", , , ] - // Old: ["NEG-OPEN", , , , ] - if v[0] == "NEG-OPEN" { - // New negentropy protocol message - if v_len == 4 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - let filter: Filter = Filter::from_json(v[2].to_string())?; - let initial_message: String = parse_json_from_value(v[3].clone())?; - return Ok(Self::neg_open(subscription_id, filter, initial_message)); - } +impl<'de> Visitor<'de> for ClientMessageVisitor { + type Value = ClientMessage<'static>; - // Old negentropy protocol message - if v_len == 5 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - let filter: Filter = Filter::from_json(v[2].to_string())?; - let id_size: u8 = v[3].as_u64().ok_or(invalid_message_format())? as u8; - let initial_message: String = parse_json_from_value(v[4].clone())?; - return Ok(Self::NegOpen { - subscription_id: Cow::Owned(subscription_id), - filter: Cow::Owned(filter), - id_size: Some(id_size), - initial_message: Cow::Owned(initial_message), - }); - } + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a client message array") + } - return Err(invalid_message_format()); + fn visit_seq(self, mut seq: A) -> Result + where + A: SeqAccess<'de>, + { + fn malformed() -> E + where + E: de::Error, + { + E::custom("invalid message format") + } + + macro_rules! next { + () => { + seq.next_element()?.ok_or_else(malformed)? + }; } - // Negentropy Message - // ["NEG-MSG", , ] - if v[0] == "NEG-MSG" { - if v_len >= 3 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - let message: String = parse_json_from_value(v[2].clone())?; - return Ok(Self::NegMsg { + let message_type: String = next!(); + + let message: ClientMessage<'static> = match message_type.as_str() { + // ["EVENT", ] + "EVENT" => ClientMessage::Event(Cow::Owned(next!())), + // ["REQ", , , , ...] + "REQ" => { + let subscription_id: SubscriptionId = next!(); + let mut filters: Vec> = Vec::new(); + while let Some(filter) = seq.next_element::()? { + filters.push(Cow::Owned(filter)); + } + if filters.is_empty() { + return Err(malformed()); + } + return Ok(ClientMessage::Req { subscription_id: Cow::Owned(subscription_id), - message: Cow::Owned(message), + filters, }); - } else { - return Err(invalid_message_format()); } - } + // ["COUNT", , ] + "COUNT" => ClientMessage::Count { + subscription_id: Cow::Owned(next!()), + filter: Cow::Owned(next!()), + }, + // ["CLOSE", ] + "CLOSE" => ClientMessage::Close(Cow::Owned(next!())), + // ["AUTH", ] + "AUTH" => ClientMessage::Auth(Cow::Owned(next!())), + // ["NEG-OPEN", , , ] + "NEG-OPEN" => { + let subscription_id: SubscriptionId = next!(); + let filter: Filter = next!(); + let initial_message: String = next!(); + + if seq.next_element::()?.is_some() { + return Err(malformed()); + } - // Negentropy Close - // ["NEG-CLOSE", ] - if v[0] == "NEG-CLOSE" { - if v_len >= 2 { - let subscription_id: SubscriptionId = parse_json_from_value(v[1].clone())?; - return Ok(Self::NegClose { + return Ok(ClientMessage::NegOpen { subscription_id: Cow::Owned(subscription_id), + filter: Cow::Owned(filter), + initial_message: Cow::Owned(initial_message), }); - } else { - return Err(invalid_message_format()); } - } + // ["NEG-MSG", , ] + "NEG-MSG" => ClientMessage::NegMsg { + subscription_id: Cow::Owned(next!()), + message: Cow::Owned(next!()), + }, + // ["NEG-CLOSE", ] + "NEG-CLOSE" => ClientMessage::NegClose { + subscription_id: Cow::Owned(next!()), + }, + _ => return Err(malformed()), + }; - Err(invalid_message_format()) - } -} + while seq.next_element::()?.is_some() {} -impl Serialize for ClientMessage<'_> { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - let json_value: Value = self.as_value(); - json_value.serialize(serializer) + Ok(message) } } -impl<'de> Deserialize<'de> for ClientMessage<'_> { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let json_value = Value::deserialize(deserializer)?; - ClientMessage::from_value(json_value).map_err(serde::de::Error::custom) - } -} - -impl_json_methods! { - ClientMessage<'_>, - from_json(json) { - let msg: &[u8] = json.as_ref(); - - if msg.is_empty() { - return Err(invalid_message_format()); - } - - let value: Value = parse_json(&msg)?; - Self::from_value(value) - } -} +impl_json_methods!(ClientMessage<'_>); #[cfg(test)] mod tests { use core::str::FromStr; use super::*; + use crate::error::ErrorKind; use crate::event::Kind; use crate::key::PublicKey; + const EVENT_JSON: &str = r#"{"id":"70b10f70c1318967eddf12527799411b1a9780ad9c43858f5e5fcd45486a13a5","pubkey":"379e863e8357163b5bce5d2688dc4f1dcc2d505222fb8d74db600f30535dfdfe","created_at":1612809991,"kind":1,"tags":[],"content":"test","sig":"273a9cd5d11455590f4359500bccb7a89428262b96b3ea87a756b770964472f8c3e87f5d5e64d8d2e859a71462a3f477b554565c4f2f326cb01dd7620db71502"}"#; + #[test] fn test_client_message_req() { let pk = @@ -392,4 +359,93 @@ mod tests { ); assert_eq!(client_req.as_json(), r##"["REQ","test",{"kinds":[22]}]"##); } + + /// Trailing elements are reserved for future extensions, so fixed-length + /// variants other than `NEG-OPEN` must continue to ignore them. + #[test] + fn parse_trailing_elements() { + let cases: [(&str, ClientMessage); 3] = [ + ( + r#"["COUNT","sub",{"kinds":[1]},"extra"]"#, + ClientMessage::count( + SubscriptionId::new("sub"), + Filter::new().kind(Kind::TextNote), + ), + ), + ( + r#"["CLOSE","sub",{"extra":true}]"#, + ClientMessage::close(SubscriptionId::new("sub")), + ), + ( + r#"["NEG-MSG","sub","deadbeef",1,2]"#, + ClientMessage::NegMsg { + subscription_id: Cow::Owned(SubscriptionId::new("sub")), + message: Cow::Borrowed("deadbeef"), + }, + ), + ]; + + for (json, expected) in cases { + assert_eq!(ClientMessage::from_json(json).unwrap(), expected, "{json}"); + } + } + + #[test] + fn round_trip_every_variant() { + let event: Event = Event::from_json(EVENT_JSON).unwrap(); + let sub = || SubscriptionId::new("sub"); + let filter = || Filter::new().kind(Kind::TextNote); + + let messages: [ClientMessage; 8] = [ + ClientMessage::event(event.clone()), + ClientMessage::req(sub(), vec![filter(), Filter::new().author(event.pubkey)]), + ClientMessage::count(sub(), filter()), + ClientMessage::close(sub()), + ClientMessage::auth(event), + ClientMessage::neg_open(sub(), filter(), String::from("deadbeef")), + ClientMessage::NegMsg { + subscription_id: Cow::Owned(sub()), + message: Cow::Borrowed("deadbeef"), + }, + ClientMessage::NegClose { + subscription_id: Cow::Owned(sub()), + }, + ]; + + for message in messages { + let json: String = message.as_json(); + assert_eq!(ClientMessage::from_json(&json).unwrap(), message, "{json}"); + } + } + + #[test] + fn parse_rejects_unknown_type_and_non_array() { + for json in [ + r#"["NOT-A-REAL-TYPE","x"]"#, + r#"{"type":"EVENT"}"#, + r#""EVENT""#, + r#"[]"#, + r#"["REQ","sub"]"#, + r#"["NEG-OPEN","sub",{},16,"deadbeef"]"#, + r#"["NEG-OPEN","sub",{},"deadbeef","extra"]"#, + ] { + let err = ClientMessage::from_json(json).unwrap_err(); + assert_eq!(err.kind(), ErrorKind::Malformed, "{json}"); + } + } + + /// An `EVENT` message must embed the event exactly as the event serializes + /// on its own. Round-tripping through `serde_json::Value` used to reorder + /// the event's keys alphabetically. + #[test] + fn event_message_embeds_canonical_event() { + let event: Event = Event::from_json(EVENT_JSON).unwrap(); + let message: ClientMessage = ClientMessage::event(event.clone()); + let json: String = message.as_json(); + + assert!( + json.contains(&event.as_json()), + "event was not embedded verbatim: {json}" + ); + } } diff --git a/nostr/src/message/mod.rs b/nostr/src/message/mod.rs index 1ca26300e..c1d60da7f 100644 --- a/nostr/src/message/mod.rs +++ b/nostr/src/message/mod.rs @@ -20,14 +20,9 @@ pub mod relay; pub use self::client::ClientMessage; pub use self::relay::{MachineReadablePrefix, RelayMessage}; -use crate::error::{Error, ErrorKind}; #[cfg(feature = "rand")] use crate::util; -fn invalid_message_format() -> Error { - Error::with_static_message(ErrorKind::Malformed, "invalid message format") -} - /// Subscription ID #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SubscriptionId(String); diff --git a/nostr/src/util/json.rs b/nostr/src/util/json.rs index ebc838bf4..37cb1df82 100644 --- a/nostr/src/util/json.rs +++ b/nostr/src/util/json.rs @@ -1,4 +1,5 @@ use serde::de; +#[cfg(feature = "nip47")] use serde_json::Value; use crate::error::Error; @@ -13,6 +14,7 @@ where } #[inline] +#[cfg(feature = "nip47")] pub(crate) fn parse_json_from_value(value: Value) -> Result where T: de::DeserializeOwned, diff --git a/nostr/src/util/mod.rs b/nostr/src/util/mod.rs index 968929233..5ef1028ec 100644 --- a/nostr/src/util/mod.rs +++ b/nostr/src/util/mod.rs @@ -24,7 +24,9 @@ pub(crate) mod hkdf; mod json; pub(crate) mod sha256; -pub(crate) use self::json::{impl_json_methods, parse_json, parse_json_from_value}; +#[cfg(feature = "nip47")] +pub(crate) use self::json::parse_json_from_value; +pub(crate) use self::json::{impl_json_methods, parse_json}; use crate::error::Error; #[cfg(any(feature = "nip04", feature = "nip44"))] use crate::key::{PublicKey, SecretKey};