Merge bitcoin/bitcoin#27468: bugfix: rest: avoid segfault for invalid URI

11422cc bugfix: rest: avoid segfault for invalid URI (pablomartin4btc)

Pull request description:

  Minimal fix to get it promptly into 25.0 release (suggested by  [stickies-v](bitcoin/bitcoin#27253 (review)) and supported by [vasild](bitcoin/bitcoin#27253 (review))  )

  Please check #27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix.

ACKs for top commit:
  achow101:
    ACK 11422cc
  stickies-v:
    re-ACK 11422cc

Tree-SHA512: 5af6b53fb266a12b463f960910556d5e97bc88b3c2a4f437ffa343886b38749e1eb058cf7bc64d62e82e1acf6232a186bddacd8f3b4500c87bf9e550a0153386

Author: fanquake

src/httpserver.cpp

@@ -673,6 +673,9 @@ std::optional<std::string> HTTPRequest::GetQueryParameter(const std::string& key
 std::optional<std::string> GetQueryParameterFromUri(const char* uri, const std::string& key)
 {
     evhttp_uri* uri_parsed{evhttp_uri_parse(uri)};
+    if (!uri_parsed) {
+        throw std::runtime_error("URI parsing failed, it likely contained RFC 3986 invalid characters");
+    }
     const char* query{evhttp_uri_get_query(uri_parsed)};
     std::optional<std::string> result;
 

src/rest.cpp

@@ -200,7 +200,11 @@ static bool rest_headers(const std::any& context,
     } else if (path.size() == 1) {
         // new path with query parameter: /rest/headers/<hash>?count=<count>
         hashStr = path[0];
-        raw_count = req->GetQueryParameter("count").value_or("5");
+        try {
+            raw_count = req->GetQueryParameter("count").value_or("5");
+        } catch (const std::runtime_error& e) {
+            return RESTERR(req, HTTP_BAD_REQUEST, e.what());
+        }
     } else {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/headers/<hash>.<ext>?count=<count>");
     }
@@ -371,7 +375,11 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
     } else if (uri_parts.size() == 2) {
         // new path with query parameter: /rest/blockfilterheaders/<filtertype>/<blockhash>?count=<count>
         raw_blockhash = uri_parts[1];
-        raw_count = req->GetQueryParameter("count").value_or("5");
+        try {
+            raw_count = req->GetQueryParameter("count").value_or("5");
+        } catch (const std::runtime_error& e) {
+            return RESTERR(req, HTTP_BAD_REQUEST, e.what());
+        }
     } else {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilterheaders/<filtertype>/<blockhash>.<ext>?count=<count>");
     }
@@ -652,11 +660,21 @@ static bool rest_mempool(const std::any& context, HTTPRequest* req, const std::s
     case RESTResponseFormat::JSON: {
         std::string str_json;
         if (param == "contents") {
-            const std::string raw_verbose{req->GetQueryParameter("verbose").value_or("true")};
+            std::string raw_verbose;
+            try {
+                raw_verbose = req->GetQueryParameter("verbose").value_or("true");
+            } catch (const std::runtime_error& e) {
+                return RESTERR(req, HTTP_BAD_REQUEST, e.what());
+            }
             if (raw_verbose != "true" && raw_verbose != "false") {
                 return RESTERR(req, HTTP_BAD_REQUEST, "The \"verbose\" query parameter must be either \"true\" or \"false\".");
             }
-            const std::string raw_mempool_sequence{req->GetQueryParameter("mempool_sequence").value_or("false")};
+            std::string raw_mempool_sequence;
+            try {
+                raw_mempool_sequence = req->GetQueryParameter("mempool_sequence").value_or("false");
+            } catch (const std::runtime_error& e) {
+                return RESTERR(req, HTTP_BAD_REQUEST, e.what());
+            }
             if (raw_mempool_sequence != "true" && raw_mempool_sequence != "false") {
                 return RESTERR(req, HTTP_BAD_REQUEST, "The \"mempool_sequence\" query parameter must be either \"true\" or \"false\".");
             }

src/test/httpserver_tests.cpp

@@ -34,5 +34,9 @@ BOOST_AUTO_TEST_CASE(test_query_parameters)
     // Invalid query string syntax is the same as not having parameters
     uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2";
     BOOST_CHECK(!GetQueryParameterFromUri(uri.c_str(), "p1").has_value());
+
+    // URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried
+    uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2%";
+    BOOST_CHECK_EXCEPTION(GetQueryParameterFromUri(uri.c_str(), "p1"), std::runtime_error, HasReason("URI parsing failed, it likely contained RFC 3986 invalid characters"));
 }
 BOOST_AUTO_TEST_SUITE_END()

test/functional/interface_rest.py

@@ -277,6 +277,10 @@ def run_test(self):
         assert_equal(len(json_obj), 1)  # ensure that there is one header in the json response
         assert_equal(json_obj[0]['hash'], bb_hash)  # request/response hash should be the same
 
+        # Check invalid uri (% symbol at the end of the request)
+        resp = self.test_rest_request(f"/headers/{bb_hash}%", ret_type=RetType.OBJ, status=400)
+        assert_equal(resp.read().decode('utf-8').rstrip(), "URI parsing failed, it likely contained RFC 3986 invalid characters")
+
         # Compare with normal RPC block response
         rpc_block_json = self.nodes[0].getblock(bb_hash)
         for key in ['hash', 'confirmations', 'height', 'version', 'merkleroot', 'time', 'nonce', 'bits', 'difficulty', 'chainwork', 'previousblockhash']: