Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9086 closed bug (fixed)

Autocomplete: Incorrect escaping in combobox demo

Reported by: Skaffen Owned by: Scott González
Priority: minor Milestone: 1.10.2
Component: ui.autocomplete Version: 1.10.0
Keywords: Cc:
Blocked by: Blocking:

Description

I know the combobox demo is "not a supported or even complete widget" and so bug reports on it may be ignored, but I thought it worth reporting anyway as people may be using that code.

It contains a regular expression to do highlighting of the matched substring:

	new RegExp(
		"(?![^&;]+;)(?!<[^<>]*)(" +
		$.ui.autocomplete.escapeRegex(request.term) +
		")(?![^<>]*>)(?![^&;]+;)", "gi"
	), "<strong>$1</strong>" ),

There were no comments in the code to explain what the stuff wrapped around the term are for but I think they're to avoid matching the request term within the middle of an html tag or html entity. (so it wouldn't e.g. try highlight "am" within "&amp;" or "hr" within "<hr>"). However the text its applied against has come from doing .text() on a select "<option>" so the source text is not html (and will only contain entities or html tags if the text literally contains those).

My guess is the regular expression for highlighting came from some code which had html source data rather than text.

The other problem is it's not escaping the source values nor is it escaping the matched string before wrapping it in <strong> so any ampersands, greater-thans or less-thans in the option text could cause problems.

For an example of the issue see where I've taken the combobox demo and added two troublesome options to the select option list: http://jsfiddle.net/4CpN9/1/

My recommendation would be to strip the bit of code doing the matched term highlighting from that example along with the monkey-patch of _renderItem to simplify the code and remove the bug. The alternative is to make the code more involved and properly handle escaping.

I've had a quick go at fixing things (not heavily tested) where I've modified the "source" function passed to autocomplete to return the label as a jquery object (building it up from parts) already as an "<a>" and changed the monkey-patched _renderItem to handle that correspondingly: http://jsfiddle.net/RYhpH/7/

Change History (5)

comment:1 Changed 7 years ago by tj.vantoll

Status: newopen
Summary: Bug in autocomplete's combobox demoAutocomplete: Incorrect escaping in combobox demo

We should at least add an inline comment there explaining what the RegExp is doing.

comment:2 Changed 7 years ago by Skaffen

Although adding an inline comment would help demystify the regexp the problem would be that what the regex is intended to deal with is not what it actually gets passed as it's given the results of calling .text() on the select option (i.e. it's not given an html/html escaped value).

comment:3 Changed 7 years ago by Scott González

Owner: set to scott.gonzalez
Status: openassigned

comment:4 Changed 7 years ago by Scott González

Resolution: fixed
Status: assignedclosed

Autocomplete demo (Combobox): Don't highlight term matches. Fixes #9086 - Autocomplete: Incorrect escaping in combobox demo.

Changeset: ba96cb47725dc6639ae366bd381f089c1750c8f5

comment:5 Changed 7 years ago by Scott González

Milestone: none1.10.2
Note: See TracTickets for help on using tickets.