From 3f07c6078db23116491d2a82c608e58a0fa3cb77 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:10:00 -0700 Subject: [PATCH 1/4] fix: Preserve percent-encoding in HttpRequest targets and appended paths The HttpRequest constructor built the Beast request target from Boost.URL's path() and query(), which percent-decode, so any encoded byte in a URL reached the wire decoded: a `basis` selector state containing CR LF split the request line, and '&', space, '#' or '%' inside a value corrupted the query. Use encoded_path() (after normalize_path(), which still resolves dot segments) and the un-normalized encoded_query() instead. AppendUrl normalized the whole URL, decoding legal query escapes (%26 -> &), and round-tripped the path through decoded path()/set_path(), double-encoding a '%' in the appended segment. Normalize only the path, join on encoded_path(), set the result with set_encoded_path(), and reject malformed escapes in the appended path. Add AppendQueryParam, which percent-encodes a key/value pair and joins it with '?' or '&' as appropriate, and cover the behaviour with unit tests plus a loopback wire test against a Beast server on an ephemeral port. --- .../launchdarkly/network/http_requester.hpp | 25 ++- libs/internal/src/network/http_requester.cpp | 64 ++++++-- libs/internal/tests/asio_requester_test.cpp | 155 ++++++++++++++++++ libs/internal/tests/http_requester_test.cpp | 76 +++++++++ 4 files changed, 300 insertions(+), 20 deletions(-) create mode 100644 libs/internal/tests/asio_requester_test.cpp 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..ffa0252d4 100644 --- a/libs/internal/src/network/http_requester.cpp +++ b/libs/internal/src/network/http_requester.cpp @@ -91,17 +91,22 @@ 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. + 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 +161,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 +189,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 (a base64url context, + // a redirect Location). 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..00d0d4082 --- /dev/null +++ b/libs/internal/tests/asio_requester_test.cpp @@ -0,0 +1,155 @@ +// 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")); +} + +TEST(AsioRequesterTest, PercentEncodedPathReachesTheServerIntact) { + std::string const target = "/ld%20relay/p%2Fq/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..09b4d080b 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,78 @@ 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()); +} + +TEST(HttpRequestTests, PathPreservesPercentEncodedPathSegments) { + HttpRequest request( + "https://some.domain.com/ld%20relay/p%2Fq/sdk/latest-all", + launchdarkly::network::HttpMethod::kGet, + HttpPropertiesBuilder().Build(), std::nullopt); + + EXPECT_EQ("/ld%20relay/p%2Fq/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%2Fq", + AppendUrl("https://the.url.com/base", "p%2Fq")); +} + +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")); +} From 84f0ac31aaf1dd8510fe223c2412a2da141fe6db Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:10:00 -0700 Subject: [PATCH 2/4] fix: Append FDv1 polling query parameters with the URL library The client and server FDv1 pollers appended `?withReasons=true` and `?filter=` with a literal '?', so a base URL that already carried a query produced `...?token=x?filter=my-filter`. Use AppendQueryParam so the parameter is joined with '&' when a query exists. Output for the default base URLs is unchanged. --- libs/client-sdk/src/data_sources/polling_data_source.cpp | 6 ++---- .../background_sync/sources/polling/polling_data_source.cpp | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) 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/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 << "'"; From 5beaafe552cb1b0a97398bae6fcbb00658d30e1b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:16:20 -0700 Subject: [PATCH 3/4] test: Check path escapes that every supported Boost release preserves Boost.URL's path normalization decodes escapes of characters that are legal in a path, and the 1.81 release used in CI includes %2F in that set (1.83 preserves it). Assert on %20 and %23 instead, which no release decodes because they cannot appear raw in a request target, and note the behaviour next to normalize_path(). --- libs/internal/src/network/http_requester.cpp | 5 ++++- libs/internal/tests/asio_requester_test.cpp | 5 ++++- libs/internal/tests/http_requester_test.cpp | 11 +++++++---- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/libs/internal/src/network/http_requester.cpp b/libs/internal/src/network/http_requester.cpp index ffa0252d4..27d8ac2a9 100644 --- a/libs/internal/src/network/http_requester.cpp +++ b/libs/internal/src/network/http_requester.cpp @@ -93,7 +93,10 @@ HttpRequest::HttpRequest(std::string const& url, boost::urls::url boost_url = uri_components.value(); // 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. + // 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(); diff --git a/libs/internal/tests/asio_requester_test.cpp b/libs/internal/tests/asio_requester_test.cpp index 00d0d4082..bbac01f2d 100644 --- a/libs/internal/tests/asio_requester_test.cpp +++ b/libs/internal/tests/asio_requester_test.cpp @@ -143,8 +143,11 @@ TEST(AsioRequesterTest, PercentEncodedQueryReachesTheServerIntact) { 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%2Fq/sdk/latest-all"; + std::string const target = "/ld%20relay/p%23q/sdk/latest-all"; auto seen = RoundTrip(target); diff --git a/libs/internal/tests/http_requester_test.cpp b/libs/internal/tests/http_requester_test.cpp index 09b4d080b..81e658112 100644 --- a/libs/internal/tests/http_requester_test.cpp +++ b/libs/internal/tests/http_requester_test.cpp @@ -110,13 +110,16 @@ TEST(HttpRequestTests, PathPreservesPercentEncodedQuery) { 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%2Fq/sdk/latest-all", + "https://some.domain.com/ld%20relay/p%23q/sdk/latest-all", launchdarkly::network::HttpMethod::kGet, HttpPropertiesBuilder().Build(), std::nullopt); - EXPECT_EQ("/ld%20relay/p%2Fq/sdk/latest-all", request.Path()); + EXPECT_EQ("/ld%20relay/p%23q/sdk/latest-all", request.Path()); } TEST(HttpRequestTests, PathOmitsAnEmptyQuery) { @@ -133,8 +136,8 @@ TEST(HttpRequestTests, AppendPreservesPercentEncoding) { AppendUrl("https://the.url.com/ld%20relay?tok=a%26b", "/sdk/latest-all")); - EXPECT_EQ("https://the.url.com/base/p%2Fq", - AppendUrl("https://the.url.com/base", "p%2Fq")); + EXPECT_EQ("https://the.url.com/base/p%23q", + AppendUrl("https://the.url.com/base", "p%23q")); } TEST(HttpRequestTests, AppendEncodesRawCharactersInTheAppendedPath) { From 494b73bcb459d90acf68650d4b22ac5be63451f4 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:36:32 -0700 Subject: [PATCH 4/4] docs: Drop the base64url example from the AppendUrl comment A base64url-encoded context is URL-safe by construction and never needs percent-encoding; a redirect Location is the appended path that can arrive encoded. --- libs/internal/src/network/http_requester.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/internal/src/network/http_requester.cpp b/libs/internal/src/network/http_requester.cpp index 27d8ac2a9..f1c05e1f3 100644 --- a/libs/internal/src/network/http_requester.cpp +++ b/libs/internal/src/network/http_requester.cpp @@ -192,9 +192,9 @@ std::optional AppendUrl(std::optional url_in, path.append(to_append, 1, to_append.length() - 1); } - // The appended path may itself be percent-encoded (a base64url context, - // a redirect Location). Keep those escapes, encode anything else that is - // not allowed in a path, and reject malformed escapes. + // 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;