Security hardening: sanitize MQTT topics, fix bounds check, reduce lo…

src/EcoWitt.Controller.Tests/MqttPathBuilderTest.cs

@@ -74,20 +74,38 @@ public void BuildMqttSubdeviceHACommandTopic_Wildcard()
     }
 
     [Test]
-    public void Sanitize_SpacesToHyphens()
+    public void SanitizeSegment_SpacesToHyphens()
     {
-        Assert.That(MqttPathBuilder.Sanitize("My Gateway Name"), Is.EqualTo("my-gateway-name"));
+        Assert.That(MqttPathBuilder.SanitizeSegment("My Gateway Name"), Is.EqualTo("my-gateway-name"));
     }
 
     [Test]
-    public void Sanitize_ToLowercase()
+    public void SanitizeSegment_ToLowercase()
     {
-        Assert.That(MqttPathBuilder.Sanitize("GW2000A"), Is.EqualTo("gw2000a"));
+        Assert.That(MqttPathBuilder.SanitizeSegment("GW2000A"), Is.EqualTo("gw2000a"));
     }
 
     [Test]
-    public void Sanitize_AlreadyClean()
+    public void SanitizeSegment_StripsSlash()
     {
-        Assert.That(MqttPathBuilder.Sanitize("gw1/sensors/tempf"), Is.EqualTo("gw1/sensors/tempf"));
+        Assert.That(MqttPathBuilder.SanitizeSegment("valve/test"), Is.EqualTo("valve-test"));
+    }
+
+    [Test]
+    public void SanitizeSegment_StripsHash()
+    {
+        Assert.That(MqttPathBuilder.SanitizeSegment("sensor#1"), Is.EqualTo("sensor-1"));
+    }
+
+    [Test]
+    public void SanitizeSegment_StripsPlus()
+    {
+        Assert.That(MqttPathBuilder.SanitizeSegment("temp+humidity"), Is.EqualTo("temp-humidity"));
+    }
+
+    [Test]
+    public void SanitizeSegment_StripsAllWildcards()
+    {
+        Assert.That(MqttPathBuilder.SanitizeSegment("a/b+c#d E"), Is.EqualTo("a-b-c-d-e"));
     }
 }

src/Ecowitt.Controller.sln

