rest: Return error when header count is not integral#23213
rest: Return error when header count is not integral#23213maflcko merged 1 commit intobitcoin:masterfrom
Conversation
|
Concept ACK Very happy to see the |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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>();
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
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.
Leaving as is for now. |
|
Force pushed with a rebase. Should be easy to re-ACK with git range-diff (6 line diff). |
|
cr ACK fa8d492 |
| long count = strtol(path[0].c_str(), nullptr, 10); | ||
| if (count < 1 || count > 2000) | ||
| const auto parsed_count{ToIntegral<size_t>(path[0])}; | ||
| if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > 2000) { |
There was a problem hiding this comment.
nit,
if (!parsed_count.has_value()) {
return RESTERR(req, HTTP_BAD_REQUEST, "Header count invalid: " + SanitizeString(path[0]));
}There was a problem hiding this comment.
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.
…gral fa8d492 rest: Return error when header count is not integral (MarcoFalke) Pull request description: Seems odd to interpret a hash (or any other string) as integer when it contains more than the digits 0 to 9. ACKs for top commit: practicalswift: cr ACK fa8d492 promag: Code review ACK fa8d492. shaavan: Code Review ACK fa8d492 Tree-SHA512: d6335b132ca2010fb8cae311dd936b2dea99a5bd0e6b2556a604f93698b8456df9190c5151345a03344243ede4aad0e2526cedc2aa8b5b1b8e8ce786cb3b6e50
7 participants
Seems odd to interpret a hash (or any other string) as integer when it contains more than the digits 0 to 9.