From d82d895fe5ba76ad5ac05fba30ae9cee6726602a Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 26 Aug 2026 09:55:03 -0500 Subject: [PATCH 1/3] chain: discover payer keys by the paired permission, not always active `get_required_keys` hard-coded `@active` for an explicit `sysio.payer` entry, while `check_authorization` pairs the marker with ANY real permission the payer declares on the same action: if (auth.actor == payer && auth.permission != config::sysio_payer_name) { foundPayer = true; The two therefore disagreed whenever the paired permission was not `active`. A self-pay transaction paired under `owner`, or under a custom permission linked to the action, is accepted by consensus but was rejected by discovery -- so /v1/chain/get_required_keys, which clio, kiod and every wallet call to learn which keys to sign with, could not produce a signable key set for a transaction the chain would have taken. The divergence stayed hidden wherever owner and active share a key, which is the default for a freshly created account. The marker needs no check of its own. It is virtual: no `permission_object` backs it and it holds no keys. Consensus already requires it to be paired with a real permission, and that entry is checked on its own iteration of the same loop -- so skipping the marker makes discovery agree with consensus by construction, rather than by keeping a second copy of the pairing rule in sync. In the common case the reported key set is unchanged; where it differs, it was previously wrong. Not a consensus path: the only callers are the read-only RPC and the snapshot provider signing its own votesnaphash transaction, so no protocol feature applies. `get_required_keys_explicit_payer_tests` covers it on `payloadless::doit`, chosen because it takes no arguments and asserts nothing about its authorization, so the authorization path is the only thing that can reject these transactions. For each of the three pairings -- `active`, `owner`, and a linked custom permission -- it asserts both the key discovery reports AND that a transaction signed with exactly that key is accepted, since agreement between the two is the property at issue and either half alone passed before. It also pins that discovery stays strict (candidates that cannot satisfy the paired permission still throw, including the neighbouring permissions in the hierarchy) and that the marker itself is attributed no key, via a two-action transaction pairing the same payer under two permissions. Verified as a guard: reinstating the `@active` check fails the `owner` and custom cases. Change-Id: I66930d2dc0da3339508afde6c129f9d2ab51f02a --- libraries/chain/authorization_manager.cpp | 26 +++-- unittests/api_tests.cpp | 123 ++++++++++++++++++++++ 2 files changed, 139 insertions(+), 10 deletions(-) diff --git a/libraries/chain/authorization_manager.cpp b/libraries/chain/authorization_manager.cpp index 55f803fc78..b98403f999 100644 --- a/libraries/chain/authorization_manager.cpp +++ b/libraries/chain/authorization_manager.cpp @@ -615,17 +615,23 @@ namespace sysio { namespace chain { for (const auto& act : trx.actions ) { for (const auto& declared_auth : act.authorization) { - if (declared_auth.permission == config::sysio_payer_name) { - auto active_auth = permission_level{declared_auth.actor, sysio::chain::config::active_name}; - SYS_ASSERT( checker.satisfied(active_auth), unsatisfied_authorization, - "transaction declares payer authority '{}', but does not have signatures for it.", - active_auth ); + // `sysio.payer` is a virtual marker, not a permission: no `permission_object` + // exists for it and it carries no keys of its own. check_authorization requires + // the payer to also appear on the same action under a REAL permission -- any + // permission other than `sysio.payer` itself -- and satisfies that entry. It is + // reached on its own iteration of this loop, so the marker needs no check here. + // + // Checking `@active` instead, as this once did, diverges from consensus + // the moment the paired permission is not `active`: a transaction the chain + // accepts under `owner` or a linked custom permission was rejected here, so the + // signing tools that discover keys through /v1/chain/get_required_keys could not + // sign it. It was masked whenever owner and active shared a key. + if (declared_auth.permission == config::sysio_payer_name) + continue; - } else { - SYS_ASSERT( checker.satisfied(declared_auth), unsatisfied_authorization, - "transaction declares authority '{}', but does not have signatures for it.", - declared_auth ); - } + SYS_ASSERT( checker.satisfied(declared_auth), unsatisfied_authorization, + "transaction declares authority '{}', but does not have signatures for it.", + declared_auth ); } } diff --git a/unittests/api_tests.cpp b/unittests/api_tests.cpp index 0bc883196d..675dea2902 100644 --- a/unittests/api_tests.cpp +++ b/unittests/api_tests.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -300,6 +301,128 @@ BOOST_FIXTURE_TEST_CASE(action_verification_tests, validating_tester) { try { BOOST_REQUIRE_EQUAL( validate(), true ); } FC_LOG_AND_RETHROW() } +/** + * get_required_keys must agree with check_authorization about which permission backs an + * explicit `sysio.payer`. + * + * Consensus pairs the virtual payer marker with ANY real permission the payer declares on the + * same action, then satisfies that entry. get_required_keys once hard-coded `@active` + * instead, so a transaction the chain accepts under `owner` or a linked custom permission could + * not be signed through /v1/chain/get_required_keys -- the discovery endpoint every signing tool + * calls. The divergence hid wherever owner and active happened to share a key. + * + * Each case asserts BOTH halves: the keys discovery reports, and that a transaction signed with + * exactly those keys is accepted. Agreement between them is the property under test -- either + * half alone passed before the fix. + * + * `payloadless::doit` is the vehicle because it takes no arguments and asserts nothing about its + * authorization, so the only thing that can reject these transactions is the authorization path. + */ +BOOST_FIXTURE_TEST_CASE(get_required_keys_explicit_payer_tests, validating_tester) { try { + produce_blocks(2); + create_account( "payloadless"_n ); + produce_block(); + set_code( "payloadless"_n, test_contracts::payloadless_wasm() ); + set_abi( "payloadless"_n, test_contracts::payloadless_abi() ); + produce_block(); + + // create_account already gives owner and active distinct keys; add a third permission under + // active and link it to the action, so all three kinds of pairing are exercised. + const auto owner_key = get_private_key( "payloadless"_n, "owner" ); + const auto active_key = get_private_key( "payloadless"_n, "active" ); + const auto custom_key = get_private_key( "payloadless"_n, "custom" ); + const auto owner_pub = owner_key.get_public_key(); + const auto active_pub = active_key.get_public_key(); + const auto custom_pub = custom_key.get_public_key(); + BOOST_REQUIRE( owner_pub != active_pub ); + BOOST_REQUIRE( active_pub != custom_pub ); + + set_authority( "payloadless"_n, "custom"_n, authority{ custom_pub }, config::active_name ); + link_authority( "payloadless"_n, "payloadless"_n, "custom"_n, "doit"_n ); + produce_block(); + + const flat_set all_keys{ owner_pub, active_pub, custom_pub }; + + // `payloadless::doit`, paid by payloadless, paired under `paired_permission`. + auto make_trx = [&]( name paired_permission ) { + signed_transaction trx; + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, paired_permission } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + return trx; + }; + + auto required_keys = [&]( const signed_transaction& trx, + const flat_set& candidates ) { + return control->get_authorization_manager().get_required_keys( trx, candidates ); + }; + + // --- Discovery reports the PAIRED permission's key, for each of the three kinds, and a + // transaction signed with exactly that key is accepted by consensus. --- + const std::vector> cases{ + { config::active_name, active_pub }, // the only case that worked before the fix + { config::owner_name, owner_pub }, // owner's key does not satisfy active + { "custom"_n, custom_pub }, // nor does a child permission's + }; + + for( const auto& one_case : cases ) { + const auto& paired = one_case.first; + const auto& expected_pub = one_case.second; + + auto trx = make_trx( paired ); + BOOST_CHECK( required_keys( trx, all_keys ) == flat_set{ expected_pub } ); + + const auto& signing_key = ( expected_pub == owner_pub ) ? owner_key + : ( expected_pub == active_pub ) ? active_key + : custom_key; + trx.sign( signing_key, control->get_chain_id() ); + push_transaction( trx ); // throws if consensus disagrees with discovery + produce_block(); + } + + // --- Still strict: candidates that cannot satisfy the paired permission throw. --- + for( auto paired : { config::owner_name, name("custom"_n) } ) { + // `active` is owner's CHILD and custom's PARENT -- it satisfies neither authority. + auto trx = make_trx( paired ); + BOOST_CHECK_EXCEPTION( required_keys( trx, flat_set{ active_pub } ), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + } + { + auto trx = make_trx( config::active_name ); + BOOST_CHECK_EXCEPTION( required_keys( trx, flat_set{} ), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + } + + // --- The marker itself is attributed no key: two actions, two different pairings, and the + // result is the union of the PAIRED permissions and nothing else. --- + { + signed_transaction trx; + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::owner_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + + BOOST_CHECK( required_keys( trx, all_keys ) + == ( flat_set{ active_pub, owner_pub } ) ); + + trx.sign( active_key, control->get_chain_id() ); + trx.sign( owner_key, control->get_chain_id() ); + push_transaction( trx ); + produce_block(); + } + + BOOST_REQUIRE_EQUAL( validate(), true ); +} FC_LOG_AND_RETHROW() } + BOOST_FIXTURE_TEST_CASE(action_tests, validating_tester) { try { produce_blocks(2); create_account( "testapi"_n ); From 86e6523dda9306303eeb93d1c63a2021513e967f Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 26 Aug 2026 12:21:56 -0500 Subject: [PATCH 2/3] chain: re-check payer pairing in get_required_keys Skipping the sysio.payer marker outright let discovery succeed for pairings consensus rejects. An action carrying {alice, sysio.payer} with {bob, active} returned bob's key for a transaction no signature set can authorize, and a payer-only action returned an empty set that a signing tool reads as "nothing to sign". /v1/chain/get_required_keys reaches this function directly and never runs validate_referenced_accounts, so it cannot lean on that earlier gate. Re-checks the three structural rules that give the marker meaning, mirroring check_authorization: position 0, at most one per action, and paired with a real permission from the same actor on the same action. Exception types and messages match the consensus validator so diagnostics agree. Deliberately duplicated rather than extracted into a helper shared with the consensus validators. This function is not on the apply path, and refactoring the consensus copies to serve it would put a non-consensus caller in a position to change consensus behaviour. The comment records that reasoning so the duplication is not tidied away later. Also corrects the context-free-action comment. A CFA carries either no authorization at all -- the ordinary case -- or exactly one, and that one only a payer marker whose actor is already a declared payer in trx.actions. The reason not to walk them is that consensus does not either: controller passes only trn.actions to check_authorization. The allowance exists for billing, since transaction_context::init bills every action including context-free ones to act.payer(), and the pairing requirement is what stops a signature-less CFA from naming a payer with nothing authorizing it. Adds negative coverage for the shapes discovery previously accepted: payer paired with a different actor, payer with no real permission, marker off index 0, two markers on one action, and cross-action pairing. Each asserts both that discovery throws and that consensus rejects the same transaction, so the two cannot drift apart. Two CFA cases assert the marker contributes no key, and that an unpaired CFA marker is left to validate_referenced_accounts -- discovery mirrors check_authorization's rules, not transaction_context's. Verified the negative cases fail without the fix. --- libraries/chain/authorization_manager.cpp | 70 +++++++++--- unittests/api_tests.cpp | 125 ++++++++++++++++++++++ 2 files changed, 182 insertions(+), 13 deletions(-) diff --git a/libraries/chain/authorization_manager.cpp b/libraries/chain/authorization_manager.cpp index b98403f999..943bf238be 100644 --- a/libraries/chain/authorization_manager.cpp +++ b/libraries/chain/authorization_manager.cpp @@ -613,26 +613,70 @@ namespace sysio { namespace chain { _noop_checktime ); + // Context-free actions are not walked, matching consensus: controller passes only + // `trn.actions` to check_authorization, so CFAs are never authorization-checked there either. + // Their only screen is validate_referenced_accounts, which permits a CFA to carry EITHER no + // authorization at all (the ordinary case -- a CFA has no access to authorization state) OR + // exactly one, and that one only a `sysio.payer` marker whose actor is already a declared + // payer in trx.actions. + // + // That allowance exists for billing, not authorization: transaction_context::init bills every + // action -- context-free included -- to act.payer(), which falls through to the contract when + // no marker is present. Without it a self-paying account's CFAs would still bill the contract, + // splitting one transaction across two payers. The pairing requirement is what keeps it safe: + // a CFA carries no signatures and no real permission, so a marker standing alone would name a + // payer with nothing authorizing it. Requiring the actor to already be a payer in trx.actions + // means the CFA inherits an authorization proven over there (index 0, paired real permission, + // satisfying signatures) rather than creating one. + // + // For discovery that means a CFA has no key to contribute: the marker is keyless, and the key + // backing that payer sits on the paired real permission in trx.actions, which is walked below. for (const auto& act : trx.actions ) { - for (const auto& declared_auth : act.authorization) { - // `sysio.payer` is a virtual marker, not a permission: no `permission_object` - // exists for it and it carries no keys of its own. check_authorization requires - // the payer to also appear on the same action under a REAL permission -- any - // permission other than `sysio.payer` itself -- and satisfies that entry. It is - // reached on its own iteration of this loop, so the marker needs no check here. - // - // Checking `@active` instead, as this once did, diverges from consensus - // the moment the paired permission is not `active`: a transaction the chain - // accepts under `owner` or a linked custom permission was rejected here, so the - // signing tools that discover keys through /v1/chain/get_required_keys could not - // sign it. It was masked whenever owner and active shared a key. - if (declared_auth.permission == config::sysio_payer_name) + // `sysio.payer` is a virtual marker, not a permission: no `permission_object` exists for + // it and it carries no keys of its own. The key that authorizes a payer is the one on the + // REAL permission the same actor must also declare on that action, which is reached on its + // own iteration below. + // + // Checking `@active` instead, as this once did, diverges from consensus the moment + // the paired permission is not `active`: a transaction the chain accepts under `owner` or a + // linked custom permission was rejected here, so the signing tools that discover keys + // through /v1/chain/get_required_keys could not sign it. It was masked whenever owner and + // active shared a key. + // + // Skipping the marker outright, however, would let discovery succeed for pairings consensus + // rejects -- `{alice, sysio.payer}, {bob, active}` would return bob's key for a transaction + // no signature set can authorize, and a payer-only action would return an empty set that + // reads as "nothing to sign". So the structural rules that give the marker meaning are + // re-checked here, mirroring check_authorization's: position 0, at most one, and paired + // with a real permission from the same actor ON THE SAME ACTION. They are deliberately + // duplicated rather than shared with the consensus validators -- this function is not on + // the apply path, and refactoring the consensus copies to serve it would put a + // non-consensus caller in a position to change consensus behaviour. + account_name payer; + for (size_t i = 0; i < act.authorization.size(); ++i) { + const auto& declared_auth = act.authorization[i]; + + if (declared_auth.permission == config::sysio_payer_name) { + SYS_ASSERT( payer.empty(), irrelevant_auth_exception, + "Multiple payers specified for action" ); + SYS_ASSERT( i == 0, irrelevant_auth_exception, + "Explicit payer must be the first declared authorization" ); + payer = declared_auth.actor; continue; + } SYS_ASSERT( checker.satisfied(declared_auth), unsatisfied_authorization, "transaction declares authority '{}', but does not have signatures for it.", declared_auth ); } + + if (!payer.empty()) { + const bool paired = std::ranges::any_of(act.authorization, [&](const auto& auth) { + return auth.actor == payer && auth.permission != config::sysio_payer_name; + }); + SYS_ASSERT( paired, unsatisfied_authorization, + "Payer authorization for {} not paired with matching auth", payer ); + } } return checker.used_keys(); diff --git a/unittests/api_tests.cpp b/unittests/api_tests.cpp index 675dea2902..be63c4551e 100644 --- a/unittests/api_tests.cpp +++ b/unittests/api_tests.cpp @@ -420,6 +420,131 @@ BOOST_FIXTURE_TEST_CASE(get_required_keys_explicit_payer_tests, validating_teste produce_block(); } + // --- Structural rules: discovery must refuse the pairings consensus refuses, rather than + // reporting a key set for a transaction no signature can authorize. Each case asserts BOTH + // that discovery throws AND that consensus rejects the same transaction, so the two cannot + // drift apart. get_required_keys is reached directly by /v1/chain/get_required_keys, which + // never runs validate_referenced_accounts, so it cannot rely on that earlier gate. --- + create_account( "bob"_n ); + produce_block(); + const auto bob_active_pub = get_public_key( "bob"_n, "active" ); + const flat_set keys_with_bob{ owner_pub, active_pub, custom_pub, bob_active_pub }; + + auto expect_both_reject = [&]( const signed_transaction& trx, const char* discovery_msg ) { + BOOST_CHECK_EXCEPTION( required_keys( trx, keys_with_bob ), fc::exception, + fc_exception_message_starts_with( discovery_msg ) ); + auto signed_trx = trx; + signed_trx.sign( active_key, control->get_chain_id() ); + signed_trx.sign( owner_key, control->get_chain_id() ); + signed_trx.sign( get_private_key( "bob"_n, "active" ), control->get_chain_id() ); + BOOST_CHECK_THROW( push_transaction( signed_trx ), fc::exception ); + }; + + auto payer_trx = [&]( vector auths ) { + signed_transaction trx; + trx.actions.emplace_back( std::move( auths ), "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + return trx; + }; + + // Payer paired with a DIFFERENT actor: bob's key satisfies bob@active, but nothing authorizes + // payloadless as payer. Skipping the marker outright would have returned bob's key here. + expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name }, + { "bob"_n, config::active_name } } ), + "Payer authorization for" ); + + // Payer with no real permission at all -- would otherwise discover an EMPTY key set, which a + // signing tool reads as "nothing to sign". + expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name } } ), + "Payer authorization for" ); + + // Marker present but not at index 0. + expect_both_reject( payer_trx( { { "payloadless"_n, config::active_name }, + { "payloadless"_n, config::sysio_payer_name } } ), + "Explicit payer must be the first declared authorization" ); + + // Two markers on one action. + expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::active_name }, + { "payloadless"_n, config::sysio_payer_name } } ), + "Multiple payers specified for action" ); + + // Pairing must be on the SAME action: the marker is on action 0 while payloadless's real + // permission sits on action 1. Consensus scopes the pairing per-action, so discovery must too. + { + signed_transaction trx; + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "bob"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + expect_both_reject( trx, "Payer authorization for" ); + } + + // --- Context-free actions contribute nothing and are not screened here. Consensus passes only + // trn.actions to check_authorization, so CFAs are never authorization-checked there either; + // the sole authorization one may carry is a keyless `sysio.payer` marker that inherits an + // authorization proven on a regular action. Discovery mirrors check_authorization's rules, + // not transaction_context's -- the CFA shape rule lives only in validate_referenced_accounts + // and is deliberately left to it. --- + { + signed_transaction trx; + trx.context_free_actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + + // The CFA's marker adds no key; only the regular action's paired permission does. + BOOST_CHECK( required_keys( trx, all_keys ) == flat_set{ active_pub } ); + } + { + // A CFA marker naming an actor that is NOT a payer in trx.actions is rejected by + // validate_referenced_accounts, but discovery does not walk CFAs and so does not throw. + // Asserting the asymmetry keeps it deliberate: a later change that starts screening CFAs + // here should have to update this expectation rather than silently widen the function. + signed_transaction trx; + trx.context_free_actions.emplace_back( + vector{ { "bob"_n, config::sysio_payer_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + + BOOST_CHECK( required_keys( trx, keys_with_bob ) == flat_set{ active_pub } ); + + auto signed_trx = trx; + signed_trx.sign( active_key, control->get_chain_id() ); + BOOST_CHECK_EXCEPTION( push_transaction( signed_trx ), sysio::chain::transaction_exception, + fc_exception_message_is( + "context-free actions can only have a valid explicit payer authorization" ) ); + } + + // A well-formed payer action alongside an unrelated one still discovers normally -- the rules + // above must not reject a transaction merely for containing more than one action. + { + signed_transaction trx; + trx.actions.emplace_back( + vector{ { "payloadless"_n, config::sysio_payer_name }, + { "payloadless"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + trx.actions.emplace_back( + vector{ { "bob"_n, config::active_name } }, + "payloadless"_n, "doit"_n, bytes{} ); + set_transaction_headers( trx ); + + BOOST_CHECK( required_keys( trx, keys_with_bob ) + == ( flat_set{ active_pub, bob_active_pub } ) ); + } + BOOST_REQUIRE_EQUAL( validate(), true ); } FC_LOG_AND_RETHROW() } From 42a44037c6e1786343063c326be0265efa2a4393 Mon Sep 17 00:00:00 2001 From: kevin Heifner Date: Wed, 26 Aug 2026 13:02:57 -0500 Subject: [PATCH 3/3] chain: track payer-marker presence separately from the actor An empty actor is valid JSON and decodes to account_name{}, so using an unset account_name as the "no payer declared" sentinel misreads {"" : sysio.payer, bob: active}: the marker assigns an empty actor, the sentinel still looks unset, the pairing check is skipped, and bob's key comes back for a transaction consensus rejects. Tracks presence with a bool instead. Consensus is not exposed to this. check_authorization uses the same idiom but validate_referenced_accounts rejects the non-existent actor first ("action's paying actor '' does not exist"), and that pass runs ahead of it on the apply path. get_required_keys is reached directly by /v1/chain/get_required_keys and never runs it, which is why the sentinel holds there and not here. The consensus copy is left alone. Also tightens the rejection tests. The helper signed active, owner and bob's keys for every malformed transaction even though owner is never declared, so had the structural guards been removed check_authorization would have thrown tx_irrelevant_sig over the unused signature and a broad throw-check would have stayed green -- rejected for carrying a useless signature rather than for the malformed payer. It now signs only the real permissions each case declares, derived from the transaction, and both halves assert the specific message rather than any fc::exception. Adds the empty-actor regression case. Verified it fails against the payer.empty() sentinel and passes with the bool. --- libraries/chain/authorization_manager.cpp | 14 +++++-- unittests/api_tests.cpp | 48 ++++++++++++++++++----- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/libraries/chain/authorization_manager.cpp b/libraries/chain/authorization_manager.cpp index 943bf238be..018aff9e69 100644 --- a/libraries/chain/authorization_manager.cpp +++ b/libraries/chain/authorization_manager.cpp @@ -652,16 +652,24 @@ namespace sysio { namespace chain { // duplicated rather than shared with the consensus validators -- this function is not on // the apply path, and refactoring the consensus copies to serve it would put a // non-consensus caller in a position to change consensus behaviour. + // Marker presence is tracked separately from the actor rather than using an empty + // account_name as the sentinel. An empty actor is valid JSON and decodes to + // account_name{}, so a `{"": sysio.payer}` entry would leave the sentinel looking unset + // and skip the pairing check below. Consensus is not exposed to that -- it rejects the + // empty actor earlier, in validate_referenced_accounts ("action's paying actor '' does + // not exist") -- but this function never runs that pass. + bool has_payer = false; account_name payer; for (size_t i = 0; i < act.authorization.size(); ++i) { const auto& declared_auth = act.authorization[i]; if (declared_auth.permission == config::sysio_payer_name) { - SYS_ASSERT( payer.empty(), irrelevant_auth_exception, + SYS_ASSERT( !has_payer, irrelevant_auth_exception, "Multiple payers specified for action" ); SYS_ASSERT( i == 0, irrelevant_auth_exception, "Explicit payer must be the first declared authorization" ); - payer = declared_auth.actor; + has_payer = true; + payer = declared_auth.actor; continue; } @@ -670,7 +678,7 @@ namespace sysio { namespace chain { declared_auth ); } - if (!payer.empty()) { + if (has_payer) { const bool paired = std::ranges::any_of(act.authorization, [&](const auto& auth) { return auth.actor == payer && auth.permission != config::sysio_payer_name; }); diff --git a/unittests/api_tests.cpp b/unittests/api_tests.cpp index be63c4551e..17031fe6a3 100644 --- a/unittests/api_tests.cpp +++ b/unittests/api_tests.cpp @@ -430,14 +430,29 @@ BOOST_FIXTURE_TEST_CASE(get_required_keys_explicit_payer_tests, validating_teste const auto bob_active_pub = get_public_key( "bob"_n, "active" ); const flat_set keys_with_bob{ owner_pub, active_pub, custom_pub, bob_active_pub }; - auto expect_both_reject = [&]( const signed_transaction& trx, const char* discovery_msg ) { + // Sign ONLY the real permissions each case actually declares. Signing spare keys would make + // check_authorization throw tx_irrelevant_sig if the structural guards were ever removed, and a + // broad throw-check would stay green for the wrong reason -- the transaction would be rejected + // for carrying a useless signature rather than for the malformed payer. Both halves also assert + // the specific message, so a case cannot pass on an unrelated failure. + auto expect_both_reject = [&]( const signed_transaction& trx, + const char* discovery_msg, + const char* consensus_msg ) { BOOST_CHECK_EXCEPTION( required_keys( trx, keys_with_bob ), fc::exception, fc_exception_message_starts_with( discovery_msg ) ); + auto signed_trx = trx; - signed_trx.sign( active_key, control->get_chain_id() ); - signed_trx.sign( owner_key, control->get_chain_id() ); - signed_trx.sign( get_private_key( "bob"_n, "active" ), control->get_chain_id() ); - BOOST_CHECK_THROW( push_transaction( signed_trx ), fc::exception ); + flat_set signed_actors; + for( const auto& act : trx.actions ) { + for( const auto& auth : act.authorization ) { + if( auth.permission == config::sysio_payer_name ) continue; + if( !signed_actors.insert( auth.actor ).second ) continue; + signed_trx.sign( get_private_key( auth.actor, auth.permission.to_string() ), + control->get_chain_id() ); + } + } + BOOST_CHECK_EXCEPTION( push_transaction( signed_trx ), fc::exception, + fc_exception_message_starts_with( consensus_msg ) ); }; auto payer_trx = [&]( vector auths ) { @@ -451,23 +466,37 @@ BOOST_FIXTURE_TEST_CASE(get_required_keys_explicit_payer_tests, validating_teste // payloadless as payer. Skipping the marker outright would have returned bob's key here. expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name }, { "bob"_n, config::active_name } } ), - "Payer authorization for" ); + "Payer authorization for", + "Payer 'payloadless' did not authorize this action" ); // Payer with no real permission at all -- would otherwise discover an EMPTY key set, which a // signing tool reads as "nothing to sign". expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name } } ), - "Payer authorization for" ); + "Payer authorization for", + "Payer 'payloadless' did not authorize this action" ); + + // An EMPTY payer actor. `account_name{}` is what an empty string decodes to, so tracking the + // marker by "is the actor still unset?" would leave this looking like no payer was declared and + // skip the pairing check -- handing back bob's key. Consensus never sees it, because + // validate_referenced_accounts rejects the non-existent actor first, and this function does not + // run that pass. + expect_both_reject( payer_trx( { { name{}, config::sysio_payer_name }, + { "bob"_n, config::active_name } } ), + "Payer authorization for", + "action's paying actor '' does not exist" ); // Marker present but not at index 0. expect_both_reject( payer_trx( { { "payloadless"_n, config::active_name }, { "payloadless"_n, config::sysio_payer_name } } ), + "Explicit payer must be the first declared authorization", "Explicit payer must be the first declared authorization" ); // Two markers on one action. expect_both_reject( payer_trx( { { "payloadless"_n, config::sysio_payer_name }, { "payloadless"_n, config::active_name }, { "payloadless"_n, config::sysio_payer_name } } ), - "Multiple payers specified for action" ); + "Multiple payers specified for action", + "action cannot have multiple payers" ); // Pairing must be on the SAME action: the marker is on action 0 while payloadless's real // permission sits on action 1. Consensus scopes the pairing per-action, so discovery must too. @@ -481,7 +510,8 @@ BOOST_FIXTURE_TEST_CASE(get_required_keys_explicit_payer_tests, validating_teste vector{ { "payloadless"_n, config::active_name } }, "payloadless"_n, "doit"_n, bytes{} ); set_transaction_headers( trx ); - expect_both_reject( trx, "Payer authorization for" ); + expect_both_reject( trx, "Payer authorization for", + "Payer 'payloadless' did not authorize this action" ); } // --- Context-free actions contribute nothing and are not screened here. Consensus passes only