Opened 10 years ago

Closed 10 years ago

#10128 closed defect (fixed)

REGRESSION: HTML injection vulnerability in dijit.Tree

Reported by: Nathan Toone Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.4.0b
Keywords: Cc: Kris Zyp
Blocked By: Blocking:

Description

To test, open dijit/tests/_data/countries.json and edit

name:'Africa'

to read

name:'Africa <font color="red">Hi</font>'

In 1.3, the HTML was properly escaped, however, in the 1.4 beta1, the HTML gets injected.

I'm classing this as a blocker, since it's a) a regression, and b) a security concern with many.

Attachments (1)

10128_treeHTMLInjectionFix.patch (1.4 KB) - added by Nathan Toone 10 years ago.
Patch which fixes this issue

Download all attachments as: .zip

Change History (8)

comment:1 Changed 10 years ago by Nathan Toone

This broke in revision [20247]

Changed 10 years ago by Nathan Toone

Patch which fixes this issue

comment:2 Changed 10 years ago by Nathan Toone

The above patch fixes this issue by allowing widgets to define "escapeHTML" on their attributeMap when innerHTML is the type. This may be an issue with other widgets, and in those cases, we'll need to add escapeHTML:true to their attribute maps as well.

comment:3 Changed 10 years ago by bill

Good catch.

I think I'd like to call this "innerText", see #9000, rather than "innerHTML" w/an additional escaping flag.

comment:4 Changed 10 years ago by bill

Priority: highnormal
severity: blockernormal

Note though that other widgets take HTML from dojo.data, see for example ComboBox (the custom label example in test_ComboBox.html, and also Button IIRC), so calling this a "regression" rather than a "bug fix" is just a matter of opinion.

comment:5 Changed 10 years ago by Nathan Toone

My only reason for calling it a "regression" is that dijit.Tree "worked in 1.3"

comment:6 Changed 10 years ago by bill

Owner: set to bill
Status: newassigned

Yes, it worked in 1.3 It also works in 1.4. Unfortunately the API documentation doesn't specify whether labelAttr/store.getLabel() is supposed to be interpreted as HTML or plain text.

Anyway, I'll make it interpret the label as plain text.

comment:7 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [20609]) Add innerText type for attributeMap (similar to innerHTML but for plain text), and use it for Tree labels, to match Tree's 1.3 behavior. Fixes #9000, #10128, refs #9230 !strict.

Note: See TracTickets for help on using tickets.