diff --git a/libs/client-sdk/src/data_sources/polling_data_source.cpp b/libs/client-sdk/src/data_sources/polling_data_source.cpp index 4e50f612b..f1db301f3 100644 --- a/libs/client-sdk/src/data_sources/polling_data_source.cpp +++ b/libs/client-sdk/src/data_sources/polling_data_source.cpp @@ -1,6 +1,6 @@ -#include #include #include +#include #include #include @@ -49,9 +49,7 @@ static network::HttpRequest MakeRequest( } if (data_source_config.with_reasons) { - if (url) { - url->append("?withReasons=true"); - } + url = network::AppendQueryParam(url, "withReasons", "true"); } config::shared::builders::HttpPropertiesBuilder diff --git a/libs/internal/include/launchdarkly/network/http_requester.hpp b/libs/internal/include/launchdarkly/network/http_requester.hpp index 26084eb25..2a413919f 100644 --- a/libs/internal/include/launchdarkly/network/http_requester.hpp +++ b/libs/internal/include/launchdarkly/network/http_requester.hpp @@ -135,9 +135,12 @@ bool IsRecoverableStatus(HttpResult::StatusCode status); /** * Append a path to a URL. This will account for query parameters on the - * original URL. This will also normalize the URL. + * original URL, and will normalize the path (resolving dot segments and + * making slashes consistent). Percent-encoding in the URL and in the appended + * path is preserved; characters not allowed in a path are percent-encoded. * - * If the input URL doesn't parse, then std::nullopt will be returned. + * If the input URL doesn't parse, or the appended path contains a malformed + * percent-escape, then std::nullopt will be returned. * * @param url_in Input URL, if std::nullopt, the method will return * std::nullopt. This is to facilitate multiple appends without having to check @@ -149,4 +152,22 @@ bool IsRecoverableStatus(HttpResult::StatusCode status); std::optional AppendUrl(std::optional url_in, std::string const& to_append); +/** + * Append a query parameter to a URL. The key and value are percent-encoded as + * needed, and the parameter is joined with '&' when the URL already carries a + * query, so a base URL that has its own parameters keeps them. + * + * If the input URL doesn't parse, then std::nullopt will be returned. + * + * @param url_in Input URL, if std::nullopt, the method will return + * std::nullopt. + * @param key The parameter name. + * @param value The parameter value. + * @return The URL with the parameter appended, or std::nullopt if the URL + * could not be parsed. + */ +std::optional AppendQueryParam(std::optional url_in, + std::string const& key, + std::string const& value); + } // namespace launchdarkly::network diff --git a/libs/internal/src/network/http_requester.cpp b/libs/internal/src/network/http_requester.cpp index 67b917ce3..f1c05e1f3 100644 --- a/libs/internal/src/network/http_requester.cpp +++ b/libs/internal/src/network/http_requester.cpp @@ -91,17 +91,25 @@ HttpRequest::HttpRequest(std::string const& url, } boost::urls::url boost_url = uri_components.value(); - // Make paths absolute and slashes consistent. - boost_url.normalize(); + // Resolve dot segments and make slashes consistent. Only the path is + // normalized: normalizing the query would decode escapes such as %26, + // changing which characters act as separators. Path normalization does + // decode escapes of characters that are legal in a path (older Boost + // releases include %2F); characters that cannot appear raw in a request + // target, such as space, '#', '?' and CR LF, stay encoded. + boost_url.normalize_path(); host_ = uri_components->host(); - // The c_str here is to remove extra nulls from normalizing the path. - // Clang calls this redundant, but it is very much required. - path_ = - boost_url.path().c_str(); // NOLINT(readability-redundant-string-cstr) - if (!boost_url.query().empty()) { + // Keep the percent-encoding. The Beast backend sends this string as the + // request target verbatim, so a decoded space, '#', '&' or CR LF coming + // from a server-supplied value (for example the FDv2 "basis" selector + // state) would corrupt the request line. + path_ = std::string(boost_url.encoded_path()); + auto const encoded_query = uri_components->encoded_query(); + if (!encoded_query.empty()) { // For a boost beast request we need the query string in the path. - path_ = path_ + "?" + uri_components->query(); + path_ += "?"; + path_ += std::string(encoded_query); } is_https_ = uri_components->scheme_id() == boost::urls::scheme::https; @@ -156,16 +164,15 @@ std::optional AppendUrl(std::optional url_in, } boost::urls::url url = uri_components.value(); - url.normalize(); - // The c_str here is to remove extra nulls from normalizing the path. - // Clang calls this redundant, but it is very much required. - std::string path = - url.path().c_str(); // NOLINT(readability-redundant-string-cstr) + // Normalize only the path (dot segments, slashes). The query is left as + // written so its percent-encoding survives. + url.normalize_path(); + std::string path(url.encoded_path()); // This sizing may not be perfect, but should be close enough on average. // The extra to is to account for a '/' and possible a '?'. - path.reserve(url.path().size() + to_append.size() + url.query().length() + - 2); + path.reserve(url.encoded_path().size() + to_append.size() + + url.encoded_query().size() + 2); // We want a single '/' between things. bool path_has_trailing_slash = @@ -185,9 +192,33 @@ std::optional AppendUrl(std::optional url_in, path.append(to_append, 1, to_append.length() - 1); } - url.set_path(path); - url.normalize(); - return url.c_str(); + // The appended path may itself be percent-encoded, as a redirect Location + // can be. Keep those escapes, encode anything else that is not allowed in + // a path, and reject malformed escapes. + auto const encoded_path = boost::urls::make_pct_string_view(path); + if (!encoded_path) { + return std::nullopt; + } + url.set_encoded_path(*encoded_path); + url.normalize_path(); + return std::string(url.buffer()); +} + +std::optional AppendQueryParam(std::optional url_in, + std::string const& key, + std::string const& value) { + if (!url_in) { + return std::nullopt; + } + + auto uri_components = boost::urls::parse_uri(*url_in); + if (!uri_components) { + return std::nullopt; + } + + boost::urls::url url = uri_components.value(); + url.params().append({key, value}); + return std::string(url.buffer()); } } // namespace launchdarkly::network diff --git a/libs/internal/tests/asio_requester_test.cpp b/libs/internal/tests/asio_requester_test.cpp new file mode 100644 index 000000000..bbac01f2d --- /dev/null +++ b/libs/internal/tests/asio_requester_test.cpp @@ -0,0 +1,158 @@ +// The Beast backend sends HttpRequest::Path() verbatim as the request target. +// These tests put percent-encoded URLs on the wire and check what a real HTTP +// parser at the other end sees. +#ifndef LD_CURL_NETWORKING + +#include + +#include +#include +#include + +#include +#include +#include +#include + +#include +#include +#include +#include + +using launchdarkly::config::shared::ClientSDK; +using launchdarkly::config::shared::builders::HttpPropertiesBuilder; +using launchdarkly::config::shared::built::HttpProperties; +using launchdarkly::network::AsioRequester; +using launchdarkly::network::HttpMethod; +using launchdarkly::network::HttpRequest; +using launchdarkly::network::HttpResult; +using namespace std::chrono_literals; + +namespace { + +HttpProperties Props() { + return HttpPropertiesBuilder() + .ConnectTimeout(2s) + .ResponseTimeout(2s) + .Build(); +} + +// Accepts one connection on an ephemeral loopback port, records the request +// as Beast parsed it, answers 200, and closes the socket. +class OneShotServer : public std::enable_shared_from_this { + public: + explicit OneShotServer(net::io_context& ioc) + : acceptor_(ioc, tcp::endpoint(net::ip::make_address("127.0.0.1"), 0)), + socket_(ioc) { + response_.result(http::status::ok); + response_.set(http::field::content_type, "application/json"); + response_.body() = "{}"; + response_.prepare_payload(); + } + + unsigned short Port() const { return acceptor_.local_endpoint().port(); } + + void Start() { + acceptor_.async_accept( + socket_, + [self = shared_from_this()](boost::system::error_code const& ec) { + boost::system::error_code ignored; + self->acceptor_.close(ignored); + if (ec) { + return; + } + http::async_read( + self->socket_, self->buffer_, self->request_, + [self](boost::system::error_code const& ec, std::size_t) { + if (ec) { + self->read_error_ = ec.message(); + self->Close(); + return; + } + self->seen_ = self->request_; + http::async_write( + self->socket_, self->response_, + [self](boost::system::error_code const&, + std::size_t) { self->Close(); }); + }); + }); + } + + std::optional> const& Seen() const { + return seen_; + } + + std::optional const& ReadError() const { return read_error_; } + + private: + void Close() { + boost::system::error_code ignored; + socket_.shutdown(tcp::socket::shutdown_both, ignored); + socket_.close(ignored); + } + + tcp::acceptor acceptor_; + tcp::socket socket_; + beast::flat_buffer buffer_; + http::request request_; + http::response response_; + std::optional> seen_; + std::optional read_error_; +}; + +// Sends one GET for the given target through the asio requester and returns +// the request exactly as the server parsed it. +std::optional> RoundTrip( + std::string const& target) { + net::io_context ioc; + auto server = std::make_shared(ioc); + server->Start(); + + auto props = Props(); + AsioRequester requester(ioc.get_executor(), props.Tls()); + std::optional result; + requester.Request(HttpRequest("http://127.0.0.1:" + + std::to_string(server->Port()) + target, + HttpMethod::kGet, props, std::nullopt), + [&](HttpResult res) { result = std::move(res); }); + ioc.run_for(5s); + + EXPECT_FALSE(server->ReadError().has_value()) + << "server could not parse the request: " << *server->ReadError(); + EXPECT_TRUE(result.has_value()); + if (result) { + EXPECT_FALSE(result->IsError()) << *result; + EXPECT_EQ(200u, result->Status()); + } + return server->Seen(); +} + +} // namespace + +// A server-supplied value that a URL builder percent-encoded, such as the +// FDv2 "basis" selector state. Decoding it again on the way out would end +// the request line at the CR LF and turn the remainder into a header. +TEST(AsioRequesterTest, PercentEncodedQueryReachesTheServerIntact) { + std::string const target = + "/sdk/poll/eval?basis=x%0D%0AX-Injected:%201&f=a%26b%20c%23d"; + + auto seen = RoundTrip(target); + + ASSERT_TRUE(seen.has_value()); + EXPECT_EQ(target, seen->target()); + EXPECT_EQ(seen->end(), seen->find("X-Injected")); +} + +// Path normalization may decode escapes of characters that are legal in a +// path (older Boost releases include %2F), so only characters that cannot +// appear raw in a request target are checked here. +TEST(AsioRequesterTest, PercentEncodedPathReachesTheServerIntact) { + std::string const target = "/ld%20relay/p%23q/sdk/latest-all"; + + auto seen = RoundTrip(target); + + ASSERT_TRUE(seen.has_value()); + EXPECT_EQ(target, seen->target()); +} + +#endif // LD_CURL_NETWORKING diff --git a/libs/internal/tests/http_requester_test.cpp b/libs/internal/tests/http_requester_test.cpp index 114a4f329..81e658112 100644 --- a/libs/internal/tests/http_requester_test.cpp +++ b/libs/internal/tests/http_requester_test.cpp @@ -6,6 +6,7 @@ using launchdarkly::config::shared::ClientSDK; using launchdarkly::config::shared::builders::HttpPropertiesBuilder; +using launchdarkly::network::AppendQueryParam; using launchdarkly::network::AppendUrl; using launchdarkly::network::HttpMethod; using launchdarkly::network::HttpRequest; @@ -90,3 +91,81 @@ TEST(HttpRequestTests, CanAppendWithParameters) { EXPECT_EQ("https://the.url.com/cheese?ham=true&egg=true", AppendUrl("https://the.url.com?ham=true&egg=true", "cheese")); } + +// The Beast backend sends Path() verbatim as the request target, so any +// percent-encoding a URL builder applied must survive. A server-supplied +// value such as the FDv2 "basis" selector state is the realistic input. +TEST(HttpRequestTests, PathPreservesPercentEncodedQuery) { + HttpRequest request( + "https://some.domain.com/sdk/poll/eval" + "?basis=x%0D%0AX-Injected:%201&f=a%26b%20c%23d", + launchdarkly::network::HttpMethod::kGet, + HttpPropertiesBuilder().Build(), std::nullopt); + + EXPECT_EQ("/sdk/poll/eval?basis=x%0D%0AX-Injected:%201&f=a%26b%20c%23d", + request.Path()); + EXPECT_EQ( + "https://some.domain.com/sdk/poll/eval" + "?basis=x%0D%0AX-Injected:%201&f=a%26b%20c%23d", + request.Url()); +} + +// Path normalization may decode escapes of characters that are legal in a +// path (older Boost releases include %2F), so only characters that cannot +// appear raw in a request target are checked here. +TEST(HttpRequestTests, PathPreservesPercentEncodedPathSegments) { + HttpRequest request( + "https://some.domain.com/ld%20relay/p%23q/sdk/latest-all", + launchdarkly::network::HttpMethod::kGet, + HttpPropertiesBuilder().Build(), std::nullopt); + + EXPECT_EQ("/ld%20relay/p%23q/sdk/latest-all", request.Path()); +} + +TEST(HttpRequestTests, PathOmitsAnEmptyQuery) { + HttpRequest request("https://some.domain.com/potato?", + launchdarkly::network::HttpMethod::kGet, + HttpPropertiesBuilder().Build(), + std::nullopt); + + EXPECT_EQ("/potato", request.Path()); +} + +TEST(HttpRequestTests, AppendPreservesPercentEncoding) { + EXPECT_EQ("https://the.url.com/ld%20relay/sdk/latest-all?tok=a%26b", + AppendUrl("https://the.url.com/ld%20relay?tok=a%26b", + "/sdk/latest-all")); + + EXPECT_EQ("https://the.url.com/base/p%23q", + AppendUrl("https://the.url.com/base", "p%23q")); +} + +TEST(HttpRequestTests, AppendEncodesRawCharactersInTheAppendedPath) { + EXPECT_EQ("https://the.url.com/has%20space", + AppendUrl("https://the.url.com", "/has space")); +} + +TEST(HttpRequestTests, AppendRejectsAnInvalidPercentEscape) { + EXPECT_EQ(std::nullopt, AppendUrl("https://the.url.com", "/bad%zz")); +} + +TEST(HttpRequestTests, AppendQueryParamUsesTheRightSeparator) { + EXPECT_EQ("https://the.url.com/x?withReasons=true", + AppendQueryParam("https://the.url.com/x", "withReasons", "true")); + + // A base URL that already carries a query keeps it. + EXPECT_EQ("https://the.url.com/x?tok=a%26b&filter=my-filter", + AppendQueryParam("https://the.url.com/x?tok=a%26b", "filter", + "my-filter")); +} + +TEST(HttpRequestTests, AppendQueryParamEncodesReservedCharacters) { + EXPECT_EQ( + "https://the.url.com/x?basis=a%26b%20c%23d%0D%0A", + AppendQueryParam("https://the.url.com/x", "basis", "a&b c#d\r\n")); +} + +TEST(HttpRequestTests, AppendQueryParamPropagatesInvalidUrls) { + EXPECT_EQ(std::nullopt, AppendQueryParam(std::nullopt, "a", "b")); + EXPECT_EQ(std::nullopt, AppendQueryParam("not a url", "a", "b")); +} diff --git a/libs/server-sdk/src/data_systems/background_sync/sources/polling/polling_data_source.cpp b/libs/server-sdk/src/data_systems/background_sync/sources/polling/polling_data_source.cpp index abb2a5478..c051d38ef 100644 --- a/libs/server-sdk/src/data_systems/background_sync/sources/polling/polling_data_source.cpp +++ b/libs/server-sdk/src/data_systems/background_sync/sources/polling/polling_data_source.cpp @@ -3,8 +3,8 @@ #include #include -#include #include +#include #include #include @@ -39,7 +39,8 @@ static network::HttpRequest MakeRequest( if (polling_config.filter_key && url) { if (detail::ValidateFilterKey(*polling_config.filter_key)) { - url->append("?filter=" + *polling_config.filter_key); + url = network::AppendQueryParam(url, "filter", + *polling_config.filter_key); LD_LOG(logger, LogLevel::kDebug) << "using payload filter '" << *polling_config.filter_key << "'";