fix: only show devices if /register succeed

packages/core/src/http/client/server_cert_verifier.rs

@@ -3,11 +3,45 @@ use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, Server
 use rustls::client::WebPkiServerVerifier;
 use rustls::pki_types::pem::PemObject;
 use rustls::pki_types::{CertificateDer, ServerName, UnixTime};
-use rustls::{CertificateError, DigitallySignedStruct, Error, RootCertStore, SignatureScheme};
-use std::fmt::{Debug, Formatter};
+use rustls::{
+    CertificateError, DigitallySignedStruct, Error, OtherError, RootCertStore, SignatureScheme,
+};
+use std::fmt::{Debug, Display, Formatter};
 use std::sync::Arc;
 use x509_parser::nom::AsBytes;
 
+/// The reason a peer certificate was rejected, as an error that rustls carries
+/// along.
+///
+/// Without this, every check in this file collapses into rustls'
+/// [`CertificateError::ApplicationVerificationFailure`], which surfaces in the
+/// app as a bare "error sending request for url (...)" with no hint that the
+/// certificate was the problem.
+struct RejectedCert(String);
+
+impl Display for RejectedCert {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+/// rustls renders `InvalidCertificate` with `Debug`, so the derived
+/// `RejectedCert("…")` would end up in the message.
+impl Debug for RejectedCert {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+impl std::error::Error for RejectedCert {}
+
+fn rejected(reason: String) -> Error {
+    tracing::warn!("{reason}");
+    Error::InvalidCertificate(CertificateError::Other(OtherError(Arc::new(RejectedCert(
+        reason,
+    )))))
+}
+
 /// Verifies the certificate of the remote peer during the TLS handshake.
 ///
 /// LocalSend peers use self-signed certificates, so there is no authority to
@@ -65,20 +99,15 @@ impl ServerCertVerifier for PinnedServerCertVerifier {
         // The hostname is deliberately ignored: peers are addressed by IP and
         // their certificates carry no matching SAN. The fingerprint below is
         // what identifies the peer.
-        crate::crypto::cert::verify_cert_from_der(end_entity.as_bytes(), None).map_err(|e| {
-            tracing::warn!("Server certificate verification failed: {e:#}");
-            Error::InvalidCertificate(CertificateError::ApplicationVerificationFailure)
-        })?;
+        crate::crypto::cert::verify_cert_from_der(end_entity.as_bytes(), None)
+            .map_err(|e| rejected(format!("server certificate is not valid: {e:#}")))?;
 
         if let Some(expected) = &self.expected_fingerprint {
             let actual = fingerprint_from_cert_der(end_entity.as_bytes());
             if &actual != expected {
-                tracing::warn!(
-                    "Server certificate fingerprint mismatch: expected {expected}, got {actual}"
-                );
-                return Err(Error::InvalidCertificate(
-                    CertificateError::ApplicationVerificationFailure,
-                ));
+                return Err(rejected(format!(
+                    "server certificate fingerprint mismatch: expected {expected}, got {actual}"
+                )));
             }
         }
 
@@ -167,7 +196,11 @@ VRus1zGVD8IVpIdPMyz01WJyS7M0fWaHXKWo+Bo=
     #[test]
     fn rejects_mismatching_fingerprint() {
         let other = "0".repeat(64);
-        assert!(verify(Some(&other)).is_err());
+        let error = verify(Some(&other)).unwrap_err();
+        let message = error.to_string();
+        assert!(message.contains("fingerprint mismatch"), "{message}");
+        assert!(message.contains(&other), "{message}");
+        assert!(message.contains(FINGERPRINT), "{message}");
     }
 
     #[test]

packages/localsend_isolates/lib/src/isolate/child/http_provider.dart

@@ -14,10 +14,15 @@ class HttpClientCollection {
   /// on that address.
   final RsHttpClient discovery;
 
+  /// The request timeout of the [discovery] client.
+  /// Also apply it to short-lived pinned requests made during discovery.
+  final int discoveryTimeout;
+
   HttpClientCollection({
     required String privateKey,
     required String certificate,
     required this.discovery,
+    required this.discoveryTimeout,
   }) : _privateKey = privateKey,
        _certificate = certificate;
 
@@ -27,12 +32,17 @@ class HttpClientCollection {
   /// The check happens during the TLS handshake, so a different peer never
   /// receives the request. Create one per upload task and reuse it for all
   /// files of that task, so the connection is kept alive between them.
-  RsHttpClient pinnedTo(String fingerprint) {
+  ///
+  /// [timeoutMs] bounds each request. Leave it out for uploads, which may
+  /// legitimately run for a long time; set it when the peer is unverified and
+  /// could keep the request open forever.
+  RsHttpClient pinnedTo(String fingerprint, {int? timeoutMs}) {
     return createClient(
       privateKey: _privateKey,
       cert: _certificate,
       version: LsHttpClientVersion.v2,
       expectedFingerprint: fingerprint,
+      timeoutMs: timeoutMs,
     );
   }
 }
@@ -51,5 +61,6 @@ final httpProvider = ViewProvider((ref) {
       expectedFingerprint: null,
       timeoutMs: discoveryTimeout,
     ),
+    discoveryTimeout: discoveryTimeout,
   );
 });

packages/localsend_isolates/lib/src/task/discovery/multicast_discovery.dart

@@ -26,16 +26,22 @@ class MulticastService {
   bool _listening = false;
 
   /// Binds the UDP sockets and listens to multicast announcements.
-  /// Announcements of other devices are answered with an HTTP register
-  /// request while the server is running.
-  Stream<Device> startListener() async* {
+  Stream<Device> startListener() {
     if (_listening) {
       _logger.info('Already listening to multicast');
-      return;
+      return const Stream.empty();
     }
 
     _listening = true;
 
+    // Verified devices are pushed in as [_answerAnnouncement] resolves, which
+    // runs concurrently: one unreachable peer must not hold up the others.
+    final devices = StreamController<Device>();
+    unawaited(_runListener(devices));
+    return devices.stream;
+  }
+
+  Future<void> _runListener(StreamController<Device> devices) async {
     while (true) {
       final syncState = _ref.read(syncProvider);
 
@@ -68,13 +74,17 @@ class MulticastService {
       unawaited(multicast.announce());
 
       await for (final event in multicast.listen()) {
-        final device = event.message.toDevice(event.ip);
-        yield device;
-
-        if (_ref.read(syncProvider).serverRunning) {
+        if (!_ref.read(syncProvider).serverRunning) {
           // only respond when server is running
-          unawaited(_answerAnnouncement(device));
+          continue;
         }
+
+        unawaited(() async {
+          final device = await _answerAnnouncement(event.message.toDevice(event.ip));
+          if (device != null && !devices.isClosed) {
+            devices.add(device);
+          }
+        }());
       }
 
       // The stream ended because [restartListener] stopped the discovery.
@@ -106,21 +116,28 @@ class MulticastService {
     await multicast.announce();
   }
 
-  /// Responds to an announcement over HTTP.
-  Future<void> _answerAnnouncement(Device peer) async {
+  /// Responds to an announcement and returns the peer.
+  /// Data from the multicast announcement is ignored.
+  ///
+  /// Returns null if the peer could not be reached or could not be verified.
+  Future<Device?> _answerAnnouncement(Device peer) async {
+    final clients = _ref.read(httpProvider);
     try {
-      await _ref
-          .read(httpProvider)
-          .discovery
+      final response = await clients
+          .pinnedTo(peer.fingerprint, timeoutMs: clients.discoveryTimeout)
           .register(
             protocol: peer.getProtocolType(),
             ip: peer.ip!,
             port: peer.port,
             payload: _ref.read(syncProvider).toRegisterDto(),
           );
+
       _logger.info('Respond to announcement of ${peer.alias} (${peer.ip}, model: ${peer.deviceModel}) via TCP');
+
+      return response.body.toDevice(peer.ip!, peer.port, peer.https, const MulticastDiscovery());
     } catch (e) {
       _logger.warning('Could not respond to announcement of ${peer.alias} (${peer.ip})', e);
+      return null;
     }
   }
 }

packages/localsend_isolates/rust/src/api/http.rs

@@ -7,6 +7,7 @@ pub use localsend::http::dto::{
     PrepareUploadRequestDto, PrepareUploadResponseDto, PrepareUploadResult, ProtocolType,
     RegisterDto, RegisterResponseDto,
 };
+use localsend::reqwest;
 
 pub struct RsHttpClient {
     inner: localsend::http::client::LsHttpClient,
@@ -195,7 +196,7 @@ impl From<ClientError> for RsHttpClientError {
                 status: e.status,
                 message: e.message,
             },
-            ClientError::Reqwest(e) => RsHttpClientError::Reqwest(e.to_string()),
+            ClientError::Reqwest(e) => RsHttpClientError::Reqwest(error_chain(&e)),
             ClientError::Json(e) => RsHttpClientError::Json(e.to_string()),
             ClientError::Io(e) => RsHttpClientError::Io(e.to_string()),
             ClientError::Other(e) => RsHttpClientError::Other(e.to_string()),
@@ -204,6 +205,21 @@ impl From<ClientError> for RsHttpClientError {
     }
 }
 
+/// Renders an error together with everything that caused it.
+///
+/// [`reqwest::Error`] alone only says "error sending request for url (...)".
+pub fn error_chain(e: &dyn std::error::Error) -> String {
+    use std::fmt::Write;
+
+    let mut message = e.to_string();
+    let mut source = e.source();
+    while let Some(current) = source {
+        let _ = write!(message, ": {current}");
+        source = current.source();
+    }
+    message
+}
+
 #[frb(mirror(LsHttpClientVersion))]
 pub enum _LsHttpClientVersion {
     V2,