Opened 7 years ago

Closed 4 years ago

#6016 closed bug (fixed)

Dialog: Title XSS Vulnerability

Reported by: shadowman131 Owned by:
Priority: major Milestone: 1.10.0
Component: ui.dialog Version: 1.8.4
Keywords: Cc:
Blocked by: Blocking:


jQuery.ui.dialog's title option is not properly escaped before being set in the dom, resulting in an XSS vulnerability for applications with dynamic titles. For instance, try:

$('<div>Hi</div>').dialog({title:'<script type="text/javascript">alert("XSS");</script>"});

If this is the desired behaviour, it should at least be noted somewhere. The best reason I could think of for this to be desired would be to use icons in <img> tags in the title. For this, maybe something like:


title: { html: '<img src="blah">' }


Would be more appropriate, as the developer indicates that they mean to use full HTML and are aware of the risks.

Change History (19)

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

Resolution: invalid
Status: newclosed

The fact that you can provide HTML doesn't create a vulnerability. This would be like saying that jQuery has an XSS vulnerability because it provides a .html() method.

comment:2 Changed 7 years ago by shadowman131

I strongly disagree - as I said in the bug, at least make it apparent in the documentation. To me, a dialog title is a text attribute - I'm sure others would agree. It doesn't make sense to not escape it by default, and not even mention the lack of escaping.

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

The documentation has been updated.

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

Milestone: TBD1.9
Resolution: invalid
Status: closedreopened

comment:5 Changed 6 years ago by Corey Frang

Even scarier is using the title attrib of the div itself:

comment:6 Changed 6 years ago by Xavi

I agree with shadowman13 (the OP) -- titles and labels should be html escaped by default. I feel most people assume that titles and label are purely textual attributes and as a result don't perceive them as possible XSS vulnerabilities.

For the the use case where someone wants to add an image to the title, we can require them to pass in a jquery object:

$("<div>Poppit</div>").dialog({ title: $("<img src='cheggit.png' />") });

Requiring a jquery object also improves error detection/reporting. Currently, if a user accidentally sets the title to a malformed piece of html (e.g. "<img src='cheggit.png'/"), jquery-ui will silently fail and leave the title blank. As opposed to passing in $("<img src='cheggit.png'/") which would immediately throw an exception.

IMO, this change would lead to less surprising code and better error handling.

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


comment:8 Changed 5 years ago by dalekocian

Owner: set to dalekocian
Status: reopenedassigned

comment:9 Changed 5 years ago by Jörn Zaefferer

If we escape the title by default, we need a way to allow the user to pass in HTML anyway. The suggestion here by Xavi is to accept a jQuery object. Do we have any examples of that elsewhere? Other ideas or opinions?

comment:10 Changed 5 years ago by mikesherov

Requiring a jQuery object everywhere a perceived XSS exists seems like a slippery slope to me. I'd rather just doc this, but I haven't really thought hard about this.

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

My plan was to just switch to text instead of HTML, like we did for autocomplete.

comment:12 Changed 5 years ago by mikesherov

Oh, if the plan is to NOT allow HTML, sure. If the plan is jQuery Obj = HTML vs. string = text, I'm against that as a generic strategy.

comment:13 Changed 5 years ago by Jörn Zaefferer

Owner: dalekocian deleted
Status: assignedopen

Can we offer a simpler override then the one necessary for autocomplete? Even if its not becoming an option, this isn't exactly trivial:

The idea about accepting a jQuery object doesn't sound so bad.

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

An extension for dialog would be simpler. Autocomplete is complicated because of data sources and plain text searching across nodes. Accepting a jQuery object for the title option seems extremely confusing to me. I understand the logic on a technical level, but from an API standpoint, it's unintuitive, confusing, and cumbersome.

comment:15 Changed 5 years ago by Jörn Zaefferer

Here's a potential patch against the dialog branch, extracting setting of the title into a _title method, making it pretty easy to override. The only drawback is that in order to set the current implicit default, I still have to use html:

Is that acceptable?

comment:16 Changed 5 years ago by mikesherov

I commented on the gist. I like the approach, it just appeared to me that there's an error. Perhaps I just don't know that code well enough yet.

comment:17 Changed 5 years ago by Jörn Zaefferer

The patch snippets were misleading, along with an incomplete refactoring. Also commented on the gist.

comment:18 Changed 4 years ago by Jörn Zaefferer

Summary: XSS Vulnerability - Dialog TitleDialog: Title XSS Vulnerability

comment:19 Changed 4 years ago by Jörn Zaefferer

Resolution: fixed
Status: openclosed

Dialog: Extract setting the title into a _title method, use .text() to prevent XSS. Fixes #6016 - Dialog: Title XSS Vulnerability.

Changeset: 7e9060c109b928769a664dbcc2c17bd21231b6f3

Note: See TracTickets for help on using tickets.