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..d95bc527 --- /dev/null +++ b/test/unit/issue-731-selector-html-injection.test.js @@ -0,0 +1,251 @@ +// 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(); + } +}); + +// 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 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({ + selector: '.issue-731-trigger', + events: { + show: function() { + shown++; + } + }, + items: {copy: {name: 'Copy'}} + }); + + $.contextMenu('destroy', {context: '.issue-731-trigger'}); + + $trigger.trigger($.Event('contextmenu')); + assert.equal(shown, 0, 'the registration was torn down through a string context'); +}); + +QUnit.test('destroy resolves a jQuery object context to the trigger element', function(assert) { + var $trigger = $('trigger').appendTo($('#qunit-fixture')); + + var shown = 0; + $.contextMenu({ + selector: '.issue-731-trigger', + events: { + show: function() { + shown++; + } + }, + items: {copy: {name: 'Copy'}} + }); + + $.contextMenu('destroy', {context: $trigger}); + + $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) { + 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'); +});