Remove getNthStringFromPath function
This utility function is being removed for several reasons. First, it
does not verify the full string on URIs and paths, so things like
/foo/bar/baz/valid_id would still pass this check.
Second, it is used for both URIs and dbus paths, both of which we have
better utility functions these days respectively, boost::url for urls
and sdbusplus::message::object_path for dbus paths. Neither of the two
is escaped properly when this function is used.
Therefore, remove it and replace it with the appropriate alternatives.
The existing URI functions were found to not accept fragments (given
they are rarely used in PATCH). Add support for fragments to cover the
getNthStringFromPath use cases.
Tested: Redfish service validator passes.
Change-Id: Ibc6755ad69397123d7fef0e0b764042bbb48888b
Signed-off-by: Ed Tanous <etanous@nvidia.com>
diff --git a/http/utility.hpp b/http/utility.hpp
index 5be9ac5..e19e258 100644
--- a/http/utility.hpp
+++ b/http/utility.hpp
@@ -4,6 +4,8 @@
#include "bmcweb_config.h"
+#include "str_utility.hpp"
+
#include <sys/types.h>
#include <boost/url/segments_view.hpp>
@@ -392,14 +394,45 @@
boost::urls::segments_view::const_iterator it = urlSegments.begin();
boost::urls::segments_view::const_iterator end = urlSegments.end();
+ std::string fragment = url.fragment();
+ std::vector<std::string> fragmentParts;
+ bmcweb::split(fragmentParts, fragment, '/');
+ auto fragIt = fragmentParts.begin();
+ auto fragEnd = fragmentParts.end();
+
+ // Url fragments start with a /, so we need to skip the first empty string
+ if (fragIt != fragEnd)
+ {
+ if (fragIt->empty())
+ {
+ fragIt++;
+ }
+ }
+
+ // There will be an empty segment at the end if the URI ends with a "/"
+ // e.g. /redfish/v1/Chassis/
+ if ((it != end) && urlSegments.back().empty())
+ {
+ end--;
+ }
+
for (const auto& segment : segments)
{
+ UrlParseResult res = UrlParseResult::Fail;
if (it == end)
{
- // If the request ends with an "any" path, this was successful
- return std::holds_alternative<OrMorePaths>(segment);
+ if (fragIt == fragEnd)
+ {
+ return std::holds_alternative<OrMorePaths>(segment);
+ }
+ res = std::visit(UrlSegmentMatcherVisitor(*fragIt), segment);
+ fragIt++;
}
- UrlParseResult res = std::visit(UrlSegmentMatcherVisitor(*it), segment);
+ else
+ {
+ res = std::visit(UrlSegmentMatcherVisitor(*it), segment);
+ it++;
+ }
if (res == UrlParseResult::Done)
{
return true;
@@ -408,15 +441,8 @@
{
return false;
}
- it++;
}
- // There will be an empty segment at the end if the URI ends with a "/"
- // e.g. /redfish/v1/Chassis/
- if ((it != end) && urlSegments.back().empty())
- {
- it++;
- }
return it == end;
}
diff --git a/include/dbus_utility.hpp b/include/dbus_utility.hpp
index 4eedcbf..407909a 100644
--- a/include/dbus_utility.hpp
+++ b/include/dbus_utility.hpp
@@ -91,11 +91,6 @@
void logError(const boost::system::error_code& ec);
-// gets the string N strings deep into a path
-// i.e. /0th/1st/2nd/3rd
-bool getNthStringFromPath(const std::string& path, int index,
- std::string& result);
-
void getAllProperties(const std::string& service, const std::string& objectPath,
const std::string& interface,
std::function<void(const boost::system::error_code&,
diff --git a/redfish-core/lib/openbmc/openbmc_managers.hpp b/redfish-core/lib/openbmc/openbmc_managers.hpp
index 6bd52e5..6601aa5 100644
--- a/redfish-core/lib/openbmc/openbmc_managers.hpp
+++ b/redfish-core/lib/openbmc/openbmc_managers.hpp
@@ -179,9 +179,10 @@
BMCWEB_REDFISH_MANAGER_URI_NAME));
if (intfPair.first == pidZoneConfigurationIface)
{
- std::string chassis;
- if (!dbus::utility::getNthStringFromPath(
- pathPair.first.str, 5, chassis))
+ sdbusplus::message::object_path pidPath(
+ pathPair.first.str);
+ std::string chassis = pidPath.filename();
+ if (chassis.empty())
{
chassis = "#IllegalValue";
}
@@ -513,14 +514,11 @@
{
return nullptr;
}
- // 5 comes from <chassis-name> being the 5th element
// /xyz/openbmc_project/inventory/system/chassis/<chassis-name>
- if (dbus::utility::getNthStringFromPath(it->first.str, 5, chassis))
- {
- return &(*it);
- }
+ sdbusplus::message::object_path path(it->first.str);
+ chassis = path.filename();
- return nullptr;
+ return &(*it);
}
inline bool getZonesFromJsonReq(
@@ -542,12 +540,24 @@
{
return false;
}
+
+ boost::system::result<boost::urls::url_view> parsed =
+ boost::urls::parse_relative_ref(path);
+ if (!parsed)
+ {
+ BMCWEB_LOG_WARNING("Got invalid path {}", path);
+ messages::propertyValueFormatError(response->res, path, "Zones");
+ return false;
+ }
+
std::string input;
// 8 below comes from
// /redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Left
- // 0 1 2 3 4 5 6 7 8
- if (!dbus::utility::getNthStringFromPath(path, 8, input))
+ std::string managerId;
+ if (!crow::utility::readUrlSegments(
+ *parsed, "redfish", "v1", "Managers", std::ref(managerId),
+ "Oem", "OpenBmc", "Fan", "FanZones", std::ref(input)))
{
BMCWEB_LOG_ERROR("Got invalid path {}", path);
BMCWEB_LOG_ERROR("Illegal Type Zones");
@@ -819,16 +829,26 @@
if (chassisId)
{
- // /redfish/v1/chassis/chassis_name/
- if (!dbus::utility::getNthStringFromPath(*chassisId, 3, chassis))
+ boost::system::result<boost::urls::url_view> parsed =
+ boost::urls::parse_relative_ref(*chassisId);
+ if (!parsed)
{
- BMCWEB_LOG_ERROR("Got invalid path {}", *chassisId);
- messages::invalidObject(
- response->res,
- boost::urls::format("/redfish/v1/Chassis/{}", *chassisId));
+ BMCWEB_LOG_WARNING("Got invalid path {}", *chassisId);
+ messages::propertyValueFormatError(response->res, *chassisId,
+ "Chassis/@odata.id");
+ return CreatePIDRet::fail;
+ }
+
+ if (!crow::utility::readUrlSegments(*parsed, "redfish", "v1",
+ "Chassis", std::ref(chassis)))
+ {
+ BMCWEB_LOG_WARNING("Got invalid path {}", *parsed);
+ messages::propertyValueFormatError(response->res, *chassisId,
+ "Chassis/@odata.id");
return CreatePIDRet::fail;
}
}
+
if (minThermalOutput)
{
output.emplace_back("MinThermalOutput", *minThermalOutput);
diff --git a/src/dbus_utility.cpp b/src/dbus_utility.cpp
index ecd0dec..76ce82d 100644
--- a/src/dbus_utility.cpp
+++ b/src/dbus_utility.cpp
@@ -42,33 +42,6 @@
}
}
-// gets the string N strings deep into a path
-// i.e. /0th/1st/2nd/3rd
-bool getNthStringFromPath(const std::string& path, int index,
- std::string& result)
-{
- if (index < 0)
- {
- return false;
- }
-
- std::filesystem::path p1(path);
- int count = -1;
- for (const auto& element : p1)
- {
- if (element.has_filename())
- {
- ++count;
- if (count == index)
- {
- result = element.stem().string();
- break;
- }
- }
- }
- return count >= index;
-}
-
void getAllProperties(const std::string& service, const std::string& objectPath,
const std::string& interface,
std::function<void(const boost::system::error_code&,
diff --git a/test/http/utility_test.cpp b/test/http/utility_test.cpp
index 1a99d6d..69887e8 100644
--- a/test/http/utility_test.cpp
+++ b/test/http/utility_test.cpp
@@ -108,7 +108,7 @@
TEST(Utility, readUrlSegments)
{
boost::system::result<boost::urls::url_view> parsed =
- boost::urls::parse_relative_ref("/redfish/v1/Chassis#/Fans/0/Reading");
+ boost::urls::parse_relative_ref("/redfish/v1/Chassis");
EXPECT_TRUE(readUrlSegments(*parsed, "redfish", "v1", "Chassis"));
@@ -164,6 +164,19 @@
EXPECT_TRUE(readUrlSegments(*parsed, OrMorePaths()));
}
+TEST(Utility, readUrlSegmentsManager)
+{
+ boost::urls::url_view url(
+ "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Left");
+ std::string managerId;
+ std::string input;
+ EXPECT_TRUE(
+ readUrlSegments(url, "redfish", "v1", "Managers", std::ref(managerId),
+ "Oem", "OpenBmc", "Fan", "FanZones", std::ref(input)));
+ EXPECT_EQ(managerId, "bmc");
+ EXPECT_EQ(input, "Left");
+}
+
TEST(Router, ParameterTagging)
{
EXPECT_EQ(1, getParameterTag("<str>"));
diff --git a/test/include/dbus_utility_test.cpp b/test/include/dbus_utility_test.cpp
deleted file mode 100644
index 1b0564f..0000000
--- a/test/include/dbus_utility_test.cpp
+++ /dev/null
@@ -1,46 +0,0 @@
-// SPDX-License-Identifier: Apache-2.0
-// SPDX-FileCopyrightText: Copyright OpenBMC Authors
-#include "dbus_utility.hpp"
-
-#include <string>
-
-#include <gtest/gtest.h>
-
-namespace dbus::utility
-{
-namespace
-{
-
-TEST(GetNthStringFromPath, ParsingSucceedsAndReturnsNthArg)
-{
- std::string path("/0th/1st/2nd/3rd");
- std::string result;
- EXPECT_TRUE(getNthStringFromPath(path, 0, result));
- EXPECT_EQ(result, "0th");
- EXPECT_TRUE(getNthStringFromPath(path, 1, result));
- EXPECT_EQ(result, "1st");
- EXPECT_TRUE(getNthStringFromPath(path, 2, result));
- EXPECT_EQ(result, "2nd");
- EXPECT_TRUE(getNthStringFromPath(path, 3, result));
- EXPECT_EQ(result, "3rd");
- EXPECT_FALSE(getNthStringFromPath(path, 4, result));
-
- path = "////0th///1st//\2nd///3rd?/";
- EXPECT_TRUE(getNthStringFromPath(path, 0, result));
- EXPECT_EQ(result, "0th");
- EXPECT_TRUE(getNthStringFromPath(path, 1, result));
- EXPECT_EQ(result, "1st");
- EXPECT_TRUE(getNthStringFromPath(path, 2, result));
- EXPECT_EQ(result, "\2nd");
- EXPECT_TRUE(getNthStringFromPath(path, 3, result));
- EXPECT_EQ(result, "3rd?");
-}
-
-TEST(GetNthStringFromPath, InvalidIndexReturnsFalse)
-{
- std::string path("////0th///1st//\2nd///3rd?/");
- std::string result;
- EXPECT_FALSE(getNthStringFromPath(path, -1, result));
-}
-} // namespace
-} // namespace dbus::utility
diff --git a/test/meson.build b/test/meson.build
index f66bc52..344a26f 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -14,7 +14,6 @@
'http/zstd_decompressor_test.cpp',
'include/async_resolve_test.cpp',
'include/credential_pipe_test.cpp',
- 'include/dbus_utility_test.cpp',
'include/http_utility_test.cpp',
'include/human_sort_test.cpp',
'include/json_html_serializer.cpp',