#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)
Change History (9)
Changed 8 years ago by
Attachment: | dojo templatedMixin.html added |
---|
comment:1 Changed 8 years ago by
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,"""); //TODO: add &? 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 8 years ago by
Milestone: | tbd → 1.10 |
---|---|
Owner: | set to bill |
Status: | new → assigned |
I'll check in the change to have a separate (overridable) methods that escapes < > ' " &.
comment:3 Changed 8 years ago by
I tested rev 3365a6d... with our app and it works just like I expected. Thanks a lot for the work!
comment:4 Changed 7 years ago by
seems it has been checked in? If confirmed shouldn't this be closed?
comment:5 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Yes, fixed in 3365a6d, for some reason this ticket didn't automatically close.
comment:6 Changed 7 years ago by
Type: | enhancement → defect |
---|
simple test case showing desired behavior