Skip to content

Limit what tooltip content may render - #25250

Open
sgiehl wants to merge 4 commits into
6.x-devfrom
harden-tooltip-sanitizer
Open

Limit what tooltip content may render#25250
sgiehl wants to merge 4 commits into
6.x-devfrom
harden-tooltip-sanitizer

Conversation

@sgiehl

@sgiehl sgiehl commented Sep 8, 2026

Copy link
Copy Markdown
Member

Description

Tooltip content is read back from a title attribute, which the browser has already decoded, and jQuery UI then inserts it with .html() — so anything in a title that looks like markup is parsed as markup. That content went through vueSanitize, the general sanitizer, which is meant for rich snippets and keeps classes and data-* attributes. A tooltip has no use for either: a title is text with at most a line break and some emphasis.

This limits what tooltip content may render to a minimal set of inline elements with no attributes at all (b, br, em, i, small, span, strong, u). It is a hardening change: it caps what any value that reaches a tooltip can do, independently of how the value got into the title — tracked data, an admin-entered name, or a plugin — and it applies to every title in the product, including templates and plugins that have not been touched.

Two things come with it:

  • Nothing is lost from a title that was not written for a tooltip. If a title carries markup outside that set, the whole title is shown as text rather than the offending part being dropped, so a value that happens to contain a < still displays in full.
  • One transform instead of five. Five places turned a title into tooltip content with their own copy of the same two lines, and they did not agree — some sanitized, one escaped — so the same title could behave differently depending on which widget showed it. They now share CoreHome.tooltipContent, which is also reachable from plain JavaScript as window.vueSanitizeTooltip().

The sanitizers move into a module of their own so their profiles can be unit-tested, and the unused class attributes on the visits log tooltip spans are dropped, since attributes are what the profile does not keep. No CSS or JS referenced those classes.

A follow-up makes the escaping of the titles themselves consistent (a tooltipAttr Twig filter and the template migration), so display correctness no longer leans on the text fallback.

Verification

  • New unit tests over both sanitizer profiles: what is kept, what is shown as text, and a property test asserting that for a battery of unescaped titles no element outside the allow-list and no attribute of any kind survives.
  • Checked in a real browser (Chrome 150) as well as jsdom, including parser abuse shapes (<math><mtext><mglyph><style>, a <noscript> breakout, <svg></p><style>, <xmp>, <select><option><style>) and markup the parser hoists into the head: nothing rendered, nothing lost.
  • Every migrated tooltip was compared against its current behaviour input by input; the display only changes where a value's markup used to be rendered or silently dropped.
  • 2244 Vue/JS tests pass.

Checklist

  • [✔] I have understood, reviewed, and tested all AI outputs before use
  • [✔] All AI instructions respect security, IP, and privacy rules

Review

@sgiehl sgiehl added this to the 6.0.0 milestone Sep 8, 2026
@sgiehl
sgiehl force-pushed the harden-tooltip-sanitizer branch 4 times, most recently from 54f932f to 0c2ab41 Compare September 9, 2026 09:05
@sgiehl
sgiehl marked this pull request as ready for review September 9, 2026 09:28
@sgiehl
sgiehl requested a review from a team September 9, 2026 09:48
@tzi

tzi commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@sgiehl You said:

Nothing is lost from a title that was not written for a tooltip. If a title carries markup outside that set, the whole title is shown as text rather than the offending part being dropped, so a value that happens to contain a < still displays in full.

I’m not sure about this statement. My feedback is not really a blocker, just to be sure you are aware of the edge cases.

first case

carriesOtherMarkup() accepts an unterminated tag: the trailing >? in the tag regex lets <b at the end of a title pass as an allowed tag, so the title takes the DOMPurify path and the parser drops the fragment. A<B renders A and Rated<b renders Rated, breaking the docblock's "nothing is lost from it" promise. Fix, as the first statement of the while loop:

if (!tag[0].endsWith('>')) {
  return true;
}

second case

Two shapes evade the regex entirely and stay lossy even then: <? and </ followed by a non-letter start a bogus comment the parser discards, so x<?y>z renders xz and x</ b>y renders xy. Replacing the indexOf('<!') check with /<[!?]|<\/(?![a-zA-Z])/ fixed those too, with no change to well-formed titles. The security profile held in every case tried: no element outside the allow-list and no attribute survived either path.

Tooltip content now renders through a single transform that keeps only simple
inline formatting and no attributes, and shows anything else as text in full.
The sanitizers move into a module of their own so they can be unit tested.
A title shown as a tooltip is rendered as HTML, so escape the values
interpolated into one for that.
@sgiehl
sgiehl force-pushed the harden-tooltip-sanitizer branch from 0c2ab41 to ca1ee2d Compare September 9, 2026 19:26
Comment thread plugins/Contents/templates/_actionContent.twig Outdated
tzi
tzi previously approved these changes Sep 9, 2026
The visitor log's action tooltip is escaped once where the plugins'
entries are combined, so the carriers other plugins render are covered
too, and the report cell tooltip normalises its value before escaping it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants