From a8fd96a2430097e69027da66c2762fe189b93160 Mon Sep 17 00:00:00 2001 From: Niklas <niklasadolfsson1@gmail.com> Date: Thu, 22 Oct 2020 13:11:36 +0200 Subject: [PATCH 1/3] [ws server]: fix broken unsubscribe. Try to parse the subscription ID as the first element of an Array or the `subscription` field of an Object/Map. If both of those fails then regard it as a error. --- src/client/ws/raw.rs | 2 +- src/ws/raw/core.rs | 17 +++++++++++++++++ src/ws/server.rs | 34 +++++++++++++++++++--------------- 3 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/client/ws/raw.rs b/src/client/ws/raw.rs index 1a0c86154a..f767030542 100644 --- a/src/client/ws/raw.rs +++ b/src/client/ws/raw.rs @@ -697,7 +697,7 @@ impl<'a> RawClientActiveSubscription<'a> { _ => panic!(), }; - let params = common::Params::Array(vec![sub_id.clone().into()]); + let params = common::Params::Array(vec![sub_id.into()]); self.client .start_impl(method_name, params, Request::Unsubscribe(self.id)) .await diff --git a/src/ws/raw/core.rs b/src/ws/raw/core.rs index 60b263b763..ab1b08aedc 100644 --- a/src/ws/raw/core.rs +++ b/src/ws/raw/core.rs @@ -31,6 +31,7 @@ use crate::ws::transport::{TransportServerEvent, WsRequestId as RequestId, WsTra use alloc::{borrow::ToOwned as _, string::String, vec, vec::Vec}; use core::{fmt, hash::Hash, num::NonZeroUsize}; use hashbrown::{hash_map::Entry, HashMap}; +use std::convert::TryFrom; /// Wraps around a "raw server" and adds capabilities. /// @@ -423,6 +424,22 @@ impl RawServerSubscriptionId { } } +// Try to parse a subscription ID from `Params` where we try both index 0 of an array or `subscription` +// in a `Map`. +impl<'a> TryFrom<Params<'a>> for RawServerSubscriptionId { + type Error = (); + + fn try_from(params: Params) -> Result<Self, Self::Error> { + if let Ok(other_id) = params.get(0) { + Self::from_wire_message(&other_id) + } else if let Ok(other_id) = params.get("subscription") { + Self::from_wire_message(&other_id) + } else { + Err(()) + } + } +} + impl<'a> ServerSubscription<'a> { /// Returns the id of the subscription. /// diff --git a/src/ws/server.rs b/src/ws/server.rs index c2b995fd5b..27db9b82d1 100644 --- a/src/ws/server.rs +++ b/src/ws/server.rs @@ -32,6 +32,7 @@ use futures::{channel::mpsc, future::Either, pin_mut, prelude::*}; use parking_lot::Mutex; use std::{ collections::{HashMap, HashSet}, + convert::TryFrom, error, net::SocketAddr, sync::{atomic, Arc}, @@ -456,8 +457,8 @@ async fn background_task( } } Either::Right(RawServerEvent::Request(request)) => { - log::debug!("[backend]: server received request: {:?}", request); if let Some(handler) = registered_methods.get_mut(request.method()) { + log::debug!("[backend]: server received request: {:?}", request); let params: &common::Params = request.params().into(); log::debug!("server called handler"); match handler.send((request.id(), params.clone())).now_or_never() { @@ -467,6 +468,7 @@ async fn background_task( } } } else if let Some(sub_unique_id) = subscribe_methods.get(request.method()) { + log::debug!("[backend]: server received subscription: {:?}", request); if let Ok(sub_id) = request.into_subscription() { debug_assert!(subscribed_clients.contains_key(&sub_unique_id)); if let Some(clients) = subscribed_clients.get_mut(&sub_unique_id) { @@ -478,22 +480,24 @@ async fn background_task( active_subscriptions.insert(sub_id, *sub_unique_id); } } else if let Some(sub_unique_id) = unsubscribe_methods.get(request.method()) { - if let Ok(sub_id) = RawServerSubscriptionId::from_wire_message(&JsonValue::Null) - { - // FIXME: from request params - debug_assert!(subscribed_clients.contains_key(&sub_unique_id)); - if let Some(clients) = subscribed_clients.get_mut(&sub_unique_id) { - // TODO: we don't actually check whether the unsubscribe comes from the right - // clients, but since this the ID is randomly-generated, it should be - // fine - if let Some(client_pos) = clients.iter().position(|c| *c == sub_id) { - clients.remove(client_pos); - } - - if let Some(s_u_id) = active_subscriptions.remove(&sub_id) { - debug_assert_eq!(s_u_id, *sub_unique_id); + log::debug!("[backend]: server received unsubscription: {:?}", request); + match RawServerSubscriptionId::try_from(request.params()) { + Ok(sub_id) => { + debug_assert!(subscribed_clients.contains_key(&sub_unique_id)); + if let Some(clients) = subscribed_clients.get_mut(&sub_unique_id) { + // TODO: we don't actually check whether the unsubscribe comes from the right + // clients, but since this the ID is randomly-generated, it should be + // fine + if let Some(client_pos) = clients.iter().position(|c| *c == sub_id) { + clients.remove(client_pos); + } + + if let Some(s_u_id) = active_subscriptions.remove(&sub_id) { + debug_assert_eq!(s_u_id, *sub_unique_id); + } } } + Err(_) => log::error!("Unsubscription of method=\"{}\" failed; The subscription ID must passed as the first argument of Array or \"subscription\" name of Object, got={:?}", request.method(), request.params()), } } else { // TODO: we assert that the request is valid because the parsing succeeded but From 15e5071c28e0a6c7d136f3529662fb9a27c0cdf8 Mon Sep 17 00:00:00 2001 From: Niklas <niklasadolfsson1@gmail.com> Date: Thu, 22 Oct 2020 13:16:33 +0200 Subject: [PATCH 2/3] fmt --- src/ws/raw/core.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ws/raw/core.rs b/src/ws/raw/core.rs index ab1b08aedc..dff249551a 100644 --- a/src/ws/raw/core.rs +++ b/src/ws/raw/core.rs @@ -431,9 +431,9 @@ impl<'a> TryFrom<Params<'a>> for RawServerSubscriptionId { fn try_from(params: Params) -> Result<Self, Self::Error> { if let Ok(other_id) = params.get(0) { - Self::from_wire_message(&other_id) + Self::from_wire_message(&other_id) } else if let Ok(other_id) = params.get("subscription") { - Self::from_wire_message(&other_id) + Self::from_wire_message(&other_id) } else { Err(()) } From 1ebd25a32ec19ec4eff027f5aad60f65135b77e3 Mon Sep 17 00:00:00 2001 From: Niklas <niklasadolfsson1@gmail.com> Date: Fri, 23 Oct 2020 15:16:21 +0200 Subject: [PATCH 3/3] fix grumbles: remove space indentation --- src/ws/server.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/ws/server.rs b/src/ws/server.rs index 83e80f7733..aaec8ab01b 100644 --- a/src/ws/server.rs +++ b/src/ws/server.rs @@ -436,23 +436,23 @@ async fn background_task(mut server: RawServer, mut from_front: mpsc::UnboundedR } else if let Some(sub_unique_id) = unsubscribe_methods.get(request.method()) { log::debug!("[backend]: server received unsubscription: {:?}", request); match RawServerSubscriptionId::try_from(request.params()) { - Ok(sub_id) => { - debug_assert!(subscribed_clients.contains_key(&sub_unique_id)); - if let Some(clients) = subscribed_clients.get_mut(&sub_unique_id) { - // TODO: we don't actually check whether the unsubscribe comes from the right - // clients, but since this the ID is randomly-generated, it should be - // fine - if let Some(client_pos) = clients.iter().position(|c| *c == sub_id) { - clients.remove(client_pos); - } - - if let Some(s_u_id) = active_subscriptions.remove(&sub_id) { - debug_assert_eq!(s_u_id, *sub_unique_id); - } - } - } - Err(_) => log::error!("Unsubscription of method=\"{}\" failed; The subscription ID must passed as the first argument of Array or \"subscription\" name of Object, got={:?}", request.method(), request.params()), - } + Ok(sub_id) => { + debug_assert!(subscribed_clients.contains_key(&sub_unique_id)); + if let Some(clients) = subscribed_clients.get_mut(&sub_unique_id) { + // TODO: we don't actually check whether the unsubscribe comes from the right + // clients, but since this the ID is randomly-generated, it should be + // fine + if let Some(client_pos) = clients.iter().position(|c| *c == sub_id) { + clients.remove(client_pos); + } + + if let Some(s_u_id) = active_subscriptions.remove(&sub_id) { + debug_assert_eq!(s_u_id, *sub_unique_id); + } + } + } + Err(_) => log::error!("Unsubscription of method=\"{}\" failed; The subscription ID must passed as the first argument of Array or \"subscription\" name of Object, got={:?}", request.method(), request.params()), + } } else { // TODO: we assert that the request is valid because the parsing succeeded but // not registered.