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/client-sdk/tests/polling_data_source_request_test.cpp b/libs/client-sdk/tests/polling_data_source_request_test.cpp new file mode 100644 index 000000000..cd332fe66 --- /dev/null +++ b/libs/client-sdk/tests/polling_data_source_request_test.cpp @@ -0,0 +1,144 @@ +// The FDv1 polling source builds its request URL from the configured base +// URL. This test sends a request from the source to a loopback server and +// checks the target it parsed, so a base URL that already carries a query +// keeps it and withReasons is joined with '&'. +#include + +#include +#include +#include +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "data_sources/data_source_status_manager.hpp" +#include "data_sources/polling_data_source.hpp" + +namespace beast = boost::beast; +namespace http = beast::http; +namespace net = boost::asio; +using tcp = net::ip::tcp; + +using namespace launchdarkly; +using namespace launchdarkly::client_side; +using namespace launchdarkly::client_side::data_sources; +using namespace std::chrono_literals; + +namespace { + +class NullSink : public IDataSourceUpdateSink { + public: + void Init(Context const&, + std::unordered_map) override {} + void Upsert(Context const&, std::string, ItemDescriptor) override {} + void Apply(Context const&, FlagChangeSet, bool) override {} +}; + +// Accepts one connection on an ephemeral loopback port, records the target +// as Beast parsed it, answers 200, then stops the io_context so the test does +// not wait for the source's next poll. +class OneShotServer : public std::enable_shared_from_this { + public: + explicit OneShotServer(net::io_context& ioc) + : ioc_(ioc), + acceptor_(ioc, tcp::endpoint(net::ip::make_address("127.0.0.1"), 0)), + socket_(ioc) {} + + std::string BaseUrl() const { + return "http://127.0.0.1:" + + std::to_string(acceptor_.local_endpoint().port()) + + "/relay?tok=a%26b"; + } + + void Start() { + acceptor_.async_accept( + socket_, + [self = shared_from_this()](boost::system::error_code const& ec) { + 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->target_ = + std::string(self->request_.target()); + } + self->response_.result(http::status::ok); + self->response_.set(http::field::content_type, + "application/json"); + self->response_.body() = "{}"; + self->response_.prepare_payload(); + http::async_write( + self->socket_, self->response_, + [self](boost::system::error_code const&, + std::size_t) { + boost::system::error_code ignored; + self->socket_.shutdown( + tcp::socket::shutdown_both, ignored); + self->socket_.close(ignored); + self->acceptor_.close(ignored); + self->ioc_.stop(); + }); + }); + }); + } + + std::optional const& Target() const { return target_; } + + private: + net::io_context& ioc_; + tcp::acceptor acceptor_; + tcp::socket socket_; + beast::flat_buffer buffer_; + http::request request_; + http::response response_; + std::optional target_; +}; + +} // namespace + +TEST(PollingDataSourceRequestTest, WithReasonsJoinsAnExistingQuery) { + net::io_context ioc; + auto server = std::make_shared(ioc); + server->Start(); + + auto const base = server->BaseUrl(); + config::shared::built::ServiceEndpoints const endpoints(base, base, base); + config::shared::built::DataSourceConfig< + config::shared::ClientSDK> const data_source_config{ + config::shared::Defaults::PollingConfig(), + /* with_reasons= */ true, /* use_report= */ false}; + auto const http_properties = + config::shared::Defaults::HttpProperties(); + + auto const context = ContextBuilder().Kind("user", "user-key").Build(); + auto const encoded_context = encoding::Base64UrlEncode( + boost::json::serialize(boost::json::value_from(context))); + + NullSink sink; + DataSourceStatusManager status_manager; + Logger logger = logging::NullLogger(); + + auto source = std::make_shared( + endpoints, data_source_config, http_properties, ioc.get_executor(), + context, sink, status_manager, logger); + source->Start(); + ioc.run_for(5s); + source->ShutdownAsync(nullptr); + + ASSERT_TRUE(server->Target().has_value()) + << "no request reached the server"; + EXPECT_EQ("/relay/msdk/evalx/contexts/" + encoded_context + + "?tok=a%26b&withReasons=true", + *server->Target()); +} diff --git a/libs/internal/include/launchdarkly/network/asio_requester.hpp b/libs/internal/include/launchdarkly/network/asio_requester.hpp index 80821c1f6..3c6d9325f 100644 --- a/libs/internal/include/launchdarkly/network/asio_requester.hpp +++ b/libs/internal/include/launchdarkly/network/asio_requester.hpp @@ -13,6 +13,7 @@ #include #include #include +#include #include "foxy/client_session.hpp" @@ -35,10 +36,6 @@ using TlsOptions = config::shared::built::TlsOptions; static unsigned char const kRedirectLimit = 20; -static bool IsAbsolute(std::string_view str) { - return str.find("://") != std::string::npos || str.find("//") == 0; -} - static bool NeedsRedirect(HttpResult const& res) { // 300, multiple choices. Not actionable. // 302, found, but not available for unforeseen reasons is not actionable. @@ -103,18 +100,20 @@ static std::optional MakeRedirectRequest(HttpRequest const& req, // Location should be verified to be present before attempting to // make the redirect request. assert(location != res.Headers().end()); - // Start the request over with the new URL. - if (IsAbsolute(location->second)) { - return HttpRequest(location->second, req.Method(), req.Properties(), - req.Body()); + // A Location header is a URI reference. It can be absolute or relative, + // so resolve it against the URL of the request that was redirected. + auto base = boost::urls::parse_uri(req.Url()); + auto reference = boost::urls::parse_uri_reference(location->second); + if (!base || !reference) { + return std::nullopt; } - auto new_url = AppendUrl(req.Url(), location->second); - if (new_url) { - return HttpRequest(*new_url, req.Method(), req.Properties(), - req.Body()); + boost::urls::url resolved; + if (!boost::urls::resolve(*base, *reference, resolved)) { + return std::nullopt; } - - return std::nullopt; + // Start the request over with the new URL. + return HttpRequest(std::string(resolved.buffer()), req.Method(), + req.Properties(), req.Body()); } static boost::optional ToOptRef( diff --git a/libs/internal/include/launchdarkly/network/http_requester.hpp b/libs/internal/include/launchdarkly/network/http_requester.hpp index 26084eb25..8448075b7 100644 --- a/libs/internal/include/launchdarkly/network/http_requester.hpp +++ b/libs/internal/include/launchdarkly/network/http_requester.hpp @@ -90,8 +90,16 @@ class HttpRequest { const; [[nodiscard]] std::string const& Host() const; [[nodiscard]] std::optional const& Port() const; + + /** + * The percent-encoded request target: the path, and the query when the + * URL has one. The Beast backend sends this as the request target. + */ [[nodiscard]] std::string const& Path() const; + /** + * The percent-encoded URL the request was created from. + */ [[nodiscard]] std::string const& Url() const; [[nodiscard]] bool Https() const; @@ -104,6 +112,15 @@ class HttpRequest { */ [[nodiscard]] bool Valid() const; + /** + * Create a request for a URL. + * + * @param url A percent-encoded URL. Values that need encoding must be + * added with AppendUrl or AppendQueryParam, which encode them. + * @param method The HTTP method. + * @param properties The properties for the request. + * @param body The request body, if any. + */ HttpRequest(std::string const& url, HttpMethod method, config::shared::built::HttpProperties properties, @@ -134,8 +151,11 @@ class HttpRequest { 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. + * Append a path to a URL. The appended path is not percent-encoded: '/' + * separates segments, and every other character that is not allowed in a + * path segment is percent-encoded, so a '%' becomes "%25". Dot segments such + * as ".." are resolved. The query of the URL, and the percent-encoding the + * URL already has, are kept. * * If the input URL doesn't parse, then std::nullopt will be returned. * @@ -143,10 +163,29 @@ bool IsRecoverableStatus(HttpResult::StatusCode status); * std::nullopt. This is to facilitate multiple appends without having to check * intermediate results. * - * @param to_append Path to append to the URL. + * @param to_append Path to append to the URL, not percent-encoded. * @return The appended URL, or std::nullopt if the URL could not be parsed. */ std::optional AppendUrl(std::optional url_in, std::string const& to_append); +/** + * Append a query parameter to a URL. The key and value are not + * percent-encoded: every character outside the unreserved set is + * percent-encoded. 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, not percent-encoded. + * @param value The parameter value, not percent-encoded. + * @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..e4d7f67d1 100644 --- a/libs/internal/src/network/http_requester.cpp +++ b/libs/internal/src/network/http_requester.cpp @@ -91,17 +91,20 @@ 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 in the path. The query is not normalized, so its + // percent-encoding stays as written. + 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()) { - // For a boost beast request we need the query string in the path. - path_ = path_ + "?" + uri_components->query(); + // The target is the percent-encoded path and query. The Beast backend + // sends it as the request target without changes. The path and query are + // joined here because encoded_target() asserts on older Boost releases + // after the path was normalized. + path_ = std::string(boost_url.encoded_path()); + auto const encoded_query = uri_components->encoded_query(); + if (!encoded_query.empty()) { + path_ += "?"; + path_ += std::string(encoded_query); } is_https_ = uri_components->scheme_id() == boost::urls::scheme::https; @@ -150,44 +153,59 @@ std::optional AppendUrl(std::optional url_in, } auto uri_components = boost::urls::parse_uri(*url_in); - if (!uri_components) { return std::nullopt; } 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) - - // 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); - - // We want a single '/' between things. - bool path_has_trailing_slash = - !path.empty() && path[path.length() - 1] == '/'; - bool append_has_leading_slash = to_append[0] == '/'; - - // One other the other already has a '/', so we can just append them. - if ((path_has_trailing_slash && !append_has_leading_slash) || - (!path_has_trailing_slash && append_has_leading_slash)) { - path.append(to_append); - } else if (!path_has_trailing_slash && !append_has_leading_slash) { - // Neither had a '/', so we need to add one. - path.append("/"); - path.append(to_append); - } else { - // Both have a '/' so append the second starting after the '/'. - path.append(to_append, 1, to_append.length() - 1); + auto segments = url.segments(); + // A trailing '/' on the URL is an empty last segment. Remove it so that a + // single '/' separates the URL from the appended path. + if (!segments.empty() && segments.back().empty()) { + segments.pop_back(); + } + + // Each part between '/' characters is one segment. The URL library + // percent-encodes the characters that are not allowed in a segment. + std::size_t start = 0; + while (start <= to_append.size()) { + std::size_t end = to_append.find('/', start); + if (end == std::string::npos) { + end = to_append.size(); + } + if (end > start) { + segments.push_back(to_append.substr(start, end - start)); + } + start = end + 1; } - url.set_path(path); - url.normalize(); - return url.c_str(); + // Resolve dot segments such as "..". + 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(); + // Percent-encode every character outside the unreserved set. The URL + // library's own parameter encoding differs between releases, so the + // result must not depend on it. + auto const encoded_key = + boost::urls::encode(key, boost::urls::unreserved_chars); + auto const encoded_value = + boost::urls::encode(value, boost::urls::unreserved_chars); + url.encoded_params().append({encoded_key, encoded_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..9319c693b --- /dev/null +++ b/libs/internal/tests/asio_requester_test.cpp @@ -0,0 +1,318 @@ +// 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 +#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(); +} + +// Answers the first request with a 301 to the given location and every +// later request with 200. Records the target of each request as Beast parsed +// it, and stops accepting after the expected number of requests. +class RedirectServer : public std::enable_shared_from_this { + public: + RedirectServer(net::io_context& ioc, std::size_t expected_requests) + : acceptor_(ioc, tcp::endpoint(net::ip::make_address("127.0.0.1"), 0)), + socket_(ioc), + expected_requests_(expected_requests) {} + + unsigned short Port() const { return acceptor_.local_endpoint().port(); } + + // The location is given here rather than in the constructor, so the + // caller can put the server's own port into it. + void Start(std::string location) { + location_ = std::move(location); + Accept(); + } + + std::vector const& Targets() const { return targets_; } + + std::optional const& ReadError() const { return read_error_; } + + private: + void Accept() { + acceptor_.async_accept( + socket_, + [self = shared_from_this()](boost::system::error_code const& ec) { + if (ec) { + return; + } + self->buffer_.consume(self->buffer_.size()); + self->request_ = {}; + 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->Finish(); + return; + } + self->targets_.emplace_back(self->request_.target()); + self->response_ = {}; + if (self->targets_.size() == 1) { + self->response_.result( + http::status::moved_permanently); + self->response_.set(http::field::location, + self->location_); + } else { + self->response_.result(http::status::ok); + self->response_.body() = "{}"; + } + self->response_.prepare_payload(); + http::async_write( + self->socket_, self->response_, + [self](boost::system::error_code const&, + std::size_t) { self->Finish(); }); + }); + }); + } + + void Finish() { + boost::system::error_code ignored; + socket_.shutdown(tcp::socket::shutdown_both, ignored); + socket_.close(ignored); + if (targets_.size() < expected_requests_ && !read_error_) { + Accept(); + return; + } + acceptor_.close(ignored); + } + + tcp::acceptor acceptor_; + tcp::socket socket_; + beast::flat_buffer buffer_; + http::request request_; + http::response response_; + std::string location_; + std::size_t expected_requests_; + std::vector targets_; + std::optional read_error_; +}; + +struct RedirectOutcome { + std::vector targets; + std::optional result; +}; + +// Sends one GET for the given target. The server answers with a 301 to the +// location, in which "{port}" is replaced with the server port. Returns the +// targets the server parsed and the result the requester delivered. +RedirectOutcome FollowRedirect(std::string const& target, + std::string const& location, + std::size_t expected_requests) { + net::io_context ioc; + auto server = std::make_shared(ioc, expected_requests); + std::string resolved_location = location; + if (auto const pos = resolved_location.find("{port}"); + pos != std::string::npos) { + resolved_location.replace(pos, 6, std::to_string(server->Port())); + } + server->Start(resolved_location); + + auto props = Props(); + AsioRequester requester(ioc.get_executor(), props.Tls()); + RedirectOutcome outcome; + requester.Request(HttpRequest("http://127.0.0.1:" + + std::to_string(server->Port()) + target, + HttpMethod::kGet, props, std::nullopt), + [&](HttpResult res) { outcome.result = std::move(res); }); + ioc.run_for(5s); + + EXPECT_FALSE(server->ReadError().has_value()) + << "server could not parse a request: " << *server->ReadError(); + outcome.targets = server->Targets(); + return outcome; +} + +} // 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()); +} + +// A relative Location is resolved against the URL of the redirected request. +// Its percent-encoding is server-supplied and reaches the server unchanged. +TEST(AsioRequesterTest, RelativeRedirectLocationIsResolvedAgainstTheRequest) { + auto outcome = FollowRedirect("/orig/path?keep=1", "/new%20path?x=%26y", 2); + + ASSERT_EQ(2u, outcome.targets.size()); + EXPECT_EQ("/orig/path?keep=1", outcome.targets[0]); + EXPECT_EQ("/new%20path?x=%26y", outcome.targets[1]); + ASSERT_TRUE(outcome.result.has_value()); + EXPECT_FALSE(outcome.result->IsError()) << *outcome.result; + EXPECT_EQ(200u, outcome.result->Status()); +} + +TEST(AsioRequesterTest, + RelativeRedirectLocationWithoutASlashKeepsTheParentPath) { + auto outcome = FollowRedirect("/orig/path", "sibling", 2); + + ASSERT_EQ(2u, outcome.targets.size()); + EXPECT_EQ("/orig/sibling", outcome.targets[1]); +} + +TEST(AsioRequesterTest, AbsoluteRedirectLocationIsFollowed) { + auto outcome = FollowRedirect( + "/orig", "http://127.0.0.1:{port}/abs%20path?basis=%0D%0A", 2); + + ASSERT_EQ(2u, outcome.targets.size()); + EXPECT_EQ("/abs%20path?basis=%0D%0A", outcome.targets[1]); + ASSERT_TRUE(outcome.result.has_value()); + EXPECT_EQ(200u, outcome.result->Status()); +} + +// A Location that is not a valid URI reference cannot be resolved, so the +// request fails instead of sending a guessed target. +TEST(AsioRequesterTest, MalformedRedirectLocationFailsTheRequest) { + auto outcome = FollowRedirect("/orig", "/bad%zz", 1); + + ASSERT_EQ(1u, outcome.targets.size()); + ASSERT_TRUE(outcome.result.has_value()); + EXPECT_TRUE(outcome.result->IsError()); +} + +#endif // LD_CURL_NETWORKING diff --git a/libs/internal/tests/http_requester_test.cpp b/libs/internal/tests/http_requester_test.cpp index 114a4f329..8e4521947 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,98 @@ 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, AppendKeepsTheEncodingOfTheUrl) { + 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/100%25/x/y", + AppendUrl("https://the.url.com/100%25/x", "y")); +} + +// The appended path is not percent-encoded, so every character that is not +// allowed in a path segment is encoded, and a '%' is data. +TEST(HttpRequestTests, AppendEncodesTheAppendedPath) { + EXPECT_EQ("https://the.url.com/has%20space", + AppendUrl("https://the.url.com", "/has space")); + + EXPECT_EQ("https://the.url.com/base/p%2523q", + AppendUrl("https://the.url.com/base", "p%23q")); + + EXPECT_EQ("https://the.url.com/a%0D%0Ab", + AppendUrl("https://the.url.com", "a\r\nb")); +} + +// The client SDK appends a base64url encoded context, which can end in '='. +TEST(HttpRequestTests, AppendKeepsBase64UrlPadding) { + EXPECT_EQ("https://the.url.com/msdk/evalx/contexts/eyJrZXkiOiJhIn0=", + AppendUrl("https://the.url.com/msdk/evalx/contexts", + "eyJrZXkiOiJhIn0=")); +} + +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")); +} + +// Every character outside the unreserved set is encoded, so the result does +// not depend on which characters a URL library release leaves raw. +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")); + + EXPECT_EQ("https://the.url.com/x?basis=a%20b%2Bc%3Bd%3De%2Ff%3Fg%25h~i", + AppendQueryParam("https://the.url.com/x", "basis", + "a b+c;d=e/f?g%h~i")); +} + +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 << "'"; diff --git a/libs/server-sdk/src/data_systems/background_sync/sources/streaming/streaming_data_source.cpp b/libs/server-sdk/src/data_systems/background_sync/sources/streaming/streaming_data_source.cpp index 5f7a27a7b..ef0998393 100644 --- a/libs/server-sdk/src/data_systems/background_sync/sources/streaming/streaming_data_source.cpp +++ b/libs/server-sdk/src/data_systems/background_sync/sources/streaming/streaming_data_source.cpp @@ -56,7 +56,8 @@ void StreamingDataSource::StartAsync( if (streaming_config_.filter_key && updated_url) { if (detail::ValidateFilterKey(*streaming_config_.filter_key)) { - updated_url->append("?filter=" + *streaming_config_.filter_key); + updated_url = network::AppendQueryParam( + updated_url, "filter", *streaming_config_.filter_key); LD_LOG(logger_, LogLevel::kDebug) << "using payload filter '" << *streaming_config_.filter_key << "'"; diff --git a/libs/server-sdk/tests/fdv1_source_request_test.cpp b/libs/server-sdk/tests/fdv1_source_request_test.cpp new file mode 100644 index 000000000..3dc65ef8b --- /dev/null +++ b/libs/server-sdk/tests/fdv1_source_request_test.cpp @@ -0,0 +1,178 @@ +// The FDv1 polling and streaming sources build their request URL from the +// configured base URL. These tests send a request from each source to a +// loopback server and check the target it parsed, so a base URL that already +// carries a query keeps it and the SDK's own parameters are joined with '&'. +#include + +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include + +namespace beast = boost::beast; +namespace http = beast::http; +namespace net = boost::asio; +using tcp = net::ip::tcp; + +using namespace launchdarkly; +using namespace launchdarkly::server_side; +using namespace launchdarkly::server_side::data_systems; +using namespace std::chrono_literals; + +namespace { + +class NullDestination : public data_interfaces::IDestination { + public: + void Init(data_model::SDKDataSet) override {} + void Upsert(std::string const&, data_model::FlagDescriptor) override {} + void Upsert(std::string const&, data_model::SegmentDescriptor) override {} + std::string const& Identity() const override { + static std::string const identity = "null"; + return identity; + } +}; + +// Accepts one connection on an ephemeral loopback port, records the target +// as Beast parsed it, answers 200 with the given content type, then stops +// the io_context so the test does not wait for the source's next poll. +class OneShotServer : public std::enable_shared_from_this { + public: + OneShotServer(net::io_context& ioc, std::string content_type) + : ioc_(ioc), + acceptor_(ioc, tcp::endpoint(net::ip::make_address("127.0.0.1"), 0)), + socket_(ioc), + content_type_(std::move(content_type)) {} + + std::string BaseUrl() const { + return "http://127.0.0.1:" + + std::to_string(acceptor_.local_endpoint().port()) + + "/relay?tok=a%26b"; + } + + void Start() { + acceptor_.async_accept( + socket_, + [self = shared_from_this()](boost::system::error_code const& ec) { + 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->target_ = + std::string(self->request_.target()); + } + self->response_.result(http::status::ok); + self->response_.set(http::field::content_type, + self->content_type_); + self->response_.body() = "{}"; + self->response_.prepare_payload(); + http::async_write( + self->socket_, self->response_, + [self](boost::system::error_code const&, + std::size_t) { + boost::system::error_code ignored; + self->socket_.shutdown( + tcp::socket::shutdown_both, ignored); + self->socket_.close(ignored); + self->acceptor_.close(ignored); + self->ioc_.stop(); + }); + }); + }); + } + + std::optional const& Target() const { return target_; } + + private: + net::io_context& ioc_; + tcp::acceptor acceptor_; + tcp::socket socket_; + beast::flat_buffer buffer_; + http::request request_; + http::response response_; + std::string content_type_; + std::optional target_; +}; + +} // namespace + +TEST(Fdv1SourceRequestTest, PollingFilterJoinsAnExistingQuery) { + net::io_context ioc; + auto server = std::make_shared(ioc, "application/json"); + server->Start(); + + auto const base = server->BaseUrl(); + server_side::config::built::ServiceEndpoints const endpoints(base, base, + base); + auto polling = launchdarkly::config::shared::Defaults< + launchdarkly::config::shared::ServerSDK>::PollingConfig(); + polling.filter_key = "my-filter"; + auto const http_properties = launchdarkly::config::shared::Defaults< + launchdarkly::config::shared::ServerSDK>::HttpProperties(); + + Logger logger = logging::NullLogger(); + data_components::DataSourceStatusManager status_manager; + NullDestination destination; + + auto source = std::make_shared( + ioc.get_executor(), logger, status_manager, endpoints, polling, + http_properties); + source->StartAsync(&destination, /* bootstrap_data= */ nullptr); + ioc.run_for(5s); + source->ShutdownAsync(nullptr); + + ASSERT_TRUE(server->Target().has_value()) + << "no request reached the server"; + EXPECT_EQ("/relay/sdk/latest-all?tok=a%26b&filter=my-filter", + *server->Target()); +} + +TEST(Fdv1SourceRequestTest, StreamingFilterJoinsAnExistingQuery) { + net::io_context ioc; + auto server = std::make_shared(ioc, "text/event-stream"); + server->Start(); + + auto const base = server->BaseUrl(); + server_side::config::built::ServiceEndpoints const endpoints(base, base, + base); + auto streaming = launchdarkly::config::shared::Defaults< + launchdarkly::config::shared::ServerSDK>::StreamingConfig(); + streaming.filter_key = "my-filter"; + auto const http_properties = launchdarkly::config::shared::Defaults< + launchdarkly::config::shared::ServerSDK>::HttpProperties(); + + Logger logger = logging::NullLogger(); + data_components::DataSourceStatusManager status_manager; + NullDestination destination; + + auto source = std::make_shared( + ioc.get_executor(), logger, status_manager, endpoints, streaming, + http_properties); + source->StartAsync(&destination, /* bootstrap_data= */ nullptr); + ioc.run_for(5s); + + // Let the stream client finish its shutdown before the io_context and the + // source are destroyed. + source->ShutdownAsync(nullptr); + ioc.restart(); + ioc.run_for(2s); + + ASSERT_TRUE(server->Target().has_value()) + << "no request reached the server"; + EXPECT_EQ("/relay/all?tok=a%26b&filter=my-filter", *server->Target()); +} diff --git a/libs/server-sdk/tests/fdv2_polling_impl_test.cpp b/libs/server-sdk/tests/fdv2_polling_impl_test.cpp index d4ecd9ee6..51932c8e2 100644 --- a/libs/server-sdk/tests/fdv2_polling_impl_test.cpp +++ b/libs/server-sdk/tests/fdv2_polling_impl_test.cpp @@ -162,6 +162,23 @@ TEST(MakeFDv2PollRequestTest, ValidFilterKeyIsIncluded) { EXPECT_EQ(req.Url(), "http://example.com/sdk/poll?filter=my-filter_1.0"); } +// The Beast backend sends Path() as the request target, so the encoding of a +// server-supplied selector state must survive into it. The state has no +// space because Boost.URL releases differ on whether a space becomes "+". +TEST(MakeFDv2PollRequestTest, BasisStateStaysPercentEncodedInThePath) { + auto logger = MakeNullLogger(); + auto props = + config::shared::Defaults::HttpProperties(); + auto req = MakeFDv2PollRequest( + "http://example.com", props, + data_model::Selector{data_model::Selector::State{7, "x\r\nY&z#"}}, + std::string{"my-filter"}, logger); + EXPECT_EQ(req.Url(), + "http://example.com/sdk/poll" + "?basis=x%0D%0AY%26z%23&filter=my-filter"); + EXPECT_EQ(req.Path(), "/sdk/poll?basis=x%0D%0AY%26z%23&filter=my-filter"); +} + TEST(MakeFDv2PollRequestTest, InvalidFilterKeyIsDropped) { auto logger = MakeNullLogger(); auto props =