Proof-of-concept ESLint rule for detecting common XSS pattern

textnode-innerhtml.js

/**
 * 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);
        }
      }
    }
  };
};

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

@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.

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.

@nzakas – Cool, thanks!