Autocomplete: fix XSS in JSONP demo

[wenz]

fixes #15048

[scottgonzalez]

This isset() is duplicative of the empty() check.

[wenz]

agreed. Fixed.

[wenz]

removed the isset() check. Thanks for the pointer!

[scottgonzalez]

This is too restrictive; it would reject many valid callback function names. Properly validating the callback name is too complex and there's no actual attack vector to worry about here.

[wenz]

I respectfully disagree.

1. in the context of the demo, the filtering works fine, since it alows jQuery's default callback name consisting of jQuery, many digits, and one underscore. In addition, which valid callback function name is rejected by the regex? We might consider making $ and . valid characters, but what else is missing?
2. PoC: put the (unpatched) file on a web server and load http://server/path/to/search.php?callback=<script>alert(1)</script>&term=X in a browser without built-in XSS protection (e.g. Firefox). The JavaScript code is executed in the security context of the target server, a classic XSS vector.

[scottgonzalez]

In addition, which valid callback function name is rejected by the regex? We might consider making $ and . valid characters, but what else is missing?

Tons and tons of unicode characters are missing.

2. PoC: put the (unpatched) file on a web server and load http://server/path/to/search.php?callback=&term=X in a browser without built-in XSS protection (e.g. Firefox). The JavaScript code is executed in the security context of the target server, a classic XSS vector.

The JavaScript code being an array, which does nothing. How is that a classic XSS vector?

[wenz]

GitHub ate the script tag in my URL. Try this:
http://server/path/to/search.php?callback=[script]alert(1)[/script]&term=X
but replace the square brackets with angle brackets.

[scottgonzalez]

So this is about linking directly to search.php and injecting HTML, not about injecting JS in the JSONP request. Simply passing the callback through htmlspecialchars() would prevent someone from pretending to load a real page and won't break any valid callback names.

[wenz]

htmlspecialchars() would also work, however I personally prefer the validation approach over escaping here, since a callback name with (for instance) angle brackets should lead to nothing being returned.

The proposed fix works for the sample code. If someone is changing the example, e.g. by using a callback name with unicode characters, it's probably their responsibility to adapt the PHP script, as well. If you wish, I could amend the PR to use htmlspecialchars() despite my reservations.

Anyway, this must be fixed asap, since the same file is also residing on the jqueryui.com domain.