Clean up tftp update to use URL

Similar to transforms we've done elsewhere, we shouldn't be parsing
urls using std::string::find, regex, or anything else, as they don't
handle URL % encoding properly.

Change-Id: I48bb30c0c737c4df2ae73f40fc49c63bac5b658f
Signed-off-by: Ed Tanous <edtanous@google.com>

3 files changed, 125 insertions, 51 deletions

diff --git a/meson.build b/meson.build
index b79e849b32..d47bdb7e2d 100644
--- a/meson.build
+++ b/meson.build
@@ -455,6 +455,7 @@ srcfiles_unittest = files(
   'test/redfish-core/lib/sensors_test.cpp',
   'test/redfish-core/lib/log_services_dump_test.cpp',
   'test/redfish-core/lib/log_services_test.cpp',
+  'test/redfish-core/lib/update_service_test.cpp',
   'test/redfish-core/lib/service_root_test.cpp',
   'test/redfish-core/lib/thermal_subsystem_test.cpp',
   'test/redfish-core/lib/power_subsystem_test.cpp',
diff --git a/redfish-core/lib/update_service.hpp b/redfish-core/lib/update_service.hpp
index 9ab1a3b16b..9a07d3c334 100644
--- a/redfish-core/lib/update_service.hpp
+++ b/redfish-core/lib/update_service.hpp
@@ -433,6 +433,70 @@ static void monitorForSoftwareAvailable(
         std::bind_front(afterUpdateErrorMatcher, asyncResp, url));
 }
 
