From e6cc7b7179fe24209b5968de5c530cd11ee0467a Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 21:13:53 +0200 Subject: [PATCH 1/2] fix: never interpolate item.icon into markup The Font Awesome icon branches built their 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 --- CHANGELOG.md | 1 + src/jquery.contextMenu.js | 8 +- .../issue-810-icon-html-injection.test.js | 153 ++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 test/unit/issue-810-icon-html-injection.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 918b6628..18f02fb1 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 5f77c0d9..d5299138 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -1934,7 +1934,11 @@ ) { // to enable font awesome $t.addClass(root.classNames.icon + ' ' + root.classNames.icon + '--fa5'); - item._icon = $(''); + // 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 = $('').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 = $(''); + item._icon = $('').addClass('fa').addClass(item.icon); } else { item._icon = root.classNames.icon + ' ' + root.classNames.icon + '-' + item.icon; } diff --git a/test/unit/issue-810-icon-html-injection.test.js b/test/unit/issue-810-icon-html-injection.test.js new file mode 100644 index 00000000..a0b1059d --- /dev/null +++ b/test/unit/issue-810-icon-html-injection.test.js @@ -0,0 +1,153 @@ +// Regression tests for https://github.com/swisnl/jQuery-contextMenu/issues/810 +// +// The Font Awesome icon branches used to build their 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 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 = ' ">'; + +function xssRan810() { + return window.__contextMenuXss810 === true; +} + +function imageCount810() { + return document.querySelectorAll('img').length; +} + +function fixture810() { + var $fixture = $('#qunit-fixture'); + if ($fixture.length === 0) { + $('
').appendTo('body'); + $fixture = $('#qunit-fixture'); + } + return $fixture; +} + +// Registering a menu builds its
    straight away, so the item markup can be +// inspected without ever opening the menu. +function buildMenuWithIcon(icon) { + fixture810().append('
    right click me
    '); + + $.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 was added to the document'); + assert.equal($item.find('img').length, 0, 'no 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 , 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 was created'); + assert.equal($icon.children().length, 0, 'the 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 with those classes', function(assert) { + var $item = buildMenuWithIcon('fas fa-trash'); + var $icon = $item.children('i'); + + assert.equal($icon.length, 1, 'an 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 with the fa base class', function(assert) { + var $item = buildMenuWithIcon('fa-trash'); + var $icon = $item.children('i'); + + assert.equal($icon.length, 1, 'an 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 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 $(''); + }); + + assert.equal($item.children('em.custom-icon').length, 1, 'the returned element was prepended'); +}); From 120da76f145f476baddc19667aaca42e9ec2238a Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 21:22:13 +0200 Subject: [PATCH 2/2] test: pin the icon element's rendered classes against the old behaviour Documents that building the 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. --- .../issue-810-icon-html-injection.test.js | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/test/unit/issue-810-icon-html-injection.test.js b/test/unit/issue-810-icon-html-injection.test.js index a0b1059d..3def69ee 100644 --- a/test/unit/issue-810-icon-html-injection.test.js +++ b/test/unit/issue-810-icon-html-injection.test.js @@ -151,3 +151,75 @@ QUnit.test('an icon function returning an element prepends that element', functi 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 +// 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 is created'); + assert.equal($icon.children().length, 0, icon + ': the 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 is built. +QUnit.test('the item keeps its icon CSS hooks and the 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 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 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 is created'); + assert.notOk($item.hasClass('context-menu-icon'), 'no icon class is applied'); +});