RemotingClient should handle undeserializable remote exceptions.

theRainbird · Dec 4, 2024 · fb8b97a · fb8b97a
1 parent 449428f
commit fb8b97a
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 19 deletions.
diff --git a/CoreRemoting.Tests/Tools/FailingService.cs b/CoreRemoting.Tests/Tools/FailingService.cs
@@ -1,7 +1,4 @@
 using System;
-using System.Data;
-using System.Threading.Tasks;
-using CoreRemoting.Tests.ExternalTypes;
 
 namespace CoreRemoting.Tests.Tools;
 

diff --git a/CoreRemoting/Channels/Websocket/WebsocketServerChannel.cs b/CoreRemoting/Channels/Websocket/WebsocketServerChannel.cs
@@ -14,8 +14,8 @@ public class WebsocketServerChannel : IServerChannel
 
     private IRemotingServer Server { get; set; }
 
-    private ConcurrentDictionary<Guid, WebsocketConnection> Connections { get; } =
-        new ConcurrentDictionary<Guid, WebsocketConnection>();
+    private ConcurrentDictionary<Guid, WebsocketServerConnection> Connections { get; } =
+        new ConcurrentDictionary<Guid, WebsocketServerConnection>();
 
     /// <inheritdoc/>
     public bool IsListening => HttpListener.IsListening;
@@ -57,7 +57,7 @@ private async Task ReceiveConnection()
         // accept websocket request and start a new session
         var websocketContext = await context.AcceptWebSocketAsync(null);
         var websocket = websocketContext.WebSocket;
-        var connection = new WebsocketConnection(websocketContext, websocket, Server);
+        var connection = new WebsocketServerConnection(websocketContext, websocket, Server);
 
         // handle incoming websocket messages
         var sessionId = connection.StartListening();

diff --git a/...Channels/Websocket/WebsocketConnection.cs → ...ls/Websocket/WebsocketServerConnection.cs b/...Channels/Websocket/WebsocketConnection.cs → ...ls/Websocket/WebsocketServerConnection.cs
@@ -9,15 +9,15 @@ namespace CoreRemoting.Channels.Websocket;
 /// <summary>
 /// Websocket connection.
 /// </summary>
-public class WebsocketConnection : IRawMessageTransport
+public class WebsocketServerConnection : IRawMessageTransport
 {
     // note: LOH threshold is ~85 kilobytes
     private const int BufferSize = 16 * 1024;
 
     /// <summary>
-    /// Initializes a new instance of the <see cref="WebsocketConnection"/> class.
+    /// Initializes a new instance of the <see cref="WebsocketServerConnection"/> class.
     /// </summary>
-    public WebsocketConnection(HttpListenerWebSocketContext websocketContext, WebSocket websocket, IRemotingServer remotingServer)
+    public WebsocketServerConnection(HttpListenerWebSocketContext websocketContext, WebSocket websocket, IRemotingServer remotingServer)
     {
         WebSocketContext = websocketContext ?? throw new ArgumentNullException(nameof(websocketContext));
         WebSocket = websocket ?? throw new ArgumentNullException(nameof(websocket));

diff --git a/CoreRemoting/RemotingClient.cs b/CoreRemoting/RemotingClient.cs
@@ -434,7 +434,7 @@ private void Authenticate()
         /// <param name="rawMessage">Raw message data</param>
         private void OnMessage(byte[] rawMessage)
         {
-            var message = Serializer.Deserialize<WireMessage>(rawMessage);
+            var message = TryDeserialize(rawMessage);
 
             switch (message.MessageType.ToLower())
             {
@@ -456,6 +456,29 @@ private void OnMessage(byte[] rawMessage)
                 case "session_closed":
                     Disconnect(quiet: true);
                     break;
+                default:
+                    // TODO: how do we handle invalid wire messages received by the client?
+                    // A wire message could have been tampered with and couldn't be deserialized
+                    break;
+            }
+        }
+
+        private WireMessage TryDeserialize(byte[] rawMessage)
+        {
+            try
+            {
+                return Serializer.Deserialize<WireMessage>(rawMessage);
+            }
+            catch // TODO: dispatch message deserialization exception?
+            {
+                return new WireMessage
+                {
+                    Data = rawMessage,
+                    Error = true,
+                    Iv = Array.Empty<byte>(),
+                    MessageType = "invalid",
+                    UniqueCallKey = Array.Empty<byte>(),
+                };
             }
         }
 
@@ -585,16 +608,27 @@ private void ProcessRpcResultMessage(WireMessage message)
 
             if (message.Error)
             {
-                var remoteException =
-                    Serializer.Deserialize<RemoteInvocationException>(
-                        MessageEncryptionManager.GetDecryptedMessageData(
-                            message: message,
-                            serializer: Serializer,
-                            sharedSecret: sharedSecret,
-                            sendersPublicKeyBlob: _serverPublicKeyBlob,
-                            sendersPublicKeySize: _keyPair?.KeySize ?? 0));
+                try
+                {
+                    var remoteException =
+                        Serializer.Deserialize<RemoteInvocationException>(
+                            MessageEncryptionManager.GetDecryptedMessageData(
+                                message: message,
+                                serializer: Serializer,
+                                sharedSecret: sharedSecret,
+                                sendersPublicKeyBlob: _serverPublicKeyBlob,
+                                sendersPublicKeySize: _keyPair?.KeySize ?? 0));
+
+                    clientRpcContext.RemoteException = remoteException;
+                }
+                catch (Exception deserializationException)
+                {
+                    var remoteException = new RemoteInvocationException(
+                        "Remote exception couldn't be deserialized",
+                            deserializationException);
 
-                clientRpcContext.RemoteException = remoteException;
+                    clientRpcContext.RemoteException = remoteException;
+                }
             }
             else
             {