diff --git a/.gitignore b/.gitignore
index eb79dd5..32bc532 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,2 +1,3 @@
node_modules
.idea
+*.swp
diff --git a/README.md b/README.md
index 6ad4813..867ec37 100644
--- a/README.md
+++ b/README.md
@@ -1,5 +1,5 @@
[](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`.
@@ -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"],
@@ -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)
diff --git a/SCHEMA.md b/SCHEMA.md
new file mode 100644
index 0000000..17ea1a0
--- /dev/null
+++ b/SCHEMA.md
@@ -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: [...]
+ }
+ ...
+ }
+ ]
+ }
+}
+```
diff --git a/lib/ruleHelper.js b/lib/ruleHelper.js
index ac51d94..32dd52c 100644
--- a/lib/ruleHelper.js
+++ b/lib/ruleHelper.js
@@ -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
*/
@@ -49,30 +54,18 @@ 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;
@@ -80,6 +73,170 @@ RuleHelper.prototype = {
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}`);
diff --git a/lib/rules/method.js b/lib/rules/method.js
index ae3885c..92ef113 100644
--- a/lib/rules/method.js
+++ b/lib/rules/method.js
@@ -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: {
@@ -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;
@@ -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");
}
diff --git a/lib/rules/property.js b/lib/rules/property.js
index 4b1ab53..da4cc0f 100644
--- a/lib/rules/property.js
+++ b/lib/rules/property.js
@@ -12,6 +12,18 @@ const RuleHelper = require("../ruleHelper");
// Rule Definition
//------------------------------------------------------------------------------
+
+const defaultRuleChecks = {
+
+ // Check unsafe assignment to innerHTML
+ innerHTML: {
+ },
+
+ // Check unsafe assignment to outerHTML
+ outerHTML: {
+ }
+};
+
module.exports = {
meta: {
docs: {
@@ -47,15 +59,10 @@ module.exports = {
if (CHECK_REQUIRED_OPERATORS.indexOf(node.operator) === -1) {
ruleHelper.reportUnsupported(node, "Unexpected Assignment", "Unsupported Operator for AssignmentExpression");
}
- if ((node.left.property.name === "innerHTML") ||
- (node.left.property.name === "outerHTML")) {
- if (!ruleHelper.allowedExpression(node.right)) {
- context.report(node, `Unsafe assignment to ${node.left.property.name}`);
- }
- }
+ ruleHelper.checkProperty(node, defaultRuleChecks);
}
}
- },
+ }
};
}
};
diff --git a/package.json b/package.json
index 61e0a5f..db94a3e 100644
--- a/package.json
+++ b/package.json
@@ -30,6 +30,6 @@
},
"scripts":{
"test": "./node_modules/.bin/mocha tests/rules/",
- "lint": "./node_modules/.bin/eslint index.js lib/**/*.js tests/**/*.js"
+ "lint": "./node_modules/.bin/eslint index.js lib/*.js lib/**/*.js tests/**/*.js"
}
}
diff --git a/tests/rules/method.js b/tests/rules/method.js
index e1b4254..9654d63 100644
--- a/tests/rules/method.js
+++ b/tests/rules/method.js
@@ -24,12 +24,6 @@ eslintTester.run("method", rule, {
// XXX this does not find z['innerHTML'] and the like.
valid: [
- // testing unwrapSafeHTML spread
- {
- code: "this.imeList.innerHTML = Sanitizer.unwrapSafeHTML(...listHtml);",
- parserOptions: { ecmaVersion: 6 }
- },
-
// tests for insertAdjacentHTML calls
{
code: "n.insertAdjacentHTML('afterend', 'meh');",
@@ -57,6 +51,51 @@ eslintTester.run("method", rule, {
code: "document.writeln(Sanitizer.escapeHTML`${evil}`);",
parserOptions: { ecmaVersion: 6 }
},
+ {
+ code: "otherNodeWeDontCheckFor.writeln(evil);",
+ parserOptions: { ecmaVersion: 6 }
+ },
+
+ // Native method (Check customize code doesn't include these)
+ {
+ code: "document.toString(evil);"
+ },
+
+ {
+ code: "document.write(escaper(x))",
+ options: [
+ {
+ escape: {
+ methods: ["escaper"]
+ }
+ }
+ ]
+ },
+
+ // Checking write can be overriden
+ {
+ code: "document.write(evilest)",
+ options: [
+ {
+ objectMatches: ["document", "documentFun"]
+ },
+ {
+ write: {
+ objectMatches: ["thing"]
+ }
+ }
+ ]
+ },
+
+ // Checking disableDefault can remove the default rules
+ {
+ code: "document.write(evil)",
+ options: [
+ {
+ defaultDisable: true
+ }
+ ]
+ }
],
// Examples of code that should trigger the rule
@@ -72,7 +111,7 @@ eslintTester.run("method", rule, {
code: "node.insertAdjacentHTML('beforebegin', htmlString);",
errors: [
{
- message: "Unsafe call to insertAdjacentHTML",
+ message: "Unsafe call to node.insertAdjacentHTML for argument 1",
type: "CallExpression"
}
]
@@ -81,7 +120,7 @@ eslintTester.run("method", rule, {
code: "node.insertAdjacentHTML('beforebegin', template.getHTML());",
errors: [
{
- message: "Unsafe call to insertAdjacentHTML",
+ message: "Unsafe call to node.insertAdjacentHTML for argument 1",
type: "CallExpression"
}
]
@@ -92,7 +131,25 @@ eslintTester.run("method", rule, {
code: "document.write(''+ htmlInput + '');",
errors: [
{
- message: "Unsafe call to document.write",
+ message: "Unsafe call to document.write for argument 0",
+ type: "CallExpression"
+ }
+ ]
+ },
+ {
+ code: "documentish.write(''+ htmlInput + '');",
+ errors: [
+ {
+ message: "Unsafe call to documentish.write for argument 0",
+ type: "CallExpression"
+ }
+ ]
+ },
+ {
+ code: "documentIframe.write(''+ htmlInput + '');",
+ errors: [
+ {
+ message: "Unsafe call to documentIframe.write for argument 0",
type: "CallExpression"
}
]
@@ -101,11 +158,56 @@ eslintTester.run("method", rule, {
code: "document.writeln(evil);",
errors: [
{
- message: "Unsafe call to document.writeln",
+ message: "Unsafe call to document.writeln for argument 0",
type: "CallExpression"
}
]
},
+ // Broken config
+ {
+ code: "b.boop(pie)",
+ options: [
+ {
+ },
+ {
+ boop: {
+ }
+ }
+ ],
+ errors: [
+ {
+ message: "Method check requires properties array in eslint rule boop",
+ type: "CallExpression"
+ }
+ ]
+ },
+
+ // Checking disableDefault can remove the default rules but also add more
+ {
+ code: "document.write(evil); b.thing(x); b.other(me);",
+ options: [
+ {
+ defaultDisable: true
+ },
+ {
+ thing: {
+ },
+ other: {
+ properties: [0]
+ }
+ }
+ ],
+ errors: [
+ {
+ message: "Method check requires properties array in eslint rule thing",
+ type: "CallExpression"
+ },
+ {
+ message: "Unsafe call to b.other for argument 0",
+ type: "CallExpression"
+ }
+ ]
+ }
]
});
diff --git a/tests/rules/property.js b/tests/rules/property.js
index 927785e..b305e79 100644
--- a/tests/rules/property.js
+++ b/tests/rules/property.js
@@ -111,7 +111,19 @@ eslintTester.run("property", rule, {
{
code: "w.innerHTML = `${'lulz'+'meh'}`;",
parserOptions: { ecmaVersion: 6 }
- }
+ },
+
+ // testing unwrapSafeHTML spread
+ {
+ code: "this.imeList.innerHTML = Sanitizer.unwrapSafeHTML(...listHtml);",
+ parserOptions: { ecmaVersion: 6 }
+ },
+
+
+ // Native method (Check customize code doesn't include these)
+ {
+ code: "document.toString = evil;"
+ },
],
// Examples of code that should trigger the rule
@@ -261,6 +273,19 @@ eslintTester.run("property", rule, {
parserOptions: { ecmaVersion: 6 }
},
+ // testing unwrapSafeHTML spread sanitizer typo
+ {
+ code: "this.imeList.innerHTML = Sanitizer.unrapSafeHTML(...listHtml);",
+ errors: [
+ {
+ message: "Unsafe assignment to innerHTML",
+ type: "AssignmentExpression"
+ }
+ ],
+ parserOptions: { ecmaVersion: 6 }
+ },
+
+
// the previous override for manual review and legacy code is now invalid
{
code: "g.innerHTML = potentiallyUnsafe; // a=legacy, bug 1155131",