From af89854a4367942d258006d739a3cb7be749cd63 Mon Sep 17 00:00:00 2001 From: Olek <115580134+oleks-rip@users.noreply.github.com> Date: Thu, 7 May 2026 08:57:50 -0400 Subject: [PATCH] fix: Stop tx processing if failed to delete expired credentials (#6715) (#6962) Co-authored-by: Ed Hennis Co-authored-by: Ayaz Salikhov Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../xrpl/ledger/helpers/CredentialHelpers.h | 8 +- .../ledger/helpers/CredentialHelpers.cpp | 35 +++-- .../ledger/helpers/PermissionedDEXHelpers.cpp | 2 +- src/libxrpl/tx/Transactor.cpp | 9 +- .../credentials/CredentialAccept.cpp | 4 +- .../credentials/CredentialDelete.cpp | 2 +- src/test/app/Credentials_test.cpp | 123 ++++++++++++++++++ .../handlers/orderbook/DepositAuthorized.cpp | 2 +- 8 files changed, 164 insertions(+), 21 deletions(-) diff --git a/include/xrpl/ledger/helpers/CredentialHelpers.h b/include/xrpl/ledger/helpers/CredentialHelpers.h index c60bd6c7fa..e06d225934 100644 --- a/include/xrpl/ledger/helpers/CredentialHelpers.h +++ b/include/xrpl/ledger/helpers/CredentialHelpers.h @@ -19,14 +19,10 @@ namespace credentials { // Check if credential sfExpiration field has passed ledger's parentCloseTime bool -checkExpired(std::shared_ptr const& sleCredential, NetClock::time_point const& closed); - -// Return true if any expired credential was found in arr (and deleted) -bool -removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j); +checkExpired(SLE const& sleCredential, NetClock::time_point const& closed); // Actually remove a credentials object from the ledger -TER +[[nodiscard]] TER deleteSLE(ApplyView& view, std::shared_ptr const& sleCredential, beast::Journal j); // Amendment and parameters checks for sfCredentialIDs field diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 22122216bc..05e45a404c 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include @@ -9,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -33,15 +35,16 @@ namespace xrpl { namespace credentials { bool -checkExpired(std::shared_ptr const& sleCredential, NetClock::time_point const& closed) +checkExpired(SLE const& sleCredential, NetClock::time_point const& closed) { std::uint32_t const exp = - (*sleCredential)[~sfExpiration].value_or(std::numeric_limits::max()); + sleCredential[~sfExpiration].value_or(std::numeric_limits::max()); std::uint32_t const now = closed.time_since_epoch().count(); return now > exp; } -bool +[[nodiscard]] +static Expected removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j) { auto const closeTime = view.header().parentCloseTime; @@ -53,11 +56,13 @@ removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j) auto const k = keylet::credential(h); auto const sleCred = view.peek(k); - if (sleCred && checkExpired(sleCred, closeTime)) + if (sleCred && checkExpired(*sleCred, closeTime)) { JLOG(j.trace()) << "Credentials are expired. Cred: " << sleCred->getText(); // delete expired credentials even if the transaction failed - deleteSLE(view, sleCred, j); + auto const err = deleteSLE(view, sleCred, j); + if (view.rules().enabled(fixSecurity3_1_3) && !isTesSuccess(err)) + return Unexpected(err); foundExpired = true; } } @@ -205,7 +210,7 @@ validDomain(ReadView const& view, uint256 domainID, AccountID const& subject) // allows expired credentials to be deleted by any transaction. if (sleCredential) { - if (checkExpired(sleCredential, closeTime)) + if (checkExpired(*sleCredential, closeTime)) { foundExpired = true; continue; @@ -324,7 +329,10 @@ verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, b credentials.pushBack(keyletCredential.key); } - bool const foundExpired = credentials::removeExpired(view, credentials, j); + auto const foundExpired = credentials::removeExpired(view, credentials, j); + if (!foundExpired.has_value()) + return foundExpired.error(); + for (auto const& h : credentials) { auto sleCredential = view.read(keylet::credential(h)); @@ -335,7 +343,7 @@ verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, b return tesSUCCESS; } - return foundExpired ? tecEXPIRED : tecNO_PERMISSION; + return *foundExpired ? tecEXPIRED : tecNO_PERMISSION; } TER @@ -355,8 +363,15 @@ verifyDepositPreauth( bool const credentialsPresent = tx.isFieldPresent(sfCredentialIDs); - if (credentialsPresent && credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j)) - return tecEXPIRED; + if (credentialsPresent) + { + auto const foundExpired = + credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j); + if (!foundExpired.has_value()) + return foundExpired.error(); + if (*foundExpired) + return tecEXPIRED; + } if (sleDst && ((sleDst->getFlags() & lsfDepositAuth) != 0u)) { diff --git a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp index 456c4e95d6..7b4194cccf 100644 --- a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp +++ b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp @@ -35,7 +35,7 @@ accountInDomain(ReadView const& view, AccountID const& account, Domain const& do if (!sleCred || !sleCred->isFlag(lsfAccepted)) return false; - return !credentials::checkExpired(sleCred, view.header().parentCloseTime); + return !credentials::checkExpired(*sleCred, view.header().parentCloseTime); }); return inDomain; diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 79dec33c66..aff46cc4bc 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1010,7 +1010,14 @@ removeExpiredCredentials(ApplyView& view, std::vector const& creds, bea for (auto const& index : creds) { if (auto const sle = view.peek(keylet::credential(index))) - credentials::deleteSLE(view, sle, viewJ); + { + if (auto const ter = credentials::deleteSLE(view, sle, viewJ); !isTesSuccess(ter)) + { + JLOG(viewJ.error()) + << "removeExpiredCredentials: failed to delete expired credential. Err: " + << transToken(ter); + } + } } } diff --git a/src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp b/src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp index 2d27d1bf23..22e0df5df5 100644 --- a/src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp +++ b/src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp @@ -105,8 +105,10 @@ CredentialAccept::doApply() auto const credType(ctx_.tx[sfCredentialType]); Keylet const credentialKey = keylet::credential(account_, issuer, credType); auto const sleCred = view().peek(credentialKey); // Checked in preclaim() + if (!sleCred) + return tefINTERNAL; // LCOV_EXCL_LINE - if (checkExpired(sleCred, view().header().parentCloseTime)) + if (checkExpired(*sleCred, view().header().parentCloseTime)) { JLOG(j_.trace()) << "Credential is expired: " << sleCred->getText(); // delete expired credentials even if the transaction failed diff --git a/src/libxrpl/tx/transactors/credentials/CredentialDelete.cpp b/src/libxrpl/tx/transactors/credentials/CredentialDelete.cpp index 595064e824..e83a8b09dc 100644 --- a/src/libxrpl/tx/transactors/credentials/CredentialDelete.cpp +++ b/src/libxrpl/tx/transactors/credentials/CredentialDelete.cpp @@ -87,7 +87,7 @@ CredentialDelete::doApply() return tefINTERNAL; // LCOV_EXCL_LINE if ((subject != account_) && (issuer != account_) && - !checkExpired(sleCred, ctx_.view().header().parentCloseTime)) + !checkExpired(*sleCred, ctx_.view().header().parentCloseTime)) { JLOG(j_.trace()) << "Can't delete non-expired credential."; return tecNO_PERMISSION; diff --git a/src/test/app/Credentials_test.cpp b/src/test/app/Credentials_test.cpp index fa180d0603..fced18c387 100644 --- a/src/test/app/Credentials_test.cpp +++ b/src/test/app/Credentials_test.cpp @@ -5,9 +5,12 @@ #include #include #include +#include #include #include #include +#include +#include #include #include #include @@ -24,11 +27,13 @@ #include #include #include +#include #include #include #include #include +#include #include #include @@ -1028,6 +1033,121 @@ struct Credentials_test : public beast::unit_test::Suite } } + void + testRemoveExpiredCorruption(FeatureBitset features) + { + bool const fixEnabled = features[fixSecurity3_1_3]; + testcase( + "removeExpired ignores deleteSLE failure " + + (fixEnabled ? std::string(" after fix") : std::string(" before fix"))); + + using namespace test::jtx; + + char const credType[] = "abcde"; + Account const issuer{"issuer"}; + Account const subject{"subject"}; + Account const becky{"becky"}; + + Env env{*this, features}; + env.fund(XRP(10000), issuer, subject, becky); + env.close(); + + // Create credential with short expiration + auto jv = credentials::create(subject, issuer, credType); + uint32_t const expiration = + env.current()->header().parentCloseTime.time_since_epoch().count() + 40; + jv[sfExpiration.jsonName] = expiration; + env(jv); + env.close(); + + auto const credLE = credentials::ledgerEntry(env, subject, issuer, credType); + std::string const credIdx = credLE[jss::result][jss::index].asString(); + + // Subject accepts the credential + env(credentials::accept(subject, issuer, credType)); + env.close(); + + // Build the credential keylet + auto const credKeylet = + keylet::credential(subject.id(), issuer.id(), Slice(credType, std::strlen(credType))); + + // Verify credential exists and is accepted + { + auto const sleCred = env.current()->read(credKeylet); + BEAST_EXPECT(sleCred && sleCred->getFlags() & lsfAccepted); + } + + // Create DepositPreauth + env(deposit::authCredentials(becky, {{subject, credType}})); + env.close(); + // env(); + auto jtx = env.jt(pay(subject, becky, XRP(100)), credentials::Ids({credIdx})); + if (!BEAST_EXPECT(jtx.stx)) + return; + auto const stx = std::make_shared(*jtx.stx); + + // Create PermissionedDomain + env(pdomain::setTx(becky, {{issuer, credType}})); + env.close(); + auto const objects = pdomain::getObjects(becky, env); + if (!BEAST_EXPECT(!objects.empty())) + return; + auto const domain = objects.begin()->first; + + using namespace std::chrono_literals; + env.close(50s); + + // Verify time has advanced past expiration + { + auto const sleCred = env.current()->read(credKeylet); + BEAST_EXPECT( + sleCred && + xrpl::credentials::checkExpired(*sleCred, env.current()->header().parentCloseTime)); + } + + // Create an ApplyViewImpl on top of the current closed ledger + // and corrupt it by erasing the issuer's account SLE + auto const open = env.current(); + ApplyViewImpl av(&*open, TapNone); + + // Erase the issuer's account to simulate ledger corruption + auto sleIssuer = av.peek(keylet::account(issuer.id())); + if (!BEAST_EXPECT(sleIssuer)) + return; + av.erase(sleIssuer); + BEAST_EXPECT(!av.exists(keylet::account(issuer.id()))); + + // Credential still exists before removeExpired + BEAST_EXPECT(av.exists(credKeylet)); + + // Call removeExpired on the corrupted view + STVector256 credHashes; + credHashes.pushBack(credKeylet.key); + beast::Journal const j{beast::Journal::getNullSink()}; + + auto const dpTer = xrpl::verifyDepositPreauth(*stx, av, subject, becky, {}, j); + auto sleCredAfter = av.read(credKeylet); + BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted)); + + auto const domTer = xrpl::verifyValidDomain(av, subject.id(), domain, j); + sleCredAfter = av.read(credKeylet); + BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted)); + + if (fixEnabled) + { + // removeExpired returns error, cred wasn't deleted + BEAST_EXPECT(dpTer == tecINTERNAL); + BEAST_EXPECT(domTer == tecINTERNAL); + } + else + { + // removeExpired returns true (claims it found & deleted expired + // creds) + BEAST_EXPECT(dpTer == tecEXPIRED); + BEAST_EXPECT(isTesSuccess(domTer)); + } + } + void run() override { @@ -1043,6 +1163,9 @@ struct Credentials_test : public beast::unit_test::Suite testFlags(all - fixInvalidTxFlags); testFlags(all); testRPC(); + + testRemoveExpiredCorruption(all - fixSecurity3_1_3); + testRemoveExpiredCorruption(all | fixSecurity3_1_3); } }; diff --git a/src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp b/src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp index 7e9f891e7f..399df7c6ca 100644 --- a/src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp +++ b/src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp @@ -141,7 +141,7 @@ doDepositAuthorized(RPC::JsonContext& context) return result; } - if (credentials::checkExpired(sleCred, ledger->header().parentCloseTime)) + if (credentials::checkExpired(*sleCred, ledger->header().parentCloseTime)) { RPC::injectError(RpcBadCredentials, "credentials are expired", result); return result;