From 4f8c7298069b422addd09e3ca5e13e465ac3ee43 Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:27:48 +0200 Subject: [PATCH 1/2] fix: never evaluate caller-supplied selector strings as HTML jQuery builds a detached DOM fragment from any string that looks like markup instead of running it as a CSS selector, so a value such as "" passed where a selector is expected ends up executing script rather than matching nothing. Three options took that path: `context` (normalised once for every operation, so it affected create, update and destroy alike), `appendTo`, and the element handed to `$.contextMenu.fromMenu()`. All three are documented as a selector string or a DOM element, never as markup. They now go through a small `resolveSelector()` helper that routes strings through `$(document).find()`, which only ever accepts a selector. Elements, jQuery objects and `document` are passed to `$()` unchanged, so `context` as an Element/jQuery object and the Element and jQuery object `selector` support added in #796 keep working. --- src/jquery.contextMenu.js | 23 ++- .../issue-731-selector-html-injection.test.js | 184 ++++++++++++++++++ 2 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 test/unit/issue-731-selector-html-injection.test.js diff --git a/src/jquery.contextMenu.js b/src/jquery.contextMenu.js index 03d0b7b0..f5e428bd 100644 --- a/src/jquery.contextMenu.js +++ b/src/jquery.contextMenu.js @@ -1772,7 +1772,7 @@ if (!opt.$node) { opt.$menu.css('display', 'none').addClass('context-menu-root'); } - opt.$menu.appendTo(opt.appendTo || document.body); + opt.$menu.appendTo(resolveSelector(opt.appendTo || document.body)); }, resize: function ($menu, nested) { var domMenu; @@ -2265,6 +2265,23 @@ (selector.nodeType === 1 || (typeof selector.jquery !== 'undefined' && typeof selector.length === 'number')); } + // resolve a caller-supplied "selector-ish" option (`context`, `appendTo`, + // the element passed to `fromMenu()`, ...) to a jQuery object without ever + // letting a string be evaluated as HTML. `$(string)` builds a detached DOM + // fragment whenever the string looks like markup instead of running it as a + // CSS selector, which turns something like `` into + // executing code. `.find()` only ever accepts a selector, so strings are + // routed through it. Elements, jQuery objects, `document` and everything + // else are handed to `$()` unchanged. + // See https://github.com/swisnl/jQuery-contextMenu/issues/731 + function resolveSelector(target) { + if (typeof target === 'string') { + return $(document).find(target); + } + + return $(target); + } + // remove every `elementSelectors` entry (and its bound handler) registered // under the given namespace. Used to tear down direct element/jQuery-object // bindings from any destroy code path, regardless of whether the menu was @@ -2364,7 +2381,7 @@ o.context = document; } else { // you never know what they throw at you... - $context = $(o.context).first(); + $context = resolveSelector(o.context).first(); o.context = $context.get(0); _hasContext = !$(o.context).is(document); } @@ -2883,7 +2900,7 @@ // convert html5 menu $.contextMenu.fromMenu = function (element) { - var $this = $(element), + var $this = resolveSelector(element), items = {}; menuChildren(items, $this.children()); diff --git a/test/unit/issue-731-selector-html-injection.test.js b/test/unit/issue-731-selector-html-injection.test.js new file mode 100644 index 00000000..1064ea1b --- /dev/null +++ b/test/unit/issue-731-selector-html-injection.test.js @@ -0,0 +1,184 @@ +// Regression tests for https://github.com/swisnl/jQuery-contextMenu/issues/731 +// +// jQuery evaluates a string that looks like markup as HTML to build instead of +// running it as a CSS selector, so any caller-supplied string that is meant to +// be a selector must never reach `$(...)` directly. +// +// 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. Feeding the payload to a hardened +// call site is expected to either do nothing or make jQuery throw its usual +// "unrecognized expression" selector error - both are fine, as long as no +// element is built and no script runs. +var XSS_PAYLOAD = ''; + +function xssRan() { + return window.__contextMenuXss731 === true; +} + +function imageCount() { + return document.querySelectorAll('img').length; +} + +function assertNotEvaluatedAsHtml(assert, imagesBefore, fn) { + var done = assert.async(); + + try { + fn(); + } catch (e) { + assert.ok( + /unrecognized expression|Syntax error/i.test(e.message || ''), + 'the payload was rejected as an invalid selector, not built as HTML' + ); + } + + assert.equal(imageCount(), imagesBefore, 'no was added to the document'); + + setTimeout(function() { + assert.notOk(xssRan(), 'the onerror payload never ran'); + done(); + }, 250); +} + +QUnit.module('issue 731 - selector strings are never parsed as HTML', { + beforeEach: function() { + window.__contextMenuXss731 = false; + this.imagesBefore = imageCount(); + }, + afterEach: function() { + $.contextMenu('destroy'); + try { + delete window.__contextMenuXss731; + } catch (e) { + window.__contextMenuXss731 = false; + } + $('img[data-xss-731]').remove(); + var $fixture = $('#qunit-fixture'); + if ($fixture.length) { + $fixture.html(''); + } + } +}); + +QUnit.test('$.contextMenu("destroy", {context: html}) does not evaluate the string as HTML', function(assert) { + assertNotEvaluatedAsHtml(assert, this.imagesBefore, function() { + $.contextMenu('destroy', {context: XSS_PAYLOAD}); + }); +}); + +QUnit.test('$.contextMenu("update", {context: html}) does not evaluate the string as HTML', function(assert) { + assertNotEvaluatedAsHtml(assert, this.imagesBefore, function() { + $.contextMenu('update', {context: XSS_PAYLOAD}); + }); +}); + +QUnit.test('$.contextMenu("create", {context: html}) does not evaluate the string as HTML', function(assert) { + assertNotEvaluatedAsHtml(assert, this.imagesBefore, function() { + $.contextMenu({ + selector: '.issue-731-trigger', + context: XSS_PAYLOAD, + items: {copy: {name: 'Copy'}} + }); + }); +}); + +QUnit.test('appendTo does not evaluate the string as HTML', function(assert) { + assertNotEvaluatedAsHtml(assert, this.imagesBefore, function() { + $.contextMenu({ + selector: '.issue-731-trigger', + appendTo: XSS_PAYLOAD, + items: {copy: {name: 'Copy'}} + }); + }); +}); + +QUnit.test('$.contextMenu.fromMenu does not evaluate the string as HTML', function(assert) { + assertNotEvaluatedAsHtml(assert, this.imagesBefore, function() { + $.contextMenu.fromMenu(XSS_PAYLOAD); + }); +}); + +QUnit.module('issue 731 - supported selector inputs keep working', { + afterEach: function() { + $.contextMenu('destroy'); + var $fixture = $('#qunit-fixture'); + if ($fixture.length) { + $fixture.html(''); + } + $('#issue-731-container').remove(); + } +}); + +QUnit.test('context may be a selector string', function(assert) { + var $fixture = $('#qunit-fixture'); + var $container = $('
').appendTo($fixture); + $('trigger').appendTo($container); + + var shown = 0; + $.contextMenu({ + context: '#issue-731-container', + selector: '.issue-731-trigger', + events: { + show: function() { + shown++; + } + }, + items: {copy: {name: 'Copy'}} + }); + + $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); + assert.equal(shown, 1, 'menu opened for a string context'); + + $.contextMenu('destroy', {context: '#issue-731-container'}); + $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); + assert.equal(shown, 1, 'menu was destroyed through a string context'); +}); + +QUnit.test('context may be an Element or a jQuery object', function(assert) { + var $fixture = $('#qunit-fixture'); + var $container = $('
').appendTo($fixture); + $('trigger').appendTo($container); + + var shown = 0; + $.contextMenu({ + context: $container.get(0), + selector: '.issue-731-trigger', + events: { + show: function() { + shown++; + } + }, + items: {copy: {name: 'Copy'}} + }); + + $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); + assert.equal(shown, 1, 'menu opened for an Element context'); + + $.contextMenu('destroy', {context: $container}); + $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); + assert.equal(shown, 1, 'menu was destroyed through a jQuery object context'); +}); + +QUnit.test('appendTo may be a selector string or an Element', function(assert) { + var $fixture = $('#qunit-fixture'); + var $container = $('
').appendTo($fixture); + + $.contextMenu({ + selector: '.issue-731-trigger', + appendTo: '#issue-731-container', + items: {copy: {name: 'Copy'}} + }); + + assert.equal($container.children('ul.context-menu-list').length, 1, 'menu appended to the string selector target'); + + $.contextMenu('destroy'); + + $.contextMenu({ + selector: '.issue-731-trigger', + appendTo: $container.get(0), + items: {copy: {name: 'Copy'}} + }); + + assert.equal($container.children('ul.context-menu-list').length, 1, 'menu appended to the Element target'); +}); From ebba1e5a3a46df13bbc1e28eb12a30be0a2175a7 Mon Sep 17 00:00:00 2001 From: bjorn Date: Wed, 29 Jul 2026 16:43:20 +0200 Subject: [PATCH 2/2] test: make the context regression assertions actually verify the behaviour Codex pointed out that the destroy assertions in the "supported selector inputs" tests could not fail. It was right about the cause, though not about the outcome: `destroy` treats `context` as the trigger element, so a container context matches no menu and tears nothing down. The assertions still passed because the menu was already open, and a second trigger on an open menu repositions instead of firing `show` again. The context tests now prove scoping instead: a trigger inside the context opens the menu, an identical trigger outside it does not. The destroy tests never open the menu first, so a destroy that silently did nothing is caught. While rewriting them, a raw Element `context` turned out not to scope anything either: an Element has no `length`, so `!o.context.length` sends it down the "no context" branch and it becomes `document`. That is pre-existing and left alone here, the test documents it instead. --- .../issue-731-selector-html-injection.test.js | 105 ++++++++++++++---- 1 file changed, 86 insertions(+), 19 deletions(-) diff --git a/test/unit/issue-731-selector-html-injection.test.js b/test/unit/issue-731-selector-html-injection.test.js index 1064ea1b..d95bc527 100644 --- a/test/unit/issue-731-selector-html-injection.test.js +++ b/test/unit/issue-731-selector-html-injection.test.js @@ -110,14 +110,88 @@ QUnit.module('issue 731 - supported selector inputs keep working', { } }); -QUnit.test('context may be a selector string', function(assert) { +// Build a container with one trigger inside it and an identical trigger +// outside it. A `context` that was resolved as a selector scopes the +// registration to the container, so only the inner trigger opens a menu. A +// `context` that got parsed as HTML instead resolves to a detached node, and +// then neither trigger would. +function setupScopedFixture() { var $fixture = $('#qunit-fixture'); - var $container = $('
').appendTo($fixture); - $('trigger').appendTo($container); + var fixture = { + $container: $('
').appendTo($fixture) + }; + + fixture.$inside = $('inside').appendTo(fixture.$container); + fixture.$outside = $('outside').appendTo($fixture); + + return fixture; +} + +function registerScopedMenu(context, counter) { + $.contextMenu({ + context: context, + selector: '.issue-731-trigger', + events: { + show: function() { + counter.shown++; + } + }, + items: {copy: {name: 'Copy'}} + }); +} + +QUnit.test('context may be a selector string', function(assert) { + var fixture = setupScopedFixture(); + var counter = {shown: 0}; + + registerScopedMenu('#issue-731-container', counter); + + fixture.$outside.trigger($.Event('contextmenu')); + assert.equal(counter.shown, 0, 'a trigger outside the string context is not handled'); + + fixture.$inside.trigger($.Event('contextmenu')); + assert.equal(counter.shown, 1, 'a trigger inside the string context opens the menu'); +}); + +// A raw Element has no `length`, so `!o.context.length` sends it down the "no +// context" branch and it silently becomes `document` - the registration is not +// scoped to the element at all. That is pre-existing behaviour, unrelated to +// this change; all that is asserted here is that an Element context still +// yields a working menu. +QUnit.test('context may be an Element', function(assert) { + var fixture = setupScopedFixture(); + var counter = {shown: 0}; + + registerScopedMenu(fixture.$container.get(0), counter); + + fixture.$inside.trigger($.Event('contextmenu')); + assert.equal(counter.shown, 1, 'a trigger inside the Element context opens the menu'); +}); + +QUnit.test('context may be a jQuery object', function(assert) { + var fixture = setupScopedFixture(); + var counter = {shown: 0}; + + registerScopedMenu(fixture.$container, counter); + + fixture.$outside.trigger($.Event('contextmenu')); + assert.equal(counter.shown, 0, 'a trigger outside the jQuery object context is not handled'); + + fixture.$inside.trigger($.Event('contextmenu')); + assert.equal(counter.shown, 1, 'a trigger inside the jQuery object context opens the menu'); +}); + +// `destroy` treats `context` as the trigger element - that is what +// `$.fn.contextMenu('destroy')` passes - so a context only tears anything down +// when it resolves to an element matching the menu's own selector. The menu is +// deliberately never opened first: an already open menu is repositioned rather +// than re-shown on a second trigger, which would mask a destroy that silently +// did nothing. +QUnit.test('destroy resolves a string context to the trigger element', function(assert) { + var $trigger = $('trigger').appendTo($('#qunit-fixture')); var shown = 0; $.contextMenu({ - context: '#issue-731-container', selector: '.issue-731-trigger', events: { show: function() { @@ -127,22 +201,17 @@ QUnit.test('context may be a selector string', function(assert) { items: {copy: {name: 'Copy'}} }); - $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); - assert.equal(shown, 1, 'menu opened for a string context'); + $.contextMenu('destroy', {context: '.issue-731-trigger'}); - $.contextMenu('destroy', {context: '#issue-731-container'}); - $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); - assert.equal(shown, 1, 'menu was destroyed through a string context'); + $trigger.trigger($.Event('contextmenu')); + assert.equal(shown, 0, 'the registration was torn down through a string context'); }); -QUnit.test('context may be an Element or a jQuery object', function(assert) { - var $fixture = $('#qunit-fixture'); - var $container = $('
').appendTo($fixture); - $('trigger').appendTo($container); +QUnit.test('destroy resolves a jQuery object context to the trigger element', function(assert) { + var $trigger = $('trigger').appendTo($('#qunit-fixture')); var shown = 0; $.contextMenu({ - context: $container.get(0), selector: '.issue-731-trigger', events: { show: function() { @@ -152,12 +221,10 @@ QUnit.test('context may be an Element or a jQuery object', function(assert) { items: {copy: {name: 'Copy'}} }); - $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); - assert.equal(shown, 1, 'menu opened for an Element context'); + $.contextMenu('destroy', {context: $trigger}); - $.contextMenu('destroy', {context: $container}); - $container.find('.issue-731-trigger').trigger($.Event('contextmenu')); - assert.equal(shown, 1, 'menu was destroyed through a jQuery object context'); + $trigger.trigger($.Event('contextmenu')); + assert.equal(shown, 0, 'the registration was torn down through a jQuery object context'); }); QUnit.test('appendTo may be a selector string or an Element', function(assert) {