Skip to content

Commit

Permalink
Exclude attributes starting "on" on HTML elements
Browse files Browse the repository at this point in the history 
Because:

* It doesn't work well with TW5's refresh mechanism, which relies on
being able to regenerate any portion of the DOM as required; this
frequently causes inline handlers to be re-executed at unexpected times
(see
http://tiddlywiki.com/static/TiddlyWiki%2520for%2520Developers.html)
* It mixes TW5 version-specific JavaScript with user content
* In multiuser environments there is a security risk to importing or
viewing tiddlers you didn't author if they can have JavaScript in them
  • Loading branch information
Jermolene committed Mar 12, 2014
1 parent 0d18f3c commit d0caf21
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
4 changes: 2 additions & 2 deletions core/modules/widgets/element.js
View file
Expand Up @@ -31,7 +31,7 @@ ElementWidget.prototype.render = function(parent,nextSibling) {
this.computeAttributes();
this.execute();
var domNode = this.document.createElementNS(this.namespace,this.parseTreeNode.tag);
this.assignAttributes(domNode);
this.assignAttributes(domNode,{excludeEventAttributes: true});
parent.insertBefore(domNode,nextSibling);
this.renderChildren(domNode,null);
this.domNodes.push(domNode);
Expand Down Expand Up @@ -65,7 +65,7 @@ ElementWidget.prototype.refresh = function(changedTiddlers) {
hasChangedAttributes = $tw.utils.count(changedAttributes) > 0;
if(hasChangedAttributes) {
// Update our attributes
this.assignAttributes(this.domNodes[0]);
this.assignAttributes(this.domNodes[0],{excludeEventAttributes: true});
}
return this.refreshChildren(changedTiddlers) || hasChangedAttributes;
};
Expand Down
9 changes: 8 additions & 1 deletion core/modules/widgets/widget.js
View file
Expand Up @@ -252,10 +252,17 @@ Widget.prototype.getAttribute = function(name,defaultText) {

/*
Assign the computed attributes of the widget to a domNode
options include:
excludeEventAttributes: ignores attributes whose name begins with "on"
*/
Widget.prototype.assignAttributes = function(domNode) {
Widget.prototype.assignAttributes = function(domNode,options) {
options = options || {};
var self = this;
$tw.utils.each(this.attributes,function(v,a) {
// Check exclusions
if(options.excludeEventAttributes && a.substr(0,2) === "on") {
v = undefined;
}
if(v !== undefined) {
// Setting certain attributes can cause a DOM error (eg xmlns on the svg element)
try {
Expand Down

2 comments on commit d0caf21

@ssokolow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on how much this is about user security as opposed to merely being about robust operation, you may want to go a bit further than that.

The latter sections of the Sanitization page in the docs for the Python Universal Feed Parser module have some good examples of other ways Javascript execution can be accomplished.

If your really want secure and don't need an explanation of the "why", there's also the WHATWG's Sanitization rules wiki page, which evolved from feedparser's rules.

@Jermolene
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point I'm not trying to fully sanitise our way around the more obscure browser issues, but to try to ensure that authors don't start using dangerous patterns, like inline event handlers script tags.

Please sign in to comment.