Guard aura and unit classification reads against Midnight secret values, add an option to hide the Self Cast Key warning - #507
Open
brconnell4 wants to merge 4 commits into
Conversation
Under 12.0.0 secret-value rules, three separate aura paths error out for tainted code once a unit has a secret aura: - UNIT_AURA's updateInfo.isFullUpdate is itself a secret in restricted contexts, so branching on it throws. addedAuras/removedAuraInstanceIDs are secret too, so there is nothing readable in the incremental path. Fall back to a full rescan when the flag can't be tested. - GetAuraSlots() does not return secrets, it refuses to run at all. That takes out ForEachAura in UnitButton and QuickAssist, AuraUtil.ForEachAura in QuickCast, and AuraUtil.FindAura behind F.FindAuraById. Run them protected and drop the pass instead of throwing; indicators come up empty until the restriction lifts. - F.FindAuraById's predicate compares against a secret spellId, which also throws, independent of the enumeration problem. F.FindDebuffByIds/F.FindAuraByDebuffTypes already checked IsAuraRestricted, but StatusIcon.lua notes that check is unreliable, so they get the same backstop. Repro: any restricted context (PvP instance, encounter) with Cell loaded. 99x Cell/RaidFrames/UnitButton.lua:1753: attempt to perform boolean test on local 'isFullUpdate' (a secret boolean value, while execution tainted by 'Cell') 48x GetAuraSlots(): Auras cannot be accessed when secret while tainted by 'Cell'
CheckConflicts() runs on every Click Castings OnShow, so anyone who deliberately keeps a SELFCAST modifier bound gets the popup each time they open the tab. Adds a "Hide Self Cast Key Warning" checkbox to General -> Misc that short-circuits the check. Defaults to off, so existing behaviour is unchanged. The new strings use the L identity fallback, matching how the other General tab strings are handled.
UnitIsCharmed returns a secret boolean for units under restricted control, so branching on it throws. Reproduces on a vehicle-seated party member: 3x Cell/RaidFrames/UnitButton.lua:2907: attempt to perform boolean test on a secret boolean value (execution tainted by 'Cell') Two call sites, UnitButton_UpdateHealthColor and UnitButton_UpdateNameTextColor. Adds F.SafeBool to flatten a possibly-secret boolean, defaulting a secret read to false; the charmed unit falls through to vehicle/class coloring rather than the purple charm color. The neighbouring UnitIsConnected/UnitIsPlayer/UnitInPartyIsAI reads are not secret, and states.inVehicle is Cell's own true/nil, so they are left alone.
UnitIsPlayer and UnitInPartyIsAI go secret in restricted contexts the same way UnitIsCharmed does, so the classification branches throw: 45x Cell/RaidFrames/UnitButton.lua:2866: attempt to perform boolean test on a secret boolean value (execution tainted by 'Cell') Guarding UnitIsCharmed alone wasn't enough, line 2866 tests all three. Rather than keep patching one call site per report, this covers every place the retail paths branch on them. F.IsPlayerUnit falls back to the GUID prefix when UnitIsPlayer is secret, which is readable in the contexts seen so far, so class coloring survives instead of degrading to NPC green. F.IsAIUnit flattens UnitInPartyIsAI to false. Both are no-ops off Midnight and when the reads aren't secret. F.SafeBool now takes an explicit default, used for UnitInPhase in StatusIcon so a secret read doesn't paint a phasing icon on the whole group. Also drops the NOTE in UnitButton_UpdateHealthColor claiming the coloring path reads only non-secret data, which is what these three reports disproved.
|
Hi @brconnell4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent changes, one commit each. Happy to split the second one into its own PR if you'd rather keep this to the bugfix.
1.
fix: guard aura reads against Midnight secret valuesOn 12.0.0, Cell throws in any restricted context (PvP instance, encounter) as soon as a unit has a secret aura. Two errors on a solo frame, in sequence:
Three distinct problems behind them:
updateInfo.isFullUpdateis itself secret.UnitButton_UpdateAurasbranches on it directly. The rest of the payload is secret too (addedAurasandremovedAuraInstanceIDsboth come through as secret tables), so there is nothing readable in the incremental path. Now falls back to a full rescan whenever the flag can't be tested. Same fix inQuickAssist_UpdateAuras.GetAuraSlots()doesn't return secrets, it refuses to run. This is the second error, and it's why fixingisFullUpdatealone isn't enough: the full-rescan path is exactly what's blocked. It affectsForEachAurain bothUnitButton.luaandQuickAssist.lua,AuraUtil.ForEachAurainQuickCast.lua, andAuraUtil.FindAurabehindF.FindAuraById. Each now runs protected and drops the pass rather than throwing. TheUnitButton/QuickAssisthelpers pass the varargs straight through, so there's no per-call table allocation on the hot path.F.FindAuraById's predicate doesidToFind == id, which throws on a secretspellIdindependent of the enumeration problem. Guarded with the existingF.IsValueNonSecret.F.FindDebuffByIds/F.FindAuraByDebuffTypesalready had anIsAuraRestricted()check, butIndicators/StatusIcon.lua:215notes that check is unreliable, so they get the same backstop behind it.Behaviour outside restricted contexts is unchanged. Inside them, aura indicators come up empty instead of erroring, which as far as I can tell is the ceiling for tainted code under the secret-aura rules rather than something more patching gets around. If there's a readable enumeration path I've missed I'd much rather use it than degrade.
2.
fix: guard UnitIsCharmed against secret booleansSame class of bug, found after the above.
UnitIsCharmedreturns a secret boolean for units under restricted control, so branching on it throws:Reproduced on a vehicle-seated party member. Two call sites,
UnitButton_UpdateHealthColorandUnitButton_UpdateNameTextColor. AddsF.SafeBoolalongside the existingF.IsValueNonSecretto flatten a possibly-secret boolean, defaulting a secret read to false, so a charmed unit falls through to vehicle/class coloring rather than the purple charm color.The neighbouring
UnitIsConnected/UnitIsPlayer/UnitInPartyIsAIreads in the same branch are not secret (they evaluate fine in the same frame that throws), andstates.inVehicleis Cell's owntrue/nil, so those are left alone.3.
fix: guard the unit classification reads against secret booleansUnitIsCharmedwas not alone.UnitIsPlayerandUnitInPartyIsAIgo secret the same way, and line 2866 tests all three, so it kept throwing after the previous commit:Rather than keep patching one call site per report, this covers every place the retail paths branch on them:
UnitButton_UpdateHealthColor,UnitButton_UpdateNameTextColor,UnitButton_UpdateStatusText,UnitButton_UpdateName, the vehicle-root and preview-class checks,F.UnitFullName,F.GetUnitClassColor,F.UnitInGroup,F.GetTargetUnitID,F.GetTargetPetID,SpotlightFrame,StatusIcon, andBuffTracker.Two helpers next to the existing
F.IsValueNonSecret:F.IsPlayerUnit(unit, guid)falls back to the GUID prefix whenUnitIsPlayeris secret. The GUID is readable in every context I've hit so far, so class coloring survives rather than degrading to NPC green. Most call sites already have astates.guidto hand it.F.IsAIUnit(unit)flattensUnitInPartyIsAIto false.F.SafeBoolnow takes an explicit default, used forUnitInPhaseinStatusIconso a secret read doesn't paint a phasing icon on the whole group.Both helpers are no-ops off Midnight and whenever the reads aren't actually secret, so nothing changes outside restricted contexts.
Also drops the
NOTEinUnitButton_UpdateHealthColorclaiming that path reads only non-secret data, which is what these reports disproved.4.
feat: add an option to hide the Self Cast Key conflict warningCheckConflicts()runs on every Click CastingsOnShow, so anyone who deliberately keeps a SELFCAST modifier bound gets the popup every single time they open the tab. Adds a "Hide Self Cast Key Warning" checkbox to General -> Misc that short-circuits it. Defaults to off, so nothing changes unless you tick it.New strings use the
Lidentity fallback, matching how the rest of the General tab strings are handled.Testing
Against r279-beta on a 120005 client. All touched files parse clean under
luac. Verified in game on the retail build that produced the tracebacks above: both errors gone, aura indicators still correct outside restricted content, and the Click Castings popup suppressed with the new option enabled.