From e2e73103de36b4ead4c0ccea6bd6b45e53e54c35 Mon Sep 17 00:00:00 2001
From: Markus Pettersson <markus.pettersson@mullvad.net>
Date: Mon, 8 Apr 2024 10:11:08 +0200
Subject: [PATCH] Fail loudly if no bridge was selected

---
 mullvad-relay-selector/src/error.rs           |  3 ++
 .../src/relay_selector/helpers.rs             | 27 +++++++-------
 .../src/relay_selector/mod.rs                 | 37 ++++++++++++-------
 3 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/mullvad-relay-selector/src/error.rs b/mullvad-relay-selector/src/error.rs
index f988be1b8931..1a00ce6941f4 100644
--- a/mullvad-relay-selector/src/error.rs
+++ b/mullvad-relay-selector/src/error.rs
@@ -28,6 +28,9 @@ pub enum Error {
         relay: EndpointErrorDetails,
     },
 
+    #[error("No candidates remain")]
+    NoCandidates,
+
     #[error("Failure in serialization of the relay list")]
     Serialize(#[from] serde_json::Error),
 
diff --git a/mullvad-relay-selector/src/relay_selector/helpers.rs b/mullvad-relay-selector/src/relay_selector/helpers.rs
index 263034d83659..f5f374ea28c8 100644
--- a/mullvad-relay-selector/src/relay_selector/helpers.rs
+++ b/mullvad-relay-selector/src/relay_selector/helpers.rs
@@ -64,19 +64,20 @@ pub fn pick_random_bridge(data: &BridgeEndpointData, relay: &Relay) -> Option<Cu
     if relay.endpoint_data != RelayEndpointData::Bridge {
         return None;
     }
-    let shadowsocks_endpoint = data.shadowsocks.choose(&mut rand::thread_rng());
-    if let Some(shadowsocks_endpoint) = shadowsocks_endpoint {
-        log::info!(
-            "Selected Shadowsocks bridge {} at {}:{}/{}",
-            relay.hostname,
-            relay.ipv4_addr_in,
-            shadowsocks_endpoint.port,
-            shadowsocks_endpoint.protocol
-        );
-        Some(shadowsocks_endpoint.to_proxy_settings(relay.ipv4_addr_in.into()))
-    } else {
-        None
-    }
+    data.shadowsocks
+        .choose(&mut rand::thread_rng())
+        .inspect(|shadowsocks_endpoint| {
+            log::info!(
+                "Selected Shadowsocks bridge {} at {}:{}/{}",
+                relay.hostname,
+                relay.ipv4_addr_in,
+                shadowsocks_endpoint.port,
+                shadowsocks_endpoint.protocol
+            );
+        })
+        .map(|shadowsocks_endpoint| {
+            shadowsocks_endpoint.to_proxy_settings(relay.ipv4_addr_in.into())
+        })
 }
 
 pub fn get_udp2tcp_obfuscator(
diff --git a/mullvad-relay-selector/src/relay_selector/mod.rs b/mullvad-relay-selector/src/relay_selector/mod.rs
index 2df4c00747aa..bfcd06b92c78 100644
--- a/mullvad-relay-selector/src/relay_selector/mod.rs
+++ b/mullvad-relay-selector/src/relay_selector/mod.rs
@@ -474,6 +474,8 @@ impl RelaySelector {
         let custom_lists = &config.custom_lists;
         Self::get_proxy_settings(parsed_relays, &constraints, near_location, custom_lists)
             .map(|(settings, _relay)| settings)
+            .inspect_err(|error| log::error!("Failed to get bridge: {error}"))
+            .ok()
     }
 
     /// Returns random relay and relay endpoint matching `query`.
@@ -892,26 +894,28 @@ impl RelaySelector {
                 }
                 TransportProtocol::Tcp => {
                     let location = relay.location.as_ref().ok_or(Error::NoRelay)?;
-                    Ok(Self::get_bridge_for(
+                    Self::get_bridge_for(
                         bridge_query,
                         location,
                         // FIXME: This is temporary while talpid-core only supports TCP proxies
                         TransportProtocol::Tcp,
                         parsed_relays,
                         custom_lists,
-                    ))
+                    )
+                    .transpose()
                 }
             }
         }
     }
 
