diff --git a/include/xrpl/protocol_autogen/transactions/TransactionProposalCreate.h b/include/xrpl/protocol_autogen/transactions/TransactionProposalCreate.h index 47eed43ee7..b436fda506 100644 --- a/include/xrpl/protocol_autogen/transactions/TransactionProposalCreate.h +++ b/include/xrpl/protocol_autogen/transactions/TransactionProposalCreate.h @@ -21,7 +21,7 @@ class TransactionProposalCreateBuilder; * Type: ttTRANSACTION_PROPOSAL_CREATE (92) * Delegable: Delegation::NotDelegable * Amendment: featureCosign - * Privileges: NoPriv + * Privileges: Privilege::NoPriv * * Immutable wrapper around STTx providing type-safe field access. * Use TransactionProposalCreateBuilder to construct new transactions. diff --git a/include/xrpl/tx/applySteps.h b/include/xrpl/tx/applySteps.h index bd495481f2..19afb740f8 100644 --- a/include/xrpl/tx/applySteps.h +++ b/include/xrpl/tx/applySteps.h @@ -385,6 +385,23 @@ preflight( PreclaimResult preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, OpenView const& view); +/** + * Type-erased overload of Transactor::invokeCheckPermission. + * + * Dispatches on the transaction type to Transactor::invokeCheckPermission + * so a caller that only has an STTx still gets the same verdict submission + * uses: transaction-level permission, then granular permissions and + * checkGranularSandbox, then that type's checkGranularSemantics. Does not + * check SignerList or signing keys. + * + * An unknown transaction type (should not occur after a successful preflight) + * is treated as an internal invariant violation: UNREACHABLE is fired and the + * type-erased fallback return is temUNKNOWN, mirroring the sibling + * invokePreflight/invokePreclaim/invokeApply overloads in this header. + */ +NotTEC +invokeCheckPermission(ReadView const& view, STTx const& tx); + /** * Compute only the expected base fee for a transaction. * diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index a654adfb3b..40b0ef57f6 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -375,6 +375,27 @@ preflight( } } +NotTEC +invokeCheckPermission(ReadView const& view, STTx const& tx) +{ + try + { + return withTxnType(view.rules(), tx.getTxnType(), [&]() { + return Transactor::invokeCheckPermission(view, tx); + }); + } + catch (UnknownTxnType const& e) + { + // Should never happen + // LCOV_EXCL_START + JLOG(debugLog().fatal()) << "Unknown transaction type in invokeCheckPermission: " + << e.txnType; + UNREACHABLE("xrpl::invokeCheckPermission : unknown transaction type"); + return temUNKNOWN; + // LCOV_EXCL_STOP + } +} + PreclaimResult preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, OpenView const& view) { diff --git a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp index 560dee044d..e9d04c41f5 100644 --- a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp +++ b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -188,12 +187,38 @@ TransactionProposalCreate::preclaim(PreclaimContext const& ctx) if (!sleSigners) return false; - auto const accountSigners = SignerEntries::deserialize(*sleSigners, ctx.j, "ledger"); - if (!accountSigners) - return std::unexpected(TER{accountSigners.error()}); + // deserialize itself returns unexpected(temMALFORMED) when + // sfSignerEntries is missing or an element is not an sfSignerEntry. + // Those are the right codes for a transaction object. Here the object + // is an on-ledger ltSIGNER_LIST (sfSignerEntries is SoeRequired; + // each element is an sfSignerEntry). A corrupt SLE can still throw + // from the STObject accessors deserialize calls: getFieldArray + // ("Wrong field type") or getAccountID/getFieldU16 ("Field not + // found") when an sfSignerEntry is missing required fields. Either + // the expected<> error or a throw is unexpected ledger state, not a + // malformed TransactionProposalCreate, so tefBAD_LEDGER (rather + // than tefINTERNAL, which is reserved for truly unreachable code + // paths) is the right code. + try + { + auto const accountSigners = + SignerEntries::deserialize(*sleSigners, ctx.j, "ledger"); + if (!accountSigners) + { + JLOG(ctx.j.fatal()) << "TransactionProposalCreate: unparseable SignerList: " + << transToken(accountSigners.error()); + return std::unexpected(tefBAD_LEDGER); + } - return std::ranges::any_of( - *accountSigners, [&](auto const& entry) { return entry.account == proposer; }); + return std::ranges::any_of( + *accountSigners, [&](auto const& entry) { return entry.account == proposer; }); + } + catch (std::exception const& e) + { + JLOG(ctx.j.fatal()) + << "TransactionProposalCreate: unparseable SignerList: " << e.what(); + return std::unexpected(tefBAD_LEDGER); + } }; auto isSigner = isAuthorizedFor(target); @@ -201,18 +226,29 @@ TransactionProposalCreate::preclaim(PreclaimContext const& ctx) return isSigner.error(); // A delegate that the target has granted permission over the - // proposed transaction's own type — or one of that delegate's own - // signers — is equally authorized: it will need to help complete - // the proposed transaction's own authorization anyway once the - // proposal is submitted. + // proposed transaction — or one of that delegate's own signers — is + // equally authorized: it will need to help complete the proposed + // transaction's own authorization anyway once the proposal is + // submitted. xrpl::invokeCheckPermission is the type-erased + // submission hierarchy (not checkTxPermission alone, which would + // reject a matching granular grant). Qualify xrpl:: so the inherited + // Transactor template is not chosen; it cannot deduce T here. A + // failed grant is still "not authorized" and becomes + // tecNO_PERMISSION below — Create is already signed, so do not leak + // the pre-sign terNO_DELEGATE_PERMISSION. if (!*isSigner && proposedTx.isFieldPresent(sfDelegate)) { AccountID const delegateAccount = proposedTx.getAccountID(sfDelegate); - // NOLINTNEXTLINE(readability-suspicious-call-argument) - auto const sleDelegate = ctx.view.read(keylet::delegate(target, delegateAccount)); - if (sleDelegate && - isTesSuccess(checkTxPermission(sleDelegate, STTx{STObject{proposedTx}}))) + STTx const proposedStTx{STObject{proposedTx}}; + if (isTesSuccess(xrpl::invokeCheckPermission(ctx.view, proposedStTx))) { + // A grant cannot exist without a funded authorize (DelegateSet + // uses tecNO_TARGET; AccountDelete of the delegatee removes the + // Delegate SLE). Do not treat a missing account as a Create-time + // user error — that would extra-validate the proposed tx. If + // permission passed anyway, the ledger is corrupt. + if (!ctx.view.exists(keylet::account(delegateAccount))) + return tefINTERNAL; // LCOV_EXCL_LINE isSigner = isAuthorizedFor(delegateAccount); if (!isSigner) return isSigner.error(); diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index c69c03945c..b17d835ff0 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -339,8 +339,20 @@ Batch::preflight(PreflightContext const& ctx) return temINVALID_FLAG; auto const innerAccount = stx.getAccountID(sfAccount); + // TransactionProposalCreate preflights a proposed Batch with + // TapDryRun | TapProposal so signature-presence checks are deferred + // to collection time (On-Chain Cosigner spec §5.3.1.2). Inner + // preflight used to pass only TapBatch, so those bits never reached + // the inners: an unsigned account-reserve SponsorshipTransfer then + // demanded sfSponsorSignature and the Create failed with + // temINVALID_INNER_BATCH. Spec §6.1.1 names an inner Sponsor as a + // collectable slot, so forward TapProposal/TapDryRun. Always OR in + // TapBatch — PreflightContext with a parentBatchId requires it. + // LoanSet already short-circuits on tfInnerBatchTxn; it is also in + // kDisabledTxTypes, so it never reaches this call. + ApplyFlags const innerFlags = TapBatch | (ctx.flags & (TapProposal | TapDryRun)); if (auto const preflightResult = - xrpl::preflight(ctx.registry, ctx.rules, parentBatchId, stx, TapBatch, ctx.j); + xrpl::preflight(ctx.registry, ctx.rules, parentBatchId, stx, innerFlags, ctx.j); !isTesSuccess(preflightResult.ter)) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " diff --git a/src/test/app/TransactionProposalCreate_test.cpp b/src/test/app/TransactionProposalCreate_test.cpp index 2b9ec279d7..9e13a4b90d 100644 --- a/src/test/app/TransactionProposalCreate_test.cpp +++ b/src/test/app/TransactionProposalCreate_test.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -20,14 +21,19 @@ #include #include +#include #include +#include #include #include #include #include #include #include +#include +#include #include +#include #include #include #include @@ -36,6 +42,7 @@ #include #include #include +#include #include #include @@ -698,6 +705,119 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite } } + // An on-ledger ltSIGNER_LIST that cannot be read as signer entries is + // unexpected ledger state, not a malformed transaction. preclaim must + // surface tefBAD_LEDGER (not temMALFORMED, not tefINTERNAL which is + // reserved for truly unreachable paths, and not the tefEXCEPTION that + // applySteps would wrap an uncaught throw with). + // + // SignerEntries::deserialize returns unexpected(temMALFORMED) when + // sfSignerEntries is missing or an element is not named sfSignerEntry. + // It still throws from STObject accessors when an sfSignerEntry is + // missing required fields (getAccountID → "Field not found: Account"). + // Do not close() after the synthetic corruption: a closed ledger would + // drop the overlay and restore a well-formed list. + void + testCorruptSignerList(FeatureBitset features) + { + testcase("unparseable on-ledger SignerList is tefBAD_LEDGER"); + + using namespace jtx; + using namespace std::chrono_literals; + + auto setup = [&](Env& env, Account const& target, Account const& signer) { + env.fund(XRP(10000), target, signer); + env.close(); + env(signers(target, 1, {{signer, 1}})); + env.close(); + // Ticket first: createTicket closes, which would drop a later + // open-ledger overlay and restore a well-formed SignerList. + return proposal::createTicket(env, target); + }; + + auto proposeAsSigner = + [&](Env& env, Account const& target, Account const& signer, std::uint32_t ticketSeq) { + env(proposal::create( + signer, + proposal::unsignedPayload(env, pay(target, signer, XRP(1)), ticketSeq), + proposal::expiration(env, 100s)), + Ter(tefBAD_LEDGER), + proposal::verify::create()); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + }; + + { + Env env{*this, features}; + Account const target{"targetMissing"}; + Account const signer{"signerMissing"}; + std::uint32_t const ticketSeq = setup(env, target, signer); + + auto const signerListKeylet = keylet::signerList(target.id()); + BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) { + auto const sle = view.read(signerListKeylet); + if (!sle) + return false; + auto replacement = std::make_shared(*sle); + if (!replacement->delField(sfSignerEntries)) + return false; + view.rawReplace(replacement); + return true; + })); + BEAST_EXPECT(env.le(signerListKeylet)); + + proposeAsSigner(env, target, signer, ticketSeq); + } + + { + Env env{*this, features}; + Account const target{"targetBadEntry"}; + Account const signer{"signerBadEntry"}; + std::uint32_t const ticketSeq = setup(env, target, signer); + + auto const signerListKeylet = keylet::signerList(target.id()); + BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) { + auto const sle = view.read(signerListKeylet); + if (!sle) + return false; + auto replacement = std::make_shared(*sle); + STArray badEntries; + badEntries.pushBack(STObject{sfSigner}); + replacement->setFieldArray(sfSignerEntries, badEntries); + view.rawReplace(replacement); + return true; + })); + BEAST_EXPECT(env.le(signerListKeylet)); + + proposeAsSigner(env, target, signer, ticketSeq); + } + + { + Env env{*this, features}; + Account const target{"targetMissingAccount"}; + Account const signer{"signerMissingAccount"}; + std::uint32_t const ticketSeq = setup(env, target, signer); + + auto const signerListKeylet = keylet::signerList(target.id()); + BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) { + auto const sle = view.read(signerListKeylet); + if (!sle) + return false; + auto replacement = std::make_shared(*sle); + STArray badEntries; + // Right inner name, but no sfAccount: deserialize calls + // getAccountID and throws (Field not found), which the + // catch maps to tefBAD_LEDGER. + badEntries.pushBack(STObject{sfSignerEntry}); + replacement->setFieldArray(sfSignerEntries, badEntries); + view.rawReplace(replacement); + return true; + })); + BEAST_EXPECT(env.le(signerListKeylet)); + + proposeAsSigner(env, target, signer, ticketSeq); + } + } + // The target account may delegate authority over the proposed // transaction's own type to another account (Permission Delegation, // XLS-75); if it does, that delegate — or an account on the delegate's @@ -781,6 +901,81 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite } } + // A delegate holding only a granular permission (XLS-75) that would + // authorize submitting the proposed transaction may also create a + // proposal for it. A granular grant that fails checkGranularSandbox + // still cannot. + void + testDelegatedGranularProposedTx(FeatureBitset features) + { + testcase("proposer authorized through granular delegate permission"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const gw{"gw"}; // issuer / proposed-tx Account + Account const alice{"alice"}; // holder of the trust line being authorized + Account const bob{"bob"}; // delegate with TrustlineAuthorize only + env.fund(XRP(10000), gw, alice, bob); + env(fset(gw, asfRequireAuth)); + env.close(); + + env(trust(alice, gw["USD"](50))); + env.close(); + env(delegate::set(gw, bob, {"TrustlineAuthorize"})); + env.close(); + + auto delegatedTrustSet = [&](std::uint32_t ticketSeq, std::uint32_t flags) { + json::Value tx = trust(gw, gw["USD"](0), alice, flags); + tx[sfDelegate.jsonName] = bob.human(); + return proposal::unsignedPayload(env, tx, ticketSeq); + }; + + // TrustlineAuthorize is sufficient for a tfSetfAuth TrustSet against + // an existing line whose limit is unchanged — the same shape that + // submits successfully under invokeCheckPermission. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, gw); + env(proposal::create( + bob, delegatedTrustSet(ticketSeq, tfSetfAuth), proposal::expiration(env, 100s)), + proposal::verify::create()); + env.close(); + BEAST_EXPECT(proposal::entry(env, gw, ticketSeq)); + } + + // tfSetFreeze is not in TrustlineAuthorize's sandbox. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, gw); + env(proposal::create( + bob, + delegatedTrustSet(ticketSeq, tfSetFreeze), + proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION), + proposal::verify::create()); + env.close(); + BEAST_EXPECT(!proposal::entry(env, gw, ticketSeq)); + } + + // sfQualityOut is a valid TrustSet field but not in the granular + // template, so checkGranularSandbox rejects it. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, gw); + json::Value tx = trust(gw, gw["USD"](0), alice, tfSetfAuth); + tx[sfDelegate.jsonName] = bob.human(); + tx[sfQualityOut.jsonName] = 100; + env(proposal::create( + bob, + proposal::unsignedPayload(env, tx, ticketSeq), + proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION), + proposal::verify::create()); + env.close(); + BEAST_EXPECT(!proposal::entry(env, gw, ticketSeq)); + } + } + // The target account must be able to authorize a transaction through a // SignerList, so a pseudo-account (here an AMM's) cannot be a target even // though it exists on-ledger (On-Chain Cosigner spec §5.3.2.5). @@ -1282,6 +1477,75 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, target) == 1 + proposal::kBatchProposalOwnerCount); } + // A proposed Batch's inner preflight must receive TapProposal, or an + // unsigned account-reserve SponsorshipTransfer is rejected at Create + // (On-Chain Cosigner spec §6.1.1: an inner Sponsor is a collectable + // signature slot). Other inner types that do not key on TapProposal keep + // their existing preflight result. + void + testProposedBatchInnerSponsorship(FeatureBitset features) + { + testcase("proposed batch inner account-reserve SponsorshipTransfer"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const target{"target"}; + Account const bob{"bob"}; // named as Sponsor; signature collected later + env.fund(XRP(10000), target, bob); + env.close(); + + auto unsignedInnerSponsorship = [&](Account const& account) { + json::Value tx = sponsor::transfer(account, tfSponsorshipCreate); + tx[sfSponsor.getJsonName()] = bob.human(); + tx[sfSponsorFlags.getJsonName()] = spfSponsorReserve; + return tx; + }; + + // Payment (unaffected by TapProposal) plus an unsigned inner + // account-reserve SponsorshipTransfer. Create succeeds only if + // TapProposal reaches the inner. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + auto const seq = env.seq(target); + json::Value const proposedTx = proposal::unsignedBatch( + env, + target, + ticketSeq, + tfAllOrNothing, + {proposal::innerTx(pay(target, bob, XRP(1)), seq), + proposal::innerTx(unsignedInnerSponsorship(target), seq + 1)}); + + env(proposal::create(target, proposedTx, proposal::expiration(env, 100s)), + proposal::verify::create()); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // Structural inner failures are unchanged: missing sfSponsor is still + // temMALFORMED in SponsorshipTransfer::preflight, collapsed by Batch + // to temINVALID_INNER_BATCH. TapProposal does not skip that. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + auto const seq = env.seq(target); + json::Value const proposedTx = proposal::unsignedBatch( + env, + target, + ticketSeq, + tfAllOrNothing, + {proposal::innerTx(pay(target, bob, XRP(1)), seq), + proposal::innerTx(sponsor::transfer(target, tfSponsorshipCreate), seq + 1)}); + + env(proposal::create(target, proposedTx, proposal::expiration(env, 100s)), + Ter(temINVALID_INNER_BATCH), + proposal::verify::create()); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + } + } + void run() override { @@ -1299,7 +1563,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // Preclaim testPreclaim(all); testProposerAuthorization(all); + testCorruptSignerList(all); testDelegatedProposedTx(all); + testDelegatedGranularProposedTx(all); testPseudoTarget(all); // Apply @@ -1312,6 +1578,7 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite testFeeSponsored(all); testBatchReserve(all); testMultiAccountBatch(all); + testProposedBatchInnerSponsorship(all); } }; diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 1450709f59..d9ad957db1 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -9,6 +9,7 @@ #include // IWYU pragma: keep #include #include +#include #include #include #include @@ -35,6 +36,7 @@ #include #include +#include // IWYU pragma: keep #include #include #include @@ -1699,6 +1701,64 @@ public: BEAST_EXPECT(res[jss::result].isMember(jss::marker)); } + // A TransactionProposal blocks AccountDelete (it is not in + // nonObligationDeleter) but was omitted from kDeletionBlockers, so + // deletion_blockers_only reported a clean directory. Tickets on the + // same account are auto-removed at delete time and must stay omitted. + void + testDeletionBlockersProposal() + { + testcase("deletion_blockers_only includes TransactionProposal"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, testableAmendments()}; + + Account const alice{"alice"}; + Account const bob{"bob"}; + env.fund(XRP(10000), alice, bob); + env.close(); + + std::uint32_t const ticketSeq = proposal::createTicket(env, alice); + env(proposal::create( + alice, + proposal::unsignedPayload(env, pay(alice, bob, XRP(1)), ticketSeq), + proposal::expiration(env, 100s)), + proposal::verify::create()); + env.close(); + + json::Value params; + params[jss::account] = alice.human(); + params[jss::deletion_blockers_only] = true; + params[jss::ledger_index] = "validated"; + + { + auto const resp = env.rpc("json", "account_objects", to_string(params)); + auto const& aobjs = resp[jss::result][jss::account_objects]; + if (BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 1)) + { + BEAST_EXPECT(aobjs[0u][sfLedgerEntryType.jsonName] == jss::TransactionProposal); + BEAST_EXPECT(aobjs[0u][sfOwner.jsonName] == alice.human()); + } + } + + { + params[jss::type] = jss::transaction_proposal; + auto const resp = env.rpc("json", "account_objects", to_string(params)); + auto const& aobjs = resp[jss::result][jss::account_objects]; + if (BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 1)) + BEAST_EXPECT(aobjs[0u][sfLedgerEntryType.jsonName] == jss::TransactionProposal); + } + + { + params[jss::type] = jss::check; + auto const resp = env.rpc("json", "account_objects", to_string(params)); + auto const& aobjs = resp[jss::result][jss::account_objects]; + BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 0); + } + } + void run() override { @@ -1711,6 +1771,7 @@ public: testAccountObjectMarker(); testSponsoredFilter(); testAccountObjectDoesntShowCancelledOffers(); + testDeletionBlockersProposal(); } }; diff --git a/src/xrpld/rpc/handlers/account/AccountObjects.cpp b/src/xrpld/rpc/handlers/account/AccountObjects.cpp index e855ed65e6..600f41d2e7 100644 --- a/src/xrpld/rpc/handlers/account/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/account/AccountObjects.cpp @@ -314,6 +314,7 @@ doAccountObjects(rpc::JsonContext& context) {.name = jss::permissioned_domain, .type = ltPERMISSIONED_DOMAIN}, {.name = jss::vault, .type = ltVAULT}, {.name = jss::sponsorship, .type = ltSPONSORSHIP}, + {.name = jss::transaction_proposal, .type = ltTRANSACTION_PROPOSAL}, }; typeFilter.emplace();