Remove regex uses in event service and consolidate
As the patch at
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/50994 can attest,
parsing urls with a regex is error prone. We should avoid it where
possible, and we have boost::urls that implements a full, correct, and
unit tested parser.
Ideally, eventually this helper function would devolve into just the
parse_uri, and setting defaults portion, and we could rely on the
boost::urls::url class to pass into things like http_client.
As a side note, because boost url implements port as a proper type-safe
uint16, some interfaces that previously accepted port by std::string&
needed to be modified, and is included in this patch.
Also, once moved, the branch on the ifdef for HTTP push support was
failing a clang-tidy validation. This is a known limitation of using
ifdefs for our code, and something we've solved with the header file, so
move the http push enabler to the header file.
Also note that given this reorganization, two EXPECT statements are
added to the unit tests for user input behaviors that the old code
previously did not handle properly.
Tested: Unit tests passing
Ran Redfish-Event-Listener, saw subscription create properly:
Subcription is successful for https://192.168.7.2, /redfish/v1/EventService/Subscriptions/2197426973
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia4127c6cbcde6002fe8a50348792024d1d615e8f
diff --git a/bmcweb_config.h.in b/bmcweb_config.h.in
index a8e4ccb..b0a3a7b 100644
--- a/bmcweb_config.h.in
+++ b/bmcweb_config.h.in
@@ -13,4 +13,5 @@
constexpr const char* mesonInstallPrefix = "@MESON_INSTALL_PREFIX@";
+constexpr const bool bmcwebInsecureEnableHttpPushStyleEventing = @BMCWEB_INSECURE_ENABLE_HTTP_PUSH_STYLE_EVENTING@ == 1;
// clang-format on
diff --git a/http/http_client.hpp b/http/http_client.hpp
index 1664d01..342ed1b 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -76,7 +76,7 @@
std::string subId;
std::string host;
- std::string port;
+ uint16_t port = 0;
uint32_t retryCount = 0;
uint32_t maxRetryAttempts = 5;
uint32_t retryIntervalSecs = 0;
@@ -380,7 +380,7 @@
public:
explicit HttpClient(boost::asio::io_context& ioc, const std::string& id,
- const std::string& destIP, const std::string& destPort,
+ const std::string& destIP, uint16_t destPort,
const std::string& destUri,
const boost::beast::http::fields& httpHeader) :
conn(ioc),
diff --git a/http/ut/utility_test.cpp b/http/ut/utility_test.cpp
index 88743e0..bab21f9 100644
--- a/http/ut/utility_test.cpp
+++ b/http/ut/utility_test.cpp
@@ -187,13 +187,13 @@
using crow::utility::validateAndSplitUrl;
std::string host;
std::string urlProto;
- std::string port;
+ uint16_t port = 0;
std::string path;
ASSERT_TRUE(validateAndSplitUrl("https://foo.com:18080/bar", urlProto, host,
port, path));
EXPECT_EQ(host, "foo.com");
EXPECT_EQ(urlProto, "https");
- EXPECT_EQ(port, "18080");
+ EXPECT_EQ(port, 18080);
EXPECT_EQ(path, "/bar");
@@ -202,21 +202,30 @@
urlProto, host, port, path));
EXPECT_EQ(path, "/bar?foobar=1");
+ // fragment
+ ASSERT_TRUE(validateAndSplitUrl("https://foo.com:18080/bar#frag", urlProto,
+ host, port, path));
+ EXPECT_EQ(path, "/bar#frag");
+
// Missing port
ASSERT_TRUE(
validateAndSplitUrl("https://foo.com/bar", urlProto, host, port, path));
- EXPECT_EQ(port, "443");
+ EXPECT_EQ(port, 443);
- // If http push eventing is allowed, allow http, if it's not, parse
- // should fail.
-#ifdef BMCWEB_INSECURE_ENABLE_HTTP_PUSH_STYLE_EVENTING
+ // Missing path defaults to "/"
ASSERT_TRUE(
- validateAndSplitUrl("http://foo.com/bar", urlProto, host, port, path));
- EXPECT_EQ(port, "80");
-#else
- ASSERT_FALSE(
- validateAndSplitUrl("http://foo.com/bar", urlProto, host, port, path));
-#endif
+ validateAndSplitUrl("https://foo.com/", urlProto, host, port, path));
+ EXPECT_EQ(path, "/");
+
+ // If http push eventing is allowed, allow http and pick a default port of
+ // 80, if it's not, parse should fail.
+ ASSERT_EQ(
+ validateAndSplitUrl("http://foo.com/bar", urlProto, host, port, path),
+ bmcwebInsecureEnableHttpPushStyleEventing);
+ if constexpr (bmcwebInsecureEnableHttpPushStyleEventing)
+ {
+ EXPECT_EQ(port, 80);
+ }
}
TEST(Router, ParameterTagging)
diff --git a/http/utility.hpp b/http/utility.hpp
index f7e456b..a1561b0 100644
--- a/http/utility.hpp
+++ b/http/utility.hpp
@@ -1,5 +1,6 @@
#pragma once
+#include <bmcweb_config.h>
#include <openssl/crypto.h>
#include <boost/date_time/posix_time/posix_time.hpp>
@@ -11,7 +12,6 @@
#include <ctime>
#include <functional>
#include <limits>
-#include <regex>
#include <stdexcept>
#include <string>
#include <string_view>
@@ -733,48 +733,87 @@
return details::readUrlSegments(urlView, {std::forward<Args>(args)...});
}
+inline std::string setProtocolDefaults(const boost::urls::url_view& url)
+{
+ if (url.scheme() == "https")
+ {
+ return "https";
+ }
+ if (url.scheme() == "http")
+ {
+ if (bmcwebInsecureEnableHttpPushStyleEventing)
+ {
+ return "http";
+ }
+ return "";
+ }
+ return "";
+}
+
+inline uint16_t setPortDefaults(const boost::urls::url_view& url)
+{
+ uint16_t port = url.port_number();
+ if (port != 0)
+ {
+ // user picked a port already.
+ return port;
+ }
+
+ // If the user hasn't explicitly stated a port, pick one explicitly for them
+ // based on the protocol defaults
+ if (url.scheme() == "http")
+ {
+ return 80;
+ }
+ if (url.scheme() == "https")
+ {
+ return 443;
+ }
+ return 0;
+}
+
inline bool validateAndSplitUrl(std::string_view destUrl, std::string& urlProto,
- std::string& host, std::string& port,
+ std::string& host, uint16_t& port,
std::string& path)
{
- // Validate URL using regex expression
- // Format: <protocol>://<host>:<port>/<path>
- // protocol: http/https
- const std::regex urlRegex(
- "(http|https)://([^/\\x20\\x3f\\x23\\x3a]+):?([0-9]*)(/"
- "([^\\x20\\x23\\x3f]*\\x3f?([^\\x20\\x23\\x3f])*)?)");
- std::cmatch match;
- if (!std::regex_match(destUrl.begin(), destUrl.end(), match, urlRegex))
+ boost::string_view urlBoost(destUrl.data(), destUrl.size());
+ boost::urls::result<boost::urls::url_view> url =
+ boost::urls::parse_uri(urlBoost);
+ if (!url)
+ {
+ return false;
+ }
+ urlProto = setProtocolDefaults(url.value());
+ if (urlProto.empty())
{
return false;
}
- urlProto = std::string(match[1].first, match[1].second);
- if (urlProto == "http")
- {
-#ifndef BMCWEB_INSECURE_ENABLE_HTTP_PUSH_STYLE_EVENTING
- return false;
-#endif
- }
+ port = setPortDefaults(url.value());
- host = std::string(match[2].first, match[2].second);
- port = std::string(match[3].first, match[3].second);
- path = std::string(match[4].first, match[4].second);
- if (port.empty())
- {
- if (urlProto == "http")
- {
- port = "80";
- }
- else
- {
- port = "443";
- }
- }
+ host = std::string_view(url->encoded_host().data(),
+ url->encoded_host().size());
+
+ path = std::string_view(url->encoded_path().data(),
+ url->encoded_path().size());
if (path.empty())
{
path = "/";
}
+ if (url->has_fragment())
+ {
+ path += '#';
+ path += std::string_view(url->encoded_fragment().data(),
+ url->encoded_fragment().size());
+ }
+
+ if (url->has_query())
+ {
+ path += '?';
+ path += std::string_view(url->encoded_query().data(),
+ url->encoded_query().size());
+ }
+
return true;
}
diff --git a/include/async_resolve.hpp b/include/async_resolve.hpp
index 7387f4d..87f53a0 100644
--- a/include/async_resolve.hpp
+++ b/include/async_resolve.hpp
@@ -26,7 +26,7 @@
Resolver& operator=(Resolver&&) = delete;
template <typename ResolveHandler>
- void asyncResolve(const std::string& host, const std::string& port,
+ void asyncResolve(const std::string& host, uint16_t port,
ResolveHandler&& handler)
{
BMCWEB_LOG_DEBUG << "Trying to resolve: " << host << ":" << port;
@@ -78,19 +78,8 @@
handler(ec, endpointList);
return;
}
- uint16_t portNum = 0;
- auto it = std::from_chars(
- // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
- port.data(), port.data() + port.size(), portNum);
- if (it.ec != std::errc())
- {
- BMCWEB_LOG_ERROR << "Failed to get the Port";
- handler(ec, endpointList);
- return;
- }
- endpoint.port(portNum);
- BMCWEB_LOG_DEBUG << "resolved endpoint is : "
- << endpoint.address().to_string();
+ endpoint.port(port);
+ BMCWEB_LOG_DEBUG << "resolved endpoint is : " << endpoint;
endpointList.push_back(endpoint);
}
// All the resolved data is filled in the endpointList
diff --git a/meson.build b/meson.build
index aa8dd74..e8897b3 100644
--- a/meson.build
+++ b/meson.build
@@ -314,6 +314,8 @@
conf_data.set10('BMCWEB_INSECURE_DISABLE_XSS_PREVENTION', xss_enabled.enabled())
enable_redfish_query = get_option('insecure-enable-redfish-query')
conf_data.set10('BMCWEB_INSECURE_ENABLE_QUERY_PARAMS', enable_redfish_query.enabled())
+insecure_push_style_notification = get_option('insecure-push-style-notification')
+conf_data.set10('BMCWEB_INSECURE_ENABLE_HTTP_PUSH_STYLE_EVENTING', insecure_push_style_notification.enabled())
conf_data.set('MESON_INSTALL_PREFIX', get_option('prefix'))
conf_data.set('HTTPS_PORT', get_option('https_port'))
configure_file(input: 'bmcweb_config.h.in',
diff --git a/redfish-core/include/event_service_manager.hpp b/redfish-core/include/event_service_manager.hpp
index 2c1ebfb..bc5af85 100644
--- a/redfish-core/include/event_service_manager.hpp
+++ b/redfish-core/include/event_service_manager.hpp
@@ -27,6 +27,7 @@
#include <dbus_utility.hpp>
#include <error_messages.hpp>
#include <event_service_store.hpp>
+#include <http/utility.hpp>
#include <http_client.hpp>
#include <persistent_data.hpp>
#include <random.hpp>
@@ -364,7 +365,7 @@
Subscription(Subscription&&) = delete;
Subscription& operator=(Subscription&&) = delete;
- Subscription(const std::string& inHost, const std::string& inPort,
+ Subscription(const std::string& inHost, uint16_t inPort,
const std::string& inPath, const std::string& inUriProto) :
eventSeqNum(1),
host(inHost), port(inPort), path(inPath), uriProto(inUriProto)
@@ -557,7 +558,7 @@
private:
uint64_t eventSeqNum;
std::string host;
- std::string port;
+ uint16_t port = 0;
std::string path;
std::string uriProto;
std::shared_ptr<crow::HttpClient> conn = nullptr;
@@ -619,7 +620,7 @@
std::string host;
std::string urlProto;
- std::string port;
+ uint16_t port = 0;
std::string path;
bool status = crow::utility::validateAndSplitUrl(
newSub->destinationUrl, urlProto, host, port, path);
diff --git a/redfish-core/lib/event_service.hpp b/redfish-core/lib/event_service.hpp
index 04b46e0..2b63955 100644
--- a/redfish-core/lib/event_service.hpp
+++ b/redfish-core/lib/event_service.hpp
@@ -18,6 +18,8 @@
#include <app.hpp>
#include <boost/beast/http/fields.hpp>
+#include <http/utility.hpp>
+#include <logging.hpp>
#include <query.hpp>
#include <registries/privilege_registry.hpp>
@@ -266,56 +268,27 @@
}
}
- // Validate the URL using regex expression
- // Format: <protocol>://<host>:<port>/<uri>
- // protocol: http/https
- // host: Exclude ' ', ':', '#', '?'
- // port: Empty or numeric value with ':' separator.
- // uri: Start with '/' and Exclude '#', ' '
- // Can include query params(ex: '/event?test=1')
- // TODO: Need to validate hostname extensively(as per rfc)
- const std::regex urlRegex(
- "(http|https)://([^/\\x20\\x3f\\x23\\x3a]+):?([0-9]*)(/"
- "([^\\x20\\x23\\x3f]*\\x3f?([^\\x20\\x23\\x3f])*)?)");
- std::cmatch match;
- if (!std::regex_match(destUrl.c_str(), match, urlRegex))
+ std::string host;
+ std::string urlProto;
+ uint16_t port = 0;
+ std::string path;
+
+ if (!crow::utility::validateAndSplitUrl(destUrl, urlProto, host,
+ port, path))
{
+ BMCWEB_LOG_WARNING
+ << "Failed to validate and split destination url";
messages::propertyValueFormatError(asyncResp->res, destUrl,
"Destination");
return;
}
- std::string uriProto = std::string(match[1].first, match[1].second);
- if (uriProto == "http")
- {
-#ifndef BMCWEB_INSECURE_ENABLE_HTTP_PUSH_STYLE_EVENTING
- messages::propertyValueFormatError(asyncResp->res, destUrl,
- "Destination");
- return;
-#endif
- }
-
- std::string host = std::string(match[2].first, match[2].second);
- std::string port = std::string(match[3].first, match[3].second);
- std::string path = std::string(match[4].first, match[4].second);
- if (port.empty())
- {
- if (uriProto == "http")
- {
- port = "80";
- }
- else
- {
- port = "443";
- }
- }
if (path.empty())
{
path = "/";
}
-
std::shared_ptr<Subscription> subValue =
- std::make_shared<Subscription>(host, port, path, uriProto);
+ std::make_shared<Subscription>(host, port, path, urlProto);
subValue->destinationUrl = destUrl;