From 46bc6c2aaa613eef526b21a06bf21e8edde31a88 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 20 Apr 2024 16:39:16 +0200 Subject: [PATCH 1/5] test: Add unit tests for urlDecode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lőrinc --- src/Makefile.test.include | 1 + src/test/common_url_tests.cpp | 70 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 src/test/common_url_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index cf88a02b95f..942e0bf69b3 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/test/common_url_tests.cpp b/src/test/common_url_tests.cpp new file mode 100644 index 00000000000..88bc17f22a8 --- /dev/null +++ b/src/test/common_url_tests.cpp @@ -0,0 +1,70 @@ +// 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 + +#include + +#include + +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) { + BOOST_CHECK_EQUAL(urlDecode("%00%00x%00%00"), ""); + BOOST_CHECK_EQUAL(urlDecode("abc%00%00"), "abc"); +} + +BOOST_AUTO_TEST_SUITE_END() From 650d43ec15f7a3ae38126f65ef8fa0b1fd3ee936 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 20 Apr 2024 16:35:39 +0200 Subject: [PATCH 2/5] refactor: Replace libevent use in urlDecode with our own code --- configure.ac | 1 - src/Makefile.am | 6 +----- src/common/url.cpp | 40 +++++++++++++++++++++++++++++++--------- src/common/url.h | 3 ++- src/wallet/rpc/util.cpp | 5 +++-- 5 files changed, 37 insertions(+), 18 deletions(-) diff --git a/configure.ac b/configure.ac index febb352cdbe..494cb3ce50d 100644 --- a/configure.ac +++ b/configure.ac @@ -1706,7 +1706,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 7673d2f5451..7387eb1eaa3 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/common/url.cpp b/src/common/url.cpp index 053e1a825c5..d0bf4e0cbe4 100644 --- a/src/common/url.cpp +++ b/src/common/url.cpp @@ -4,19 +4,41 @@ #include -#include - -#include +#include #include +#include +#include -std::string urlDecode(const std::string &urlEncoded) { +std::string urlDecode(std::string_view urlEncoded) +{ 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(urlEncoded.size()); + + for (size_t i = 0; i < urlEncoded.size(); ++i) { + char c = urlEncoded[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 < urlEncoded.size()) { + unsigned int decoded_value{0}; + auto [p, ec] = std::from_chars(urlEncoded.data() + i + 1, urlEncoded.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 == urlEncoded.data() + i + 3) { + // A null character terminates the string + if (decoded_value == 0) { + return res; + } + + res += static_cast(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 b16b8241af5..7b0a9447760 100644 --- a/src/common/url.h +++ b/src/common/url.h @@ -6,8 +6,9 @@ #define BITCOIN_COMMON_URL_H #include +#include -using UrlDecodeFn = std::string(const std::string& url_encoded); +using UrlDecodeFn = std::string(std::string_view url_encoded); UrlDecodeFn urlDecode; extern UrlDecodeFn* const URL_DECODE; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 06ec7db1bc2..f44ee7696c8 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -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 (URL_DECODE && request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = URL_DECODE(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; From 8f39aaae417c33490e0e41fb97620eb23ced3d05 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 20 Apr 2024 16:49:02 +0200 Subject: [PATCH 3/5] refactor: Remove hooking code for urlDecode The point of this was to be able to build bitcoin-tx and bitcoin-wallet without libevent, see #18504. Now that we use our own implementation of urlDecode this is not needed anymore. Co-authored-by: stickies-v --- src/bitcoin-cli.cpp | 2 -- src/bitcoin-wallet.cpp | 2 -- src/bitcoind.cpp | 2 -- src/common/url.h | 8 +++++--- src/qt/main.cpp | 2 -- src/test/util/setup_common.cpp | 2 -- src/wallet/rpc/util.cpp | 4 ++-- 7 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 129deeec600..830171f63a6 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -51,7 +50,6 @@ using CliClock = std::chrono::system_clock; const std::function 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 d5dfbbec271..fe90958a5f5 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -28,7 +27,6 @@ #include const std::function 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 4f0a816388e..54796c5abbf 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include @@ -35,7 +34,6 @@ using node::NodeContext; const std::function G_TRANSLATION_FUN = nullptr; -UrlDecodeFn* const URL_DECODE = urlDecode; #if HAVE_DECL_FORK diff --git a/src/common/url.h b/src/common/url.h index 7b0a9447760..42e982fb941 100644 --- a/src/common/url.h +++ b/src/common/url.h @@ -8,8 +8,10 @@ #include #include -using UrlDecodeFn = std::string(std::string_view 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 ded057dbfaf..16befd99e89 100644 --- a/src/qt/main.cpp +++ b/src/qt/main.cpp @@ -4,7 +4,6 @@ #include -#include #include #include @@ -17,7 +16,6 @@ extern const std::function G_TRANSLATION_FUN = [](const char* psz) { return QCoreApplication::translate("bitcoin-core", psz).toStdString(); }; -UrlDecodeFn* const URL_DECODE = urlDecode; const std::function G_TEST_GET_FULL_NAME{}; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2c18184261e..38350b33cc5 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -14,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -81,7 +80,6 @@ using node::RegenerateCommitments; using node::VerifyLoadedChainstate; const std::function 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 f44ee7696c8..eb081e765ad 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -62,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.starts_with(WALLET_ENDPOINT_BASE)) { + if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = URL_DECODE(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = urlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; From 099fa571511f113e0056d4bc27b3153a42f9dc65 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 20 Apr 2024 17:05:18 +0200 Subject: [PATCH 4/5] scripted-diff: Modernize name of urlDecode function and param -BEGIN VERIFY SCRIPT- sed -i 's/urlDecode/UrlDecode/g' $(git grep -l 'urlDecode' ./src) sed -i 's/urlEncoded/url_encoded/g' $(git grep -l 'urlEncoded' ./src) -END VERIFY SCRIPT- --- src/common/url.cpp | 14 ++++---- src/common/url.h | 2 +- src/test/common_url_tests.cpp | 68 +++++++++++++++++------------------ src/test/fuzz/string.cpp | 2 +- src/wallet/rpc/util.cpp | 2 +- 5 files changed, 44 insertions(+), 44 deletions(-) diff --git a/src/common/url.cpp b/src/common/url.cpp index d0bf4e0cbe4..f27bb2b73fc 100644 --- a/src/common/url.cpp +++ b/src/common/url.cpp @@ -9,22 +9,22 @@ #include #include -std::string urlDecode(std::string_view urlEncoded) +std::string UrlDecode(std::string_view url_encoded) { std::string res; - res.reserve(urlEncoded.size()); + res.reserve(url_encoded.size()); - for (size_t i = 0; i < urlEncoded.size(); ++i) { - char c = urlEncoded[i]; + 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 < urlEncoded.size()) { + if (c == '%' && i + 2 < url_encoded.size()) { unsigned int decoded_value{0}; - auto [p, ec] = std::from_chars(urlEncoded.data() + i + 1, urlEncoded.data() + i + 3, decoded_value, 16); + 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 == urlEncoded.data() + i + 3) { + if (ec == std::errc{} && p == url_encoded.data() + i + 3) { // A null character terminates the string if (decoded_value == 0) { return res; diff --git a/src/common/url.h b/src/common/url.h index 42e982fb941..203f41c70f2 100644 --- a/src/common/url.h +++ b/src/common/url.h @@ -12,6 +12,6 @@ * * Notably this implementation does not decode a '+' to a ' '. */ -std::string urlDecode(std::string_view url_encoded); +std::string UrlDecode(std::string_view url_encoded); #endif // BITCOIN_COMMON_URL_H diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp index 88bc17f22a8..eb92c2ceeea 100644 --- a/src/test/common_url_tests.cpp +++ b/src/test/common_url_tests.cpp @@ -11,60 +11,60 @@ 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. +// 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-.~_"), + 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"), + 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_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("%%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("+"), "+"); - 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("%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("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("% 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_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_CHECK_EQUAL(UrlDecode("%f0%a0%b0"), "\xf0\xa0\xb0"); } BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) { - BOOST_CHECK_EQUAL(urlDecode("%00%00x%00%00"), ""); - BOOST_CHECK_EQUAL(urlDecode("abc%00%00"), "abc"); + BOOST_CHECK_EQUAL(UrlDecode("%00%00x%00%00"), ""); + BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), "abc"); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index e81efac6e0d..631da13803a 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/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index eb081e765ad..1252843e9d0 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -64,7 +64,7 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& { if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) { // wallet endpoint was used - wallet_name = urlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); + wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size())); return true; } return false; From 992c714451676cee33d3dff49f36329423270c1c Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 21 Apr 2024 21:48:05 +0200 Subject: [PATCH 5/5] common: Don't terminate on null character in UrlDecode The previous behavior was the result of casting the result returned from the libevent function evhttp_uridecode to std:string but this was probably not intended. --- src/common/url.cpp | 5 ----- src/test/common_url_tests.cpp | 6 ++++-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/common/url.cpp b/src/common/url.cpp index f27bb2b73fc..ecf88d07ea5 100644 --- a/src/common/url.cpp +++ b/src/common/url.cpp @@ -25,11 +25,6 @@ std::string UrlDecode(std::string_view url_encoded) // 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) { - // A null character terminates the string - if (decoded_value == 0) { - return res; - } - res += static_cast(decoded_value); // Next two characters are part of the percent encoding i += 2; diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp index eb92c2ceeea..cc893cbed72 100644 --- a/src/test/common_url_tests.cpp +++ b/src/test/common_url_tests.cpp @@ -63,8 +63,10 @@ BOOST_AUTO_TEST_CASE(decode_lowercase_hex_test) { } BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) { - BOOST_CHECK_EQUAL(UrlDecode("%00%00x%00%00"), ""); - BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), "abc"); + 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()