fix: never interpolate item.icon into markup

CHANGELOG.md

@@ -16,6 +16,7 @@
 * `dataAttr` is now applied to the menu items themselves, using the correct attribute name (fixes #732, fixes #712)
 * Upgraded the embedded jQuery UI Position to 1.13.2 (CVE-2021-41184) (fixes #765)
 * Caller-supplied selector strings are no longer evaluated as HTML (fixes #731)
+* `item.icon` is no longer interpolated into markup on the Font Awesome paths (fixes #810)
 * A left-click trigger no longer leaks a synthetic `contextmenu` event to unrelated ancestor listeners (fixes #754)
 * `$.contextMenu('update')` no longer throws when a `build` menu has not been shown yet (fixes #740)
 * `autoHide` now works for a nested trigger registered with a different trigger mode (fixes #727)

src/jquery.contextMenu.js

@@ -1934,7 +1934,11 @@
                                 ) {
                                     // to enable font awesome
                                     $t.addClass(root.classNames.icon + ' ' + root.classNames.icon + '--fa5');
-                                    item._icon = $('<i class="' + item.icon + '"></i>');
+                                    // built with addClass instead of interpolating the icon
+                                    // into a markup string: an icon name coming from stored
+                                    // or otherwise non-literal data could otherwise close the
+                                    // class attribute and inject arbitrary markup.
+                                    item._icon = $('<i></i>').addClass(item.icon);
                                 } else if (typeof(item.icon) === 'string' && item.icon.substring(0, 3) === 'fa-') {
                                     // legacy Font Awesome 4 style icon class (e.g. "fa-trash"), kept for
                                     // backwards compatibility. Just like the fas/far/fab/fad/fal syntax
@@ -1944,7 +1948,7 @@
                                     // which used to bleed into (and bold) the item's label text and clash
                                     // with this plugin's own icon styling.
                                     $t.addClass(root.classNames.icon + ' ' + root.classNames.icon + '--fa5');
-                                    item._icon = $('<i class="fa ' + item.icon + '"></i>');
+                                    item._icon = $('<i></i>').addClass('fa').addClass(item.icon);
                                 } else {
                                     item._icon = root.classNames.icon + ' ' + root.classNames.icon + '-' + item.icon;
                                 }

test/unit/issue-810-icon-html-injection.test.js

@@ -0,0 +1,225 @@
+// Regression tests for https://github.com/swisnl/jQuery-contextMenu/issues/810
+//
+// The Font Awesome icon branches used to build their <i> element by string
+// concatenation, so an `item.icon` value containing a quote or an angle bracket
+// broke out of the class attribute and injected arbitrary markup. That matters
+// for any menu whose icon names come from stored or otherwise non-literal data.
+//
+// The payload below is an <img> with an invalid data URI, so the browser fires
+// its `error` handler without needing a network round trip. The handler sets a
+// flag, which is what the assertions check.
+var ICON_XSS_TAIL = ' "><img data-xss-810="1" src="data:image/png;base64,not-an-image" ' +
+    'onerror="window.__contextMenuXss810 = true;">';
+
+function xssRan810() {
+  return window.__contextMenuXss810 === true;
+}
+
+function imageCount810() {
+  return document.querySelectorAll('img').length;
+}
+
+function fixture810() {
+  var $fixture = $('#qunit-fixture');
+  if ($fixture.length === 0) {
+    $('<div id="qunit-fixture">').appendTo('body');
+    $fixture = $('#qunit-fixture');
+  }
+  return $fixture;
+}
+
+// Registering a menu builds its <ul> straight away, so the item markup can be
+// inspected without ever opening the menu.
+function buildMenuWithIcon(icon) {
+  fixture810().append('<div class="t810">right click me</div>');
+
+  $.contextMenu({
+    selector: '.t810',
+    items: {
+      first: {name: 'First', icon: icon}
+    }
+  });
+
+  return $('ul.context-menu-list').find('li.context-menu-item').first();
+}
+
+function assertIconNotParsedAsHtml(assert, imagesBefore, $item) {
+  var done = assert.async();
+
+  assert.equal(imageCount810(), imagesBefore, 'no <img> was added to the document');
+  assert.equal($item.find('img').length, 0, 'no <img> was injected into the menu item');
+
+  setTimeout(function() {
+    assert.notOk(xssRan810(), 'the onerror payload never ran');
+    done();
+  }, 250);
+}
+
+QUnit.module('issue 810 - item.icon is never parsed as HTML', {
+  beforeEach: function() {
+    window.__contextMenuXss810 = false;
+    this.imagesBefore = imageCount810();
+  },
+  afterEach: function() {
+    $.contextMenu('destroy');
+    try {
+      delete window.__contextMenuXss810;
+    } catch (e) {
+      window.__contextMenuXss810 = false;
+    }
+    $('img[data-xss-810]').remove();
+    var $fixture = $('#qunit-fixture');
+    if ($fixture.length) {
+      $fixture.html('');
+    }
+  }
+});
+
+QUnit.test('a Font Awesome 5 style icon cannot break out of the class attribute', function(assert) {
+  var $item = buildMenuWithIcon('fas fa-trash' + ICON_XSS_TAIL);
+
+  assertIconNotParsedAsHtml(assert, this.imagesBefore, $item);
+});
+
+QUnit.test('a legacy fa- style icon cannot break out of the class attribute', function(assert) {
+  var $item = buildMenuWithIcon('fa-trash' + ICON_XSS_TAIL);
+
+  assertIconNotParsedAsHtml(assert, this.imagesBefore, $item);
+});
+
+QUnit.test('the whole payload ends up as class names on the <i>, markup and all', function(assert) {
+  var $item = buildMenuWithIcon('fas fa-trash' + ICON_XSS_TAIL);
+  var $icon = $item.children('i');
+
+  assert.equal($icon.length, 1, 'exactly one <i> was created');
+  assert.equal($icon.children().length, 0, 'the <i> has no child elements');
+  assert.ok($icon.hasClass('fas'), 'the leading legitimate class survives');
+  assert.ok($icon.hasClass('fa-trash'), 'the icon class survives');
+});
+
+QUnit.module('issue 810 - supported icon inputs keep working', {
+  afterEach: function() {
+    $.contextMenu('destroy');
+    var $fixture = $('#qunit-fixture');
+    if ($fixture.length) {
+      $fixture.html('');
+    }
+  }
+});
+
+QUnit.test('a Font Awesome 5 style icon becomes an <i> with those classes', function(assert) {
+  var $item = buildMenuWithIcon('fas fa-trash');
+  var $icon = $item.children('i');
+
+  assert.equal($icon.length, 1, 'an <i> was prepended to the item');
+  assert.ok($icon.hasClass('fas'), 'the style class is applied');
+  assert.ok($icon.hasClass('fa-trash'), 'the icon class is applied');
+  assert.ok($item.hasClass('context-menu-icon'), 'the item is flagged as having an icon');
+  assert.ok($item.hasClass('context-menu-icon--fa5'), 'the item gets the Font Awesome modifier');
+});
+
+QUnit.test('a legacy fa- icon becomes an <i> with the fa base class', function(assert) {
+  var $item = buildMenuWithIcon('fa-trash');
+  var $icon = $item.children('i');
+
+  assert.equal($icon.length, 1, 'an <i> was prepended to the item');
+  assert.ok($icon.hasClass('fa'), 'the fa base class is applied');
+  assert.ok($icon.hasClass('fa-trash'), 'the icon class is applied');
+  assert.ok($item.hasClass('context-menu-icon--fa5'), 'the item gets the Font Awesome modifier');
+});
+
+QUnit.test('a built-in icon name stays a class on the item itself', function(assert) {
+  var $item = buildMenuWithIcon('copy');
+
+  assert.equal($item.children('i').length, 0, 'no <i> is created for the built-in icon font');
+  assert.ok($item.hasClass('context-menu-icon'), 'the base icon class is applied to the item');
+  assert.ok($item.hasClass('context-menu-icon-copy'), 'the icon name class is applied to the item');
+});
+
+QUnit.test('an icon function returning a class string is applied to the item', function(assert) {
+  var $item = buildMenuWithIcon(function() {
+    return 'my-icon-class';
+  });
+
+  assert.ok($item.hasClass('my-icon-class'), 'the returned class string is applied to the item');
+});
+
+QUnit.test('an icon function returning an element prepends that element', function(assert) {
+  var $item = buildMenuWithIcon(function() {
+    return $('<em class="custom-icon"></em>');
+  });
+
+  assert.equal($item.children('em.custom-icon').length, 1, 'the returned element was prepended');
+});
+
+// The two changed branches are only reachable for a string that already starts
+// with a known Font Awesome prefix, so a falsy, non-string or empty icon can
+// never get there. These cover the values that can, to make sure building the
+// <i> with addClass renders exactly what concatenating the markup used to.
+QUnit.module('issue 810 - the icon element is built exactly as before', {
+  afterEach: function() {
+    $.contextMenu('destroy');
+    var $fixture = $('#qunit-fixture');
+    if ($fixture.length) {
+      $fixture.html('');
+    }
+  }
+});
+
+function assertIconClasses(assert, icon, expected) {
+  var $icon = buildMenuWithIcon(icon).children('i');
+
+  assert.equal($icon.length, 1, icon + ': one <i> is created');
+  assert.equal($icon.children().length, 0, icon + ': the <i> has no child elements');
+
+  $.each(expected, function(i, className) {
+    assert.ok($icon.hasClass(className), icon + ': has class "' + className + '"');
+  });
+}
+
+QUnit.test('several space-separated classes are all applied', function(assert) {
+  assertIconClasses(assert, 'fas fa-trash fa-lg fa-fw', ['fas', 'fa-trash', 'fa-lg', 'fa-fw']);
+});
+
+QUnit.test('several space-separated classes are all applied on the legacy branch', function(assert) {
+  assertIconClasses(assert, 'fa-trash fa-lg', ['fa', 'fa-trash', 'fa-lg']);
+});
+
+// Concatenation used to put the trailing space straight into the attribute.
+// addClass drops it, which the browser's own class parsing did anyway, so the
+// resulting class list - and therefore every CSS rule and hasClass call - is
+// identical.
+QUnit.test('trailing whitespace does not change the resulting classes', function(assert) {
+  assertIconClasses(assert, 'fas fa-trash  ', ['fas', 'fa-trash']);
+});
+
+QUnit.test('repeated inner whitespace does not change the resulting classes', function(assert) {
+  assertIconClasses(assert, 'fas   fa-trash', ['fas', 'fa-trash']);
+});
+
+// The item-level CSS hooks are what themes style against, so they must be
+// untouched by the way the <i> is built.
+QUnit.test('the item keeps its icon CSS hooks and the <i> stays the first child', function(assert) {
+  var $item = buildMenuWithIcon('fas fa-trash');
+
+  assert.ok($item.hasClass('context-menu-icon'), 'context-menu-icon is still applied');
+  assert.ok($item.hasClass('context-menu-icon--fa5'), 'context-menu-icon--fa5 is still applied');
+  assert.equal($item.children().first().prop('tagName'), 'I', 'the <i> is still prepended as the first child');
+});
+
+// An icon that is neither a function nor a string never reaches the changed
+// branches; it keeps falling through to the built-in icon-font branch, which
+// stringifies it into a class name on the item itself.
+QUnit.test('a non-string, non-function icon still goes down the class-name branch', function(assert) {
+  var $item = buildMenuWithIcon(42);
+
+  assert.equal($item.children('i').length, 0, 'no <i> is created');
+  assert.ok($item.hasClass('context-menu-icon-42'), 'the value is stringified into the icon class name');
+});
+
+QUnit.test('a falsy icon is ignored entirely', function(assert) {
+  var $item = buildMenuWithIcon('');
+
+  assert.equal($item.children('i').length, 0, 'no <i> is created');
+  assert.notOk($item.hasClass('context-menu-icon'), 'no icon class is applied');
+});