Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for special attribute filtering. #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow for special attribute filtering. #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ let compiler = {
attr.attrName = this.adjustAttrName(elType, attr.attrName);
if (hasRef) {
this.renderers[tdIndex] += ` td.setAttribute(td.getNodeAtIdxPath(root, ${JSON.stringify(indexesClone)}), '${attr.attrName}', ${this.walkAttrs(attr.value)});\n`;
// this.renderers[tdIndex] += ` td.getNodeAtIdxPath(root, ${JSON.stringify(indexesClone)}).setAttribute('${attr.attrName}', td.getAttrValue('${elType}', '${attr.attrName}', ${this.walkAttrs(attr.value)}));\n`;
} else {
this.fragments[tdIndex] += ` el${this.context.htmlBodies[tdIndex].count}.setAttribute('${attr.attrName}', ${this.walkAttrs(attr.value)});\n`;
this.fragments[tdIndex] += ` el${this.context.htmlBodies[tdIndex].count}.setAttribute('${attr.attrName}', td.getAttrValue('${elType}', '${attr.attrName}', ${this.walkAttrs(attr.value)}));\n`;
}
});
this.context.state = previousState;
Expand Down
33 changes: 32 additions & 1 deletion src/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ let tornado = {
*/
setAttribute(node, attrName, vals) {
Promise.all(vals).then(values => {
node.setAttribute(attrName, values.join(''));
node.setAttribute(attrName, this.getAttrValue(node.tagName, attrName, values.join('')));
});
},

Expand Down Expand Up @@ -330,6 +330,31 @@ let tornado = {
return div.innerHTML;
},

/**
* Get a secure value for a given attribute on a given node type
* @param {String} nodeType The type of HTML element the attribute will be applied to.
* @param {String} attrType The type of attribute.
* @param {String} val The initial value, to be run through the filters
*/
getAttrValue(nodeType, attrType, val) {
let filter = this.getAttrFilter(`${nodeType.toLowerCase()}[${attrType.toLowerCase()}]`);
val = Array.isArray(val) ? val.join('') : val;
if (filter) {
return filter(val);
}
return val;
},

/**
* Return a filter method for a filter selector.
* TODO: Make these filters registerable
* @param {String} filterSelector A filter selector of type `'el-name[attr-name]'`. For example,
* a filter for `href`s on an `a` tag would be `'a[href]'`.
*/
getAttrFilter(filterSelector) {
return this.attrFilters[filterSelector];
},

util: {
/**
* Determine if a value is an object
Expand Down Expand Up @@ -399,6 +424,12 @@ let tornado = {
}
return result;
}
},

attrFilters: {
'a[href]': function(val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may have to repeat this key a bunch of times. a[href], img[src]. is there a way to gather a bunch of contexts to filter on.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We could pretty easily define a single function and then assign it to multiple contexts:

function anchorsAndImages (val) {
  ...
}

attrFilters: {
  'a[href]': anchorsAndImages,
  'img[src]': anchorsAndImages
}

I was also thinking of having some more generic rules. For example, 'a' would match all attributes in an anchor tag, and '[src]' would match src attributes on any tag. That leads to a few questions:

  1. If multiple filters are found for a single attribute, are they all executed or just the most specific?
  2. If they are all executed, in what order are they executed? From most specific to least specific? Random?

return val.replace(/^javascript:.*/, '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

data: '(white spaces )javascript:alert(1)'

Copy link
Owner Author

Choose a reason for hiding this comment

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

This was meant to be an example of what is possible rather than a legitimate filter. I was hoping to get your input on what the real filters should look like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I've seen have all been whitelists. Restrict urls to be one of :
// - full urls with https? ftp and mailto protocols (javascript: is not allowed)
// - relative paths (/foo)
// - hash fragments (#foo) and querystrings (?a=b)

and disallow everything else.

there's a regex for this if you need it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, that would be great.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you guys discussed the regex?

}
}
};

Expand Down