@@ -10,7 +10,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution", "Solution", "{B3
 		..\diagrams.drawio = ..\diagrams.drawio
 		..\LICENSE = ..\LICENSE
 		..\README.md = ..\README.md
-		..\.github\workflows\main.yml = ..\.github\workflows\main.yml
 		..\CONTRIBUTING.md = ..\CONTRIBUTING.md
 	EndProjectSection
 EndProject
@@ -31,6 +30,12 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Docs", "Docs", "{02EA681E-C
 		..\docs\architecture.md = ..\docs\architecture.md
 	EndProjectSection
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Github", "Github", "{BEB106CA-DDE3-4277-934E-A7E2910187D7}"
+	ProjectSection(SolutionItems) = preProject
+		..\.github\workflows\dev.yml = ..\.github\workflows\dev.yml
+		..\.github\workflows\main.yml = ..\.github\workflows\main.yml
+	EndProjectSection
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU

src/Ecowitt.Controller/Ecowitt.Controller.csproj

@@ -20,7 +20,6 @@
     </ItemGroup>
 
     <ItemGroup>
-        <PackageReference Include="FluentValidation.DependencyInjectionExtensions" Version="12.1.1" />
         <PackageReference Include="Microsoft.Extensions.Http.Polly" Version="10.0.3" />
         <PackageReference Include="Microsoft.VisualStudio.Azure.Containers.Tools.Targets" Version="1.23.0" />
         <PackageReference Include="MQTTnet" Version="5.1.0.1559" />
@@ -36,11 +35,8 @@
         <PackageReference Include="SlimMessageBus" Version="3.0.0" />
         <PackageReference Include="SlimMessageBus.Host" Version="3.3.6" />
         <PackageReference Include="SlimMessageBus.Host.AspNetCore" Version="3.3.6" />
-        <PackageReference Include="SlimMessageBus.Host.FluentValidation" Version="3.3.6" />
         <PackageReference Include="SlimMessageBus.Host.Memory" Version="3.3.6" />
         <PackageReference Include="SlimMessageBus.Host.Serialization.Json" Version="3.3.6" />
-        <PackageReference Include="Swashbuckle.AspNetCore" Version="10.1.4" />
     </ItemGroup>
 
-
 </Project>

src/Ecowitt.Controller/Service/Mqtt/MqttPathBuilder.cs

@@ -4,57 +4,61 @@ public static class MqttPathBuilder
 {
     public static string BuildMqttGatewayTopic(string gwName)
     {
-        return Sanitize(gwName);
+        return SanitizeSegment(gwName);
     }
 
     public static string BuildMqttGatewaySensorTopic(string gwName, string sensorName)
     {
-        return Sanitize($"{gwName}/sensors/{sensorName}");
+        return $"{SanitizeSegment(gwName)}/sensors/{SanitizeSegment(sensorName)}";
     }
 
     public static string BuildMqttGatewayDiagnosticTopic(string gwName, string sensorName)
     {
-        return Sanitize($"{gwName}/diag/{sensorName}");
+        return $"{SanitizeSegment(gwName)}/diag/{SanitizeSegment(sensorName)}";
     }
 
     public static string BuildMqttSubdeviceTopic(string gwName, string subdeviceName)
     {
-        return Sanitize($"{gwName}/subdevices/{subdeviceName}");
+        return $"{SanitizeSegment(gwName)}/subdevices/{SanitizeSegment(subdeviceName)}";
     }
 
     public static string BuildMqttSubdeviceSensorTopic(string gwName, string subdeviceName, string sensorName)
     {
-        return Sanitize($"{gwName}/subdevices/{subdeviceName}/sensors/{sensorName}");
+        return $"{SanitizeSegment(gwName)}/subdevices/{SanitizeSegment(subdeviceName)}/sensors/{SanitizeSegment(sensorName)}";
     }
 
-
     public static string BuildMqttSubdeviceDiagnosticTopic(string gwName, string subdeviceName, string sensorName)
     {
-        return Sanitize($"{gwName}/subdevices/{subdeviceName}/diag/{sensorName}");
+        return $"{SanitizeSegment(gwName)}/subdevices/{SanitizeSegment(subdeviceName)}/diag/{SanitizeSegment(sensorName)}";
     }
 
     public static string BuildMqttSubdeviceCommandTopic(string gwName, string subdeviceName)
     {
-        return Sanitize($"{gwName}/subdevices/{subdeviceName}/cmd");
+        return $"{SanitizeSegment(gwName)}/subdevices/{SanitizeSegment(subdeviceName)}/cmd";
     }
 
     public static string BuildMqttSubdeviceCommandTopic()
     {
-        return Sanitize($"+/subdevices/+/cmd");
+        return "+/subdevices/+/cmd";
     }
 
     public static string BuildMqttSubdeviceHACommandTopic(string gwName, string subdeviceName)
     {
-        return Sanitize($"{gwName}/subdevices/{subdeviceName}/cmd/homeassistant");
+        return $"{SanitizeSegment(gwName)}/subdevices/{SanitizeSegment(subdeviceName)}/cmd/homeassistant";
     }
 
     public static string BuildMqttSubdeviceHACommandTopic()
     {
-        return Sanitize($"+/subdevices/+/cmd/homeassistant");
+        return "+/subdevices/+/cmd/homeassistant";
     }
 
-    public static string Sanitize(string input)
+    public static string SanitizeSegment(string input)
     {
-        return input.Replace(' ', '-').ToLowerInvariant();
+        return input
+            .Replace(' ', '-')
+            .Replace('/', '-')
+            .Replace('#', '-')
+            .Replace('+', '-')
+            .ToLowerInvariant();
     }
-}
+}

src/Ecowitt.Controller/Service/Mqtt/MqttService.Consumer.cs

@@ -66,7 +66,7 @@ public async Task OnHandle(DiscoveryRemovalEvent message, CancellationToken canc
             foreach (var sensor in message.Sensors)
             {
                 var sensorClassTopic = BuildSensorClassTopic(sensor.SensorClass);
-                var topic = $"homeassistant/{MqttPathBuilder.Sanitize($"{sensorClassTopic}/{message.DeviceName}_{sensor.Name}")}/config";
+                var topic = $"homeassistant/{sensorClassTopic}/{MqttPathBuilder.SanitizeSegment($"{message.DeviceName}_{sensor.Name}")}/config";
                 await RemoveDiscoveryMessage(topic);
                 _logger.LogInformation("Removed discovery for {DeviceName}/{SensorName}", message.DeviceName, sensor.Name);
             }

src/Ecowitt.Controller/Service/Mqtt/MqttService.DiscoveryPublisher.cs

@@ -21,7 +21,7 @@ private async Task PublishGatewayDiscovery(Device gw)
 
             var config = DiscoveryBuilder.BuildGatewayConfig(device, _origin, "Availability", id, availabilityTopic, availabilityTopic);
 
-            await PublishDiscoveryMessage("sensor", MqttPathBuilder.Sanitize($"sensor/{gw.Name}"), config);
+            await PublishDiscoveryMessage("sensor", $"sensor/{MqttPathBuilder.SanitizeSegment(gw.Name)}", config);
         }
 
         private async Task PublishSubdeviceDiscovery(Device gw, Ecowitt.Controller.Model.Subdevice subdevice)
@@ -33,7 +33,7 @@ private async Task PublishSubdeviceDiscovery(Device gw, Ecowitt.Controller.Model
 
             var config = DiscoveryBuilder.BuildGatewayConfig(device, _origin, "Availability", id, availabilityTopic, availabilityTopic);
 
-            await PublishDiscoveryMessage("sensor", MqttPathBuilder.Sanitize($"sensor/{subdevice.Nickname}"), config);
+            await PublishDiscoveryMessage("sensor", $"sensor/{MqttPathBuilder.SanitizeSegment(subdevice.Nickname)}", config);
         }
 
         private async Task PublishSubdeviceSwitchDiscovery(Device gw, Ecowitt.Controller.Model.Subdevice subdevice)
@@ -47,7 +47,7 @@ private async Task PublishSubdeviceSwitchDiscovery(Device gw, Ecowitt.Controller
             var config =
                 DiscoveryBuilder.BuildSwitchConfig(device, _origin, "switch", id, statetopic, cmdTopic, valueTemplate: valueTemplate);
 
-            await PublishDiscoveryMessage("switch", MqttPathBuilder.Sanitize($"switch/{subdevice.Nickname}"), config);
+            await PublishDiscoveryMessage("switch", $"switch/{MqttPathBuilder.SanitizeSegment(subdevice.Nickname)}", config);
         }
 
         private async Task PublishSensorDiscovery(Device gw, ISensor sensor)
@@ -83,7 +83,7 @@ private async Task PublishSensorDiscovery(Model.Discovery.Device device, ISensor
 
             var sensorClassTopic = BuildSensorClassTopic(sensor.SensorClass);
 
-            await PublishDiscoveryMessage(sensorClassTopic, MqttPathBuilder.Sanitize($"{sensorClassTopic}/{device.Name}_{sensor.Name}"), config);
+            await PublishDiscoveryMessage(sensorClassTopic, $"{sensorClassTopic}/{MqttPathBuilder.SanitizeSegment($"{device.Name}_{sensor.Name}")}", config);
         }
 
         private async Task PublishDiscoveryMessage(string domain, string topic, Config config)

src/Ecowitt.Controller/Service/Mqtt/MqttService.Events.cs

@@ -44,7 +44,11 @@ private async Task ClientOnApplicationMessageReceivedAsync(MqttApplicationMessag
             else if (topic.EndsWith("cmd/homeassistant"))
             {
                 // commands coming from home assistant
-                if (int.TryParse(topic.Split('/')[3], out var result))
+                // find the subdevice id relative to the "subdevices" segment, not a fixed index,
+                // so multi-segment base topics (e.g. "home/ecowitt") don't break parsing
+                var parts = topic.Split('/');
+                var subdevicesIndex = Array.IndexOf(parts, "subdevices");
+                if (subdevicesIndex >= 0 && subdevicesIndex + 1 < parts.Length && int.TryParse(parts[subdevicesIndex + 1], out var result))
                 {
                     var cmd = payload.Equals("ON", StringComparison.InvariantCultureIgnoreCase) ? Command.Start : Command.Stop; //I know, everything that's not "ON" is "OFF"
                     await _messageBus.Publish(new SubdeviceApiCommand() { Cmd = cmd, Id = result });

src/Ecowitt.Controller/Service/Mqtt/MqttService.Publisher.cs

@@ -76,9 +76,9 @@ private async Task<bool> Publish(string topic, string message, bool retain = fal
                     _logger.LogDebug("Message {message} ", message);
                     return true;
                 }
-                else _logger.LogWarning("Failed to publish message {Message} to topic {Topic}. Reason: {MqttClientPublishReasonCode}", message, topic, result.ReasonCode);
+                else _logger.LogWarning("Failed to publish message to topic {Topic}. Reason: {MqttClientPublishReasonCode}", topic, result.ReasonCode);
             }
-            else _logger.LogWarning("Can't publish message {Message} to topic {Topic}. Client not connected.", message, topic);
+            else _logger.LogWarning("Can't publish message to topic {Topic}. Client not connected.", topic);
             return false;
         }
     }