fix: never interpolate item.icon into markup - #814
Merged
Conversation
The Font Awesome icon branches built their <i> element by concatenating item.icon into a markup string, so an icon name containing a quote or an angle bracket closed the class attribute and injected arbitrary markup. Build the element and set the class instead, which is behaviourally identical for every legitimate icon name. Fixes #810
Documents that building the <i> with addClass produces exactly the same class list as the old markup concatenation for every legitimate icon value: multiple classes, trailing whitespace, repeated inner whitespace, and the non-string and falsy values that never reach the changed branches at all. These all pass against the pre-fix source too.
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.
Problem
The two Font Awesome branches built the icon element by concatenating
item.iconinto a markup string:An icon value containing
"or>closes the class attribute and injects arbitrary markup, which jQuery then parses and builds. That is a real concern for menus whose icon names come from stored or otherwise non-literal data. I confirmed it: anonerrorpayload smuggled throughitem.iconactually executes on current master.Fix
Build the element and set the class without parsing:
A hostile value now just lands as a set of nonsense class names on the
<i>, which renders nothing.I swept the rest of the item construction path for the same pattern. Every other
$('<...>')call insrc/is a literal tag with no interpolation, so these two were the only occurrences.Backwards compatibility
Both changed lines sit behind guards that narrow the input a long way before they are reached:
if (item.icon)excludes every falsy value, and each branch additionally requirestypeof item.icon === 'string'and a leadingfab/fas/fad/far/falorfa-. Sonull,undefined,'',0, and any non-string can never reach the changed code. They keep falling through to the untouched built-in icon-font branch exactly as before.For the values that can reach it,
addClassand concatenation produce the same class list:addClassdrops it. The browser's own class parsing discarded it anyway, so the resulting class list, every CSS rule, and everyhasClasscall are identicalNothing else moves: same
<i>tag, still prepended as the first child, samecontext-menu-iconandcontext-menu-icon--fa5hooks on the item. No rendered class name, DOM structure, or CSS hook changes, so anything users style against is untouched.item.iconstill accepts the same things it did, and the function and object forms are not touched at all. OnlyaddClass, which has been in jQuery since 1.0, is used, so jQuery 1.12+ support is intact (CI covers jQuery 1, 2, 3 and 4).I verified this empirically rather than by inspection: the whole "built exactly as before" test module passes against the pre-fix source as well as the fixed one. Only the three injection tests flip.
One intentional behaviour change, for the release notes: an
item.iconthat deliberately smuggles extra markup into the icon element no longer does so. That has been possible for years, so someone may be relying on it. It is the bug being fixed, so the fix is not contorted to preserve it, but it is the one thing that changes for existing callers. Anyone doing that should use the function form ofitem.icon, which can return an arbitrary element and is the supported way to do it.Tests
New
test/unit/issue-810-icon-html-injection.test.js, mirroring the structure of the #731 regression tests:fa-branches cannot break out of the class attribute<i>gets no child elementsThe three injection tests fail on master (the
onerrorhandler runs) and pass with the fix; everything else passes on both. Full unit suite: 95/95. Acceptance suite: 36/36, including the existingmenu-title-icon-alignmentspecs.Closes #810