rest: Return error when header count is not integral #23213

MarcoFalke commented at 9:00 AM on October 7, 2021: member

Seems odd to interpret a hash (or any other string) as integer when it contains more than the digits 0 to 9.

practicalswift commented at 11:40 AM on October 7, 2021: contributor

Concept ACK

Very happy to see the KNOWN_VIOLATIONS list in test/lint/lint-locale-dependence.sh shrink. Only a few violations left to tackle! :)

DrahtBot added the label RPC/REST/ZMQ on Oct 7, 2021

DrahtBot commented at 5:44 PM on October 7, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)
#17631 (Expose block filters over REST. by TheBlueMatt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/interface_rest.py:283 in fabff34c6d outdated

 278 | @@ -279,6 +279,28 @@ def run_test(self):
 279 |          json_obj = self.test_rest_request(f"/headers/5/{bb_hash}")
 280 |          assert_equal(len(json_obj), 5)  # now we should have 5 header objects
 281 |  
 282 | +        # Test number parsing
 283 | +        assert_equal(

brunoerg commented at 12:51 PM on October 9, 2021:

Wouldn't it be better using a loop to avoid repetition? like:

items = ['5a', '-5', '0', '2001', '99999999999999999999999999999999999']
   for i in items:
     assert_equal(
        bytes(f'Header count out of range: {i}\r\n', 'utf8'),
        self.test_rest_request(f"/headers/{i}/{bb_hash}", ret_type=RetType.BYTES, status=400),
     )

MarcoFalke commented at 9:21 AM on October 11, 2021:

Thanks, done

MarcoFalke force-pushed on Oct 11, 2021

jonatack commented at 5:48 PM on October 11, 2021: member

ACK fa21fd44e24bb9e4e19e3e8bc4f0de8df34d9535

a couple ideas, feel free to ignore

diff --git a/src/rest.cpp b/src/rest.cpp
index d2fc35e818..0c2cb96622 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -194,7 +194,7 @@ static bool rest_headers(const std::any& context,
         return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
     }
 
-    std::string hashStr = path[1];
+    const std::string& hashStr = path[1];
     uint256 hash;
     if (!ParseHashStr(hashStr, hash))
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index b5088d3c33..bcff4b37a8 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1527,6 +1527,7 @@ BOOST_AUTO_TEST_CASE(test_ToIntegral)
     BOOST_CHECK_EQUAL(ToIntegral<int32_t>("-1234").value(), -1'234);
     BOOST_CHECK_EQUAL(ToIntegral<int32_t>("-1").value(), -1);
 
+    RunToIntegralTests<size_t>();
     RunToIntegralTests<uint64_t>();
     RunToIntegralTests<int64_t>();
     RunToIntegralTests<uint32_t>();

in src/rest.cpp:193 in fa21fd44e2 outdated

 188 | @@ -189,18 +189,19 @@ static bool rest_headers(const std::any& context,
 189 |      if (path.size() != 2)
 190 |          return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
 191 |  
 192 | -    long count = strtol(path[0].c_str(), nullptr, 10);
 193 | -    if (count < 1 || count > 2000)
 194 | +    const auto parsed_count{ToIntegral<size_t>(path[0])};
 195 | +    if (!parsed_count || *parsed_count < 1 || *parsed_count > 2000) {

shaavan commented at 6:00 PM on October 11, 2021:

For a very similar reason as specified here I would suggest making the following change.

    if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > 2000) {

Doubt: In the PR, 23227, you used checked access for the variable num. But in this PR, you used unchecked access for the variable parsed_count, though they had a very similar use case. Is there any specific reason behind doing so? (Checked Unchecked access reference)

MarcoFalke commented at 7:07 AM on October 12, 2021:

Doubt: In the PR, 23227, you used checked access for the variable num. But in this PR, you used unchecked access for the variable parsed_count, though they had a very similar use case. Is there any specific reason behind doing so? (Checked Unchecked access reference)

bitcoin-tx is allowed to throw exceptions, so there is no downside in using .value(), thus doing the check twice. rest is not allowed to do that, so we do the check only once, which is sufficient.

MarcoFalke commented at 7:09 AM on October 12, 2021:

has_value

Thanks, done.

shaavan commented at 12:24 PM on October 12, 2021:

bitcoin-tx is allowed to throw exceptions, so there is no downside in using .value(), thus doing the check twice. rest is not allowed to do that, so we do the check only once, which is sufficient.

That makes sense. Thanks for clearing it out!

shaavan commented at 6:04 PM on October 11, 2021: contributor

Concept ACK

I agree with the concept of PR. It shouldn’t be possible for a non-integer header count to be valid, and this PR makes sure that it will always cause an error. The tests are correct, too, and appropriately check the added code.

I have some suggestions and doubts regarding this PR, though. And I would kindly appreciate it if you would help me clear them out. Thank You.

rest: Return error when header count is not integral fa8d492894

MarcoFalke force-pushed on Oct 12, 2021

MarcoFalke commented at 7:11 AM on October 12, 2021: member

a couple ideas, feel free to ignore

Leaving as is for now.

MarcoFalke commented at 7:12 AM on October 12, 2021: member

Force pushed with a rebase. Should be easy to re-ACK with git range-diff (6 line diff).

practicalswift commented at 8:08 AM on October 12, 2021: contributor

cr ACK fa8d49289479b8eda7ba7530515c414d1cd566a3

in src/rest.cpp:193 in fa8d492894

 188 | @@ -189,18 +189,19 @@ static bool rest_headers(const std::any& context,
 189 |      if (path.size() != 2)
 190 |          return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
 191 |  
 192 | -    long count = strtol(path[0].c_str(), nullptr, 10);
 193 | -    if (count < 1 || count > 2000)
 194 | +    const auto parsed_count{ToIntegral<size_t>(path[0])};
 195 | +    if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > 2000) {

promag commented at 8:10 AM on October 12, 2021:

nit,

if (!parsed_count.has_value()) {
    return RESTERR(req, HTTP_BAD_REQUEST, "Header count invalid: " + SanitizeString(path[0]));
}

MarcoFalke commented at 8:58 AM on October 12, 2021:

This would also print "invalid" instead of "out of range" for -1 and 99999999999999999999999999999999999. I think the current code is fine. Leaving as is for now.

promag commented at 8:12 AM on October 12, 2021: member

Code review ACK fa8d49289479b8eda7ba7530515c414d1cd566a3.

Only suggestion is to have a different error.

shaavan approved

shaavan commented at 12:31 PM on October 12, 2021: contributor

Code Review ACK fa8d49289479b8eda7ba7530515c414d1cd566a3

Changes since my last review:

!parsed_count -> !parsed_count.has_value()
Rebased on current master

MarcoFalke merged this on Oct 12, 2021

MarcoFalke closed this on Oct 12, 2021

MarcoFalke deleted the branch on Oct 12, 2021

sidhujag referenced this in commit 3d1d864365 on Oct 12, 2021

DrahtBot locked this on Oct 30, 2022