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/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 <gtest/gtest.h>
+
+#include <chrono>
+#include <memory>
+#include <optional>
+#include <string>
+
+#include <boost/asio.hpp>
+#include <boost/beast/core.hpp>
+#include <boost/beast/http.hpp>
+#include <boost/json.hpp>
+
+#include <launchdarkly/config/shared/defaults.hpp>
+#include <launchdarkly/context_builder.hpp>
+#include <launchdarkly/encoding/base_64.hpp>
+#include <launchdarkly/logging/null_logger.hpp>
+#include <launchdarkly/serialization/json_context.hpp>
+
+#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<std::string, ItemDescriptor>) 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<OneShotServer> {
+   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<std::string> const& Target() const { return target_; }
+
+   private:
+    net::io_context& ioc_;
+    tcp::acceptor acceptor_;
+    tcp::socket socket_;
+    beast::flat_buffer buffer_;
+    http::request<http::string_body> request_;
+    http::response<http::string_body> response_;
+    std::optional<std::string> target_;
+};
+
+}  // namespace
+
+TEST(PollingDataSourceRequestTest, WithReasonsJoinsAnExistingQuery) {
+    net::io_context ioc;
+    auto server = std::make_shared<OneShotServer>(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<config::shared::ClientSDK>::PollingConfig(),
+        /* with_reasons= */ true, /* use_report= */ false};
+    auto const http_properties =
+        config::shared::Defaults<config::shared::ClientSDK>::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<PollingDataSource>(
+        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());
+}

libs/internal/include/launchdarkly/network/asio_requester.hpp

@@ -13,6 +13,7 @@
 #include <boost/beast/ssl.hpp>
 #include <boost/beast/version.hpp>
 #include <boost/core/ignore_unused.hpp>
+#include <boost/url.hpp>
 
 #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<HttpRequest> 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<net::ssl::context&> ToOptRef(

libs/internal/include/launchdarkly/network/http_requester.hpp

@@ -90,8 +90,16 @@ class HttpRequest {
         const;
     [[nodiscard]] std::string const& Host() const;
     [[nodiscard]] std::optional<std::string> 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,19 +151,41 @@ 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.
  *
  * @param url_in Input URL, if std::nullopt, the method will return
  * 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<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 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<std::string> AppendQueryParam(std::optional<std::string> url_in,
+                                            std::string const& key,
+                                            std::string const& value);
+
 }  // namespace launchdarkly::network