Add HookName validation to ClaimReward hookCan process

include/xrpl/protocol/detail/features.macro

@@ -34,6 +34,7 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.
 
+XRPL_FIX    (HookNameValidation,         Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FIX    (HookMap,                    Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FIX    (GuardDepth32,               Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(NamedHooks,                 Supported::yes, VoteBehavior::DefaultNo)

src/test/app/ClaimReward_test.cpp

@@ -563,6 +563,20 @@ public:
                 env(reward::claim(alice),
                     reward::issuer(issuer),
                     ter(tecNO_TARGET));
+                env.close();
+            }
+            // Invalid HookName
+            {
+                auto hookObj = hso(jtx::genesis::AcceptHook, overrideFlag);
+                hookObj[jss::HookOn] = to_string(~UINT256_BIT[ttCLAIM_REWARD]);
+                hookObj[jss::HookName] = "4142434445";
+                env(hook(issuer, {{hookObj}}, 0), fee(XRP(1)));
+                env.close();
+
+                env(reward::claim(alice),
+                    reward::issuer(issuer),
+                    ter(tecNO_TARGET));
+                env.close();
             }
             // Vaild HookOn
             {

src/test/app/SetHook_test.cpp

@@ -2002,11 +2002,35 @@ public:
             BEAST_EXPECT(!env.meta()->isFieldPresent(sfHookEmissions));
         }
 
-        // Call named hook with the wrong hook name
+        // Call named hook with the wrong hook name (size == 0)
+        for (auto const fix : {true, false})
+        {
+            auto f = features - fixHookNameValidation;
+            if (fix)
+                f = f | fixHookNameValidation;
+            Env env{*this, f};
+
+            env.fund(XRP(10000), alice);
+            // execute both named and non-named hooks
+
+            auto jv = invoke::invoke(alice);
+            jv[jss::HookName] = "";
+
+            auto const expected = fix ? ter(temMALFORMED) : ter(tesSUCCESS);
+            env(jv,
+                M("Call named hook with the wrong hook name (size == 0)"),
+                HSFEE,
+                ter(expected));
+            env.close();
+        }
+
+        // Call named hook with the wrong hook name (size > 0)
         {
             auto jv = invoke::invoke(alice);
             jv[jss::HookName] = "41424345";
-            env(jv, M("Call named hook with the wrong hook name"), HSFEE);
+            env(jv,
+                M("Call named hook with the wrong hook name (size > 0)"),
+                HSFEE);
             env.close();
             // execute only non-named hook
             BEAST_EXPECT(!env.meta()->isFieldPresent(sfHookEmissions));

src/xrpld/app/hook/applyHook.h

@@ -82,10 +82,13 @@ namespace hook_api {
 namespace hook {
 
 bool
-canHook(ripple::TxType txType, ripple::uint256 hookOn);
+canHook(
+    STTx const& tx,
+    ripple::uint256 const& hookOn,
+    std::optional<ripple::Blob> const& hookName);
 
 bool
-canEmit(ripple::TxType txType, ripple::uint256 hookCanEmit);
+canEmit(ripple::TxType const& txType, ripple::uint256 const& hookCanEmit);
 
 ripple::uint256
 getHookCanEmit(ripple::STObject const& hookObj, SLE::pointer const& hookDef);

src/xrpld/app/hook/detail/applyHook.cpp

@@ -813,21 +813,44 @@ hook::removeHookNamespaceEntry(ripple::SLE& sleAccount, ripple::uint256 ns)
 // transactions. If you wish to set a hook that has control over ttHOOK_SET then
 // set bit 1U<<22.
 bool
-hook::canHook(ripple::TxType txType, ripple::uint256 hookOn)
+canHookTT(ripple::TxType const& txType, ripple::uint256 const& hookOn)
 {
+    uint256 hookOnCopy = hookOn;
+
     // invert ttHOOK_SET bit
-    hookOn ^= UINT256_BIT[ttHOOK_SET];
+    hookOnCopy ^= UINT256_BIT[ttHOOK_SET];
 
     // invert entire field
-    hookOn = ~hookOn;
+    hookOnCopy = ~hookOnCopy;
 
-    return (hookOn & UINT256_BIT[txType]) != beast::zero;
+    return (hookOnCopy & UINT256_BIT[txType]) != beast::zero;
 }
 
 bool
-hook::canEmit(ripple::TxType txType, ripple::uint256 hookCanEmit)
+hook::canEmit(ripple::TxType const& txType, ripple::uint256 const& hookCanEmit)
 {
-    return hook::canHook(txType, hookCanEmit);
+    return canHookTT(txType, hookCanEmit);
+}
+
+bool
+hook::canHook(
+    STTx const& tx,
+    ripple::uint256 const& hookOn,
+    std::optional<ripple::Blob> const& hookName)
+{
+    if (!canHookTT(tx.getTxnType(), hookOn))
+        return false;
+
+    if (!hookName)
+        // no hook name specified to hook, so we can always hook
+        return true;
+
+    if (!tx.isFieldPresent(sfHookName))
+        // hook name specified hook, but no hook name specified in the
+        // transaction, so we can't hook without the hook name
+        return false;
+
+    return tx.getFieldVL(sfHookName) == hookName;
 }
 
 ripple::uint256

src/xrpld/app/tx/detail/ClaimReward.cpp

@@ -164,7 +164,12 @@ ClaimReward::preclaim(PreclaimContext const& ctx)
 
                 auto const& hookOn =
                     hook::getHookOn(hook, sleDef, sfHookOnIncoming);
-                if (hook::canHook(ttCLAIM_REWARD, hookOn))
+                auto const& hookName =
+                    ctx.view.rules().enabled(fixHookNameValidation) &&
+                        hook.isFieldPresent(sfHookName)
+                    ? std::optional<ripple::Blob>(hook.getFieldVL(sfHookName))
+                    : std::nullopt;
+                if (hook::canHook(ctx.tx, hookOn, hookName))
                 {
                     hasClaimRewardHook = true;
                     break;

src/xrpld/app/tx/detail/Transactor.cpp

@@ -155,7 +155,11 @@ preflight1(PreflightContext const& ctx)
             !ctx.rules.enabled(featureNamedHooks))
             return temMALFORMED;
 
-        if (!SetHook::validateHookName(ctx.tx.getFieldVL(sfHookName), ctx.j))
+        auto const& name = ctx.tx.getFieldVL(sfHookName);
+
+        if (name.size() == 0 && ctx.rules.enabled(fixHookNameValidation))
+            return temMALFORMED;
+        if (!SetHook::validateHookName(name, ctx.j))
             return temMALFORMED;
     }
 
@@ -283,19 +287,10 @@ Transactor::calculateHookChainFee(
             // LCOV_EXCL_STOP
         }
 
-        std::optional<Blob> requiredHookName;
+        std::optional<Blob> hookName;
         if (hookObj.isFieldPresent(sfHookName) &&
             hookObj.getFieldVL(sfHookName).size() > 0)
-            requiredHookName = hookObj.getFieldVL(sfHookName);
-
-        if (requiredHookName)
-        {
-            // need to specify same hook name in the transaction
-            if (!tx.isFieldPresent(sfHookName))
-                continue;
-            if (*requiredHookName != tx.getFieldVL(sfHookName))
-                continue;
-        }
+            hookName = hookObj.getFieldVL(sfHookName);
 
         uint32_t flags = 0;
         if (hookObj.isFieldPresent(sfFlags))
@@ -307,7 +302,7 @@ Transactor::calculateHookChainFee(
         uint256 hookOn = hook::getHookOn(
             hookObj, hookDef, isOutgoing ? sfHookOnOutgoing : sfHookOnIncoming);
 
-        if (hook::canHook(tx.getTxnType(), hookOn) &&
+        if (hook::canHook(tx, hookOn, hookName) &&
             (!collectCallsOnly || (flags & hook::hsfCOLLECT)))
         {
             XRPAmount const toAdd{hookDef->getFieldAmount(sfFee).xrp().drops()};
@@ -1354,25 +1349,16 @@ Transactor::executeHookChain(
             // LCOV_EXCL_STOP
         }
 
-        std::optional<Blob> requiredHookName;
+        std::optional<Blob> hookName;
         if (hookObj.isFieldPresent(sfHookName) &&
             hookObj.getFieldVL(sfHookName).size() > 0)
-            requiredHookName = hookObj.getFieldVL(sfHookName);
-
-        if (requiredHookName)
-        {
-            // need to specify same hook name in the transaction
-            if (!ctx_.tx.isFieldPresent(sfHookName))
-                continue;
-            if (*requiredHookName != ctx_.tx.getFieldVL(sfHookName))
-                continue;
-        }
+            hookName = hookObj.getFieldVL(sfHookName);
 
         // check if the hook can fire
         uint256 hookOn = hook::getHookOn(
             hookObj, hookDef, isOutgoing ? sfHookOnOutgoing : sfHookOnIncoming);
 
-        if (!hook::canHook(ctx_.tx.getTxnType(), hookOn))
+        if (!hook::canHook(ctx_.tx, hookOn, hookName))
             continue;  // skip if it can't
 
         uint256 hookCanEmit = hook::getHookCanEmit(hookObj, hookDef);