fix: Preserve percent-encoding in HttpRequest targets and query construction

libs/client-sdk/src/data_sources/polling_data_source.cpp

@@ -1,6 +1,6 @@
-#include <boost/json.hpp>
 #include <boost/asio/error.hpp>
 #include <boost/asio/post.hpp>
+#include <boost/json.hpp>
 
 #include <launchdarkly/client_side/data_source_status.hpp>
 #include <launchdarkly/config/shared/builders/http_properties_builder.hpp>
@@ -49,9 +49,7 @@
     }
 
     if (data_source_config.with_reasons) {
-        if (url) {
-            url->append("?withReasons=true");
-        }
+        url = network::AppendQueryParam(url, "withReasons", "true");
     }
 
     config::shared::builders::HttpPropertiesBuilder<config::shared::ClientSDK>
@@ -66,7 +64,7 @@
     config::shared::built::DataSourceConfig<config::shared::ClientSDK> const&
         data_source_config,
     config::shared::built::HttpProperties const& http_properties,
     boost::asio::any_io_executor ioc,
     Context const& context,
     IDataSourceUpdateSink& handler,
     DataSourceStatusManager& status_manager,
@@ -113,7 +111,7 @@
     });
 }
 
 void PollingDataSource::HandlePollResult(network::HttpResult res) {
     auto header_etag = res.Headers().find("etag");
     bool has_etag = header_etag != res.Headers().end();
 
@@ -143,13 +141,13 @@
         status_manager_.SetState(
             DataSourceStatus::DataSourceState::kInterrupted,
             DataSourceStatus::ErrorInfo::ErrorKind::kNetworkError,
             res.ErrorMessage() ? *res.ErrorMessage() : "unknown error");
         LD_LOG(logger_, LogLevel::kWarn)
             << "Polling for feature flag updates failed: "
             << (res.ErrorMessage() ? *res.ErrorMessage() : "unknown error");
     } else if (res.Status() == 200) {
         data_source_handler_.HandleMessage("put", res.Body().value());
     } else if (res.Status() == 304) {
         // This should be handled ahead of here, but if we get a 304,
         // and it didn't have an etag, we still don't want to try to
         // parse the body.

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<std::string> AppendUrl(std::optional<std::string> 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<std::string> AppendQueryParam(std::optional<std::string> url_in,
+                                            std::string const& key,
+                                            std::string const& value);
+
 }  // namespace launchdarkly::network

libs/internal/src/network/http_requester.cpp

@@ -91,17 +91,25 @@
     }
 
     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;
@@ -135,7 +143,7 @@
 }
 
 bool IsRecoverableStatus(HttpResult::StatusCode status) {
     return status < 400 || status > 499 || status == 400 || status == 408 ||
            status == 429;
 }
 
@@ -156,16 +164,15 @@
     }
 
     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 @@
         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<std::string> AppendQueryParam(std::optional<std::string> 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

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 <gtest/gtest.h>
+
+#include <launchdarkly/config/shared/builders/http_properties_builder.hpp>
+#include <launchdarkly/config/shared/sdks.hpp>
+#include <launchdarkly/network/asio_requester.hpp>
+
+#include <boost/asio/io_context.hpp>
+#include <boost/asio/ip/tcp.hpp>
+#include <boost/beast/core.hpp>
+#include <boost/beast/http.hpp>
+
+#include <chrono>
+#include <memory>
+#include <optional>
+#include <string>
+
+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<ClientSDK>()
+        .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<OneShotServer> {
+   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<http::request<http::string_body>> const& Seen() const {
+        return seen_;
+    }
+
+    std::optional<std::string> 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<http::string_body> request_;
+    http::response<http::string_body> response_;
+    std::optional<http::request<http::string_body>> seen_;
+    std::optional<std::string> 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<http::request<http::string_body>> RoundTrip(
+    std::string const& target) {
+    net::io_context ioc;
+    auto server = std::make_shared<OneShotServer>(ioc);
+    server->Start();
+
+    auto props = Props();
+    AsioRequester requester(ioc.get_executor(), props.Tls());
+    std::optional<HttpResult> 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

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<ClientSDK>().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<ClientSDK>().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<ClientSDK>().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"));
+}