Make url by value in Request
There's some tough-to-track-down safety problems in http Request. This
commit is an attempt to make things more safe, even if it isn't clear
how the old code was wrong.
Previously, the old code took a url_view from the target() string for a
given URI. This was effectively a pointer, and needed to be updated in
custom move/copy constructors that were error prone to write.
This commit moves to taking the URI by non-view, which involves a copy,
but allows us to use the default move and copy constructors, as well as
have no internal references within Request, which should improve the
safety and reviewability.
There's already so many string copies in bmcweb, that this is unlikely
to show up as any sort of performance regression, and simple code is
much better in this case.
Note, because of a bug in boost::url, we have to explicitly construct a
url_view in any case where we want to use segments() or query() on a
const Request. This has been reported to the boost maintainers, and is
being worked for a long term solution.
https://github.com/boostorg/url/pull/704
Tested: Redfish service validator passed on last commit in series.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I49a7710e642dff624d578ec1dde088428f284627
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index 0daf665..c28cde3 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -25,7 +25,6 @@
#include <boost/beast/http/write.hpp>
#include <boost/beast/ssl/ssl_stream.hpp>
#include <boost/beast/websocket.hpp>
-#include <boost/url/url_view.hpp>
#include <atomic>
#include <chrono>
@@ -225,12 +224,14 @@
}
keepAlive = thisReq.keepAlive();
#ifndef BMCWEB_INSECURE_DISABLE_AUTHX
- if (!crow::authentication::isOnAllowlist(req->url, req->method()) &&
+ if (!crow::authentication::isOnAllowlist(req->url().buffer(),
+ req->method()) &&
thisReq.session == nullptr)
{
BMCWEB_LOG_WARNING << "Authentication failed";
forward_unauthorized::sendUnauthorized(
- req->url, req->getHeaderValue("X-Requested-With"),
+ req->url().encoded_path(),
+ req->getHeaderValue("X-Requested-With"),
req->getHeaderValue("Accept"), res);
completeRequest(res);
return;
@@ -307,8 +308,9 @@
res = std::move(thisRes);
res.keepAlive(keepAlive);
- BMCWEB_LOG_INFO << "Response: " << this << ' ' << req->url << ' '
- << res.resultInt() << " keepalive=" << keepAlive;
+ BMCWEB_LOG_INFO << "Response: " << this << ' '
+ << req->url().encoded_path() << ' ' << res.resultInt()
+ << " keepalive=" << keepAlive;
addSecurityHeaders(*req, res);
diff --git a/http/http_request.hpp b/http/http_request.hpp
index e59f84b..3fd9d4d 100644
--- a/http/http_request.hpp
+++ b/http/http_request.hpp
@@ -8,7 +8,7 @@
#include <boost/beast/http/message.hpp>
#include <boost/beast/http/string_body.hpp>
#include <boost/beast/websocket.hpp>
-#include <boost/url/url_view.hpp>
+#include <boost/url/url.hpp>
#include <string>
#include <string_view>
@@ -20,9 +20,11 @@
struct Request
{
boost::beast::http::request<boost::beast::http::string_body> req;
- std::string_view url{};
- boost::urls::url_view urlView{};
+ private:
+ boost::urls::url urlBase{};
+
+ public:
bool isSecure{false};
boost::asio::io_context* ioService{};
@@ -49,21 +51,8 @@
}
}
- Request(const Request& other) :
- req(other.req), isSecure(other.isSecure), ioService(other.ioService),
- ipAddress(other.ipAddress), session(other.session),
- userRole(other.userRole)
- {
- setUrlInfo();
- }
-
- Request(Request&& other) noexcept :
- req(std::move(other.req)), isSecure(other.isSecure),
- ioService(other.ioService), ipAddress(std::move(other.ipAddress)),
- session(std::move(other.session)), userRole(std::move(other.userRole))
- {
- setUrlInfo();
- }
+ Request(const Request& other) = default;
+ Request(Request&& other) = default;
Request& operator=(const Request&) = delete;
Request& operator=(const Request&&) = delete;
@@ -94,6 +83,11 @@
return req.target();
}
+ boost::urls::url_view url() const
+ {
+ return {urlBase};
+ }
+
const boost::beast::http::fields& fields() const
{
return req.base();
@@ -134,8 +128,7 @@
{
return false;
}
- urlView = *result;
- url = urlView.encoded_path();
+ urlBase = *result;
return true;
}
};
diff --git a/http/routing.hpp b/http/routing.hpp
index 736ec38..5ebc11f 100644
--- a/http/routing.hpp
+++ b/http/routing.hpp
@@ -1218,7 +1218,8 @@
// Make sure it's safe to deference the array at that index
static_assert(maxVerbIndex <
std::tuple_size_v<decltype(perMethods)>);
- FindRoute route = findRouteByIndex(req.url, perMethodIndex);
+ FindRoute route =
+ findRouteByIndex(req.url().encoded_path(), perMethodIndex);
if (route.rule == nullptr)
{
continue;
@@ -1251,11 +1252,13 @@
Trie& trie = perMethod.trie;
std::vector<BaseRule*>& rules = perMethod.rules;
- const std::pair<unsigned, RoutingParams>& found = trie.find(req.url);
+ const std::pair<unsigned, RoutingParams>& found =
+ trie.find(req.url().encoded_path());
unsigned ruleIndex = found.first;
if (ruleIndex == 0U)
{
- BMCWEB_LOG_DEBUG << "Cannot match rules " << req.url;
+ BMCWEB_LOG_DEBUG << "Cannot match rules "
+ << req.url().encoded_path();
res.result(boost::beast::http::status::not_found);
res.end();
return;
@@ -1269,8 +1272,9 @@
if ((rules[ruleIndex]->getMethods() &
(1U << static_cast<size_t>(*verb))) == 0)
{
- BMCWEB_LOG_DEBUG << "Rule found but method mismatch: " << req.url
- << " with " << req.methodString() << "("
+ BMCWEB_LOG_DEBUG << "Rule found but method mismatch: "
+ << req.url().encoded_path() << " with "
+ << req.methodString() << "("
<< static_cast<uint32_t>(*verb) << ") / "
<< rules[ruleIndex]->getMethods();
res.result(boost::beast::http::status::not_found);
@@ -1324,13 +1328,14 @@
// route
if (foundRoute.allowHeader.empty())
{
- foundRoute.route = findRouteByIndex(req.url, notFoundIndex);
+ foundRoute.route =
+ findRouteByIndex(req.url().encoded_path(), notFoundIndex);
}
else
{
// See if we have a method not allowed (405) handler
- foundRoute.route =
- findRouteByIndex(req.url, methodNotAllowedIndex);
+ foundRoute.route = findRouteByIndex(req.url().encoded_path(),
+ methodNotAllowedIndex);
}
}
diff --git a/http/utility.hpp b/http/utility.hpp
index 71f39c8..4e6ed71 100644
--- a/http/utility.hpp
+++ b/http/utility.hpp
@@ -643,10 +643,10 @@
std::string_view segment;
};
-inline bool readUrlSegments(const boost::urls::url_view& urlView,
+inline bool readUrlSegments(const boost::urls::url_view& url,
std::initializer_list<UrlSegment>&& segments)
{
- const boost::urls::segments_view& urlSegments = urlView.segments();
+ const boost::urls::segments_view& urlSegments = url.segments();
if (!urlSegments.is_absolute())
{
@@ -687,10 +687,9 @@
} // namespace details
template <typename... Args>
-inline bool readUrlSegments(const boost::urls::url_view& urlView,
- Args&&... args)
+inline bool readUrlSegments(const boost::urls::url_view& url, Args&&... args)
{
- return details::readUrlSegments(urlView, {std::forward<Args>(args)...});
+ return details::readUrlSegments(url, {std::forward<Args>(args)...});
}
inline boost::urls::url replaceUrlSegment(const boost::urls::url_view& urlView,
@@ -723,13 +722,13 @@
return url;
}
-inline std::string setProtocolDefaults(const boost::urls::url_view& url)
+inline std::string setProtocolDefaults(const boost::urls::url_view& urlView)
{
- if (url.scheme() == "https")
+ if (urlView.scheme() == "https")
{
return "https";
}
- if (url.scheme() == "http")
+ if (urlView.scheme() == "http")
{
if (bmcwebInsecureEnableHttpPushStyleEventing)
{
diff --git a/redfish-core/include/query.hpp b/redfish-core/include/query.hpp
index 0fa6c18..c5d537b 100644
--- a/redfish-core/include/query.hpp
+++ b/redfish-core/include/query.hpp
@@ -78,7 +78,8 @@
boost::system::error_code ec;
// Try to GET the same resource
- crow::Request newReq({boost::beast::http::verb::get, req.url, 11}, ec);
+ crow::Request newReq(
+ {boost::beast::http::verb::get, req.url().encoded_path(), 11}, ec);
if (ec)
{
@@ -127,7 +128,7 @@
asyncResp->res.addHeader("OData-Version", "4.0");
std::optional<query_param::Query> queryOpt =
- query_param::parseParameters(req.urlView.params(), asyncResp->res);
+ query_param::parseParameters(req.url().params(), asyncResp->res);
if (queryOpt == std::nullopt)
{
return false;
diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index 7ace802..de51ca6 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -482,7 +482,7 @@
}
// We didn't recognize the prefix and need to return a 404
- std::string nameStr = req.urlView.segments().back();
+ std::string nameStr = req.url().segments().back();
messages::resourceNotFound(asyncResp->res, "", nameStr);
}
@@ -507,7 +507,7 @@
// don't need to write an error code
if (isCollection == AggregationType::Resource)
{
- std::string nameStr = sharedReq->urlView.segments().back();
+ std::string nameStr = sharedReq->url().segments().back();
messages::resourceNotFound(asyncResp->res, "", nameStr);
}
return;
@@ -529,8 +529,7 @@
return;
}
- const boost::urls::segments_view urlSegments =
- thisReq.urlView.segments();
+ const boost::urls::segments_view urlSegments = thisReq.url().segments();
boost::urls::url currentUrl("/");
boost::urls::segments_view::iterator it = urlSegments.begin();
const boost::urls::segments_view::const_iterator end =
@@ -823,7 +822,7 @@
{
using crow::utility::OrMorePaths;
using crow::utility::readUrlSegments;
- const boost::urls::url_view url = thisReq.urlView;
+ const boost::urls::url_view url = thisReq.url();
// We don't need to aggregate JsonSchemas due to potential issues such
// as version mismatches between aggregator and satellite BMCs. For
diff --git a/redfish-core/lib/log_services.hpp b/redfish-core/lib/log_services.hpp
index 1a9166b..6dcfd78 100644
--- a/redfish-core/lib/log_services.hpp
+++ b/redfish-core/lib/log_services.hpp
@@ -3302,7 +3302,7 @@
}
auto getStoredLogCallback =
- [asyncResp, logID, fileName, url(boost::urls::url(req.urlView))](
+ [asyncResp, logID, fileName, url(boost::urls::url(req.url()))](
const boost::system::error_code& ec,
const std::vector<
std::pair<std::string, dbus::utility::DbusVariantType>>&
diff --git a/redfish-core/lib/processor.hpp b/redfish-core/lib/processor.hpp
index cd9850e..4566e4b 100644
--- a/redfish-core/lib/processor.hpp
+++ b/redfish-core/lib/processor.hpp
@@ -1072,7 +1072,9 @@
}
asyncResp->res.jsonValue["@odata.type"] =
"#OperatingConfigCollection.OperatingConfigCollection";
- asyncResp->res.jsonValue["@odata.id"] = req.url;
+ asyncResp->res.jsonValue["@odata.id"] = crow::utility::urlFromPieces(
+ "redfish", "v1", "Systems", "system", "Processors", cpuName,
+ "OperatingConfigs");
asyncResp->res.jsonValue["Name"] = "Operating Config Collection";
// First find the matching CPU object so we know how to
@@ -1140,7 +1142,7 @@
"xyz.openbmc_project.Inventory.Item.Cpu.OperatingConfig"};
dbus::utility::getSubTree(
"/xyz/openbmc_project/inventory", 0, interfaces,
- [asyncResp, cpuName, configName, reqUrl{req.url}](
+ [asyncResp, cpuName, configName](
const boost::system::error_code& ec,
const dbus::utility::MapperGetSubTreeResponse& subtree) {
if (ec)
@@ -1161,7 +1163,9 @@
nlohmann::json& json = asyncResp->res.jsonValue;
json["@odata.type"] = "#OperatingConfig.v1_0_0.OperatingConfig";
- json["@odata.id"] = reqUrl;
+ json["@odata.id"] = crow::utility::urlFromPieces(
+ "redfish", "v1", "Systems", "system", "Processors", cpuName,
+ "OperatingConfigs", configName);
json["Name"] = "Processor Profile";
json["Id"] = configName;
diff --git a/redfish-core/lib/redfish_sessions.hpp b/redfish-core/lib/redfish_sessions.hpp
index 7293ed7..35d5d5c 100644
--- a/redfish-core/lib/redfish_sessions.hpp
+++ b/redfish-core/lib/redfish_sessions.hpp
@@ -212,7 +212,7 @@
bool isConfigureSelfOnly = pamrc == PAM_NEW_AUTHTOK_REQD;
if ((pamrc != PAM_SUCCESS) && !isConfigureSelfOnly)
{
- messages::resourceAtUriUnauthorized(asyncResp->res, req.urlView,
+ messages::resourceAtUriUnauthorized(asyncResp->res, req.url(),
"Invalid username or password");
return;
}
diff --git a/redfish-core/lib/redfish_v1.hpp b/redfish-core/lib/redfish_v1.hpp
index d707d2d..8ea46d8 100644
--- a/redfish-core/lib/redfish_v1.hpp
+++ b/redfish-core/lib/redfish_v1.hpp
@@ -39,7 +39,7 @@
BMCWEB_LOG_WARNING << "404 on path " << path;
- std::string name = req.urlView.segments().back();
+ std::string name = req.url().segments().back();
// Note, if we hit the wildcard route, we don't know the "type" the user was
// actually requesting, but giving them a return with an empty string is
// still better than nothing.
diff --git a/redfish-core/lib/task.hpp b/redfish-core/lib/task.hpp
index 2e7481a..4edfe18 100644
--- a/redfish-core/lib/task.hpp
+++ b/redfish-core/lib/task.hpp
@@ -47,7 +47,7 @@
struct Payload
{
explicit Payload(const crow::Request& req) :
- targetUri(req.url), httpOperation(req.methodString()),
+ targetUri(req.url().encoded_path()), httpOperation(req.methodString()),
httpHeaders(nlohmann::json::array())
{
using field_ns = boost::beast::http::field;