Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#17286 closed defect (fixed)

_TemplatedMixin should fully escape html entities

Reported by: gbranchaud Owned by: bill
Priority: undecided Milestone: 1.10
Component: Dijit Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

The _TemplatedMixin does some basic escaping for properties that are not prefixed by a "!" (i.e. "${foo}" will be escaped, but not "${!foo}").

The current escaping logic simply replaces double-quotes by their equivalent html entity. To me this is not sufficient.

I would like support for replacing all characters into their html entity (dojox/html/entities does this) or at least for the basic 4-5 ones recommended by OWASP.

Attached to this case is a basic test case illustrating the desired behavior.

Note: this might be considered a change too breaking to be implemented. In that case, please consider making the escaping logic easily user-extendable in _TemplatedMixin (with a protected escapeValue(value) method, maybe?) so that custom widgets can extend a single method with more complex escaping.

Attachments (1)

dojo templatedMixin.html (936 bytes) - added by gbranchaud 6 years ago.
simple test case showing desired behavior

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by gbranchaud

Attachment: dojo templatedMixin.html added

simple test case showing desired behavior

comment:1 in reply to:  description Changed 6 years ago by bill

Replying to gbranchaud:

The current escaping logic simply replaces double-quotes by their equivalent html entity. To me this is not sufficient.

Yah, I think the code is designed for attributes, for example a template like:

<div id="${id}"></div>

... although that's not needed nowadays since you can achieve the same functionality in your prototype:

_setIdAttr: "domNode"

The code also mentions it's for attributes:

// Safer substitution, see heading "Attribute values" in
// http://www.w3.org/TR/REC-html40/appendix/notes.html#h-B.3.2
value.toString().replace(/"/g,"&quot;"); //TODO: add &amp? use encodeXML method?

I agree that it should work for innerHTML too, and replacing the 5 entities in that link (" ' < > &) seems reasonable. OTOH _TemplatedMixin will probably be replaced completely in 2.0.

But I patched it anyway. Try https://github.com/dojo/dijit/pull/1 and tell me if it works for you. BTW, turns out that we already had a test case for this but it wasn't catching the error.

comment:2 Changed 6 years ago by bill

Milestone: tbd1.10
Owner: set to bill
Status: newassigned

I'll check in the change to have a separate (overridable) methods that escapes < > ' ".

Version 0, edited 6 years ago by bill (next)

comment:3 Changed 6 years ago by gbranchaud

I tested rev 3365a6d... with our app and it works just like I expected. Thanks a lot for the work!

comment:4 Changed 5 years ago by cjolif

seems it has been checked in? If confirmed shouldn't this be closed?

comment:5 Changed 5 years ago by bill

Resolution: fixed
Status: assignedclosed

Yes, fixed in 3365a6d, for some reason this ticket didn't automatically close.

comment:6 Changed 5 years ago by bill

Type: enhancementdefect

comment:7 Changed 5 years ago by Bill Keese <bill@…>

In 5f94249123a5e434b01fb3ba897c855af69bbf76/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:8 Changed 5 years ago by Bill Keese <bill@…>

In f65925debca2997f5d8cd347f7b552ae133fff20/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.