Core/Chat: Fix hyperlink validation for inspected item links, for real this time.

4 files changed, 26 insertions, 6 deletions

diff --git a/src/server/game/Chat/HyperlinkTags.cpp b/src/server/game/Chat/HyperlinkTags.cpp
index b7ee7c3e905..acc9139f9f7 100644
--- a/src/server/game/Chat/HyperlinkTags.cpp
+++ b/src/server/game/Chat/HyperlinkTags.cpp
@@ -20,6 +20,7 @@
 #include "ObjectMgr.h"
 #include "SpellInfo.h"
 #include "SpellMgr.h"
+#include <limits>
 
 static constexpr char HYPERLINK_DATA_DELIMITER = ':';
 
@@ -113,17 +114,31 @@ bool Trinity::Hyperlinks::LinkTags::item::StoreTo(ItemLinkData& val, std::string
     if (!t.TryConsumeTo(itemId))
         return false;
     val.Item = sObjectMgr->GetItemTemplate(itemId);
-
-    int randomPropertyId;
+    val.IsBuggedInspectLink = false;
+
+    // randomPropertyId is actually a int16 in the client
+    // positive values index ItemRandomSuffix.dbc, while negative values index ItemRandomProperties.dbc
+    // however, there is also a client bug in inspect packet handling that causes a int16 to be cast to uint16, then int32 (dropping sign extension along the way)
+    // this results in the wrong value being sent in the link; DBC lookup clientside fails, so it sends the link without suffix
+    // to detect and allow these invalid links, we first read randomPropertyId as a full int32
+    int32 randomPropertyId;
     if (!(val.Item && t.TryConsumeTo(val.EnchantId) && t.TryConsumeTo(val.GemEnchantId[0]) && t.TryConsumeTo(val.GemEnchantId[1]) &&
-        t.TryConsumeTo(val.GemEnchantId[2]) && t.TryConsumeTo(dummy) && t.TryConsumeTo(randomPropertyId) && t.TryConsumeTo(val.PropertySeed) &&
+        t.TryConsumeTo(val.GemEnchantId[2]) && t.TryConsumeTo(dummy) && t.TryConsumeTo(randomPropertyId) && t.TryConsumeTo(val.RandomSuffixBaseAmount) &&
         t.TryConsumeTo(val.RenderLevel) && t.IsEmpty() && !dummy))
         return false;
 
+    if ((static_cast<int32>(std::numeric_limits<int16>::max()) < randomPropertyId) && (randomPropertyId <= std::numeric_limits<uint16>::max()))
+    { // this is the bug case, the id we received is actually static_cast<uint16>(i16RandomPropertyId)
+        randomPropertyId = static_cast<int16>(randomPropertyId);
+        val.IsBuggedInspectLink = true;
+    }
+
     if (randomPropertyId < 0)
     {
         if (!val.Item->RandomSuffix)
             return false;
+        if (randomPropertyId < -static_cast<int32>(sItemRandomSuffixStore.GetNumRows()))
+            return false;
         if (ItemRandomSuffixEntry const* suffixEntry = sItemRandomSuffixStore.LookupEntry(-randomPropertyId))
         {
             val.RandomSuffix = suffixEntry;
@@ -150,7 +165,7 @@ bool Trinity::Hyperlinks::LinkTags::item::StoreTo(ItemLinkData& val, std::string
         val.RandomProperty = nullptr;
     }
 
-    if ((val.RandomSuffix && !val.PropertySeed) || (val.PropertySeed && !val.RandomSuffix))
+    if ((val.RandomSuffix && !val.RandomSuffixBaseAmount) || (val.RandomSuffixBaseAmount && !val.RandomSuffix))
         return false;
 
     return true;
diff --git a/src/server/game/Chat/Hyperlinks.cpp b/src/server/game/Chat/Hyperlinks.cpp
index 3ed3c478077..9df1c21643c 100644
--- a/src/server/game/Chat/Hyperlinks.cpp
+++ b/src/server/game/Chat/Hyperlinks.cpp
@@ -138,6 +138,9 @@ struct LinkValidator<LinkTags::item>
         else if (data.RandomSuffix)
             randomSuffixes = &data.RandomSuffix->Name;
 
+        if (data.IsBuggedInspectLink) /* DBC lookup will have failed on the client, so the link should've arrived without suffix */
+            randomSuffixes = nullptr;
+
         for (uint8 i = 0; i < TOTAL_LOCALES; ++i)
         {
             if (!locale && i != DEFAULT_LOCALE)
diff --git a/src/server/game/Chat/Hyperlinks.h b/src/server/game/Chat/Hyperlinks.h
index 773576ce4ec..008aec24fde 100644
--- a/src/server/game/Chat/Hyperlinks.h
+++ b/src/server/game/Chat/Hyperlinks.h
@@ -63,8 +63,9 @@ namespace Trinity::Hyperlinks
         std::array<uint32, 3> GemEnchantId;
         ItemRandomPropertiesEntry const* RandomProperty;
         ItemRandomSuffixEntry const* RandomSuffix;
-        uint32 PropertySeed;
+        uint32 RandomSuffixBaseAmount; /* ITEM_FIELD_PROPERTY_SEED - only nonzero for RandomSuffix items, AllocationPct from DBC are multiplied with this, then floored, to get stat value */
         uint8 RenderLevel;
+        bool IsBuggedInspectLink;
     };
 
     struct QuestLinkData
diff --git a/tests/game/Hyperlinks.cpp b/tests/game/Hyperlinks.cpp
index 47031adbaba..80dc87574a5 100644
--- a/tests/game/Hyperlinks.cpp
+++ b/tests/game/Hyperlinks.cpp
@@ -88,7 +88,7 @@ TEST_CASE("|Hitem validation", "[Hyperlinks]")
 
     SECTION("Item link with random suffix")
     {
-        REQUIRE(true == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-45:43:80|h[Vrykul Shield of the Champion]|h|r"));
+        REQUIRE(true  == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-45:43:80|h[Vrykul Shield of the Champion]|h|r"));
         REQUIRE(false == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-45:43:80|h[Vrykul Shield]|h|r"));
         REQUIRE(false == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-1:43:80|h[Vrykul Shield]|h|r"));
         REQUIRE(false == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:45:43:80|h[Vrykul Shield of the Champion]|h|r"));
@@ -98,6 +98,7 @@ TEST_CASE("|Hitem validation", "[Hyperlinks]")
         REQUIRE(false == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-45:0:80|h[Escudo vrykul del Campe\xc3\xb3n]|h|r"));
         REQUIRE(false == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:-45:-1:80|h[Vrykul Shield of the Champion]|h|r"));
         REQUIRE(false == CheckAllLinks("|cffffffff|Hitem:6948:0:0:0:0:0:-45:43:80|h[Hearthstone of the Champion]|h|r"));
+        REQUIRE(true  == CheckAllLinks("|cff1eff00|Hitem:36449:0:0:0:0:0:65491:43:80|h[Vrykul Shield]|h|r"));
     }
 }