diff --git a/CHANGELOG.md b/CHANGELOG.md index f587ffa4ace5..4a4d2a19da6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [5.0.2] + +[5.0.2]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.2 + +### Bug Fixes + +- The `/tx` endpoint returns more accurate error messages for incorrectly formed transactions ids (#6359). + ## [5.0.1] [5.0.1]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.1 diff --git a/include/ccf/tx_id.h b/include/ccf/tx_id.h index 2978e0618673..8ffab32ab439 100644 --- a/include/ccf/tx_id.h +++ b/include/ccf/tx_id.h @@ -42,8 +42,8 @@ namespace ccf // transaction executed by CCF. struct TxID { - View view; - SeqNo seqno; + View view = VIEW_UNKNOWN; + SeqNo seqno = SEQNO_UNKNOWN; std::string to_str() const { @@ -64,7 +64,8 @@ namespace ccf const auto view_sv = sv.substr(0, separator_idx); const auto [p, ec] = std::from_chars(view_sv.begin(), view_sv.end(), tx_id.view); - if (ec != std::errc() || p != view_sv.end()) + if ( + ec != std::errc() || p != view_sv.end() || tx_id.view == VIEW_UNKNOWN) { return std::nullopt; } @@ -74,7 +75,9 @@ namespace ccf const auto seqno_sv = sv.substr(separator_idx + 1); const auto [p, ec] = std::from_chars(seqno_sv.begin(), seqno_sv.end(), tx_id.seqno); - if (ec != std::errc() || p != seqno_sv.end()) + if ( + ec != std::errc() || p != seqno_sv.end() || + tx_id.seqno == SEQNO_UNKNOWN) { return std::nullopt; } diff --git a/src/node/rpc/test/frontend_test.cpp b/src/node/rpc/test/frontend_test.cpp index 21d5fe93b600..93792320c759 100644 --- a/src/node/rpc/test/frontend_test.cpp +++ b/src/node/rpc/test/frontend_test.cpp @@ -485,7 +485,8 @@ void prepare_callers(NetworkState& network) init_network(network); - InternalTablesAccess::create_service(tx, network.identity->cert, ccf::TxID{}); + InternalTablesAccess::create_service( + tx, network.identity->cert, ccf::TxID{2, 1}); user_id = InternalTablesAccess::add_user(tx, {user_caller}); member_id = InternalTablesAccess::add_member(tx, member_cert); invalid_member_id = InternalTablesAccess::add_member(tx, invalid_caller); diff --git a/src/node/rpc/test/node_frontend_test.cpp b/src/node/rpc/test/node_frontend_test.cpp index 81324870b315..79bdcd92bf7e 100644 --- a/src/node/rpc/test/node_frontend_test.cpp +++ b/src/node/rpc/test/node_frontend_test.cpp @@ -98,7 +98,7 @@ TEST_CASE("Add a node to an opening service") } InternalTablesAccess::create_service( - gen_tx, network.identity->cert, ccf::TxID{}); + gen_tx, network.identity->cert, ccf::TxID{2, 1}); REQUIRE(gen_tx.commit() == ccf::kv::CommitResult::SUCCESS); auto tx = network.tables->create_tx(); @@ -199,7 +199,7 @@ TEST_CASE("Add a node to an open service") up_to_ledger_secret_seqno, make_ledger_secret()); InternalTablesAccess::create_service( - gen_tx, network.identity->cert, ccf::TxID{}); + gen_tx, network.identity->cert, ccf::TxID{2, 1}); InternalTablesAccess::init_configuration(gen_tx, {1}); InternalTablesAccess::activate_member( gen_tx, diff --git a/tests/e2e_logging.py b/tests/e2e_logging.py index 7c9b567e9215..9c44e927a7c5 100644 --- a/tests/e2e_logging.py +++ b/tests/e2e_logging.py @@ -273,6 +273,35 @@ def send_corrupt_variations(content): return network +@reqs.description("Test invalid transactions ids in /tx endpoint") +def test_invalid_txids(network, args): + primary, _ = network.find_primary() + + # These are not valid transactions IDs at all, one cannot ask about their status + invalid_params = ["a.b", "-1.-1", "0.0", "1.0", "0.1", "2.0"] + + with primary.client() as c: + for txid in invalid_params: + r = c.get(f"/tx?transaction_id={txid}") + assert r.status_code == http.HTTPStatus.BAD_REQUEST, r.status_code + assert ( + r.body.json()["error"]["code"] == "InvalidQueryParameterValue" + ), r.body.json() + + # These are valid transaction IDs, but cannot happen in CCF because views start + # at 2, so while it is ok to ask about them, their consensus status is always Invalid, + # meaning that they are not, and can never be committed. + invalid_txs = ["1.1", "1.2"] + + with primary.client() as c: + for txid in invalid_txs: + r = c.get(f"/tx?transaction_id={txid}") + assert r.status_code == http.HTTPStatus.OK, r.status_code + assert r.body.json()["status"] == "Invalid", r.body.json() + + return network + + @reqs.description("Alternative protocols") @reqs.supports_methods("/log/private", "/log/public") @reqs.at_least_n_nodes(2) @@ -1303,7 +1332,7 @@ def test_view_history(network, args): seqno_to_views = {} for seqno in range(1, commit_tx_id.seqno + 1): views = [] - for view in range(1, commit_tx_id.view + 1): + for view in range(2, commit_tx_id.view + 1): r = c.get(f"/node/tx?transaction_id={view}.{seqno}", log_capture=[]) check(r) status = TxStatus(r.body.json()["status"]) @@ -2056,6 +2085,7 @@ def run_parsing_errors(args): test_illegal(network, args) test_protocols(network, args) + test_invalid_txids(network, args) if __name__ == "__main__":