diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5355878fa79..ceca1eaa6fc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -73,27 +73,235 @@ The source must be formatted according to the style guide below. Header includes must be [levelized](./Builds/levelization). +Changes should be usually squashed down into a single commit. +Some larger or more complicated change sets make more sense, +and are easier to review if organized into multiple logical commits. +Either way, all commits should fit the following criteria: +* Changes should be presented in a single commit or a logical + sequence of commits. + Specifically, chronological commits that simply + reflect the history of how the author implemented + the change, "warts and all", are not useful to + reviewers. +* Every commit should have a [good message](#good-commit-messages). + to explain a specific aspects of the change. +* Every commit should be signed. +* Every commit should be well-formed (builds successfully, + unit tests passing), as this helps to resolve merge + conflicts, and makes it easier to use `git bisect` + to find bugs. + +### Good commit messages + +Refer to +["How to Write a Git Commit Message"](https://cbea.ms/git-commit/) +for general rules on writing a good commit message. + +In addition to those guidelines, please add one of the following +prefixes to the subject line if appropriate. +* `fix:` - The primary purpose is to fix an existing bug. +* `perf:` - The primary purpose is performance improvements. +* `refactor:` - The changes refactor code without affecting + functionality. +* `test:` - The changes _only_ affect unit tests. +* `docs:` - The changes _only_ affect documentation. This can + include code comments in addition to `.md` files like this one. +* `build:` - The changes _only_ affect the build process, + including CMake and/or Conan settings. +* `chore:` - Other tasks that don't affect the binary, but don't fit + any of the other cases. e.g. formatting, git settings, updating + Github Actions jobs. + +Whenever possible, when updating commits after the PR is open, please +add the PR number to the end of the subject line. e.g. `test: Add +unit tests for Feature X (#1234)`. ## Pull requests In general, pull requests use `develop` as the base branch. - (Hotfixes are an exception.) -Changes to pull requests must be added as new commits. -Once code reviewers have started looking at your code, please avoid -force-pushing a branch in a pull request. +If your changes are not quite ready, but you want to make it easily available +for preliminary examination or review, you can create a "Draft" pull request. +While a pull request is marked as a "Draft", you can rebase or reorganize the +commits in the pull request as desired. + +Github pull requests are created as "Ready" by default, or you can mark +a "Draft" pull request as "Ready". +Once a pull request is marked as "Ready", +any changes must be added as new commits. Do not +force-push to a branch in a pull request under review. +(This includes rebasing your branch onto the updated base branch. +Use a merge operation, instead or hit the "Update branch" button +at the bottom of the Github PR page.) This preserves the ability for reviewers to filter changes since their last review. -A pull request must obtain **approvals from at least two reviewers** before it -can be considered for merge by a Maintainer. +A pull request must obtain **approvals from at least two reviewers** +before it can be considered for merge by a Maintainer. Maintainers retain discretion to require more approvals if they feel the credibility of the existing approvals is insufficient. Pull requests must be merged by [squash-and-merge][2] to preserve a linear history for the `develop` branch. +### When and how to merge pull requests + +#### "Passed" + +A pull request should only have the "Passed" label added when it +meets a few criteria: + +1. It must have two approving reviews [as described + above](#pull-requests). (Exception: PRs that are deemed "trivial" + only need one approval.) +2. All CI checks must be complete and passed. (One-off failures may + be acceptable if they are related to a known issue.) +3. The PR must have a [good commit message](#good-commit-messages). + * If the PR started with a good commit message, and it doesn't + need to be updated, the author can indicate that in a comment. + * Any contributor, preferably the author, can leave a comment + suggesting a commit message. + * If the author squashes and rebases the code in preparation for + merge, they should also ensure the commit message(s) are updated + as well. +4. The PR branch must be up to date with the base branch (usually + `develop`). This is usually accomplised by merging the base branch + into the feature branch, but if the other criteria are met, the + changes can be squashed and rebased on top of the base branch. +5. Finally, and most importantly, the author of the PR must + positively indicate that the PR is ready to merge. That can be + accomplished by adding the "Passed" label if their role allows, + or by leaving a comment to the effect that the PR is ready to + merge. + +Once the "Passed" label is added, a maintainer may merge the PR at +any time, so don't use it lightly. + +#### Instructions for maintainers + +The maintainer should double-check that the PR has met all the +necessary criteria, and can request additional information from the +owner, or additional reviews, and can always feel free to remove the +"Passed" label if appropriate. The maintainer has final say on +whether a PR gets merged, and are encouraged to communicate and +issues or concerns to other maintainers. + +##### Most pull requests: "Squash and merge" + +Most pull requests don't need special handling, and can simply be +merged using the "Squash and merge" button on the Github UI. Update +the suggested commit message if necessary. + +##### Slightly more complicated pull requests + +Some pull requests need to be pushed to `develop` as more than one +commit. There are multiple ways to accomplish this. If the author +describes a process, and it is reasonable, follow it. Otherwise, do +a fast forward only merge (`--ff-only`) on the command line and push. + +Either way, check that: +* The commits are based on the current tip of `develop`. +* The commits are clean: No merge commits (except when reverse + merging), no "[FOLD]" or "fixup!" messages. +* All commits are signed. If the commits are not signed by the author, use + `git commit --amend -S` to sign them yourself. +* At least one (but preferably all) of the commits has the PR number + in the commit message. + +**Never use the "Create a merge commit" or "Rebase and merge" + functions!** + +##### Releases, release candidates, and betas + +All releases, including release candidates and betas, are handled +differently from typical PRs. Most importantly, never use +the Github UI to merge a release. + +1. There are two possible conditions that the `develop` branch will + be in when preparing a release. + 1. Ready or almost ready to go: There may be one or two PRs that + need to be merged, but otherwise, the only change needed is to + update the version number in `BuildInfo.cpp`. In this case, + merge those PRs as appropriate, updating the second one, and + waiting for CI to finish in between. Then update + `BuildInfo.cpp`. + 2. Several pending PRs: In this case, do not use the Github UI, + because the delays waiting for CI in between each merge will be + unnecessarily onerous. Instead, create a working branch (e.g. + `develop-next`) based off of `develop`. Squash the changes + from each PR onto the branch, one commit each (unless + more are needed), being sure to sign each commit and update + the commit message to include the PR number. You may be able + to use a fast-forward merge for the first PR. The workflow may + look something like: +``` +git fetch upstream +git checkout upstream/develop +git checkout -b develop-next +# Use -S on the ff-only merge if prbranch1 isn't signed. +# Or do another branch first. +git merge --ff-only user1/prbranch1 +git merge --squash user2/prbranch2 +git commit -S +git merge --squash user3/prbranch3 +git commit -S +[...] +git push --set-upstream origin develop-next + +``` +2. Create the Pull Request with `release` as the base branch. If any + of the included PRs are still open, + [use closing keywords](https://docs.github.com/articles/closing-issues-using-keywords) + in the description to ensure they are closed when the code is + released. e.g. "Closes #1234" +3. Instead of the default template, reuse and update the message from + the previous release. Include the following verbiage somewhere in + the description: +``` +The base branch is release. All releases (including betas) go in +release. This PR will be merged with --ff-only (not squashed or +rebased, and not using the GitHub UI) to both release and develop. +``` +4. Sign-offs for the three platforms usually occur offline, but at + least one approval will be needed on the PR. +5. Once everything is ready to go, open a terminal, and do the + fast-forward merges manually. Do not push any branches until you + verify that all of them update correctly. +``` +git fetch upstream +git checkout -b upstream--develop -t upstream/develop || git checkout upstream--develop +git reset --hard upstream/develop +# develop-next must be signed already! +git merge --ff-only origin/develop-next +git checkout -b upstream--release -t upstream/release || git checkout upstream--release +git reset --hard upstream/release +git merge --ff-only origin/develop-next +# Only do these 3 steps if pushing a release. No betas or RCs +git checkout -b upstream--master -t upstream/master || git checkout upstream--master +git reset --hard upstream/master +git merge --ff-only origin/develop-next +# Check that all of the branches are updated +git log -1 --oneline +# The output should look like: +# 02ec8b7962 (HEAD -> upstream--master, origin/develop-next, upstream--release, upstream--develop, develop-next) Set version to 2.2.0-rc1 +# Note that all of the upstream--develop/release/master are on this commit. +# (Master will be missing for betas, etc.) +# Just to be safe, do a dry run first: +git push --dry-run upstream-push HEAD:develop +git push --dry-run upstream-push HEAD:release +# git push --dry-run upstream-push HEAD:master +# Now push +git push upstream-push HEAD:develop +git push upstream-push HEAD:release +# git push upstream-push HEAD:master +# Don't forget to tag the release, too. +git tag +git push upstream-push +``` +6. Finally +[create a new release on Github](https://github.com/XRPLF/rippled/releases). + # Style guide diff --git a/include/xrpl/protocol/detail/b58_utils.h b/include/xrpl/protocol/detail/b58_utils.h index f21b416042b..b060fc7e166 100644 --- a/include/xrpl/protocol/detail/b58_utils.h +++ b/include/xrpl/protocol/detail/b58_utils.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_B58_UTILS_H_INCLUDED #include +#include #include #include @@ -71,12 +72,12 @@ carrying_add(std::uint64_t a, std::uint64_t b) // (i.e a[0] is the 2^0 coefficient, a[n] is the 2^(64*n) coefficient) // panics if overflows (this is a specialized adder for b58 decoding. // it should never overflow). -inline void +[[nodiscard]] inline TokenCodecErrc inplace_bigint_add(std::span a, std::uint64_t b) { if (a.size() <= 1) { - ripple::LogicError("Input span too small for inplace_bigint_add"); + return TokenCodecErrc::inputTooSmall; } std::uint64_t carry; @@ -86,28 +87,29 @@ inplace_bigint_add(std::span a, std::uint64_t b) { if (!carry) { - return; + return TokenCodecErrc::success; } std::tie(v, carry) = carrying_add(v, 1); } if (carry) { - LogicError("Overflow in inplace_bigint_add"); + return TokenCodecErrc::overflowAdd; } + return TokenCodecErrc::success; } -inline void +[[nodiscard]] inline TokenCodecErrc inplace_bigint_mul(std::span a, std::uint64_t b) { if (a.empty()) { - LogicError("Empty span passed to inplace_bigint_mul"); + return TokenCodecErrc::inputTooSmall; } auto const last_index = a.size() - 1; if (a[last_index] != 0) { - LogicError("Non-zero element in inplace_bigint_mul last index"); + return TokenCodecErrc::inputTooLarge; } std::uint64_t carry = 0; @@ -116,7 +118,9 @@ inplace_bigint_mul(std::span a, std::uint64_t b) std::tie(coeff, carry) = carrying_mul(coeff, b, carry); } a[last_index] = carry; + return TokenCodecErrc::success; } + // divide a "big uint" value inplace and return the mod // numerator is stored so smallest coefficients come first [[nodiscard]] inline std::uint64_t @@ -166,11 +170,7 @@ inplace_bigint_div_rem(std::span numerator, std::uint64_t divisor) b58_10_to_b58_be(std::uint64_t input) { constexpr std::uint64_t B_58_10 = 430804206899405824; // 58^10; - if (input >= B_58_10) - { - LogicError("Input to b58_10_to_b58_be equals or exceeds 58^10."); - } - + assert(input < B_58_10); constexpr std::size_t resultSize = 10; std::array result{}; int i = 0; diff --git a/include/xrpl/protocol/detail/token_errors.h b/include/xrpl/protocol/detail/token_errors.h index 59b09974149..23a46bd1c5e 100644 --- a/include/xrpl/protocol/detail/token_errors.h +++ b/include/xrpl/protocol/detail/token_errors.h @@ -32,6 +32,7 @@ enum class TokenCodecErrc { mismatchedTokenType, mismatchedChecksum, invalidEncodingChar, + overflowAdd, unknown, }; } diff --git a/src/libxrpl/protocol/tokens.cpp b/src/libxrpl/protocol/tokens.cpp index bec16945654..454ed803f75 100644 --- a/src/libxrpl/protocol/tokens.cpp +++ b/src/libxrpl/protocol/tokens.cpp @@ -467,6 +467,11 @@ b256_to_b58_be(std::span input, std::span out) { continue; } + static constexpr std::uint64_t B_58_10 = 430804206899405824; // 58^10; + if (base_58_10_coeff[i] >= B_58_10) + { + return Unexpected(TokenCodecErrc::inputTooLarge); + } std::array const b58_be = ripple::b58_fast::detail::b58_10_to_b58_be(base_58_10_coeff[i]); std::size_t to_skip = 0; @@ -565,10 +570,23 @@ b58_to_b256_be(std::string_view input, std::span out) for (int i = 1; i < num_b_58_10_coeffs; ++i) { std::uint64_t const c = b_58_10_coeff[i]; - ripple::b58_fast::detail::inplace_bigint_mul( - std::span(&result[0], cur_result_size + 1), B_58_10); - ripple::b58_fast::detail::inplace_bigint_add( - std::span(&result[0], cur_result_size + 1), c); + + { + auto code = ripple::b58_fast::detail::inplace_bigint_mul( + std::span(&result[0], cur_result_size + 1), B_58_10); + if (code != TokenCodecErrc::success) + { + return Unexpected(code); + } + } + { + auto code = ripple::b58_fast::detail::inplace_bigint_add( + std::span(&result[0], cur_result_size + 1), c); + if (code != TokenCodecErrc::success) + { + return Unexpected(code); + } + } if (result[cur_result_size] != 0) { cur_result_size += 1; diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 0dffe5e388f..e49e5cbd6dc 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -873,6 +873,25 @@ struct PayChan_test : public beast::unit_test::suite auto const chan1Str = to_string(channel(alice, bob, env.seq(alice))); env(create(alice, bob, channelFunds, settleDelay, pk)); env.close(); + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_channels", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } { auto const r = env.rpc("account_channels", alice.human(), bob.human()); diff --git a/src/test/basics/base58_test.cpp b/src/test/basics/base58_test.cpp index f204cdd0e3a..799f6537dc6 100644 --- a/src/test/basics/base58_test.cpp +++ b/src/test/basics/base58_test.cpp @@ -177,6 +177,7 @@ class base58_test : public beast::unit_test::suite constexpr std::size_t iters = 100000; auto eng = randEngine(); std::uniform_int_distribution dist; + std::uniform_int_distribution dist1(1); for (int i = 0; i < iters; ++i) { std::uint64_t const d = dist(eng); @@ -209,12 +210,31 @@ class base58_test : public beast::unit_test::suite auto const refAdd = boostBigInt + d; - b58_fast::detail::inplace_bigint_add( + auto const result = b58_fast::detail::inplace_bigint_add( std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::success); auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); BEAST_EXPECT(refAdd == foundAdd); } for (int i = 0; i < iters; ++i) + { + std::uint64_t const d = dist1(eng); + // Force overflow + std::vector bigInt( + 5, std::numeric_limits::max()); + + auto const boostBigInt = multiprecision_utils::toBoostMP( + std::span(bigInt.data(), bigInt.size())); + + auto const refAdd = boostBigInt + d; + + auto const result = b58_fast::detail::inplace_bigint_add( + std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::overflowAdd); + auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); + BEAST_EXPECT(refAdd != foundAdd); + } + for (int i = 0; i < iters; ++i) { std::uint64_t const d = dist(eng); auto bigInt = multiprecision_utils::randomBigInt(/* minSize */ 2); @@ -226,11 +246,29 @@ class base58_test : public beast::unit_test::suite auto const refMul = boostBigInt * d; - b58_fast::detail::inplace_bigint_mul( + auto const result = b58_fast::detail::inplace_bigint_mul( std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::success); auto const foundMul = multiprecision_utils::toBoostMP(bigInt); BEAST_EXPECT(refMul == foundMul); } + for (int i = 0; i < iters; ++i) + { + std::uint64_t const d = dist1(eng); + // Force overflow + std::vector bigInt( + 5, std::numeric_limits::max()); + auto const boostBigInt = multiprecision_utils::toBoostMP( + std::span(bigInt.data(), bigInt.size())); + + auto const refMul = boostBigInt * d; + + auto const result = b58_fast::detail::inplace_bigint_mul( + std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::inputTooLarge); + auto const foundMul = multiprecision_utils::toBoostMP(bigInt); + BEAST_EXPECT(refMul != foundMul); + } } void diff --git a/src/test/rpc/AccountCurrencies_test.cpp b/src/test/rpc/AccountCurrencies_test.cpp index 472dc02c438..7d16ca48a93 100644 --- a/src/test/rpc/AccountCurrencies_test.cpp +++ b/src/test/rpc/AccountCurrencies_test.cpp @@ -39,6 +39,7 @@ class AccountCurrencies_test : public beast::unit_test::suite { // invalid ledger (hash) Json::Value params; + params[jss::account] = Account{"bob"}.human(); params[jss::ledger_hash] = 1; auto const result = env.rpc( "json", @@ -56,6 +57,50 @@ class AccountCurrencies_test : public beast::unit_test::suite result[jss::error_message] == "Missing field 'account'."); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", + "account_currencies", + to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + + { + // test ident non-string + auto testInvalidIdentParam = [&](auto const& param) { + Json::Value params; + params[jss::ident] = param; + auto jrr = env.rpc( + "json", + "account_currencies", + to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'ident'."); + }; + + testInvalidIdentParam(1); + testInvalidIdentParam(1.1); + testInvalidIdentParam(true); + testInvalidIdentParam(Json::Value(Json::nullValue)); + testInvalidIdentParam(Json::Value(Json::objectValue)); + testInvalidIdentParam(Json::Value(Json::arrayValue)); + } + { Json::Value params; params[jss::account] = @@ -198,6 +243,6 @@ class AccountCurrencies_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountCurrencies, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountCurrencies, rpc, ripple); } // namespace ripple diff --git a/src/test/rpc/AccountInfo_test.cpp b/src/test/rpc/AccountInfo_test.cpp index 8c69dce13d1..cb9712aef56 100644 --- a/src/test/rpc/AccountInfo_test.cpp +++ b/src/test/rpc/AccountInfo_test.cpp @@ -36,6 +36,7 @@ class AccountInfo_test : public beast::unit_test::suite void testErrors() { + testcase("Errors"); using namespace jtx; Env env(*this); { @@ -78,12 +79,53 @@ class AccountInfo_test : public beast::unit_test::suite BEAST_EXPECT( info[jss::result][jss::error_message] == "Account malformed."); } + { + // Cannot pass a non-string into the `account` param + + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_info", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { + // Cannot pass a non-string into the `ident` param + + auto testInvalidIdentParam = [&](auto const& param) { + Json::Value params; + params[jss::ident] = param; + auto jrr = env.rpc( + "json", "account_info", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'ident'."); + }; + + testInvalidIdentParam(1); + testInvalidIdentParam(1.1); + testInvalidIdentParam(true); + testInvalidIdentParam(Json::Value(Json::nullValue)); + testInvalidIdentParam(Json::Value(Json::objectValue)); + testInvalidIdentParam(Json::Value(Json::arrayValue)); + } } // Test the "signer_lists" argument in account_info. void testSignerLists() { + testcase("Signer lists"); using namespace jtx; Env env(*this); Account const alice{"alice"}; @@ -205,6 +247,7 @@ class AccountInfo_test : public beast::unit_test::suite void testSignerListsApiVersion2() { + testcase("Signer lists APIv2"); using namespace jtx; Env env{*this}; Account const alice{"alice"}; @@ -326,6 +369,7 @@ class AccountInfo_test : public beast::unit_test::suite void testSignerListsV2() { + testcase("Signer lists v2"); using namespace jtx; Env env(*this); Account const alice{"alice"}; @@ -515,6 +559,7 @@ class AccountInfo_test : public beast::unit_test::suite void testAccountFlags(FeatureBitset const& features) { + testcase("Account flags"); using namespace jtx; Env env(*this, features); @@ -652,7 +697,7 @@ class AccountInfo_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountInfo, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountInfo, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLines_test.cpp similarity index 98% rename from src/test/rpc/AccountLinesRPC_test.cpp rename to src/test/rpc/AccountLines_test.cpp index 3c25dbe7810..d104ea14b0a 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLines_test.cpp @@ -27,7 +27,7 @@ namespace ripple { namespace RPC { -class AccountLinesRPC_test : public beast::unit_test::suite +class AccountLines_test : public beast::unit_test::suite { public: void @@ -55,6 +55,25 @@ class AccountLinesRPC_test : public beast::unit_test::suite lines[jss::result][jss::error_message] == RPC::make_error(rpcACT_MALFORMED)[jss::error_message]); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_lines", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } Account const alice{"alice"}; { // account_lines on an unfunded account. @@ -1474,7 +1493,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountLinesRPC, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountLines, rpc, ripple); } // namespace RPC } // namespace ripple diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 0757ca40bf0..f58446e66c9 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -125,8 +125,30 @@ class AccountObjects_test : public beast::unit_test::suite // test error on no account { - auto resp = env.rpc("json", "account_objects"); - BEAST_EXPECT(resp[jss::error_message] == "Syntax error."); + Json::Value params; + auto resp = env.rpc("json", "account_objects", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Missing field 'account'."); + } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_objects", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); } // test error on malformed account string. { @@ -1169,6 +1191,35 @@ class AccountObjects_test : public beast::unit_test::suite "Invalid field \'marker\'.")); } + void + testAccountNFTs() + { + testcase("account_nfts"); + + using namespace jtx; + Env env(*this); + + // test validation + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_nfts", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + } + void run() override { @@ -1177,10 +1228,11 @@ class AccountObjects_test : public beast::unit_test::suite testUnsteppedThenSteppedWithNFTs(); testObjectTypes(); testNFTsMarker(); + testAccountNFTs(); } }; -BEAST_DEFINE_TESTSUITE(AccountObjects, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountObjects, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index d53fb9ecb0b..da6acc97e98 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -37,6 +37,8 @@ class AccountOffers_test : public beast::unit_test::suite void testNonAdminMinLimit() { + testcase("Non-Admin Min Limit"); + using namespace jtx; Env env{*this, envconfig(no_admin)}; Account const gw("G1"); @@ -81,6 +83,9 @@ class AccountOffers_test : public beast::unit_test::suite void testSequential(bool asAdmin) { + testcase( + std::string("Sequential - ") + (asAdmin ? "admin" : "non-admin")); + using namespace jtx; Env env{*this, asAdmin ? envconfig() : envconfig(no_admin)}; Account const gw("G1"); @@ -215,6 +220,8 @@ class AccountOffers_test : public beast::unit_test::suite void testBadInput() { + testcase("Bad input"); + using namespace jtx; Env env(*this); Account const gw("G1"); @@ -233,6 +240,26 @@ class AccountOffers_test : public beast::unit_test::suite BEAST_EXPECT(jrr[jss::error_message] == "Syntax error."); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_offers", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { // empty string account Json::Value jvParams; @@ -282,7 +309,9 @@ class AccountOffers_test : public beast::unit_test::suite jvParams.toStyledString())[jss::result]; BEAST_EXPECT(jrr[jss::error] == "invalidParams"); BEAST_EXPECT(jrr[jss::status] == "error"); - BEAST_EXPECT(jrr[jss::error_message] == "Invalid parameters."); + BEAST_EXPECTS( + jrr[jss::error_message] == "Invalid field 'marker'.", + jrr.toStyledString()); } { @@ -326,7 +355,7 @@ class AccountOffers_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountOffers, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountOffers, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 54ca3f1f418..f6a9225ec48 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -109,6 +109,7 @@ class AccountTx_test : public beast::unit_test::suite void testParameters(unsigned int apiVersion) { + testcase("Parameters APIv" + std::to_string(apiVersion)); using namespace test::jtx; Env env(*this); @@ -353,6 +354,25 @@ class AccountTx_test : public beast::unit_test::suite env.rpc("json", "account_tx", to_string(p)), rpcLGR_IDX_MALFORMED)); } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_tx", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } // test binary and forward for bool/non bool values { Json::Value p{jParms}; @@ -388,6 +408,8 @@ class AccountTx_test : public beast::unit_test::suite void testContents() { + testcase("Contents"); + // Get results for all transaction types that can be associated // with an account. Start by generating all transaction types. using namespace test::jtx; @@ -600,6 +622,8 @@ class AccountTx_test : public beast::unit_test::suite void testAccountDelete() { + testcase("AccountDelete"); + // Verify that if an account is resurrected then the account_tx RPC // command still recovers all transactions on that account before // and after resurrection. @@ -740,7 +764,7 @@ class AccountTx_test : public beast::unit_test::suite testAccountDelete(); } }; -BEAST_DEFINE_TESTSUITE(AccountTx, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountTx, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/NoRippleCheck_test.cpp b/src/test/rpc/NoRippleCheck_test.cpp index 58f10c66dc1..4551365029f 100644 --- a/src/test/rpc/NoRippleCheck_test.cpp +++ b/src/test/rpc/NoRippleCheck_test.cpp @@ -64,6 +64,27 @@ class NoRippleCheck_test : public beast::unit_test::suite BEAST_EXPECT(result[jss::error_message] == "Missing field 'role'."); } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + params[jss::role] = "user"; + auto jrr = env.rpc( + "json", "noripple_check", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { // invalid role field Json::Value params; params[jss::account] = alice.human(); @@ -369,12 +390,12 @@ class NoRippleCheckLimits_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(NoRippleCheck, app, ripple); +BEAST_DEFINE_TESTSUITE(NoRippleCheck, rpc, ripple); // These tests that deal with limit amounts are slow because of the // offer/account setup, so making them manual -- the additional coverage // provided by them is minimal -BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(NoRippleCheckLimits, app, ripple, 1); +BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(NoRippleCheckLimits, rpc, ripple, 1); } // namespace ripple diff --git a/src/xrpld/rpc/handlers/AccountChannels.cpp b/src/xrpld/rpc/handlers/AccountChannels.cpp index 5ae87e32a12..ad591b04a1c 100644 --- a/src/xrpld/rpc/handlers/AccountChannels.cpp +++ b/src/xrpld/rpc/handlers/AccountChannels.cpp @@ -71,6 +71,9 @@ doAccountChannels(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) diff --git a/src/xrpld/rpc/handlers/AccountCurrenciesHandler.cpp b/src/xrpld/rpc/handlers/AccountCurrenciesHandler.cpp index 04336114987..6c8fe282674 100644 --- a/src/xrpld/rpc/handlers/AccountCurrenciesHandler.cpp +++ b/src/xrpld/rpc/handlers/AccountCurrenciesHandler.cpp @@ -33,19 +33,29 @@ doAccountCurrencies(RPC::JsonContext& context) { auto& params = context.params; + if (!(params.isMember(jss::account) || params.isMember(jss::ident))) + return RPC::missing_field_error(jss::account); + + std::string strIdent; + if (params.isMember(jss::account)) + { + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + strIdent = params[jss::account].asString(); + } + else if (params.isMember(jss::ident)) + { + if (!params[jss::ident].isString()) + return RPC::invalid_field_error(jss::ident); + strIdent = params[jss::ident].asString(); + } + // Get the current ledger std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) return result; - if (!(params.isMember(jss::account) || params.isMember(jss::ident))) - return RPC::missing_field_error(jss::account); - - std::string const strIdent( - params.isMember(jss::account) ? params[jss::account].asString() - : params[jss::ident].asString()); - // Get info on account. auto id = parseBase58(strIdent); if (!id) diff --git a/src/xrpld/rpc/handlers/AccountInfo.cpp b/src/xrpld/rpc/handlers/AccountInfo.cpp index 29b8679505d..dab0274e9dd 100644 --- a/src/xrpld/rpc/handlers/AccountInfo.cpp +++ b/src/xrpld/rpc/handlers/AccountInfo.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -53,9 +54,17 @@ doAccountInfo(RPC::JsonContext& context) std::string strIdent; if (params.isMember(jss::account)) + { + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); strIdent = params[jss::account].asString(); + } else if (params.isMember(jss::ident)) + { + if (!params[jss::ident].isString()) + return RPC::invalid_field_error(jss::ident); strIdent = params[jss::ident].asString(); + } else return RPC::missing_field_error(jss::account); diff --git a/src/xrpld/rpc/handlers/AccountLines.cpp b/src/xrpld/rpc/handlers/AccountLines.cpp index cace3487bb8..64ca95ebe56 100644 --- a/src/xrpld/rpc/handlers/AccountLines.cpp +++ b/src/xrpld/rpc/handlers/AccountLines.cpp @@ -80,6 +80,9 @@ doAccountLines(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) diff --git a/src/xrpld/rpc/handlers/AccountObjects.cpp b/src/xrpld/rpc/handlers/AccountObjects.cpp index d2eb2eacde6..c192fbf9071 100644 --- a/src/xrpld/rpc/handlers/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/AccountObjects.cpp @@ -54,17 +54,19 @@ doAccountNFTs(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); - std::shared_ptr ledger; - auto result = RPC::lookupLedger(ledger, context); - if (ledger == nullptr) - return result; + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); auto id = parseBase58(params[jss::account].asString()); if (!id) { - RPC::inject_error(rpcACT_MALFORMED, result); - return result; + return rpcError(rpcACT_MALFORMED); } + + std::shared_ptr ledger; + auto result = RPC::lookupLedger(ledger, context); + if (ledger == nullptr) + return result; auto const accountID{id.value()}; if (!ledger->exists(keylet::account(accountID))) @@ -183,6 +185,9 @@ doAccountObjects(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (ledger == nullptr) diff --git a/src/xrpld/rpc/handlers/AccountOffers.cpp b/src/xrpld/rpc/handlers/AccountOffers.cpp index 1a3b5227ed8..3c4a4404984 100644 --- a/src/xrpld/rpc/handlers/AccountOffers.cpp +++ b/src/xrpld/rpc/handlers/AccountOffers.cpp @@ -60,6 +60,9 @@ doAccountOffers(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) @@ -84,7 +87,7 @@ doAccountOffers(RPC::JsonContext& context) return *err; if (limit == 0) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::limit); Json::Value& jsonOffers(result[jss::offers] = Json::arrayValue); std::vector> offers; @@ -101,13 +104,13 @@ doAccountOffers(RPC::JsonContext& context) std::stringstream marker(params[jss::marker].asString()); std::string value; if (!std::getline(marker, value, ',')) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); if (!startAfter.parseHex(value)) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); if (!std::getline(marker, value, ',')) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); try { @@ -115,7 +118,7 @@ doAccountOffers(RPC::JsonContext& context) } catch (boost::bad_lexical_cast&) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); } // We then must check if the object pointed to by the marker is actually diff --git a/src/xrpld/rpc/handlers/AccountTx.cpp b/src/xrpld/rpc/handlers/AccountTx.cpp index 4f9431ce081..3b9165eecf1 100644 --- a/src/xrpld/rpc/handlers/AccountTx.cpp +++ b/src/xrpld/rpc/handlers/AccountTx.cpp @@ -426,12 +426,12 @@ doAccountTxJson(RPC::JsonContext& context) if (context.apiVersion > 1u && params.isMember(jss::binary) && !params[jss::binary].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::binary); } if (context.apiVersion > 1u && params.isMember(jss::forward) && !params[jss::forward].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::forward); } args.limit = params.isMember(jss::limit) ? params[jss::limit].asUInt() : 0; @@ -440,7 +440,10 @@ doAccountTxJson(RPC::JsonContext& context) params.isMember(jss::forward) && params[jss::forward].asBool(); if (!params.isMember(jss::account)) - return rpcError(rpcINVALID_PARAMS); + return RPC::missing_field_error(jss::account); + + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); auto const account = parseBase58(params[jss::account].asString()); diff --git a/src/xrpld/rpc/handlers/NoRippleCheck.cpp b/src/xrpld/rpc/handlers/NoRippleCheck.cpp index e4012468434..94830a4f397 100644 --- a/src/xrpld/rpc/handlers/NoRippleCheck.cpp +++ b/src/xrpld/rpc/handlers/NoRippleCheck.cpp @@ -66,6 +66,10 @@ doNoRippleCheck(RPC::JsonContext& context) if (!params.isMember("role")) return RPC::missing_field_error("role"); + + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + bool roleGateway = false; { std::string const role = params["role"].asString(); @@ -90,7 +94,7 @@ doNoRippleCheck(RPC::JsonContext& context) if (context.apiVersion > 1u && params.isMember(jss::transactions) && !params[jss::transactions].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::transactions); } std::shared_ptr ledger;