[#478] Fix handling of plus (+) and use Boost.URL (main)

core/include/irods/private/http_api/common.hpp

@@ -151,8 +151,8 @@ namespace irods::http
 
 	auto encode(std::string_view _to_encode) -> std::string;
 
-	// TODO Create a better name.
-	auto to_argument_list(const std::string_view _urlencoded_string) -> std::unordered_map<std::string, std::string>;
+	auto parse_urlencoded_data(const std::string_view _urlencoded_string)
+		-> std::unordered_map<std::string, std::string>;
 
 	auto get_url_path(const std::string& _url) -> std::optional<std::string>;
 

core/src/common.cpp

@@ -30,6 +30,7 @@
 #include <boost/asio.hpp>
 #include <boost/algorithm/string.hpp>
 #include <boost/beast.hpp>
+#include <boost/url.hpp>
 
 #include <curl/curl.h>
 #include <fmt/format.h>
@@ -116,8 +117,8 @@ namespace irods::http
 		return encoded_data;
 	} // encode
 
-	// TODO Create a better name.
-	auto to_argument_list(const std::string_view _urlencoded_string) -> std::unordered_map<std::string, std::string>
+	auto parse_urlencoded_data(const std::string_view _urlencoded_string)
+		-> std::unordered_map<std::string, std::string>
 	{
 		if (_urlencoded_string.empty()) {
 			return {};
@@ -128,25 +129,37 @@ namespace irods::http
 		std::vector<std::string> tokens;
 		boost::split(tokens, _urlencoded_string, boost::is_any_of("&"));
 
-		std::vector<std::string> kvp;
-
 		for (auto&& t : tokens) {
-			boost::split(kvp, t, boost::is_any_of("="));
+			if (t.empty()) {
+				continue;
+			}
 
-			if (kvp.size() == 2) {
-				auto value = decode(kvp[1]);
-				boost::replace_all(value, "+", " ");
-				kvps.insert_or_assign(std::move(kvp[0]), value);
+			std::string name;
+			std::string value;
+
+			if (const auto pos = t.find('='); pos != std::string::npos) {
+				name = t.substr(0, pos);
+				value = t.substr(pos + 1);
+			}
+			else {
+				name = t;
+			}
+
+			if (!name.empty()) {
+				boost::replace_all(name, "+", " ");
+				name = decode(name);
 			}
-			else if (kvp.size() == 1) {
-				kvps.insert_or_assign(std::move(kvp[0]), "");
+
+			if (!value.empty()) {
+				boost::replace_all(value, "+", " ");
+				value = decode(value);
 			}
 
-			kvp.clear();
+			kvps.insert_or_assign(std::move(name), std::move(value));
 		}
 
 		return kvps;
-	} // to_argument_list
+	} // parse_urlencoded_data
 
 	auto get_url_path(const std::string& _url) -> std::optional<std::string>
 	{
@@ -180,65 +193,36 @@ namespace irods::http
 		return std::nullopt;
 	} // get_url_path
 
-	auto parse_url(const std::string& _url) -> url
+	auto parse_url(const request_type& _req) -> url
 	{
 		namespace logging = irods::http::log;
 
-		std::unique_ptr<CURLU, void (*)(CURLU*)> curl{curl_url(), curl_url_cleanup};
-
-		if (!curl) {
-			logging::error("{}: Could not initialize CURLU handle.", __func__);
-			THROW(SYS_LIBRARY_ERROR, "curl_url error.");
-		}
-
-		// Include a bogus prefix. We only care about the path and query parts of the URL.
-		if (const auto ec = curl_url_set(curl.get(), CURLUPART_URL, _url.c_str(), 0); ec) {
-			logging::error("{}: curl_url_set error: {}", __func__, ec);
-			THROW(SYS_LIBRARY_ERROR, "curl_url_set(CURLUPART_URL) error.");
-		}
-
 		url url;
 
-		using curl_string = std::unique_ptr<char, void (*)(void*)>;
-
-		// Extract the path.
-		// This is what we use to route requests to the various endpoints.
-		char* path{};
-		if (const auto ec = curl_url_get(curl.get(), CURLUPART_PATH, &path, 0); ec == 0) {
-			curl_string cpath{path, curl_free};
-			if (path) {
-				url.path = path;
-			}
-		}
-		else {
-			logging::error("{}: curl_url_get(CURLUPART_PATH) error: {}", __func__, ec);
-			THROW(SYS_LIBRARY_ERROR, "curl_url_get(CURLUPART_PATH) error.");
+		auto parse_result = boost::urls::parse_origin_form(_req.target());
+		if (!parse_result) {
+			const auto& ec = parse_result.error();
+			logging::error(
+				"{}: Could not parse URL path [{}]; error code=[{}], message=[{}].",
+				__func__,
+				_req.target(),
+				ec.value(),
+				ec.message());
+			THROW(SYS_LIBRARY_ERROR, "URL parse error");
 		}
 
-		// Extract the query.
-		// ChatGPT states that the values in the key value pairs must escape embedded equal signs.
-		// This allows the HTTP server to parse the query string correctly. Therefore, we don't have
-		// to protect against that case. The client must send the correct URL escaped input.
-		char* query{};
-		if (const auto ec = curl_url_get(curl.get(), CURLUPART_QUERY, &query, 0); ec == 0) {
-			curl_string cs{query, curl_free};
-			if (query) {
-				url.query = to_argument_list(query);
-			}
-		}
-		else {
-			logging::error("{}: curl_url_get(CURLUPART_QUERY) error: {}", __func__, ec);
-			THROW(SYS_LIBRARY_ERROR, "curl_url_get(CURLUPART_QUERY) error.");
+		// Extract the path. This is what we use to route requests to the various endpoints.
+		auto uv = *parse_result;
+		url.path = uv.encoded_path();
+
+		// Extract the query and create a mapping of key-value pairs.
+		for (auto&& p : uv.params()) {
+			url.query.insert_or_assign(p.key, p.value);
 		}
 
 		return url;
 	} // parse_url
 
-	auto parse_url(const request_type& _req) -> url
-	{
-		return parse_url(fmt::format("http://ignored{}", _req.target()));
-	} // parse_url
-
 	auto url_encode_body(const body_arguments& _args) -> std::string
 	{
 		auto encode_pair{[](const body_arguments::value_type& i) {
@@ -466,7 +450,7 @@ namespace irods::http
 				args = irods::http::parse_multipart_form_data(*boundary, _req.body());
 			}
 			else if (boost::istarts_with(content_type, "application/x-www-form-urlencoded")) {
-				args = irods::http::to_argument_list(_req.body());
+				args = irods::http::parse_urlencoded_data(_req.body());
 			}
 			else {
 				logging::error("{}: Content type [{}] not supported.", __func__, content_type);

test/test_irods_http_api.py

@@ -733,6 +733,46 @@ def test_server_reports_error_when_http_method_is_not_supported(self):
     def test_server_reports_error_when_op_is_not_supported(self):
         do_test_server_reports_error_when_op_is_not_supported(self)
 
+    def test_server_preserves_plus_sign_in_application_x_www_form_urlencoded_content(self):
+        rodsuser_headers = {'Authorization': f'Bearer {self.rodsuser_bearer_token}'}
+        collection = f'/{self.zone_name}/home/{self.rodsuser_username}/=issue+478'
+
+        try:
+            # Create a collection that has a "+" in its logical path.
+            r = requests.post(
+                self.url_endpoint,
+                headers={
+                    'Authorization': rodsuser_headers['Authorization'],
+                    'Content-Type': 'application/x-www-form-urlencoded'
+                },
+                data={
+                    'op': 'create',
+                    'lpath': collection
+                }
+            )
+            self.logger.debug(r.content)
+            self.assertEqual(r.status_code, 200)
+            self.assertEqual(r.json()['irods_response']['status_code'], 0)
+
+            # Show the "+" is preserved and NOT converted to a space (" ").
+            r = requests.get(f'{self.url_base}/query', headers=rodsuser_headers, params={
+                'op': 'execute_genquery',
+                'query': f"select COLL_NAME where COLL_NAME = '{collection}'"
+            })
+            self.logger.debug(r.content)
+            self.assertEqual(r.status_code, 200)
+            result = r.json()
+            self.assertEqual(result['irods_response']['status_code'], 0)
+            self.assertEqual(len(result['rows']), 1)
+            self.assertEqual(result['rows'][0][0], collection)
+
+        finally:
+            r = requests.post(self.url_endpoint, headers=rodsuser_headers, data={
+                'op': 'remove',
+                'lpath': collection
+            })
+            self.logger.debug(r.content)
+
     @unittest.skip('Test needs to be implemented.')
     def test_return_error_on_missing_parameters(self):
         pass