From 84f9158f3abf12f89f561bd3a7f1910e34884e02 Mon Sep 17 00:00:00 2001 From: Lasse Nielsen Date: Mon, 20 Apr 2026 09:33:36 +0200 Subject: [PATCH 1/2] fix: guard loot and money chat listeners against secret string taint (#176) Retail CHAT_MSG_LOOT and CHAT_MSG_MONEY payloads occasionally arrive as Blizzard "secret" (censored) strings. Any index operation on them (including msg:match) raises: attempt to index local 'msg' (a secret string value tainted by 'DragonToast') Add three layers of defense to OnChatMsgLoot and OnChatMsgMoney in LootListener_Shared.lua: 1. Type guard: skip if msg is not a string. 2. Censor check: extract lineID from the event payload and skip when C_ChatInfo.IsChatLineCensored(lineID) is true. Nil-guarded so the check no-ops on TBC Anniversary and MoP Classic where the API does not exist. 3. pcall wrapper: parser errors surface through geterrorhandler() so they are visible to BugSack/BugGrabber/the default error handler without breaking the AceEvent dispatcher. The shared IsIndexableChatMessage helper encapsulates layers 1 and 2 for both handlers. C_ChatInfo and geterrorhandler added to .luacheckrc read_globals for DragonToast. Closes #176 Closes #177 --- .luacheckrc | 2 + DragonToast/Listeners/LootListener_Shared.lua | 55 +++++++++++++++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/.luacheckrc b/.luacheckrc index 256019d..1fe8086 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -51,6 +51,7 @@ files["DragonToast/"] = { read_globals = { -- WoW API + "C_ChatInfo", "IsInInstance", "UnitName", "UnitClass", "UnitFactionGroup", "GetItemInfo", "GetItemQualityColor", "GetItemCount", "C_Item", "C_Container", @@ -60,6 +61,7 @@ files["DragonToast/"] = { "ChatFrame_OpenChat", "IsShiftKeyDown", "InCombatLockdown", "hooksecurefunc", "InterfaceOptionsFrame_OpenToCategory", "Settings", + "geterrorhandler", -- WoW Globals "Enum", "RAID_CLASS_COLORS", "ITEM_QUALITY_COLORS", diff --git a/DragonToast/Listeners/LootListener_Shared.lua b/DragonToast/Listeners/LootListener_Shared.lua index e03218a..f9a9c3b 100644 --- a/DragonToast/Listeners/LootListener_Shared.lua +++ b/DragonToast/Listeners/LootListener_Shared.lua @@ -16,11 +16,35 @@ local Utils = ns.ListenerUtils local GetItemInfo = GetItemInfo local GetTime = GetTime local UnitName = UnitName +local C_ChatInfo = C_ChatInfo local error = error +local geterrorhandler = geterrorhandler local ipairs = ipairs +local pcall = pcall +local select = select local tonumber = tonumber +local tostring = tostring local type = type +------------------------------------------------------------------------------- +-- Chat message sanity guard +-- +-- Retail occasionally emits CHAT_MSG_LOOT / CHAT_MSG_MONEY payloads as +-- Blizzard "secret" (censored) strings. Any index / match operation on them +-- raises a tainted-string error. Guard handlers with a string type check and +-- a C_ChatInfo.IsChatLineCensored probe before touching the message. The +-- C_ChatInfo namespace does not exist on TBC / MoP Classic, so nil-check it. +------------------------------------------------------------------------------- + +local function IsIndexableChatMessage(msg, lineID) + if type(msg) ~= "string" then return false end + if C_ChatInfo and C_ChatInfo.IsChatLineCensored and lineID + and C_ChatInfo.IsChatLineCensored(lineID) then + return false + end + return true +end + local PLAYER_UNIT = "player" local owner @@ -340,9 +364,21 @@ function ns.LootListenerShared.Create(config) local moneyPatterns = BuildMoneyPatterns(config.moneyPatterns or DEFAULT_MONEY_PATTERNS) local listener = {} - local function OnChatMsgLoot(_, msg) + local function OnChatMsgLoot(_, msg, ...) + -- CHAT_MSG_* payload position 11 is lineID; with (_, msg) consuming + -- event+text, lineID is the 10th element of the remaining varargs. + local lineID = select(10, ...) + -- Skip non-string or censored (tainted) payloads to avoid retail secret-string errors. + if not IsIndexableChatMessage(msg, lineID) then return end + local playerName = UnitName(PLAYER_UNIT) or UNKNOWN - local itemLink, quantity, looter, isSelf = ParseLootMessage(msg, lootCategories, playerName) + -- Parse under pcall; a tainted string can still slip through if Blizzard changes the + -- censoring contract, and a parser error must not break the event dispatcher. + local ok, itemLink, quantity, looter, isSelf = pcall(ParseLootMessage, msg, lootCategories, playerName) + if not ok then + geterrorhandler()("DragonToast: ParseLootMessage failed: " .. tostring(itemLink)) + return + end if not itemLink then return end Utils.WaitForItem( @@ -353,12 +389,23 @@ function ns.LootListenerShared.Create(config) ) end - local function OnChatMsgMoney(_, msg) + local function OnChatMsgMoney(_, msg, ...) + -- CHAT_MSG_* payload position 11 is lineID; with (_, msg) consuming + -- event+text, lineID is the 10th element of the remaining varargs. + local lineID = select(10, ...) + -- Skip non-string or censored (tainted) payloads to avoid retail secret-string errors. + if not IsIndexableChatMessage(msg, lineID) then return end + local db = owner.db.profile if not db.enabled or not db.filters.showGold then return end local playerName = UnitName(PLAYER_UNIT) or UNKNOWN - local amount, looter, isSelf = ParseMoneyMessage(msg, moneyPatterns, playerName) + -- Defensive pcall: same tainted-string safety net as the loot handler. + local ok, amount, looter, isSelf = pcall(ParseMoneyMessage, msg, moneyPatterns, playerName) + if not ok then + geterrorhandler()("DragonToast: ParseMoneyMessage failed: " .. tostring(amount)) + return + end if not amount then return end QueueMoneyToast(amount, looter, isSelf) From aa8deac1491c6b07249f18ca457e93810201d2ac Mon Sep 17 00:00:00 2001 From: Lasse Nielsen Date: Mon, 20 Apr 2026 14:03:21 +0200 Subject: [PATCH 2/2] refactor: hoist IsIndexableChatMessage into ns.ListenerUtils Per CodeRabbit review on #178. The secret-string / censored-chat guard is not loot-specific - any future CHAT_MSG_* listener that parses the message text faces the same retail taint risk. Move the helper to the shared ns.ListenerUtils table so text-parsing listeners can adopt it when they need it, without copy/paste drift. No behavioral change: logic is byte-identical to the prior file-local. Only the two existing call sites in LootListener_Shared.lua are updated; other listeners are unaffected. --- DragonToast/Core/ListenerUtils.lua | 24 +++++++++++++++++++ DragonToast/Listeners/LootListener_Shared.lua | 22 +++-------------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/DragonToast/Core/ListenerUtils.lua b/DragonToast/Core/ListenerUtils.lua index 0e3331a..0dd90ee 100644 --- a/DragonToast/Core/ListenerUtils.lua +++ b/DragonToast/Core/ListenerUtils.lua @@ -11,6 +11,7 @@ local Utils = ns.ListenerUtils -- Cache Lua globals local tostring = tostring local tonumber = tonumber +local type = type local math_floor = math.floor local string_format = string.format local table_concat = table.concat @@ -18,6 +19,9 @@ local table_insert = table.insert local ipairs = ipairs local next = next +-- Cache WoW API +local C_ChatInfo = C_ChatInfo + -- Pending item lookups: itemID (number) -> { buildFunc, filterFunc } -- Multiple entries can share the same itemID (e.g. two loots of the same item in quick succession) -- Use an array of entries per itemID to handle that correctly. @@ -158,6 +162,26 @@ function Utils.FormatGold(copper) return string_format("|T%d:0:0:0:0|t%s", Utils.GOLD_ICON, table_concat(parts, " ")) end +------------------------------------------------------------------------------- +-- IsIndexableChatMessage(msg, lineID) +-- +-- Retail occasionally emits CHAT_MSG_* payloads as Blizzard "secret" +-- (censored) strings. Any index / match operation on them raises a +-- tainted-string error. Callers guard handlers with a string type check +-- and a C_ChatInfo.IsChatLineCensored probe before touching the message. +-- The C_ChatInfo namespace does not exist on TBC / MoP Classic, so it is +-- nil-checked here. +------------------------------------------------------------------------------- + +function Utils.IsIndexableChatMessage(msg, lineID) + if type(msg) ~= "string" then return false end + if C_ChatInfo and C_ChatInfo.IsChatLineCensored and lineID + and C_ChatInfo.IsChatLineCensored(lineID) then + return false + end + return true +end + ------------------------------------------------------------------------------- -- RetryWithTimer(addon, buildFunc, filterFunc, retries) -- Retries a build function up to MAX_RETRIES times at RETRY_INTERVAL seconds. diff --git a/DragonToast/Listeners/LootListener_Shared.lua b/DragonToast/Listeners/LootListener_Shared.lua index f9a9c3b..066ffd4 100644 --- a/DragonToast/Listeners/LootListener_Shared.lua +++ b/DragonToast/Listeners/LootListener_Shared.lua @@ -16,7 +16,6 @@ local Utils = ns.ListenerUtils local GetItemInfo = GetItemInfo local GetTime = GetTime local UnitName = UnitName -local C_ChatInfo = C_ChatInfo local error = error local geterrorhandler = geterrorhandler local ipairs = ipairs @@ -27,24 +26,9 @@ local tostring = tostring local type = type ------------------------------------------------------------------------------- --- Chat message sanity guard --- --- Retail occasionally emits CHAT_MSG_LOOT / CHAT_MSG_MONEY payloads as --- Blizzard "secret" (censored) strings. Any index / match operation on them --- raises a tainted-string error. Guard handlers with a string type check and --- a C_ChatInfo.IsChatLineCensored probe before touching the message. The --- C_ChatInfo namespace does not exist on TBC / MoP Classic, so nil-check it. +-- Chat message sanity guard - see Utils.IsIndexableChatMessage ------------------------------------------------------------------------------- -local function IsIndexableChatMessage(msg, lineID) - if type(msg) ~= "string" then return false end - if C_ChatInfo and C_ChatInfo.IsChatLineCensored and lineID - and C_ChatInfo.IsChatLineCensored(lineID) then - return false - end - return true -end - local PLAYER_UNIT = "player" local owner @@ -369,7 +353,7 @@ function ns.LootListenerShared.Create(config) -- event+text, lineID is the 10th element of the remaining varargs. local lineID = select(10, ...) -- Skip non-string or censored (tainted) payloads to avoid retail secret-string errors. - if not IsIndexableChatMessage(msg, lineID) then return end + if not Utils.IsIndexableChatMessage(msg, lineID) then return end local playerName = UnitName(PLAYER_UNIT) or UNKNOWN -- Parse under pcall; a tainted string can still slip through if Blizzard changes the @@ -394,7 +378,7 @@ function ns.LootListenerShared.Create(config) -- event+text, lineID is the 10th element of the remaining varargs. local lineID = select(10, ...) -- Skip non-string or censored (tainted) payloads to avoid retail secret-string errors. - if not IsIndexableChatMessage(msg, lineID) then return end + if not Utils.IsIndexableChatMessage(msg, lineID) then return end local db = owner.db.profile if not db.enabled or not db.filters.showGold then return end