Merge bitcoin/bitcoin#29904: refactor: Use our own implementation of urlDecode

992c714451676cee33d3dff49f36329423270c1c common: Don't terminate on null character in UrlDecode (Fabian Jahr)
099fa571511f113e0056d4bc27b3153a42f9dc65 scripted-diff: Modernize name of urlDecode function and param (Fabian Jahr)
8f39aaae417c33490e0e41fb97620eb23ced3d05 refactor: Remove hooking code for urlDecode (Fabian Jahr)
650d43ec15f7a3ae38126f65ef8fa0b1fd3ee936 refactor: Replace libevent use in urlDecode with our own code (Fabian Jahr)
46bc6c2aaa613eef526b21a06bf21e8edde31a88 test: Add unit tests for urlDecode (Fabian Jahr)

Pull request description:

  Fixes #29654 (as a side-effect)

  Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.

  This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3542) we were using in `urlDecode` could be easily emulated in fewer LOC. This also ports the [applicable test vectors over from libevent](https://github.com/libevent/libevent/blob/master/test/regress_http.c#L3430).

ACKs for top commit:
  achow101:
    ACK 992c714451676cee33d3dff49f36329423270c1c
  theStack:
    Code-review ACK 992c714451676cee33d3dff49f36329423270c1c
  maflcko:
    ACK 992c714451676cee33d3dff49f36329423270c1c 👈
  stickies-v:
    ACK 992c714451676cee33d3dff49f36329423270c1c

Tree-SHA512: 78f76ae7ab3b6710eab2aaac20f55eb0da7803e057eaa6220e865f328666a5399ef1a479702aaf630b2f974ad3aa15e2b6adac9c11bc8c3d4be21e8af1667fea

13 files changed, 110 insertions, 31 deletions

diff --git a/configure.ac b/configure.ac
index 49fe3a22b0..c7d124a1f1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1707,7 +1707,6 @@ AM_CONDITIONAL([ENABLE_QT_TESTS], [test "$BUILD_TEST_QT" = "yes"])
 AM_CONDITIONAL([ENABLE_BENCH], [test "$use_bench" = "yes"])
 AM_CONDITIONAL([USE_QRCODE], [test "$use_qr" = "yes"])
 AM_CONDITIONAL([USE_LCOV], [test "$use_lcov" = "yes"])
-AM_CONDITIONAL([USE_LIBEVENT], [test "$use_libevent" = "yes"])
 AM_CONDITIONAL([HARDEN], [test "$use_hardening" = "yes"])
 AM_CONDITIONAL([ENABLE_SSE42], [test "$enable_sse42" = "yes"])
 AM_CONDITIONAL([ENABLE_SSE41], [test "$enable_sse41" = "yes"])
diff --git a/src/Makefile.am b/src/Makefile.am
index f48ed4d442..c2e0c7b5b8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -679,6 +679,7 @@ libbitcoin_common_a_SOURCES = \
   common/run_command.cpp \
   common/settings.cpp \
   common/system.cpp \
+  common/url.cpp \
   compressor.cpp \
   core_read.cpp \
   core_write.cpp \
@@ -711,11 +712,6 @@ libbitcoin_common_a_SOURCES = \
   script/solver.cpp \
   warnings.cpp \
   $(BITCOIN_CORE_H)
-
-if USE_LIBEVENT
-libbitcoin_common_a_CPPFLAGS += $(EVENT_CFLAGS)
-libbitcoin_common_a_SOURCES += common/url.cpp
-endif
 #
 
 # util #
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index cf88a02b95..942e0bf69b 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -85,6 +85,7 @@ BITCOIN_TESTS =\
   test/checkqueue_tests.cpp \
   test/coins_tests.cpp \
   test/coinstatsindex_tests.cpp \
+  test/common_url_tests.cpp \
   test/compilerbug_tests.cpp \
   test/compress_tests.cpp \
   test/crypto_tests.cpp \
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 161d703282..8901d10ef6 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -11,7 +11,6 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <compat/stdin.h>
 #include <policy/feerate.h>
@@ -51,7 +50,6 @@
 using CliClock = std::chrono::system_clock;
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
-UrlDecodeFn* const URL_DECODE = urlDecode;
 
 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
 static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
index d5dfbbec27..fe90958a5f 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -11,7 +11,6 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <interfaces/init.h>
 #include <key.h>
@@ -28,7 +27,6 @@
 #include <tuple>
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
-UrlDecodeFn* const URL_DECODE = nullptr;
 
 static void SetupWalletToolArgs(ArgsManager& argsman)
 {
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index 4f0a816388..54796c5abb 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -12,7 +12,6 @@
 #include <common/args.h>
 #include <common/init.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <compat/compat.h>
 #include <init.h>
 #include <interfaces/chain.h>
@@ -35,7 +34,6 @@
 using node::NodeContext;
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
-UrlDecodeFn* const URL_DECODE = urlDecode;
 
 #if HAVE_DECL_FORK
 
diff --git a/src/common/url.cpp b/src/common/url.cpp
index 053e1a825c..ecf88d07ea 100644
--- a/src/common/url.cpp
+++ b/src/common/url.cpp
@@ -4,19 +4,36 @@
 
 #include <common/url.h>
 
-#include <event2/http.h>
-
-#include <cstdlib>
+#include <charconv>
 #include <string>
+#include <string_view>
+#include <system_error>
 
-std::string urlDecode(const std::string &urlEncoded) {
+std::string UrlDecode(std::string_view url_encoded)
+{
     std::string res;
-    if (!urlEncoded.empty()) {
-        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
-        if (decoded) {
-            res = std::string(decoded);
-            free(decoded);
+    res.reserve(url_encoded.size());
+
+    for (size_t i = 0; i < url_encoded.size(); ++i) {
+        char c = url_encoded[i];
+        // Special handling for percent which should be followed by two hex digits
+        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
+        if (c == '%' && i + 2 < url_encoded.size()) {
+            unsigned int decoded_value{0};
+            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);
+
+            // Only if there is no error and the pointer is set to the end of
+            // the string, we can be sure both characters were valid hex
+            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
+                res += static_cast<char>(decoded_value);
+                // Next two characters are part of the percent encoding
+                i += 2;
+                continue;
+            }
+            // In case of invalid percent encoding, add the '%' and continue
         }
+        res += c;
     }
+
     return res;
 }
diff --git a/src/common/url.h b/src/common/url.h
index b16b8241af..203f41c70f 100644
--- a/src/common/url.h
+++ b/src/common/url.h
@@ -6,9 +6,12 @@
 #define BITCOIN_COMMON_URL_H
 
 #include <string>
+#include <string_view>
 
-using UrlDecodeFn = std::string(const std::string& url_encoded);
-UrlDecodeFn urlDecode;
-extern UrlDecodeFn* const URL_DECODE;
+/* Decode a URL.
+ *
+ * Notably this implementation does not decode a '+' to a ' '.
+ */
+std::string UrlDecode(std::string_view url_encoded);
 
 #endif // BITCOIN_COMMON_URL_H
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
index ded057dbfa..16befd99e8 100644
--- a/src/qt/main.cpp
+++ b/src/qt/main.cpp
@@ -4,7 +4,6 @@
 
 #include <qt/bitcoin.h>
 
-#include <common/url.h>
 #include <compat/compat.h>
 #include <util/translation.h>
 
@@ -17,7 +16,6 @@
 extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
     return QCoreApplication::translate("bitcoin-core", psz).toStdString();
 };
-UrlDecodeFn* const URL_DECODE = urlDecode;
 
 const std::function<std::string()> G_TEST_GET_FULL_NAME{};
 
diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp
new file mode 100644
index 0000000000..cc893cbed7
--- /dev/null
+++ b/src/test/common_url_tests.cpp
@@ -0,0 +1,72 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://opensource.org/license/mit/.
+
+#include <common/url.h>
+
+#include <string>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_AUTO_TEST_SUITE(common_url_tests)
+
+// These test vectors were ported from test/regress.c in the libevent library
+// which used to be a dependency of the UrlDecode function.
+
+BOOST_AUTO_TEST_CASE(encode_decode_test) {
+    BOOST_CHECK_EQUAL(UrlDecode("Hello"), "Hello");
+    BOOST_CHECK_EQUAL(UrlDecode("99"), "99");
+    BOOST_CHECK_EQUAL(UrlDecode(""), "");
+    BOOST_CHECK_EQUAL(UrlDecode("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_"),
+                      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789-.~_");
+    BOOST_CHECK_EQUAL(UrlDecode("%20"), " ");
+    BOOST_CHECK_EQUAL(UrlDecode("%FF%F0%E0"), "\xff\xf0\xe0");
+    BOOST_CHECK_EQUAL(UrlDecode("%01%19"), "\x01\x19");
+    BOOST_CHECK_EQUAL(UrlDecode("http%3A%2F%2Fwww.ietf.org%2Frfc%2Frfc3986.txt"),
+                      "http://www.ietf.org/rfc/rfc3986.txt");
+    BOOST_CHECK_EQUAL(UrlDecode("1%2B2%3D3"), "1+2=3");
+}
+
+BOOST_AUTO_TEST_CASE(decode_malformed_test) {
+    BOOST_CHECK_EQUAL(UrlDecode("%%xhello th+ere \xff"), "%%xhello th+ere \xff");
+
+    BOOST_CHECK_EQUAL(UrlDecode("%"), "%");
+    BOOST_CHECK_EQUAL(UrlDecode("%%"), "%%");
+    BOOST_CHECK_EQUAL(UrlDecode("%%%"), "%%%");
+    BOOST_CHECK_EQUAL(UrlDecode("%%%%"), "%%%%");
+
+    BOOST_CHECK_EQUAL(UrlDecode("+"), "+");
+    BOOST_CHECK_EQUAL(UrlDecode("++"), "++");
+
+    BOOST_CHECK_EQUAL(UrlDecode("?"), "?");
+    BOOST_CHECK_EQUAL(UrlDecode("??"), "??");
+
+    BOOST_CHECK_EQUAL(UrlDecode("%G1"), "%G1");
+    BOOST_CHECK_EQUAL(UrlDecode("%2"), "%2");
+    BOOST_CHECK_EQUAL(UrlDecode("%ZX"), "%ZX");
+
+    BOOST_CHECK_EQUAL(UrlDecode("valid%20string%G1"), "valid string%G1");
+    BOOST_CHECK_EQUAL(UrlDecode("%20invalid%ZX"), " invalid%ZX");
+    BOOST_CHECK_EQUAL(UrlDecode("%20%G1%ZX"), " %G1%ZX");
+
+    BOOST_CHECK_EQUAL(UrlDecode("%1 "), "%1 ");
+    BOOST_CHECK_EQUAL(UrlDecode("% 9"), "% 9");
+    BOOST_CHECK_EQUAL(UrlDecode(" %Z "), " %Z ");
+    BOOST_CHECK_EQUAL(UrlDecode(" % X"), " % X");
+
+    BOOST_CHECK_EQUAL(UrlDecode("%-1"), "%-1");
+    BOOST_CHECK_EQUAL(UrlDecode("%1-"), "%1-");
+}
+
+BOOST_AUTO_TEST_CASE(decode_lowercase_hex_test) {
+    BOOST_CHECK_EQUAL(UrlDecode("%f0%a0%b0"), "\xf0\xa0\xb0");
+}
+
+BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) {
+    std::string result1{"\0\0x\0\0", 5};
+    BOOST_CHECK_EQUAL(UrlDecode("%00%00x%00%00"), result1);
+    std::string result2{"abc\0\0", 5};
+    BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), result2);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp
index e81efac6e0..631da13803 100644
--- a/src/test/fuzz/string.cpp
+++ b/src/test/fuzz/string.cpp
@@ -90,7 +90,7 @@ FUZZ_TARGET(string)
     (void)ToUpper(random_string_1);
     (void)TrimString(random_string_1);
     (void)TrimString(random_string_1, random_string_2);
-    (void)urlDecode(random_string_1);
+    (void)UrlDecode(random_string_1);
     (void)ContainsNoNUL(random_string_1);
     (void)_(random_string_1.c_str());
     try {
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 2c18184261..38350b33cc 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -14,7 +14,6 @@
 #include <banman.h>
 #include <chainparams.h>
 #include <common/system.h>
-#include <common/url.h>
 #include <consensus/consensus.h>
 #include <consensus/params.h>
 #include <consensus/validation.h>
@@ -81,7 +80,6 @@ using node::RegenerateCommitments;
 using node::VerifyLoadedChainstate;
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
-UrlDecodeFn* const URL_DECODE = nullptr;
 
 /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
 static FastRandomContext g_insecure_rand_ctx_temp_path;
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 06ec7db1bc..1252843e9d 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -11,6 +11,7 @@
 #include <wallet/context.h>
 #include <wallet/wallet.h>
 
+#include <string_view>
 #include <univalue.h>
 
 #include <boost/date_time/posix_time/posix_time.hpp>
@@ -61,9 +62,9 @@ bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wal
 
 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
 {
-    if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
+    if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) {
         // wallet endpoint was used
-        wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
+        wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size()));
         return true;
     }
     return false;