+    // TODO(markus): Document return value
     fn get_bridge_for(
         query: &BridgeQuery,
         location: &Location,
         transport_protocol: TransportProtocol,
         parsed_relays: &ParsedRelays,
         custom_lists: &CustomListsSettings,
-    ) -> Option<SelectedBridge> {
+    ) -> Option<Result<SelectedBridge, Error>> {
         match query {
             BridgeQuery::Normal(settings) => {
                 let bridge_constraints = InternalBridgeConstraints {
@@ -921,15 +925,18 @@ impl RelaySelector {
                     transport_protocol: Constraint::Only(transport_protocol),
                 };
 
-                Self::get_proxy_settings(
+                let bridge = Self::get_proxy_settings(
                     parsed_relays,
                     &bridge_constraints,
                     Some(location),
                     custom_lists,
                 )
-                .map(|(settings, relay)| SelectedBridge::Normal { settings, relay })
+                .map(|(settings, relay)| SelectedBridge::Normal { settings, relay });
+                Some(bridge)
+            }
+            BridgeQuery::Custom(settings) => {
+                settings.clone().map(SelectedBridge::Custom).map(Result::Ok)
             }
-            BridgeQuery::Custom(settings) => settings.clone().map(SelectedBridge::Custom),
             BridgeQuery::Off | BridgeQuery::Auto => None,
         }
     }
@@ -942,22 +949,24 @@ impl RelaySelector {
         constraints: &InternalBridgeConstraints,
         location: Option<T>,
         custom_lists: &CustomListsSettings,
-    ) -> Option<(CustomProxy, Relay)> {
+    ) -> Result<(CustomProxy, Relay), Error> {
         let bridges = filter_matching_bridges(constraints, parsed_relays.relays(), custom_lists);
+        let bridge_data = &parsed_relays.parsed_list().bridge;
         let bridge = match location {
             Some(location) => Self::get_proximate_bridge(bridges, location),
-            None => helpers::pick_random_relay(&bridges).cloned(),
+            None => helpers::pick_random_relay(&bridges)
+                .cloned()
+                .ok_or(Error::NoRelay),
         }?;
-
-        let bridge_data = &parsed_relays.parsed_list().bridge;
-        helpers::pick_random_bridge(bridge_data, &bridge).map(|endpoint| (endpoint, bridge.clone()))
+        let endpoint = helpers::pick_random_bridge(bridge_data, &bridge).ok_or(Error::NoBridge)?;
+        Ok((endpoint, bridge))
     }
 
     /// Try to get a bridge which is close to `location`.
     fn get_proximate_bridge<T: Into<Coordinates>>(
         relays: Vec<Relay>,
         location: T,
-    ) -> Option<Relay> {
+    ) -> Result<Relay, Error> {
         /// Number of bridges to keep for selection by distance.
         const MIN_BRIDGE_COUNT: usize = 5;
         let location = location.into();
@@ -983,13 +992,15 @@ impl RelaySelector {
         let greatest_distance: f64 = matching_bridges
             .iter()
             .map(|relay| relay.distance)
-            .reduce(f64::max)?;
+            .reduce(f64::max)
+            .ok_or(Error::NoCandidates)?;
         // Define the weight function to prioritize bridges which are closer to `location`.
         let weight_fn = |relay: &RelayWithDistance| 1 + (greatest_distance - relay.distance) as u64;
 
         helpers::pick_random_relay_weighted(&matching_bridges, weight_fn)
             .cloned()
             .map(|relay_with_distance| relay_with_distance.relay)
+            .ok_or(Error::NoBridge)
     }
 
     /// Returns the average location of relays that match the given constraints.