Skip to content

Commit

Permalink
Custom rules for configuring rules. Fixes #29 (#51)
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanKingston authored and mozfreddyb committed Apr 21, 2017
1 parent 570ea4d commit db42f81
Show file tree
Hide file tree
Showing 9 changed files with 440 additions and 53 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
node_modules
.idea
*.swp
39 changes: 38 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[![Build Status](https://travis-ci.org/mozilla/eslint-plugin-no-unsanitized.svg?branch=master)](https://travis-ci.org/mozilla/eslint-plugin-no-unsanitized)
# Disallow unsanitized DOM access (no-unsanitized)
# Disallow unsanitized code (no-unsanitized)

These rules disallow unsafe coding practices that may result into security vulnerabilities. We will disallow assignments to innerHTML as well as calls to insertAdjacentHTML without the use of a pre-defined escaping function. The escaping functions must be called with a template string. The function names are hardcoded as `Sanitizer.escapeHTML` and `escapeHTML`.

Expand Down Expand Up @@ -31,6 +31,7 @@ This rule is being used within Mozilla to maintain and improve the security of o

In your eslint.json file enable this rule with the following:


```
{
"plugins": ["no-unsanitized"],
Expand All @@ -40,3 +41,39 @@ In your eslint.json file enable this rule with the following:
}
}
```

## Advanced configuration

```js
{
"plugins": ["no-unsanitized"],
"env": {
"no-unsanitized/method": [
"error",
{
disableDefault: true,
escape: {
taggedTemplates: ["safeHTML"]
}
},
{
html: {
properties: [0]
}
}
],
"no-unsanitized/method": [
"error",
{
},
{
innerHTML: {
objectMatches: ["document.*"]
}
}
]
}
}
```

[To see all available options vitit](./SCHEMA.md)
43 changes: 43 additions & 0 deletions SCHEMA.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
## Schema

```js
{
"plugins": ["no-unsanitized"],
"env": {
// $ruleName is either "method" or "property"
"no-unsanitized/$ruleName": ["errorlevel",
// This object is optional would use default config for all default ruleChecks
{
// optional, omitting or length 0 would be considered permitting all objectNames
objectMatches: ["$stringRegex"],

// optional, removes default $ruleCheckName's provided by this rule
disableDefault: $boolean,

// optional, if omitted would disallow use of matched "$ruleCheckName" prop/method
escape: {
// optional: Tagged template permitted as arguments/assignments
taggedTemplates: ["$taggedTemplateFunctionName"],

// optional: method permitted as arguments/assignments
methods: ["$MethodName"]
}
},
// optional would use default ruleChecks, will merge each $ruleCheckName with default rule setup so user doesn't have to specify properties if already defined
{
"$ruleCheckName": {
// optional, same as above and overrides parent even if blank array
objectMatches: ["$stringRegex"],

// optional, same as above, overrides parent even if blank object
escape: {...}

// indices to check for arguments passed to a method call. Only applies to $ruleName="method"
properties: [...]
}
...
}
]
}
}
```
195 changes: 176 additions & 19 deletions lib/ruleHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,14 @@ RuleHelper.prototype = {
* Using the Esprima Demo, for example: http://esprima.org/demo/parse.html
*
* @param {Object} expression Checks whether this node is an allowed expression.
* @param {Object} escapeObject contains keys "methods" and "taggedTemplates" which are arrays of strings
* of matching escaping function names.
* @returns {boolean}
*/
allowedExpression(expression) {
allowedExpression(expression, escapeObject) {
if (!escapeObject) {
escapeObject = {};
}
/*
expression = { right-hand side of innerHTML or 2nd param to insertAdjacentHTML
*/
Expand All @@ -49,37 +54,189 @@ RuleHelper.prototype = {
// check for ${..} expressions
for (let e = 0; e < expression.expressions.length; e++) {
const templateExpression = expression.expressions[e];
if (!this.allowedExpression(templateExpression)) {
if (!this.allowedExpression(templateExpression, escapeObject)) {
allowed = false;
break;
}
}
} else if (expression.type === "TaggedTemplateExpression") {
//TODO avoid code-duplication with CallExpression-case below.
const functionName = this.context.getSource(expression.tag);
if (VALID_ESCAPERS.indexOf(functionName) !== -1) {
allowed = true;
} else {
allowed = false;
}
} else if ((expression.type === "CallExpression") &&
(expression.callee.property || expression.callee.name)) {
const funcName = expression.callee.name || expression.callee.property.name;
if (funcName && VALID_UNWRAPPERS.indexOf(funcName) !== -1) {
allowed = true;
} else {
allowed = false;
}
allowed = this.isAllowedCallExpression(expression.tag, escapeObject.taggedTemplates || VALID_ESCAPERS);
} else if (expression.type === "CallExpression") {
allowed = this.isAllowedCallExpression(expression.callee, escapeObject.methods || VALID_UNWRAPPERS);
} else if (expression.type === "BinaryExpression") {
allowed = ((this.allowedExpression(expression.left))
&& (this.allowedExpression(expression.right)));
allowed = ((this.allowedExpression(expression.left, escapeObject))
&& (this.allowedExpression(expression.right, escapeObject)));
} else {
// everything that doesn't match is unsafe:
allowed = false;
}
return allowed;
},

/**
* Check if a callee is in the list allowed sanitizers
* @param {Object} callee function that is being called expression.tag or expression.callee
* @param {Array} allowedSanitizers List of valid wrapping expressions
* @returns {boolean}
*/
isAllowedCallExpression(callee, allowedSanitizers) {
const funcName = this.getCodeName(callee);
let allowed = false;
if (funcName && allowedSanitizers.indexOf(funcName) !== -1) {
allowed = true;
}
return allowed;
},

/**
* Captures safely any new node types that have been missed and throw when we don't support them
* this normalizes the passed in identifier type to return the same shape
*/
normalizeMethodCall(node) {
let methodName;
let objectName;
switch (node.type) {
case "Identifier":
methodName = node.name;
break;
case "MemberExpression":
methodName = node.property.name;
objectName = node.object.name;
break;
case "ArrowFunctionExpression":
methodName = "";
break;
default:
this.reportUnsupported(node, "Unexpected callable", `unexpected ${node.type} in normalizeMethodName`);
}
return {
objectName,
methodName
};
},

/**
* Returns functionName or objectName.methodName of an expression
* @param {Object} node
* @returns {String} nice name to expression call
*/
getCodeName(node) {
const normalizedMethodCall = this.normalizeMethodCall(node);
let codeName = normalizedMethodCall.methodName;
if (normalizedMethodCall.objectName) {
codeName = `${normalizedMethodCall.objectName}.${codeName}`;
}
return codeName;
},

/**
* Checks to see if a method or function should be called
* If objectMatches isn't present or blank array the code should not be checked
* If we do have object filters and the call is a function then it should not be checked
* Checks if there are objectMatches we need to apply
* @param {Object} node call expression node
* @param {Object} objectMatches strings that are checked as regex to match an object name
* @returns {Boolean} if to run checks expression
*/
shouldCheckMethodCall(node, objectMatches) {
const normalizedMethodCall = this.normalizeMethodCall(node.callee);
let matched = false;

// If objectMatches isn't present we should match all
if (!objectMatches) {
return true;
}

// if blank array the code should not be checked, this is a quick way to disable rules
// TODO should we make this match all instead and let the $ruleCheck be false instead?
if (objectMatches.length === 0) {
return false;
}

// If we do have object filters and the call is a function then it should not be checked
if ("objectName" in normalizedMethodCall) {
for (const objectMatch of objectMatches) {
const match = new RegExp(objectMatch, "gi");
if (normalizedMethodCall.objectName.match(match)) {
matched = true;
break;
}
}
}

// if we don't have a objectName return false as bare function call
// if we didn't match also return false
return matched;
},

/**
* Algorithm used to decide on merging ruleChecks with this.context
* @returns {Object} merged ruleChecks
*/
combineRuleChecks(defaultRuleChecks) {
const parentRuleChecks = this.context.options[0] || {};
let childRuleChecks = Object.assign({}, this.context.options[1]);
const ruleCheckOutput = {};

if (!("defaultDisable" in parentRuleChecks)
|| !parentRuleChecks.defaultDisable) {
childRuleChecks = Object.assign({}, defaultRuleChecks, childRuleChecks);
}

// If we have defined child rules lets ignore default rules
Object.keys(childRuleChecks).forEach((ruleCheckKey) => {
// However if they have missing keys merge with default
const ruleCheck = Object.assign({},
defaultRuleChecks[ruleCheckKey],
parentRuleChecks,
childRuleChecks[ruleCheckKey]);
ruleCheckOutput[ruleCheckKey] = ruleCheck;
});
return ruleCheckOutput;
},

/**
* Runs the checks against a CallExpression
* @param {Object} node call expression node
* @param {Object} defaultRuleChecks default rules to merge with this.context
*/
checkMethod(node, defaultRuleChecks) {
const ruleChecks = this.combineRuleChecks(defaultRuleChecks);
const normalizeMethodCall = this.normalizeMethodCall(node.callee);
const methodName = normalizeMethodCall.methodName;

if (ruleChecks.hasOwnProperty(methodName)) {
const ruleCheck = ruleChecks[methodName];
if (!Array.isArray(ruleCheck.properties)) {
this.context.report(node, `Method check requires properties array in eslint rule ${methodName}`);
return;
}
ruleCheck.properties.forEach((propertyId) => {
const argument = node.arguments[propertyId];
if (this.shouldCheckMethodCall(node, ruleCheck.objectMatches)
&& !this.allowedExpression(argument, ruleCheck.escape)) {
this.context.report(node, `Unsafe call to ${this.getCodeName(node.callee)} for argument ${propertyId}`);
}
});
}
},

/**
* Runs the checks against an assignment expression
* @param {Object} node assignment expression node
* @param {Object} defaultRuleChecks default rules to merge with this.context
*/
checkProperty(node, defaultRuleChecks) {
const ruleChecks = this.combineRuleChecks(defaultRuleChecks);

if (ruleChecks.hasOwnProperty(node.left.property.name)) {
const ruleCheck = ruleChecks[node.left.property.name];
if (!this.allowedExpression(node.right, ruleCheck.escape)) {
this.context.report(node, `Unsafe assignment to ${node.left.property.name}`);
}
}
},

reportUnsupported(node, reason, errorTitle) {
const bugPath = `https://github.com/mozilla/eslint-plugin-no-unsanitized/issues/new?title=${encodeURIComponent(errorTitle)}`;
this.context.report(node, `Error in no-unsanitized: ${reason}. Please report a minimal code snippet to the developers at ${bugPath}`);
Expand Down
43 changes: 29 additions & 14 deletions lib/rules/method.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,31 @@ const RuleHelper = require("../ruleHelper");
// Rule Definition
//------------------------------------------------------------------------------


const defaultRuleChecks = {

// check second parameter to .insertAdjacentHTML()
insertAdjacentHTML: {
properties: [1]
},

// check first parameter to .write(), as long as the preceeding object matches the regex "document"
write: {
objectMatches: [
"document"
],
properties: [0]
},

// check first parameter to .writeLn(), as long as the preceeding object matches the regex "document"
writeln: {
objectMatches: [
"document"
],
properties: [0]
}
};

module.exports = {
meta: {
docs: {
Expand All @@ -34,20 +59,8 @@ module.exports = {
switch(node.callee.type) {
case "Identifier":
case "MemberExpression":
if ("property" in node.callee && node.arguments.length > 0) {
if (node.callee.property.name === "insertAdjacentHTML") {
if (!ruleHelper.allowedExpression(node.arguments[1])) {
context.report(node, "Unsafe call to insertAdjacentHTML");
}
} else if (context.getSource(node.callee) === "document.write") {
if (!ruleHelper.allowedExpression(node.arguments[0])) {
context.report(node, "Unsafe call to document.write");
}
} else if (context.getSource(node.callee) === "document.writeln") {
if (!ruleHelper.allowedExpression(node.arguments[0])) {
context.report(node, "Unsafe call to document.writeln");
}
}
if (node.arguments.length > 0) {
ruleHelper.checkMethod(node, defaultRuleChecks);
}
break;

Expand All @@ -58,6 +71,8 @@ module.exports = {
break;
case "Super":
break;

// If we don't cater for this expression throw an error
default:
context.reportUnsupported(node, "Unexpected Callee", "Unsupported Callee for CallExpression");
}
Expand Down
Loading

0 comments on commit db42f81

Please sign in to comment.