fix: Enabling carbons crashes

packages/moxxmpp/lib/src/connection.dart

@@ -29,22 +29,35 @@ import 'package:moxxmpp/src/xeps/xep_0352.dart';
 import 'package:synchronized/synchronized.dart';
 import 'package:uuid/uuid.dart';
 
+/// The states the XmppConnection can be in
 enum XmppConnectionState {
+  /// The XmppConnection instance is not connected to the server. This is either the
+  /// case before connecting or after disconnecting.
   notConnected,
+
+  /// We are currently trying to connect to the server.
   connecting,
+
+  /// We are currently connected to the server.
   connected,
+
+  /// We have received an unrecoverable error and the server killed the connection
   error
 }
 
+/// Metadata for [XmppConnection.sendStanza].
 enum StanzaFromType {
-  // Add the full JID to the stanza as the from attribute
+  /// Add the full JID to the stanza as the from attribute
   full,
-  // Add the bare JID to the stanza as the from attribute
+
+  /// Add the bare JID to the stanza as the from attribute
   bare,
-  // Add no JID as the from attribute
+
-  none
+  /// Add no JID as the from attribute
+  none,
 }
 
+/// Nonza describing the XMPP stream header.
 class StreamHeaderNonza extends XMLNode {
   StreamHeaderNonza(String serverDomain) : super(
       tag: 'stream:stream',
@@ -59,6 +72,7 @@ class StreamHeaderNonza extends XMLNode {
     );
 }
 
+/// The result of an awaited connection.
 class XmppConnectionResult {
   const XmppConnectionResult(
     this.success,
@@ -67,16 +81,24 @@ class XmppConnectionResult {
     }
   );
 
+  /// True if the connection was successful. False if it failed for any reason.
   final bool success;
+
   // If a connection attempt fails, i.e. success is false, then this indicates the
   // reason the connection failed.
   final XmppError? error;
 }
 
+/// A surrogate key for awaiting stanzas.
 @immutable
 class _StanzaAwaitableData {
   const _StanzaAwaitableData(this.sentTo, this.id);
+
+  /// The JID the original stanza was sent to. We expect the result to come from the
+  /// same JID.
   final String sentTo;
+
+  /// The ID of the original stanza. We expect the result to have the same ID.
   final String id;
 
   @override
@@ -90,10 +112,8 @@ class _StanzaAwaitableData {
   }
 }
 
+/// This class is a connection to the server.
 class XmppConnection {
-  /// [_socket] is for debugging purposes.
-  /// [connectionPingDuration] is the duration after which a ping will be sent to keep
-  /// the connection open. Defaults to 15 minutes.
   XmppConnection(
     ReconnectionPolicy reconnectionPolicy,
     this._socket,
@@ -101,26 +121,7 @@ class XmppConnection {
       this.connectionPingDuration = const Duration(minutes: 3),
       this.connectingTimeout = const Duration(minutes: 2),
     }
-  ) :
+  ) : _reconnectionPolicy = reconnectionPolicy {
-    _connectionState = XmppConnectionState.notConnected,
-    _routingState = RoutingState.preConnection,
-    _eventStreamController = StreamController.broadcast(),
-    _resource = '',
-    _streamBuffer = XmlStreamBuffer(),
-    _uuid = const Uuid(),
-    _awaitingResponse = {},
-    _awaitingResponseLock = Lock(),
-    _xmppManagers = {},
-    _incomingStanzaHandlers = List.empty(growable: true),
-    _incomingPreStanzaHandlers = List.empty(growable: true),
-    _outgoingPreStanzaHandlers = List.empty(growable: true),
-    _outgoingPostStanzaHandlers = List.empty(growable: true),
-    _reconnectionPolicy = reconnectionPolicy,
-    _featureNegotiators = {},
-    _streamFeatures = List.empty(growable: true),
-    _negotiationLock = Lock(),
-    _isAuthenticated = false,
-    _log = Logger('XmppConnection') {
       // Allow the reconnection policy to perform reconnections by itself
       _reconnectionPolicy.register(
         _attemptReconnection,
@@ -134,69 +135,103 @@ class XmppConnection {
   }
 
 
-  /// Connection properties
+  /// The state that the connection is currently in
-  ///
+  XmppConnectionState _connectionState = XmppConnectionState.notConnected;
-  /// The state that the connection currently is in
+
-  XmppConnectionState _connectionState;
   /// The socket that we are using for the connection and its data stream
   final BaseSocketWrapper _socket;
+
+  /// The data stream of the socket
   late final Stream<String> _socketStream;
-  /// Account settings
+
+
+  /// Connection settings
   late ConnectionSettings _connectionSettings;
+
   /// A policy on how to reconnect 
   final ReconnectionPolicy _reconnectionPolicy;
-  /// A list of stanzas we are tracking with its corresponding critical section
-  final Map<_StanzaAwaitableData, Completer<XMLNode>> _awaitingResponse;
-  final Lock _awaitingResponseLock;
 
-  /// Helpers
+  /// A list of stanzas we are tracking with its corresponding critical section lock
-  ///
+  final Map<_StanzaAwaitableData, Completer<XMLNode>> _awaitingResponse = {};
-  final List<StanzaHandler> _incomingStanzaHandlers;
+  final Lock _awaitingResponseLock = Lock();
-  final List<StanzaHandler> _incomingPreStanzaHandlers;
+  
-  final List<StanzaHandler> _outgoingPreStanzaHandlers;
+  /// Sorted list of handlers that we call or incoming and outgoing stanzas
-  final List<StanzaHandler> _outgoingPostStanzaHandlers;
+  final List<StanzaHandler> _incomingStanzaHandlers = List.empty(growable: true);
-  final StreamController<XmppEvent> _eventStreamController;
+  final List<StanzaHandler> _incomingPreStanzaHandlers = List.empty(growable: true);
-  final Map<String, XmppManagerBase> _xmppManagers;
+  final List<StanzaHandler> _outgoingPreStanzaHandlers = List.empty(growable: true);
+  final List<StanzaHandler> _outgoingPostStanzaHandlers = List.empty(growable: true);
+  final StreamController<XmppEvent> _eventStreamController = StreamController.broadcast();
+  final Map<String, XmppManagerBase> _xmppManagers = {};
   
-  /// Stream properties
-  ///
   /// Disco info we got after binding a resource (xmlns)
   final List<String> _serverFeatures = List.empty(growable: true);
+
   /// The buffer object to keep split up stanzas together
-  final XmlStreamBuffer _streamBuffer;
+  final XmlStreamBuffer _streamBuffer = XmlStreamBuffer();
+
   /// UUID object to generate stanza and origin IDs
-  final Uuid _uuid;
+  final Uuid _uuid = const Uuid();
+
   /// The time between sending a ping to keep the connection open
   // TODO(Unknown): Only start the timer if we did not send a stanza after n seconds
   final Duration connectionPingDuration;
+
   /// The time that we may spent in the "connecting" state
   final Duration connectingTimeout;
+
   /// The current state of the connection handling state machine.
-  RoutingState _routingState;
+  RoutingState _routingState = RoutingState.preConnection;
+
   /// The currently bound resource or '' if none has been bound yet.
-  String _resource;
+  String _resource = '';
+
   /// True if we are authenticated. False if not.
-  bool _isAuthenticated;
+  bool _isAuthenticated = false;
+
   /// Timer for the keep-alive ping.
   Timer? _connectionPingTimer;
+
   /// Timer for the connecting timeout
   Timer? _connectingTimeoutTimer;
+
   /// Completers for certain actions
   // ignore: use_late_for_private_fields_and_variables
   Completer<XmppConnectionResult>? _connectionCompleter;
+
   /// Controls whether an XmppSocketClosureEvent triggers a reconnection.
   bool _socketClosureTriggersReconnect = true;
 
   /// Negotiators
-  final Map<String, XmppFeatureNegotiatorBase> _featureNegotiators;
+  final Map<String, XmppFeatureNegotiatorBase> _featureNegotiators = {};
   XmppFeatureNegotiatorBase? _currentNegotiator;
-  final List<XMLNode> _streamFeatures;
+  final List<XMLNode> _streamFeatures = List.empty(growable: true);
   /// Prevent data from being passed to _currentNegotiator.negotiator while the negotiator
   /// is still running.
-  final Lock _negotiationLock;
+  final Lock _negotiationLock = Lock();
   
-  /// Misc
+  /// The logger for the class
-  final Logger _log;
+  final Logger _log = Logger('XmppConnection');
+
+  /// A value indicating whether a connection attempt is currently running or not
+  bool _isConnectionRunning = false;
+  final Lock _connectionRunningLock = Lock();
+
+  /// Enters the critical section for accessing [XmppConnection._isConnectionRunning]
+  /// and does the following:
+  /// - if _isConnectionRunning is false, set it to true and return false.
+  /// - if _isConnectionRunning is true, return true.
+  Future<bool> _testAndSetIsConnectionRunning() async => _connectionRunningLock.synchronized(() {
+    if (!_isConnectionRunning) {
+      _isConnectionRunning = true;
+      return false;
+    }
+
+    return true;
+  });
+
+  /// Enters the critical section for accessing [XmppConnection._isConnectionRunning]
+  /// and sets it to false.
+  Future<void> _resetIsConnectionRunning() async => _connectionRunningLock.synchronized(() => _isConnectionRunning = false);
   
   ReconnectionPolicy get reconnectionPolicy => _reconnectionPolicy;
   
@@ -353,6 +388,11 @@ class XmppConnection {
 
   /// Attempts to reconnect to the server by following an exponential backoff.
   Future<void> _attemptReconnection() async {
+    if (await _testAndSetIsConnectionRunning()) {
+      _log.warning('_attemptReconnection is called but connection attempt is already running. Ignoring...');
+      return;
+    }
+
     _log.finest('_attemptReconnection: Setting state to notConnected');
     await _setConnectionState(XmppConnectionState.notConnected);
     _log.finest('_attemptReconnection: Done');
@@ -388,6 +428,7 @@ class XmppConnection {
     }
 
     await _setConnectionState(XmppConnectionState.error);
+    await _resetIsConnectionRunning();
     await _reconnectionPolicy.onFailure();
   }
 
@@ -785,6 +826,7 @@ class XmppConnection {
   /// a disco sweep among other things.
   Future<void> _onNegotiationsDone() async {
     // Set the connection state
+    await _resetIsConnectionRunning();
     await _setConnectionState(XmppConnectionState.connected);
 
     // Resolve the connection completion future
@@ -968,9 +1010,10 @@ class XmppConnection {
   }
 
   /// To be called when we lost the network connection.
-  void _onNetworkConnectionLost() {
+  Future<void> _onNetworkConnectionLost() async {
     _socket.close();
-    _setConnectionState(XmppConnectionState.notConnected);
+    await _resetIsConnectionRunning();
+    await _setConnectionState(XmppConnectionState.notConnected);
   }
 
   /// Attempt to gracefully close the session
@@ -1011,11 +1054,12 @@ class XmppConnection {
   
   /// Like [connect] but the Future resolves when the resource binding is either done or
   /// SASL has failed.
-  Future<XmppConnectionResult> connectAwaitable({ String? lastResource }) {
+  Future<XmppConnectionResult> connectAwaitable({ String? lastResource }) async {
     _runPreConnectionAssertions();
+    await _resetIsConnectionRunning();
     _connectionCompleter = Completer();
     _log.finest('Calling connect() from connectAwaitable');
-    connect(lastResource: lastResource);
+    await connect(lastResource: lastResource);
     return _connectionCompleter!.future;
   }
  
@@ -1027,6 +1071,7 @@ class XmppConnection {
     }
     
     _runPreConnectionAssertions();
+    await _resetIsConnectionRunning();
     _reconnectionPolicy.setShouldReconnect(true);
     
     if (lastResource != null) {

packages/moxxmpp/lib/src/jid.dart

@@ -1,10 +1,11 @@
 import 'package:meta/meta.dart';
 
+/// Represents a Jabber ID in parsed form.
 @immutable
 class JID {
-
   const JID(this.local, this.domain, this.resource);
 
+  /// Parses the string [jid] into a JID instance.
   factory JID.fromString(String jid) {
     // Algorithm taken from here: https://blog.samwhited.com/2021/02/xmpp-addresses/
     var localPart = '';
@@ -43,12 +44,29 @@ class JID {
   final String domain;
   final String resource;
 
+  /// Returns true if the JID is bare.
   bool isBare() => resource.isEmpty;
+
+  /// Returns true if the JID is full.
   bool isFull() => resource.isNotEmpty;
 
+  /// Converts the JID into a bare JID.
   JID toBare() => JID(local, domain, '');
+
+  /// Converts the JID into one with a resource part of [resource].
   JID withResource(String resource) => JID(local, domain, resource);
 
+  /// Compares the JID with [other]. This function assumes that JID and [other]
+  /// are bare, i.e. only the domain- and localparts are compared. If [ensureBare]
+  /// is optionally set to true, then [other] MUST be bare. Otherwise, false is returned.
+  bool bareCompare(JID other, { bool ensureBare = false }) {
+    if (ensureBare && !other.isBare()) return false;
+
+    return local == other.local && domain == other.domain;
+  }
+  
+  /// Converts to JID instance into its string representation of
+  /// localpart@domainpart/resource.
   @override
   String toString() {
     var result = '';

packages/moxxmpp/lib/src/reconnect.dart

@@ -4,27 +4,33 @@ import 'package:logging/logging.dart';
 import 'package:meta/meta.dart';
 import 'package:synchronized/synchronized.dart';
 
-abstract class ReconnectionPolicy {
+/// A callback function to be called when the connection to the server has been lost.
+typedef ConnectionLostCallback = Future<void> Function();
 
-  ReconnectionPolicy()
+/// A function that, when called, causes the XmppConnection to connect to the server, if
-    : _shouldAttemptReconnection = false,
+/// another reconnection is not already running.
-      _isReconnecting = false,
+typedef PerformReconnectFunction = Future<void> Function();
-      _isReconnectingLock = Lock();
+
+abstract class ReconnectionPolicy {
   /// Function provided by XmppConnection that allows the policy
   /// to perform a reconnection.
-  Future<void> Function()? performReconnect;
+  PerformReconnectFunction? performReconnect;
+
   /// Function provided by XmppConnection that allows the policy
   /// to say that we lost the connection.
-  void Function()? triggerConnectionLost;
+  ConnectionLostCallback? triggerConnectionLost;
+
   /// Indicate if should try to reconnect.
-  bool _shouldAttemptReconnection;
+  bool _shouldAttemptReconnection = false;
+
   /// Indicate if a reconnection attempt is currently running.
-  bool _isReconnecting;
+  bool _isReconnecting = false;
+
   /// And the corresponding lock
-  final Lock _isReconnectingLock;
+  final Lock _isReconnectingLock = Lock();
   
   /// Called by XmppConnection to register the policy.
-  void register(Future<void> Function() performReconnect, void Function() triggerConnectionLost) {
+  void register(PerformReconnectFunction performReconnect, ConnectionLostCallback triggerConnectionLost) {
     this.performReconnect = performReconnect;
     this.triggerConnectionLost = triggerConnectionLost;
 

packages/moxxmpp/lib/src/xeps/xep_0280.dart

@@ -12,10 +12,17 @@ import 'package:moxxmpp/src/stringxml.dart';
 import 'package:moxxmpp/src/xeps/xep_0030/xep_0030.dart';
 import 'package:moxxmpp/src/xeps/xep_0297.dart';
 
+/// This manager class implements support for XEP-0280.
 class CarbonsManager extends XmppManagerBase {
   CarbonsManager() : super();
+
+  /// Indicates that message carbons are enabled.
   bool _isEnabled = false;
+
+  /// Indicates that the server supports message carbons.
   bool _supported = false;
+
+  /// Indicates that we know that [CarbonsManager._supported] is accurate.
   bool _gotSupported = false;
   
   @override
@@ -102,9 +109,14 @@ class CarbonsManager extends XmppManagerBase {
     );
   }
 
+  /// Send a request to the server, asking it to enable Message Carbons.
+  ///
+  /// Returns true if carbons were enabled. False, if not.
   Future<bool> enableCarbons() async {
-    final result = await getAttributes().sendStanza(
+    final attrs = getAttributes();
+    final result = await attrs.sendStanza(
       Stanza.iq(
+        to: attrs.getFullJID().toBare().toString(),
         type: 'set',
         children: [
           XMLNode.xmlns(
@@ -129,6 +141,9 @@ class CarbonsManager extends XmppManagerBase {
     return true;
   }
 
+  /// Send a request to the server, asking it to disable Message Carbons.
+  ///
+  /// Returns true if carbons were disabled. False, if not.
   Future<bool> disableCarbons() async {
     final result = await getAttributes().sendStanza(
       Stanza.iq(
@@ -164,7 +179,14 @@ class CarbonsManager extends XmppManagerBase {
     _isEnabled = true;
   }
 
+  /// Checks if a carbon sent by [senderJid] is valid to prevent vulnerabilities like
+  /// the ones listed at https://xmpp.org/extensions/xep-0280.html#security.
+  ///
+  /// Returns true if the carbon is valid. Returns false if not.
   bool isCarbonValid(JID senderJid) {
-    return _isEnabled && senderJid == getAttributes().getConnectionSettings().jid.toBare();
+    return _isEnabled && getAttributes().getFullJID().bareCompare(
+      senderJid,
+      ensureBare: true,
+    );
   }
 }

packages/moxxmpp/test/jid_test.dart

@@ -1,6 +1,5 @@
 import 'package:moxxmpp/moxxmpp.dart';
 import 'package:test/test.dart';
-
 void main() {
   test('Parse a full JID', () {
     final jid = JID.fromString('test@server/abc');
@@ -42,4 +41,15 @@ void main() {
   test('Parse resource with a slash', () {
     expect(JID.fromString('hallo@welt.example./test/welt') == JID('hallo', 'welt.example', 'test/welt'), true);
   });
+
+  test('bareCompare', () {
+    final jid1 = JID('hallo', 'welt', 'lol');
+    final jid2 = JID('hallo', 'welt', '');
+    final jid3 = JID('hallo', 'earth', 'true');
+
+    expect(jid1.bareCompare(jid2), true);
+    expect(jid2.bareCompare(jid1), true);
+    expect(jid2.bareCompare(jid1, ensureBare: true), false);
+    expect(jid2.bareCompare(jid3), false);
+  });
 }

packages/moxxmpp/test/xeps/xep_0280_test.dart

@@ -4,33 +4,33 @@ import '../helpers/xmpp.dart';
 
 void main() {
   test("Test if we're vulnerable against CVE-2020-26547 style vulnerabilities", () async {
-      final attributes = XmppManagerAttributes(
+    final attributes = XmppManagerAttributes(
-        sendStanza: (stanza, { StanzaFromType addFrom = StanzaFromType.full, bool addId = true, bool retransmitted = false, bool awaitable = true, bool encrypted = false }) async {
+      sendStanza: (stanza, { StanzaFromType addFrom = StanzaFromType.full, bool addId = true, bool retransmitted = false, bool awaitable = true, bool encrypted = false }) async {
-          // ignore: avoid_print
+        // ignore: avoid_print
-          print('==> ${stanza.toXml()}');
+        print('==> ${stanza.toXml()}');
-          return XMLNode(tag: 'iq', attributes: { 'type': 'result' });
+        return XMLNode(tag: 'iq', attributes: { 'type': 'result' });
-        },
+      },
-        sendNonza: (nonza) {},
+      sendNonza: (nonza) {},
-        sendEvent: (event) {},
+      sendEvent: (event) {},
-        getManagerById: getManagerNullStub,
+      getManagerById: getManagerNullStub,
-        getConnectionSettings: () => ConnectionSettings(
+      getConnectionSettings: () => ConnectionSettings(
-          jid: JID.fromString('bob@xmpp.example'),
+        jid: JID.fromString('bob@xmpp.example'),
-          password: 'password',
+        password: 'password',
-          useDirectTLS: true,
+        useDirectTLS: true,
-          allowPlainAuth: false,
+        allowPlainAuth: false,
-        ),
+      ),
-        isFeatureSupported: (_) => false,
+      isFeatureSupported: (_) => false,
-        getFullJID: () => JID.fromString('bob@xmpp.example/uwu'),
+      getFullJID: () => JID.fromString('bob@xmpp.example/uwu'),
-        getSocket: () => StubTCPSocket(play: []),
+      getSocket: () => StubTCPSocket(play: []),
-        getConnection: () => XmppConnection(TestingReconnectionPolicy(), StubTCPSocket(play: [])),
+      getConnection: () => XmppConnection(TestingReconnectionPolicy(), StubTCPSocket(play: [])),
-        getNegotiatorById: getNegotiatorNullStub,
+      getNegotiatorById: getNegotiatorNullStub,
-      );
+    );
-      final manager = CarbonsManager();
+    final manager = CarbonsManager();
-      manager.register(attributes);
+    manager.register(attributes);
-      await manager.enableCarbons();
+    await manager.enableCarbons();
 
-      expect(manager.isCarbonValid(JID.fromString('mallory@evil.example')), false);
+    expect(manager.isCarbonValid(JID.fromString('mallory@evil.example')), false);
-      expect(manager.isCarbonValid(JID.fromString('bob@xmpp.example')), true);
+    expect(manager.isCarbonValid(JID.fromString('bob@xmpp.example')), true);
-      expect(manager.isCarbonValid(JID.fromString('bob@xmpp.example/abc')), false);
+    expect(manager.isCarbonValid(JID.fromString('bob@xmpp.example/abc')), false);
   });
 }