fix: never evaluate caller-supplied selector strings as HTML

[bbrala]

Hardens selector handling so a caller-supplied string is never parsed as HTML.

Problem

jQuery builds a detached DOM fragment from any string that looks like markup, rather than running it as a CSS selector. A value like <img src=x onerror=...> passed where the plugin expects a selector therefore ends up building an element and running its inline handler instead of simply matching nothing.

Three options reached $() with a raw string:

• context, normalised once at the top of $.contextMenu(), so it applied equally to create, update and destroy (the destroy branch is the one reported in Potentail XSS vulnerability #731)
• appendTo, used in op.create()
• the element argument of $.contextMenu.fromMenu()

All three are documented as a selector string or a DOM element, never as markup.

Change

A small resolveSelector() helper routes strings through $(document).find(), which only ever treats its argument as a selector. Anything that is not a string (Element, jQuery object, document) is handed to $() unchanged, so the existing supported inputs keep working, including the Element and jQuery object selector support added in #796.

A markup-looking string now either matches nothing or makes jQuery raise its usual "unrecognized expression" selector error, depending on whether it happens to be valid selector syntax. Neither builds an element.

Tests

test/unit/issue-731-selector-html-injection.test.js covers each hardened call site with an <img> payload whose onerror sets a global flag, asserting that the flag stays unset and that no <img> is added to the document. It also covers the inputs that must keep working: context as a selector string, as an Element and as a jQuery object, and appendTo as a selector string and as an Element.

Verified failing before the change and passing after, on jQuery 1, 2, 3 and 4. Acceptance tests and eslint are unchanged.

Not included

• item.icon is interpolated into markup for the Font Awesome paths ($('<i class="' + item.icon + '"></i>')). That is a different mechanism and icon may well be considered trusted config, so it is left alone here. $('<i></i>').addClass('fa ' + item.icon) would be an equivalent, non-parsing alternative if you want it changed.
• inputLabel() builds label[for="' + node.id + '"]. The id comes from the DOM and the string never starts with <, so it is not parsed as HTML, but a quote in an id would still break the selector.
• $.contextMenu('update', {context: ...}) throws for unrelated, pre-existing reasons (op.update() receives the context itself, which has no $menu). Left as is; the test for that path only asserts the string is not evaluated.

Closes #731

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f8c729806

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fix the context-destroy expectations

When this test destroys with the container as context, the _hasContext branch only removes registrations when $(context).is(o.selector); this container does not match .issue-731-trigger, so the registration remains active. The next trigger increments shown to 2, making this assertion—and the analogous jQuery-object assertion below—deterministically fail in the unit suite.

Useful? React with 👍 / 👎.

[bbrala]

Thanks, addressed in ebba1e5.

Addressed: the destroy assertions in the "supported selector inputs" tests could not fail. The diagnosis was right, the predicted outcome was not: the tests passed on all nine CI jobs, because the menu was already open at that point and a second trigger on an open menu repositions rather than firing show again, so shown stayed at 1 whether or not destroy did anything.

The underlying point stands, so those tests were rewritten:

• the context tests now prove scoping. A trigger inside the context opens the menu, an identical trigger outside it does not. Both would fail if the string were resolved to a detached node.
• the destroy tests no longer open the menu first, and use a context that resolves to an element matching the menu's selector, which is the case destroy actually handles. Verified they fail when the context is pointed at a non-matching selector.

Not addressed, reported instead: while rewriting these, a raw Element passed as 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 silently becomes document. Only jQuery objects and selector strings scope a registration today. That is pre-existing and unrelated to this change, so it is left alone here rather than folded into a security fix. The test documents the current behaviour so a later fix has something to update.

[bbrala]

👍