From 3cdf6d126150479f2f806dd7ffe1a90d0c117871 Mon Sep 17 00:00:00 2001
From: HansM <HansM2013@gmx.de>
Date: Sun, 5 Sep 2021 11:42:42 +0200
Subject: [PATCH] [MQTTnet, MQTTnet.Extensions.ManagedClient] Fixed bug that
 allowed invalid subscriptions (Thanks to @marcelwinh). (#1226)

---
 Build/MQTTnet.nuspec                          |  2 +-
 .../ManagedMqttClient.cs                      |  5 ++
 Source/MQTTnet/Client/MqttClient.cs           |  5 ++
 Source/MQTTnet/Protocol/MqttTopicValidator.cs | 10 ++++
 Source/MQTTnet/Server/MqttServer.cs           |  7 ++-
 .../MqttTopicValidatorSubscribe_Tests.cs      | 54 +++++++++++++++++++
 6 files changed, 81 insertions(+), 2 deletions(-)
 create mode 100644 Tests/MQTTnet.Core.Tests/MqttTopicValidatorSubscribe_Tests.cs

diff --git a/Build/MQTTnet.nuspec b/Build/MQTTnet.nuspec
index f232bafc..720cdb2b 100644
--- a/Build/MQTTnet.nuspec
+++ b/Build/MQTTnet.nuspec
@@ -13,7 +13,7 @@
     <description>MQTTnet is a high performance .NET library for MQTT based communication. It provides a MQTT client and a MQTT server (broker) and supports v3.1.0, v3.1.1 and v5.0.0 of the MQTT protocol.</description>
     <releaseNotes>
 * [MQTTnet.Server] Moved server project to a dedicated GitHub repository.
-
+* [MQTTnet, MQTTnet.Extensions.ManagedClient] Fixed bug that allowed invalid subscriptions (Thanks to @marcelwinh).
 Git commit: $gitCommit
     </releaseNotes>
     <copyright>Copyright Christian Kratky 2016-2020</copyright>
diff --git a/Source/MQTTnet.Extensions.ManagedClient/ManagedMqttClient.cs b/Source/MQTTnet.Extensions.ManagedClient/ManagedMqttClient.cs
index 931326bc..d349f81e 100644
--- a/Source/MQTTnet.Extensions.ManagedClient/ManagedMqttClient.cs
+++ b/Source/MQTTnet.Extensions.ManagedClient/ManagedMqttClient.cs
@@ -215,6 +215,11 @@ namespace MQTTnet.Extensions.ManagedClient
 
             if (topicFilters == null) throw new ArgumentNullException(nameof(topicFilters));
 
+            foreach (var topicFilter in topicFilters)
+            {
+                MqttTopicValidator.ThrowIfInvalidSubscribe(topicFilter.Topic);
+            }
+
             lock (_subscriptions)
             {
                 foreach (var topicFilter in topicFilters)
diff --git a/Source/MQTTnet/Client/MqttClient.cs b/Source/MQTTnet/Client/MqttClient.cs
index 9dd7cb95..d519e78c 100644
--- a/Source/MQTTnet/Client/MqttClient.cs
+++ b/Source/MQTTnet/Client/MqttClient.cs
@@ -194,6 +194,11 @@ namespace MQTTnet.Client
         {
             if (options == null) throw new ArgumentNullException(nameof(options));
 
+            foreach (var topicFilter in options.TopicFilters)
+            {
+                MqttTopicValidator.ThrowIfInvalidSubscribe(topicFilter.Topic);
+            }
+
             ThrowIfDisposed();
             ThrowIfNotConnected();
 
diff --git a/Source/MQTTnet/Protocol/MqttTopicValidator.cs b/Source/MQTTnet/Protocol/MqttTopicValidator.cs
index 00b16793..b8f02950 100644
--- a/Source/MQTTnet/Protocol/MqttTopicValidator.cs
+++ b/Source/MQTTnet/Protocol/MqttTopicValidator.cs
@@ -42,5 +42,15 @@ namespace MQTTnet.Protocol
                 }
             }
         }
+
+        public static void ThrowIfInvalidSubscribe(string topic)
+        {
+            if (string.IsNullOrEmpty(topic))
+            {
+                throw new MqttProtocolViolationException("Topic should not be empty.");
+            }
+
+            if (topic.IndexOf("#") != -1 && topic.IndexOf("#") != topic.Length - 1) throw new MqttProtocolViolationException("The character '#' is only allowed as last character");
+        }
     }
 }
diff --git a/Source/MQTTnet/Server/MqttServer.cs b/Source/MQTTnet/Server/MqttServer.cs
index 1dbe39a7..92ef34f4 100644
--- a/Source/MQTTnet/Server/MqttServer.cs
+++ b/Source/MQTTnet/Server/MqttServer.cs
@@ -113,7 +113,12 @@ namespace MQTTnet.Server
         {
             if (clientId == null) throw new ArgumentNullException(nameof(clientId));
             if (topicFilters == null) throw new ArgumentNullException(nameof(topicFilters));
-            
+
+            foreach (var topicFilter in topicFilters)
+            {
+                MqttTopicValidator.ThrowIfInvalidSubscribe(topicFilter.Topic);
+            }
+
             ThrowIfDisposed();
             ThrowIfNotStarted();
 
diff --git a/Tests/MQTTnet.Core.Tests/MqttTopicValidatorSubscribe_Tests.cs b/Tests/MQTTnet.Core.Tests/MqttTopicValidatorSubscribe_Tests.cs
new file mode 100644
index 00000000..14797a0b
--- /dev/null
+++ b/Tests/MQTTnet.Core.Tests/MqttTopicValidatorSubscribe_Tests.cs
@@ -0,0 +1,54 @@
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using MQTTnet.Exceptions;
+using MQTTnet.Protocol;
+
+namespace MQTTnet.Tests
+{
+    [TestClass]
+    public class MqttTopicValidatorSubscribe_Tests
+    {
+        [TestMethod]
+        public void Valid_Topic()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("/a/b/c");
+        }
+
+        [TestMethod]
+        public void Valid_Topic_Plus_In_Between()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("/a/+/c");
+        }
+
+        [TestMethod]
+        public void Valid_Topic_Plus_Last_Char()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("/a/+");
+        }
+
+        [TestMethod]
+        public void Valid_Topic_Hash_Last_Char()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("/a/#");
+        }
+
+        [TestMethod]
+        public void Valid_Topic_Only_Hash()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("#");
+        }
+
+        [TestMethod]
+        [ExpectedException(typeof(MqttProtocolViolationException))]
+        public void Invalid_Topic_Hash_In_Between()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe("/a/#/c");
+        }
+
+        [TestMethod]
+        [ExpectedException(typeof(MqttProtocolViolationException))]
+        public void Invalid_Topic_Empty()
+        {
+            MqttTopicValidator.ThrowIfInvalidSubscribe(string.Empty);
+        }
+    }
+}