fix: guard loot and money chat listeners against secret string taint (#176)#178
fix: guard loot and money chat listeners against secret string taint (#176)#178
Conversation
…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
📝 WalkthroughWalkthroughAdds defensive guards and error handling to chat loot/money handlers to avoid indexing tainted/secret chat strings, exposes a utility to validate indexable chat messages (including censorship checks), and updates luacheck config to recognize new WoW globals. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DragonToast/Listeners/LootListener_Shared.lua (1)
29-46: Consider hoisting this guard tons.ListenerUtilsfor cross-listener reuse. 🧰The secret-string taint issue isn't unique to loot/money chat — any
CHAT_MSG_*driven listener (e.g.XPListener,HonorListener, future additions) is exposed to the same Blizzard censoring contract. Sincens.ListenerUtilsis the sanctioned shared sub-table per project conventions, exposing this helper there would:
- keep the defense consistent across all chat-backed listeners,
- avoid copy/paste drift when Blizzard adds a new
IsChatLineCensored-adjacent API,- remove the need for each listener file to cache
C_ChatInfolocally.Non-blocking — the current placement is perfectly functional. Just a "Dragon soul bind on the whole raid, not just the main tank" kind of suggestion.
♻️ Sketch
-- In ListenerUtils.lua +local C_ChatInfo = C_ChatInfo + +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 +endThen in
LootListener_Shared.lua, replace the local withUtils.IsIndexableChatMessage(msg, lineID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast/Listeners/LootListener_Shared.lua` around lines 29 - 46, Move the local IsIndexableChatMessage helper into the shared namespace table (ns.ListenerUtils) so other chat-driven listeners can reuse it: add a function ns.ListenerUtils.IsIndexableChatMessage(msg, lineID) with the exact same logic (type(msg) check and C_ChatInfo and IsChatLineCensored nil-check) and remove the local declaration from LootListener_Shared.lua; then replace the local call sites in LootListener_Shared.lua with ns.ListenerUtils.IsIndexableChatMessage(msg, lineID) (and update any other listeners to call ns.ListenerUtils.IsIndexableChatMessage as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DragonToast/Listeners/LootListener_Shared.lua`:
- Around line 29-46: Move the local IsIndexableChatMessage helper into the
shared namespace table (ns.ListenerUtils) so other chat-driven listeners can
reuse it: add a function ns.ListenerUtils.IsIndexableChatMessage(msg, lineID)
with the exact same logic (type(msg) check and C_ChatInfo and IsChatLineCensored
nil-check) and remove the local declaration from LootListener_Shared.lua; then
replace the local call sites in LootListener_Shared.lua with
ns.ListenerUtils.IsIndexableChatMessage(msg, lineID) (and update any other
listeners to call ns.ListenerUtils.IsIndexableChatMessage as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 91e93c39-85e0-4746-99b4-a441352fad0c
📒 Files selected for processing (2)
.luacheckrcDragonToast/Listeners/LootListener_Shared.lua
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.
|
@coderabbitai good call - hoisted \ into \ in aa8deac. Kept scope limited to the loot/money handlers since those are the only confirmed callers of \ today; other \ listeners can adopt the shared helper when they're next touched. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai good call - hoisted |
|
Tip For best results, initiate chat on the files or code changes.
The old learning pointing to ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
1 participant
Description
Fixes the retail secret-string taint error in
LootListener_Shared.lua:250affecting both loot and money chat handlers.Closes #176
Closes #177
Root Cause
On retail, some
CHAT_MSG_LOOT/CHAT_MSG_MONEYpayloads arrive as Blizzard "secret" (censored) strings. Any index operation on them - includingmsg:match(patternPair.multi)- raisesattempt to index local 'msg' (a secret string value tainted by 'DragonToast'). The handler passedmsgstraight into the parsers with no guard.Fix
Three layers of defense applied to
OnChatMsgLootandOnChatMsgMoney:type(msg) ~= "string"short-circuits non-string payloads.lineIDfrom the event payload and skip whenC_ChatInfo.IsChatLineCensored(lineID)is true. Nil-guarded so it no-ops on TBC Anniversary and MoP Classic where the API does not exist.geterrorhandler()so they show up in BugSack / the default error handler, instead of being swallowed or taking down the event dispatcher.Layers 1 and 2 are encapsulated in a shared
IsIndexableChatMessage(msg, lineID)helper..luacheckrcgainsC_ChatInfoandgeterrorhandlerin the DragonToastread_globalsblock.Type of Change
Testing
luacheck .)Checklist
Summary by CodeRabbit
Bug Fixes
Chores