Skip to content

Instantly share code, notes, and snippets.

@benvinegar
Last active August 29, 2015 14:06
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save benvinegar/3e2c909e31a7c9483978 to your computer and use it in GitHub Desktop.
Save benvinegar/3e2c909e31a7c9483978 to your computer and use it in GitHub Desktop.
Proof-of-concept ESLint rule for detecting common XSS pattern
/**
* Proof of concept ESLint rule for warning about potential
* XSS vulnerabilities caused by mixing innerHTML/text node
* property access.
*
* More here: http://benv.ca/2012/10/2/you-are-probably-misusing-DOM-text-methods/
*/
'use strict';
var WARNING_MSG =
'innerHTML and text node properties used together may result in XSS vulnerabilities';
var TEXT_NODE_PROP_NAMES = ['createTextNode', 'innerText', 'textContent'];
module.exports = function(context) {
var firstAccess = Object.create(null);
// Walks up the AST looking for an ancestor node with the
// given type. If none is found, returns the top-most node in
// the AST (Program).
function findClosestWithType(node, types) {
while (node.parent) {
node = node.parent;
if (types.indexOf(node.type) >= 0) {
break;
}
}
return node;
}
function isInnerHTMLAccess(node) {
return node.property.name === 'innerHTML';
}
return {
MemberExpression: function(node) {
var propName = node.property.name,
isInnerHTMLExpr = isInnerHTMLAccess(node),
isTextNodeExpr = TEXT_NODE_PROP_NAMES.indexOf(propName) >= 0;
if (!isInnerHTMLExpr && !isTextNodeExpr) {
return;
}
// Find closest ancestor inside this function block
var top = '' + findClosestWithType(node, ['FunctionDeclaration', 'FunctionExpression']).range[0];
if (!(top in firstAccess)) {
// We haven't seen this ancestor; store it for later, keyed by prop name.
firstAccess[top] = node;
} else {
// If we've already seen this ancestor, it means we've already processed
// an innerHTML/text node member expression. If it's the "bad" member, e.g.
// we are currently processing innerHTML but we've already seen textContent
// in this function, then report to ESLint.
var entry = firstAccess[top];
if (isInnerHTMLExpr && !isInnerHTMLAccess(entry) || isTextNodeExpr && isInnerHTMLAccess(entry)) {
context.report(node, WARNING_MSG);
}
}
}
};
};
@nzakas
Copy link

nzakas commented Sep 12, 2014

I'm having a hard time understanding what pattern you're trying to detect.

@benvinegar
Copy link
Author

@Zakas – Yeah, I hastily threw this together on my train ride home with some ad-hoc / failsauce testing. My apologies.

@michaelficarra and I wrote a better (actually working) version – which I've updated here.

The purpose of the rule is to locate function expressions / declarations that access both innerHTML and either textContent, innerText, or createTextNode. If so, it triggers a warning to ESLint that using these properties/methods together could result in an XSS vulnerability.

The rule implementation is obviously, flawed. It will trigger false positives. But I think it could catch some legitimate cases – or at least cause people to inspect their code closer, and then manually disable the rule when they're confident no XSS vulnerability is present.

I don't think this should be any kind of official ESLint rule or anything. I just wanted to get your opinion on whether there's merit to using ESLint – even in a fuzzy, somewhat-error-prone way – to attempt to scan for possible XSS vectors like this.

@nzakas
Copy link

nzakas commented Sep 13, 2014

Ah yeah, I think there's some merit to doing that. The core ESLint rules are all intentionally not fuzzy to avoid confusion by people who might not have enough context to understand the rules on their own.

@benvinegar
Copy link
Author

@nzakas – Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment