diff --git a/libraries/chain/authorization_manager.cpp b/libraries/chain/authorization_manager.cpp index 55f803fc78..018aff9e69 100644 --- a/libraries/chain/authorization_manager.cpp +++ b/libraries/chain/authorization_manager.cpp @@ -613,19 +613,77 @@ 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) { - 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. 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. + // 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]; - } else { - SYS_ASSERT( checker.satisfied(declared_auth), unsatisfied_authorization, - "transaction declares authority '{}', but does not have signatures for it.", - declared_auth ); + if (declared_auth.permission == config::sysio_payer_name) { + 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" ); + has_payer = true; + 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 (has_payer) { + 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 ); } } diff --git a/unittests/api_tests.cpp b/unittests/api_tests.cpp index 0bc883196d..17031fe6a3 100644 --- a/unittests/api_tests.cpp +++ b/unittests/api_tests.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -300,6 +301,283 @@ 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(); + } + + // --- 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 }; + + // 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; + 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 ) { + 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 '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 '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", + "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. + { + 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", + "Payer 'payloadless' did not authorize this action" ); + } + + // --- 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() } + BOOST_FIXTURE_TEST_CASE(action_tests, validating_tester) { try { produce_blocks(2); create_account( "testapi"_n );