fix(upnp): accept disconnected IGD status from MiniUPnPc 2.2.8+ (#5913)

src/upnp.cpp

@@ -39,16 +39,22 @@ namespace upnp {
     std::string description;  ///< Human-readable UPnP lease description advertised to the gateway.
   };
 
-  static std::string_view status_string(int status) {
+  static std::string_view status_string(igd_status_e status) {
+    using enum igd_status_e;
+
     switch (status) {
-      case 0:
+      case none:
         return "No IGD device found"sv;
-      case 1:
+      case connected:
         return "Valid IGD device found"sv;
-      case 2:
-        return "Valid IGD device found,  but it isn't connected"sv;
-      case 3:
-        return "A UPnP device has been found,  but it wasn't recognized as an IGD"sv;
+#if (MINIUPNPC_API_VERSION >= 18)
+      case private_ip:
+        return "Valid IGD device found, but it has a private IP address"sv;
+#endif
+      case disconnected:
+        return "Valid IGD device found, but it isn't connected"sv;
+      case unknown_device:
+        return "A UPnP device has been found, but it wasn't recognized as an IGD"sv;
     }
 
     return "Unknown status"sv;
@@ -58,12 +64,28 @@ namespace upnp {
    * This function is a wrapper around UPNP_GetValidIGD() that returns the status code. There is a pre-processor
    * check to determine which version of the function to call based on the version of the MiniUPnPc library.
    */
-  int UPNP_GetValidIGDStatus(device_t &device, urls_t *urls, IGDdatas *data, std::array<char, INET6_ADDRESS_STRLEN> &lan_addr) {
+  igd_status_e UPNP_GetValidIGDStatus(device_t &device, urls_t *urls, IGDdatas *data, std::array<char, INET6_ADDRESS_STRLEN> &lan_addr) {
 #if (MINIUPNPC_API_VERSION >= 18)
-    return UPNP_GetValidIGD(device.get(), &urls->el, data, lan_addr.data(), (int) lan_addr.size(), nullptr, 0);
+    auto status = UPNP_GetValidIGD(device.get(), &urls->el, data, lan_addr.data(), (int) lan_addr.size(), nullptr, 0);
 #else
-    return UPNP_GetValidIGD(device.get(), &urls->el, data, lan_addr.data(), (int) lan_addr.size());
+    auto status = UPNP_GetValidIGD(device.get(), &urls->el, data, lan_addr.data(), (int) lan_addr.size());
 #endif
+    return static_cast<igd_status_e>(status);
+  }
+
+  bool is_valid_igd(igd_status_e status) {
+    using enum igd_status_e;
+
+    switch (status) {
+      case connected:
+#if (MINIUPNPC_API_VERSION >= 18)
+      case private_ip:
+#endif
+      case disconnected:
+        return true;
+      default:
+        return false;
+    }
   }
 
   /**
@@ -122,7 +144,7 @@ namespace upnp {
       urls_t urls;
       std::array<char, INET6_ADDRESS_STRLEN> lan_addr;
       auto status = upnp::UPNP_GetValidIGDStatus(device, &urls, &data, lan_addr);
-      if (status != 1 && status != 2) {
+      if (!is_valid_igd(status)) {
         BOOST_LOG(debug) << "No valid IPv6 IGD: "sv << status_string(status);
         return false;
       }
@@ -336,7 +358,7 @@ namespace upnp {
 
         urls_t urls;
         auto status = upnp::UPNP_GetValidIGDStatus(device, &urls, &data, lan_addr);
-        if (status != 1 && status != 2) {
+        if (!is_valid_igd(status)) {
           BOOST_LOG(error) << status_string(status);
           mapped = false;
           continue;

src/upnp.h

@@ -29,19 +29,48 @@ namespace upnp {
     FreeUPNPUrls(&el);
   });  ///< Alias for element type.
 
+  /**
+   * @brief Result codes returned by the installed MiniUPnPc library.
+   * @details Project-owned equivalent of the UPNP_GetValidIGD() result codes. MiniUPnPc 2.2.8
+   * (API version 18) introduced the private-address result, renumbering the values that follow it;
+   * the library's own UPNP_* symbolic names only exist starting with MiniUPnPc 2.3.0 (API version 19),
+   * so this enum centralizes the version-dependent numbering instead of relying on those names.
+   */
+  enum class igd_status_e : int {
+    none = 0,  ///< No IGD was found.
+    connected = 1,  ///< A connected IGD was found.
+#if (MINIUPNPC_API_VERSION >= 18)
+    private_ip = 2,  ///< A connected IGD with a reserved address was found.
+#endif
+    disconnected,  ///< A disconnected IGD was found.
+    unknown_device,  ///< The device was not recognized as an IGD.
+  };
+
+  /**
+   * @brief Check whether a UPNP_GetValidIGD() status code identifies a usable IGD.
+   * @details Accepts every status code that identifies a valid IGD (connected, private-IP, or
+   * disconnected). The private_ip member only exists with MiniUPnPc >= 2.2.8 (API version 18);
+   * disconnected is 2 before API 18 and 3 starting with API 18, and unknown_device is 3 or 4
+   * respectively. Callers don't need to handle the version difference themselves.
+   * @param status The status returned by UPNP_GetValidIGDStatus(), converted to igd_status_e.
+   * @return `true` if the status identifies a usable IGD, `false` otherwise.
+   */
+  [[nodiscard]] bool is_valid_igd(igd_status_e status);
+
   /**
    * @brief Get the valid IGD status.
    * @param device The device.
    * @param urls The URLs.
    * @param data The IGD data.
    * @param lan_addr The LAN address.
-   * @return The UPnP Status.
-   * @retval 0 No IGD found.
-   * @retval 1 A valid connected IGD has been found.
-   * @retval 2 A valid IGD has been found but it reported as not connected.
-   * @retval 3 An UPnP device has been found but was not recognized as an IGD.
+   * @return The IGD status as an igd_status_e value, converted from the raw UPNP_GetValidIGD() result.
+   * @retval igd_status_e::none No IGD found.
+   * @retval igd_status_e::connected A valid connected IGD has been found.
+   * @retval igd_status_e::private_ip A valid connected IGD with a private IP address (MiniUPnPc >= 2.2.8 only).
+   * @retval igd_status_e::disconnected A valid IGD has been found but it reported as not connected.
+   * @retval igd_status_e::unknown_device An UPnP device has been found but was not recognized as an IGD.
    */
-  int UPNP_GetValidIGDStatus(device_t &device, urls_t *urls, IGDdatas *data, std::array<char, INET6_ADDRESS_STRLEN> &lan_addr);
+  igd_status_e UPNP_GetValidIGDStatus(device_t &device, urls_t *urls, IGDdatas *data, std::array<char, INET6_ADDRESS_STRLEN> &lan_addr);
 
   /**
    * @brief Start UPnP port mapping and return its shutdown guard.

tests/unit/test_upnp.cpp

@@ -0,0 +1,42 @@
+/**
+ * @file tests/unit/test_upnp.cpp
+ * @brief Test src/upnp.*
+ */
+
+// test includes
+#include "../tests_common.h"
+
+// local includes
+#include <src/upnp.h>
+
+TEST(UpnpIsValidIgdTest, RejectsNoIgd) {
+  ASSERT_FALSE(upnp::is_valid_igd(upnp::igd_status_e::none));
+}
+
+TEST(UpnpIsValidIgdTest, AcceptsConnectedIgd) {
+  ASSERT_TRUE(upnp::is_valid_igd(upnp::igd_status_e::connected));
+}
+
+#if (MINIUPNPC_API_VERSION >= 18)
+TEST(UpnpIsValidIgdTest, AcceptsPrivateIpIgd) {
+  // MiniUPnPc 2.2.8+ reports a valid connected IGD with a private IP address as igd_status_e::private_ip.
+  ASSERT_TRUE(upnp::is_valid_igd(upnp::igd_status_e::private_ip));
+}
+#endif
+
+TEST(UpnpIsValidIgdTest, AcceptsDisconnectedIgd) {
+  // Regression test for #5462: MiniUPnPc 2.2.8+ reports a valid but disconnected IGD as
+  // igd_status_e::disconnected. The old check (status == 1 || status == 2) rejected it, so UPnP port
+  // mapping silently failed for routers advertising a valid IGD with NewConnectionStatus=Disconnected.
+  ASSERT_TRUE(upnp::is_valid_igd(upnp::igd_status_e::disconnected));
+}
+
+TEST(UpnpIsValidIgdTest, RejectsUnknownDevice) {
+  ASSERT_FALSE(upnp::is_valid_igd(upnp::igd_status_e::unknown_device));
+}
+
+TEST(UpnpIsValidIgdTest, RejectsOutOfRange) {
+  ASSERT_FALSE(upnp::is_valid_igd(static_cast<upnp::igd_status_e>(-1)));
+  ASSERT_FALSE(upnp::is_valid_igd(static_cast<upnp::igd_status_e>(5)));
+  ASSERT_FALSE(upnp::is_valid_igd(static_cast<upnp::igd_status_e>(100)));
+}