Merge pull request #340 from mcdonaldseanp/PE36763

mcdonaldseanp · web-flow · commit 36b804bf0833 · 2024-02-16T15:33:07.000-06:00
(PE-36763) Add backwards-compat support for CURLOPT_PROTOCOLS_STR
diff --git a/.github/actions/run_cmake_and_make/action.yaml b/.github/actions/run_cmake_and_make/action.yaml
@@ -1,4 +1,4 @@
-name: docker_pull_and_make
+name: run_cmake_and_make
 
 inputs:
   pkg_suffix:
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,6 +1,9 @@
 cmake_minimum_required(VERSION 3.2.2)
 project(leatherman VERSION 1.12.12)
 
+# Set the cmake policy that allows -D<LIB>_ROOT settings, so that you can point to a specific boost or curl
+cmake_policy(SET CMP0074 NEW)
+
 option(DYNAMICBASE "Add dynamicbase linker option" ON)
 if (WIN32)
     if (DYNAMICBASE)
diff --git a/curl/inc/leatherman/curl/client.hpp b/curl/inc/leatherman/curl/client.hpp
@@ -366,6 +366,21 @@ namespace leatherman { namespace curl {
 
         /**
          * Set and limit what protocols curl will support
+         * @param client_protocols String indicating which protocol will be supported
+         *        (see more: http://curl.haxx.se/libcurl/c/CURLOPT_PROTOCOLS_STR.html)
+         */
+        void set_supported_protocols(std::string const& client_protocols);
+
+        /**
+         * WARNING: USING THE BITMASK IS TECHNICALLY DEPRECATED, USE THE STRING VERSION OF
+         *          THIS FUNCTION FOR ANY FUTURE USES OF set_supported_protocols.
+         *
+         * The bitmask version of this function is left here for compatibility purposes,
+         * and only supports passing the CURLPROTO_HTTP/HTTPS bitmasks. DO NOT ADD SUPPORT FOR ANY OTHER
+         * BITMASKS, NEW PROTOCOLS SHOULD BE USING cURL 8 AND PASSING THE STRING VERSION OF THE PROTOCOLS
+         *
+         * Parse the bitmask and convert to string, then use the string version of
+         * set_supported_protocols to set and limit what protocols curl will support
          * @param client_protocols bitmask of CURLPROTO_*
          *        (see more: http://curl.haxx.se/libcurl/c/CURLOPT_PROTOCOLS.html)
          */
@@ -403,7 +418,7 @@ namespace leatherman { namespace curl {
         std::string _client_key;
         std::string _client_crl;
         std::string _proxy;
-        long _client_protocols = CURLPROTO_ALL;
+        std::string _client_protocols = "all";
 
         response perform(http_method method, request const& req);
         void download_file_helper(request const& req,
diff --git a/curl/src/client.cc b/curl/src/client.cc
@@ -322,10 +322,30 @@ namespace leatherman { namespace curl {
         _client_key = client_key;
     }
 
-    void client::set_supported_protocols(long client_protocols) {
+    void client::set_supported_protocols(string const& client_protocols) {
         _client_protocols = client_protocols;
     }
 
+    void client::set_supported_protocols(long client_protocols) {
+        std::vector<std::string> protocols;
+
+        if (client_protocols == CURLPROTO_ALL) {
+            _client_protocols = "all";
+        } else {
+            if (client_protocols & CURLPROTO_HTTPS) {
+                protocols.push_back("https");
+            }
+            if (client_protocols & CURLPROTO_HTTP) {
+                protocols.push_back("http");
+            }
+            // If there was anything that _wasn't_ http or https we throw
+            if (client_protocols & ~(CURLPROTO_HTTP | CURLPROTO_HTTPS)) {
+                throw http_exception(_("Passing CURLPROTO_* bitmasks to set supported protocols is deprecated! Upgrade to cURL 8 and use the string version of set_supported_protocols instead"));
+            }
+        }
+        _client_protocols = boost::join(protocols, ",");
+    }
+
     void client::set_method(context& ctx, http_method method)
     {
         switch (method) {
@@ -469,7 +489,30 @@ namespace leatherman { namespace curl {
     }
 
     void client::set_client_protocols(context& ctx) {
-        curl_easy_setopt_maybe(ctx, CURLOPT_PROTOCOLS, _client_protocols);
+        #ifdef CURLOPT_PROTOCOLS_STR
+            curl_easy_setopt_maybe(ctx, CURLOPT_PROTOCOLS_STR, _client_protocols.c_str());
+        #else
+            // If leatherman is still being compiled with cURL versions less than 8, parse
+            // the string versions of the protocols back to bit masks. Allow use of HTTP/HTTPS and
+            // ALL, but otherwise fail and warn the user they need to upgrade cURL.
+            long protocols = 0;
+            if (_client_protocols == "all") {
+                curl_easy_setopt_maybe(ctx, CURLOPT_PROTOCOLS, CURLPROTO_ALL);
+            } else {
+                std::vector<std::string> split_protocols;
+                boost::split(split_protocols, _client_protocols, boost::is_any_of(","));
+                for (auto proto = split_protocols.begin(); proto != split_protocols.end(); proto++) {
+                    if (*proto == "https") {
+                        protocols |= CURLPROTO_HTTPS;
+                    } else if (*proto == "http") {
+                        protocols |= CURLPROTO_HTTP;
+                    } else {
+                        throw http_exception(_("Passing CURLPROTO_* bitmasks to set supported protocols is deprecated! Upgrade to cURL 8 and use the string version of set_supported_protocols instead"));
+                    }
+                }
+                curl_easy_setopt_maybe(ctx, CURLOPT_PROTOCOLS, protocols);
+            }
+        #endif
     }
 
     size_t client::read_body(char* buffer, size_t size, size_t count, void* ptr)
diff --git a/curl/tests/client_test.cc b/curl/tests/client_test.cc
@@ -34,7 +34,7 @@ fs::path find_matching_file(const boost::regex& re) {
         fs::recursive_directory_iterator(fs::current_path()),
         fs::recursive_directory_iterator(),
         [re](const fs::path& f) { return boost::regex_match(f.filename().string(), re); });
-  
+
     // throw exception, as this means that the matching file does not exist.
     if (file == fs::recursive_directory_iterator()) {
       throw std::runtime_error("matching file not found");
@@ -297,18 +297,43 @@ TEST_CASE("curl::client CA bundle and SSL setup") {
     }
 
     SECTION("cURL should make an HTTP request with the specified HTTP protocol") {
-        test_client.set_supported_protocols(CURLPROTO_HTTP);
+        test_client.set_supported_protocols("http");
+        auto resp = test_client.get(test_request);
+        CURL* const& handle = test_client.get_handle();
+        auto test_impl = reinterpret_cast<curl_impl* const>(handle);
+        REQUIRE(test_impl->protocols == "http");
+    }
+
+    SECTION("cURL should make an HTTP request with multiple specified protocols") {
+        test_client.set_supported_protocols("http,https");
+        auto resp = test_client.get(test_request);
+        CURL* const& handle = test_client.get_handle();
+        auto test_impl = reinterpret_cast<curl_impl* const>(handle);
+        // Either order is fine
+        REQUIRE((test_impl->protocols == "http,https" || test_impl->protocols == "https,http"));
+    }
+
+    SECTION("leatherman should throw an error when multiple specified protocols include anything other than http, https") {
+        REQUIRE_THROWS_AS(test_client.set_supported_protocols(CURLPROTO_HTTP | CURLPROTO_HTTPS | CURLPROTO_SFTP), http_exception);
+    }
+
+    SECTION("cURL should make an HTTP request with the HTTPS protocol when given the CURLPROTO_HTTPS bitmask") {
+        test_client.set_supported_protocols(CURLPROTO_HTTPS);
         auto resp = test_client.get(test_request);
         CURL* const& handle = test_client.get_handle();
         auto test_impl = reinterpret_cast<curl_impl* const>(handle);
-        REQUIRE(test_impl->protocols == CURLPROTO_HTTP);
+        REQUIRE(test_impl->protocols == "https");
+    }
+
+    SECTION("leatherman should throw an error when any bitmask other than CURLPROTO_HTTP/HTTPS/ALL is given to set_supported_protocols") {
+        REQUIRE_THROWS_AS(test_client.set_supported_protocols(CURLPROTO_SCP), http_exception);
     }
 
     SECTION("cURL defaults to all protocols if no protocols are specified") {
         auto resp = test_client.get(test_request);
         CURL* const& handle = test_client.get_handle();
         auto test_impl = reinterpret_cast<curl_impl* const>(handle);
-        REQUIRE(test_impl->protocols == CURLPROTO_ALL);
+        REQUIRE(test_impl->protocols == "all");
     }
 }
 
@@ -418,7 +443,7 @@ TEST_CASE("curl::client errors") {
     }
 
     SECTION("client fails to make http call with https protocol only enabled") {
-        test_client.set_supported_protocols(CURLPROTO_HTTPS);
+        test_client.set_supported_protocols("https");
         test_impl->test_failure_mode = curl_impl::error_mode::protocol_error;
         REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
     }
@@ -427,7 +452,7 @@ TEST_CASE("curl::client errors") {
     TEST_CASE("curl::client download_file") {
         mock_client test_client;
         temp_directory temp_dir;
-        fs::path temp_dir_path = fs::path(temp_dir.get_dir_name()); 
+        fs::path temp_dir_path = fs::path(temp_dir.get_dir_name());
         CURL* const& handle = test_client.get_handle();
         auto test_impl = reinterpret_cast<curl_impl* const>(handle);
         std::string url = "https://download.com";
@@ -440,7 +465,7 @@ TEST_CASE("curl::client errors") {
 
                 test_client.set_ca_cert(ca_file);
                 test_client.set_client_cert(cert_file, key_file);
-                test_client.set_supported_protocols(CURLPROTO_HTTPS);
+                test_client.set_supported_protocols("https");
 
                 std::string file_path = (temp_dir_path / "test_file").string();
                 std::string token = "token";
@@ -455,7 +480,7 @@ TEST_CASE("curl::client errors") {
                 REQUIRE(test_impl->cacert == ca_file);
                 REQUIRE(test_impl->client_cert == cert_file);
                 REQUIRE(test_impl->client_key == key_file);
-                REQUIRE(test_impl->protocols == CURLPROTO_HTTPS);
+                REQUIRE(test_impl->protocols == "https");
                 REQUIRE(test_impl->connect_timeout == connect_timeout);
                 REQUIRE(std::string(test_impl->header->data) == ("X-Authentication: " + token));
                 if (test_impl->header->next) {
@@ -487,7 +512,7 @@ TEST_CASE("curl::client errors") {
               std::string url = "https://download_trigger_404.com";
               auto file_path = (temp_dir_path / "404_test_file").string();
               request req(url);
-              test_client.download_file(req, file_path); 
+              test_client.download_file(req, file_path);
 
               // now check that the file was actually downloaded and written with the right
               // contents.
@@ -507,7 +532,7 @@ TEST_CASE("curl::client errors") {
 
               test_client.set_ca_cert(ca_file);
               test_client.set_client_cert(cert_file, key_file);
-              test_client.set_supported_protocols(CURLPROTO_HTTPS);
+              test_client.set_supported_protocols("https");
 
               std::string file_path = (temp_dir_path / "test_file").string();
               std::string token = "token";
@@ -523,7 +548,7 @@ TEST_CASE("curl::client errors") {
               REQUIRE(test_impl->cacert == ca_file);
               REQUIRE(test_impl->client_cert == cert_file);
               REQUIRE(test_impl->client_key == key_file);
-              REQUIRE(test_impl->protocols == CURLPROTO_HTTPS);
+              REQUIRE(test_impl->protocols == "https");
               REQUIRE(test_impl->connect_timeout == connect_timeout);
               REQUIRE(std::string(test_impl->header->data) == ("X-Authentication: " + token));
               if (test_impl->header->next) {
@@ -561,11 +586,11 @@ TEST_CASE("curl::client errors") {
               auto file_path = (temp_dir_path / "404_test_file").string();
               request req(url);
               response res;
-              test_client.download_file(req, file_path, res); 
+              test_client.download_file(req, file_path, res);
 
               REQUIRE(res.status_code() == 404);
               REQUIRE(res.body() == "Not found");
-              // check that the file was not downloaded 
+              // check that the file was not downloaded
               REQUIRE(!fs::exists(file_path));
           }
       }
@@ -574,7 +599,7 @@ TEST_CASE("curl::client errors") {
 TEST_CASE("curl::client download_file errors") {
     mock_client test_client;
     temp_directory temp_dir;
-    fs::path temp_dir_path = fs::path(temp_dir.get_dir_name()); 
+    fs::path temp_dir_path = fs::path(temp_dir.get_dir_name());
     CURL* const& handle = test_client.get_handle();
     auto test_impl = reinterpret_cast<curl_impl* const>(handle);
 
@@ -600,7 +625,7 @@ TEST_CASE("curl::client download_file errors") {
     SECTION("when curl_easy_perform fails due to a CURLE_WRITE_ERROR, but the temporary file is removed, an http_file_operation_exception is thrown") {
         std::string file_path = (temp_dir_path / "file").string();
         request req("");
-        test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_write_error; 
+        test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_write_error;
         REQUIRE_THROWS_AS_WITH(
             test_client.download_file(req, file_path),
             http_file_operation_exception,
@@ -610,7 +635,7 @@ TEST_CASE("curl::client download_file errors") {
     SECTION("when curl_easy_perform fails for reasons other than a CURLE_WRITE_ERROR, but the temporary file is removed, only the errbuf message is contained in the thrown http_file_download_exception") {
         std::string file_path = (temp_dir_path / "file").string();
         request req("");
-        test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_error; 
+        test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_error;
         REQUIRE_THROWS_AS_WITH(
             test_client.download_file(req, file_path),
             http_file_download_exception,
@@ -623,7 +648,7 @@ TEST_CASE("curl::client download_file errors") {
     SECTION("when renaming the temporary file to the user-provided file path fails, an http_file_operation_exception is thrown") {
         std::string file_path = (temp_dir_path / "file").string();
         request req("https://download.com");
-        test_impl->trigger_external_failure = remove_temp_file; 
+        test_impl->trigger_external_failure = remove_temp_file;
         REQUIRE_THROWS_AS_WITH(
             test_client.download_file(req, file_path),
             http_file_operation_exception,
diff --git a/curl/tests/mock_curl.cc b/curl/tests/mock_curl.cc
@@ -4,7 +4,9 @@
 #include <stdio.h>
 #include <array>
 #include <algorithm>
+#include <vector>
 #include "mock_curl.hpp"
+#include <boost/algorithm/string.hpp>
 
 using namespace std;
 
@@ -105,6 +107,8 @@ CURL *curl_easy_init()
 CURLcode curl_easy_setopt(CURL *handle, CURLoption option, ...)
 {
     auto h = reinterpret_cast<curl_impl*>(handle);
+    long curlopt;
+    std::vector<std::string> given_protocols;
     va_list vl;
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wvarargs"
@@ -259,10 +263,39 @@ CURLcode curl_easy_setopt(CURL *handle, CURLoption option, ...)
                 va_end(vl);
                 return CURLE_COULDNT_CONNECT;
             }
-            h->protocols = va_arg(vl, long);
+            // If still compiling with cURL versions less than 8, CURLOPT_PROTOCOLS
+            // will recieve a bitmask as the param, but the protocol entry type is
+            // a string. Using the string type avoids the need for additional special
+            // logic on creating and checking the value of this entry, but it also
+            // means now we need to parse the bitmasks back to strings.
+            //
+            // When compiling with cURL 8 or newer the client code will use the string
+            // version of the CURLOPT_PROTOCOLS function, so this logic won't be used.
+            curlopt = va_arg(vl, int);
+
+            if (curlopt == CURLPROTO_ALL) {
+                h->protocols = "all";
+            } else {
+                if (curlopt & CURLPROTO_HTTPS) {
+                    given_protocols.push_back("https");
+                }
+                if (curlopt & CURLPROTO_HTTP) {
+                    given_protocols.push_back("http") ;
+                }
+                h->protocols = boost::join(given_protocols,",");
+            }
+            break;
+        #ifdef CURLOPT_PROTOCOLS_STR
+        case CURLOPT_PROTOCOLS_STR:
+            if (h->test_failure_mode == curl_impl::error_mode::protocol_error) {
+                va_end(vl);
+                return CURLE_COULDNT_CONNECT;
+            }
+            h->protocols = va_arg(vl, char*);
             break;
+        #endif
         case CURLOPT_ERRORBUFFER:
-            h->errbuf = va_arg(vl, char*); 
+            h->errbuf = va_arg(vl, char*);
             break;
         default:
             break;
@@ -283,7 +316,7 @@ CURLcode curl_easy_perform(CURL * easy_handle)
     }
     if (h->test_failure_mode == curl_impl::error_mode::easy_perform_error) {
         if (h->errbuf) {
-            strcpy(h->errbuf, "easy perform failed"); 
+            strcpy(h->errbuf, "easy perform failed");
         }
         return CURLE_COULDNT_CONNECT;
     }
diff --git a/curl/tests/mock_curl.hpp b/curl/tests/mock_curl.hpp
@@ -59,7 +59,7 @@ struct curl_impl
     void* read_data; // Where to read the request body from
 
     std::string request_url, cookie, cacert, client_cert, client_key, client_crl, proxy;
-    long protocols;
+    std::string protocols;
     long connect_timeout;
     http_method method = http_method::get;
 

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-name: docker_pull_and_make`
	`1`	`+name: run_cmake_and_make`
`2`	`2`
`3`	`3`	`inputs:`
`4`	`4`	`pkg_suffix:`