+struct TftpUrl
+{
+    std::string fwFile;
+    std::string tftpServer;
+};
+
+inline std::optional<TftpUrl>
+    parseTftpUrl(std::string imageURI,
+                 std::optional<std::string> transferProtocol,
+                 crow::Response& res)
+{
+    if (imageURI.find("://") == std::string::npos)
+    {
+        if (imageURI.starts_with("/"))
+        {
+            messages::actionParameterValueTypeError(
+                res, imageURI, "ImageURI", "UpdateService.SimpleUpdate");
+            return std::nullopt;
+        }
+        if (!transferProtocol)
+        {
+            messages::actionParameterValueTypeError(
+                res, imageURI, "ImageURI", "UpdateService.SimpleUpdate");
+            return std::nullopt;
+        }
+        // OpenBMC currently only supports TFTP
+        if (*transferProtocol != "TFTP")
+        {
+            messages::actionParameterNotSupported(res, "TransferProtocol",
+                                                  *transferProtocol);
+            BMCWEB_LOG_ERROR("Request incorrect protocol parameter: {}",
+                             *transferProtocol);
+            return std::nullopt;
+        }
+        imageURI = "tftp://" + imageURI;
+    }
+
+    boost::system::result<boost::urls::url> url =
+        boost::urls::parse_absolute_uri(imageURI);
+    if (!url)
+    {
+        messages::actionParameterValueTypeError(res, imageURI, "ImageURI",
+                                                "UpdateService.SimpleUpdate");
+
+        return std::nullopt;
+    }
+    url->normalize();
+
+    if (url->scheme() != "tftp")
+    {
+        messages::actionParameterNotSupported(res, "ImageURI", imageURI);
+        return std::nullopt;
+    }
+    std::string path(url->encoded_path());
+    if (path.size() < 2)
+    {
+        messages::actionParameterNotSupported(res, "ImageURI", imageURI);
+        return std::nullopt;
+    }
+    path.erase(0, 1);
+    std::string host(url->encoded_host_and_port());
+    return TftpUrl{path, host};
+}
+
 /**
  * UpdateServiceActionsSimpleUpdate class supports handle POST method for
  * SimpleUpdate action.
@@ -467,60 +531,14 @@ inline void requestRoutesUpdateServiceActionsSimpleUpdate(App& app)
             BMCWEB_LOG_DEBUG("Missing TransferProtocol or ImageURI parameter");
             return;
         }
-        if (!transferProtocol)
-        {
-            // Must be option 2
-            // Verify ImageURI has transfer protocol in it
-            size_t separator = imageURI.find(':');
-            if ((separator == std::string::npos) ||
-                ((separator + 1) > imageURI.size()))
-            {
-                messages::actionParameterValueTypeError(
-                    asyncResp->res, imageURI, "ImageURI",
-                    "UpdateService.SimpleUpdate");
-                BMCWEB_LOG_ERROR("ImageURI missing transfer protocol: {}",
-                                 imageURI);
-                return;
-            }
-            transferProtocol = imageURI.substr(0, separator);
-            // Ensure protocol is upper case for a common comparison path
-            // below
-            boost::to_upper(*transferProtocol);
-            BMCWEB_LOG_DEBUG("Encoded transfer protocol {}", *transferProtocol);
-
-            // Adjust imageURI to not have the protocol on it for parsing
-            // below
-            // ex. tftp://1.1.1.1/myfile.bin -> 1.1.1.1/myfile.bin
-            imageURI = imageURI.substr(separator + 3);
-            BMCWEB_LOG_DEBUG("Adjusted imageUri {}", imageURI);
-        }
-
-        // OpenBMC currently only supports TFTP
-        if (*transferProtocol != "TFTP")
-        {
-            messages::actionParameterNotSupported(asyncResp->res,
-                                                  "TransferProtocol",
-                                                  "UpdateService.SimpleUpdate");
-            BMCWEB_LOG_ERROR("Request incorrect protocol parameter: {}",
-                             *transferProtocol);
-            return;
-        }
-
-        // Format should be <IP or Hostname>/<file> for imageURI
-        size_t separator = imageURI.find('/');
-        if ((separator == std::string::npos) ||
-            ((separator + 1) > imageURI.size()))
+        std::optional<TftpUrl> ret = parseTftpUrl(imageURI, transferProtocol,
+                                                  asyncResp->res);
+        if (!ret)
         {
-            messages::actionParameterValueTypeError(
-                asyncResp->res, imageURI, "ImageURI",
-                "UpdateService.SimpleUpdate");
-            BMCWEB_LOG_ERROR("Invalid ImageURI: {}", imageURI);
             return;
         }
 
-        std::string tftpServer = imageURI.substr(0, separator);
-        std::string fwFile = imageURI.substr(separator + 1);
-        BMCWEB_LOG_DEBUG("Server: {}{}", tftpServer + " File: ", fwFile);
+        BMCWEB_LOG_DEBUG("Server: {} File: {}", ret->tftpServer, ret->fwFile);
 
         // Setup callback for when new software detected
         // Give TFTP 10 minutes to complete
@@ -552,7 +570,7 @@ inline void requestRoutesUpdateServiceActionsSimpleUpdate(App& app)
         },
             "xyz.openbmc_project.Software.Download",
             "/xyz/openbmc_project/software", "xyz.openbmc_project.Common.TFTP",
-            "DownloadViaTFTP", fwFile, tftpServer);
+            "DownloadViaTFTP", ret->fwFile, ret->tftpServer);
 
         BMCWEB_LOG_DEBUG("Exit UpdateService.SimpleUpdate doPost");
     });
diff --git a/test/redfish-core/lib/update_service_test.cpp b/test/redfish-core/lib/update_service_test.cpp
new file mode 100644
index 0000000000..01f2a0e7c9
--- /dev/null
+++ b/test/redfish-core/lib/update_service_test.cpp
@@ -0,0 +1,55 @@
+
+#include "update_service.hpp"
+
+#include <gtest/gtest.h>
+
+namespace redfish
+{
+namespace
+{
+
+TEST(UpdateService, ParseTFTPPostitive)
+{
+    crow::Response res;
+    {
+        // No protocol, schema on url
+        std::optional<TftpUrl> ret = parseTftpUrl("tftp://1.1.1.1/path",
+                                                  std::nullopt, res);
+        ASSERT_NE(ret, std::nullopt);
+        EXPECT_EQ(ret->tftpServer, "1.1.1.1");
+        EXPECT_EQ(ret->fwFile, "path");
+    }
+    {
+        // Protocol, no schema on url
+        std::optional<TftpUrl> ret = parseTftpUrl("1.1.1.1/path", "TFTP", res);
+        ASSERT_NE(ret, std::nullopt);
+        EXPECT_EQ(ret->tftpServer, "1.1.1.1");
+        EXPECT_EQ(ret->fwFile, "path");
+    }
+    {
+        // Both protocl and schema on url
+        std::optional<TftpUrl> ret = parseTftpUrl("tftp://1.1.1.1/path", "TFTP",
+                                                  res);
+        ASSERT_NE(ret, std::nullopt);
+        EXPECT_EQ(ret->tftpServer, "1.1.1.1");
+        EXPECT_EQ(ret->fwFile, "path");
+    }
+}
+
+TEST(UpdateService, ParseTFTPNegative)
+{
+    crow::Response res;
+    // No protocol, no schema
+    ASSERT_EQ(parseTftpUrl("1.1.1.1/path", std::nullopt, res), std::nullopt);
+    // No host
+    ASSERT_EQ(parseTftpUrl("/path", "TFTP", res), std::nullopt);
+
+    // No host
+    ASSERT_EQ(parseTftpUrl("path", "TFTP", res), std::nullopt);
+
+    // No path
+    ASSERT_EQ(parseTftpUrl("tftp://1.1.1.1", "TFTP", res), std::nullopt);
+    ASSERT_EQ(parseTftpUrl("tftp://1.1.1.1/", "TFTP", res), std::nullopt);
+}
+} // namespace
+} // namespace redfish