diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9affb04..7e02c4b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,3 +33,6 @@ jobs: - name: Test run: yarn run test + + - name: Test (integration tests) + run: yarn run test-integration diff --git a/README.md b/README.md index 83965cb..1a1d8b3 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ $ npm install --save-dev eslint-plugin-no-unsanitized ```js import nounsanitized from "eslint-plugin-no-unsanitized"; -export default config = [nounsanitized.configs.recommended]; +export default [...nounsanitized.configs.recommended]; ``` or @@ -69,7 +69,7 @@ or ```js import nounsanitized from "eslint-plugin-no-unsanitized"; -export default config = [ +export default [ { files: ["**/*.js"], plugins: { nounsanitized }, diff --git a/docs/rules/parsing_method.md b/docs/rules/parsing_method.md new file mode 100644 index 0000000..8a1d8f3 --- /dev/null +++ b/docs/rules/parsing_method.md @@ -0,0 +1,13 @@ +# parsing_method + +The _parsing_method_ rule in _eslint-plugin-no-unsanitized_ performs basic security +checks for function calls that parse strings into new Document or DocumentFragment +instances. The idea of these checks is to allow developers to opt-in/opt-out of detecting +use of these methods, separately from the `method` rule which is reporting violation +as errors by default. + +### Further Reading + +- Advanced guidance on [Fixing rule violations](fixing-violations.md) +- This rule has some [customization](customization.md) options that allow you + to add or remove functions that should not be called diff --git a/index.js b/index.js index 8924161..ab1f3ac 100644 --- a/index.js +++ b/index.js @@ -11,6 +11,7 @@ const plugin = { rules: { property: require("./lib/rules/property"), method: require("./lib/rules/method"), + parsing_method: require("./lib/rules/parsing_method"), }, configs: {}, }; @@ -18,6 +19,7 @@ const plugin = { const rules = { "no-unsanitized/property": "error", "no-unsanitized/method": "error", + "no-unsanitized/parsing_method": "warn", }; Object.assign(plugin.configs, { diff --git a/lib/base_method.js b/lib/base_method.js new file mode 100644 index 0000000..70dd4ea --- /dev/null +++ b/lib/base_method.js @@ -0,0 +1,194 @@ +/** + * @file ESLint rule to disallow unsanitized method calls + * @author Frederik Braun et al. + * @copyright 2015-2017 Mozilla Corporation. All rights reserved. + */ +"use strict"; + +const RuleHelper = require("./ruleHelper"); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +/** + * On newer parsers, `import(foo)` gets parsed as a keyword. + * + * @param {object} ruleHelper a RuleHelper instance + * @param {object} importExpr The ImportExpression we triggered on + * @returns {undefined} Does not return + */ +function checkImport(ruleHelper, importExpr) { + const fakeCall = { + callee: { type: "Import" }, + arguments: [importExpr.source], + }; + Object.assign(fakeCall, importExpr); + ruleHelper.checkMethod(fakeCall); +} + +/** + * Run ruleHelper.checkMethod for all but irrelevant callees (FunctionExpression, etc.) + * + * @param {object} ruleHelper a RuleHelper instance + * @param {object} callExpr The CallExpression we triggered on + * @param {object} node The callee node + * @returns {undefined} Does not return + */ +function checkCallExpression(ruleHelper, callExpr, node) { + switch (node.type) { + case "Identifier": + case "MemberExpression": + if (callExpr.arguments && callExpr.arguments.length > 0) { + ruleHelper.checkMethod(callExpr); + } + break; + + case "TSNonNullExpression": { + const newCallExpr = Object.assign({}, callExpr); + newCallExpr.callee = node.expression; + checkCallExpression(ruleHelper, newCallExpr, node.expression); + break; + } + + case "TaggedTemplateExpression": { + const newCallExpr = Object.assign({}, callExpr); + newCallExpr.callee = node.tag; + const expressions = node.quasi.expressions; + const strings = node.quasi.quasis; + newCallExpr.arguments = [strings, ...expressions]; + checkCallExpression(ruleHelper, newCallExpr, node.tag); + break; + } + + case "TypeCastExpression": { + const newCallExpr = Object.assign({}, callExpr); + newCallExpr.callee = node.expression; + checkCallExpression(ruleHelper, newCallExpr, node.expression); + break; + } + + case "AssignmentExpression": + if (node.right.type === "MemberExpression") { + const newCallExpr = Object.assign({}, callExpr); + newCallExpr.callee = node.right; + checkCallExpression(ruleHelper, newCallExpr, node.right); + break; + } + checkCallExpression(ruleHelper, callExpr, node.right); + break; + + case "Import": + ruleHelper.checkMethod(callExpr); + break; + + case "SequenceExpression": { + // the return value of a SequenceExpression is the last expression. + // So, we create a new mock CallExpression with the actually called + // ... expression as the callee node and pass it to checkMethod() + + const newCallExpr = Object.assign({}, callExpr); + const idx = node.expressions.length - 1; + const called = node.expressions[idx]; + newCallExpr.callee = called; + ruleHelper.checkMethod(newCallExpr); + break; + } + + case "TSAsExpression": + break; + + // those are fine: + case "LogicalExpression": // Should we scan these? issue #62. + case "ConditionalExpression": + case "ArrowFunctionExpression": + case "FunctionExpression": + case "Super": + case "CallExpression": + case "ThisExpression": + case "NewExpression": + case "TSTypeAssertion": + case "AwaitExpression": // see issue #122 + break; + + // If we don't cater for this expression throw an error + default: + ruleHelper.reportUnsupported( + node, + "Unexpected Callee", + `Unsupported Callee of type ${node.type} for CallExpression` + ); + } +} + +const defaultMeta = { + type: "problem", + // Expected to be filled by the concrete rules through the exported createMethodsRule function. + docs: {}, + schema: [ + { + type: "object", + properties: { + defaultDisable: { + type: "boolean", + }, + escape: { + type: "object", + properties: { + taggedTemplates: { + type: "array", + items: [{ type: "string" }], + }, + methods: { + type: "array", + items: [{ type: "string" }], + }, + }, + }, + objectMatches: { + type: "array", + }, + properties: { + type: "array", + }, + variableTracing: { type: "boolean" }, + }, + additionalProperties: false, + }, + { + type: "object", + }, + ], +}; + +module.exports = function createMethodsRule({ meta, defaultRuleChecks }) { + return { + meta: { + ...defaultMeta, + ...meta, + }, + create(context) { + const ruleHelper = new RuleHelper(context, defaultRuleChecks); + return { + CallExpression(node) { + checkCallExpression(ruleHelper, node, node.callee); + }, + ImportExpression(node) { + checkImport(ruleHelper, node); + }, + + // Tagged template expressions pass arguments in a special format we need to + // map to our existing function call logic + // foo`bar${var1}${var2}` will run as foo(['bar', ''], var1, var2) + TaggedTemplateExpression(node) { + const newCallExpr = Object.assign({}, node); + newCallExpr.callee = node.tag; + const expressions = node.quasi.expressions; + const strings = node.quasi.quasis; + newCallExpr.arguments = [strings, ...expressions]; + checkCallExpression(ruleHelper, newCallExpr, node.tag); + }, + }; + }, + }; +}; diff --git a/lib/ruleHelper.js b/lib/ruleHelper.js index b4e1345..ae8c838 100644 --- a/lib/ruleHelper.js +++ b/lib/ruleHelper.js @@ -501,16 +501,22 @@ RuleHelper.prototype = { ) { // Include the additional details if available (e.g. name of a disallowed variable // and the position of the expression that made it disallowed). + const baseMessage = + typeof ruleCheck.getCustomMessage === "function" + ? ruleCheck.getCustomMessage( + this.getCodeName(node.callee) + ) + : `Unsafe call to ${this.getCodeName(node.callee)}`; if (details.message) { this.context.report( node, - `Unsafe call to ${this.getCodeName(node.callee)} for argument ${propertyId} (${details.message})` + `${baseMessage} for argument ${propertyId} (${details.message})` ); return; } this.context.report( node, - `Unsafe call to ${this.getCodeName(node.callee)} for argument ${propertyId}` + `${baseMessage} for argument ${propertyId}` ); } }); diff --git a/lib/rules/method.js b/lib/rules/method.js index 45c5020..c5d7318 100644 --- a/lib/rules/method.js +++ b/lib/rules/method.js @@ -5,7 +5,7 @@ */ "use strict"; -const RuleHelper = require("../ruleHelper"); +const createMethodsRule = require("../base_method"); //------------------------------------------------------------------------------ // Rule Definition @@ -45,180 +45,15 @@ const defaultRuleChecks = { }, }; -/** - * On newer parsers, `import(foo)` gets parsed as a keyword. - * - * @param {object} ruleHelper a RuleHelper instance - * @param {object} importExpr The ImportExpression we triggered on - * @returns {undefined} Does not return - */ -function checkImport(ruleHelper, importExpr) { - const fakeCall = { - callee: { type: "Import" }, - arguments: [importExpr.source], - }; - Object.assign(fakeCall, importExpr); - ruleHelper.checkMethod(fakeCall); -} - -/** - * Run ruleHelper.checkMethod for all but irrelevant callees (FunctionExpression, etc.) - * - * @param {object} ruleHelper a RuleHelper instance - * @param {object} callExpr The CallExpression we triggered on - * @param {object} node The callee node - * @returns {undefined} Does not return - */ -function checkCallExpression(ruleHelper, callExpr, node) { - switch (node.type) { - case "Identifier": - case "MemberExpression": - if (callExpr.arguments && callExpr.arguments.length > 0) { - ruleHelper.checkMethod(callExpr); - } - break; - - case "TSNonNullExpression": { - const newCallExpr = Object.assign({}, callExpr); - newCallExpr.callee = node.expression; - checkCallExpression(ruleHelper, newCallExpr, node.expression); - break; - } - - case "TaggedTemplateExpression": { - const newCallExpr = Object.assign({}, callExpr); - newCallExpr.callee = node.tag; - const expressions = node.quasi.expressions; - const strings = node.quasi.quasis; - newCallExpr.arguments = [strings, ...expressions]; - checkCallExpression(ruleHelper, newCallExpr, node.tag); - break; - } - - case "TypeCastExpression": { - const newCallExpr = Object.assign({}, callExpr); - newCallExpr.callee = node.expression; - checkCallExpression(ruleHelper, newCallExpr, node.expression); - break; - } - - case "AssignmentExpression": - if (node.right.type === "MemberExpression") { - const newCallExpr = Object.assign({}, callExpr); - newCallExpr.callee = node.right; - checkCallExpression(ruleHelper, newCallExpr, node.right); - break; - } - checkCallExpression(ruleHelper, callExpr, node.right); - break; - - case "Import": - ruleHelper.checkMethod(callExpr); - break; - - case "SequenceExpression": { - // the return value of a SequenceExpression is the last expression. - // So, we create a new mock CallExpression with the actually called - // ... expression as the callee node and pass it to checkMethod() - - const newCallExpr = Object.assign({}, callExpr); - const idx = node.expressions.length - 1; - const called = node.expressions[idx]; - newCallExpr.callee = called; - ruleHelper.checkMethod(newCallExpr); - break; - } - - case "TSAsExpression": - break; - - // those are fine: - case "LogicalExpression": // Should we scan these? issue #62. - case "ConditionalExpression": - case "ArrowFunctionExpression": - case "FunctionExpression": - case "Super": - case "CallExpression": - case "ThisExpression": - case "NewExpression": - case "TSTypeAssertion": - case "AwaitExpression": // see issue #122 - break; - - // If we don't cater for this expression throw an error - default: - ruleHelper.reportUnsupported( - node, - "Unexpected Callee", - `Unsupported Callee of type ${node.type} for CallExpression` - ); - } -} - -module.exports = { - meta: { - type: "problem", - docs: { - description: "ESLint rule to disallow unsanitized method calls", - category: "possible-errors", - url: "https://github.com/mozilla/eslint-plugin-no-unsanitized/tree/master/docs/rules/method.md", - }, - schema: [ - { - type: "object", - properties: { - defaultDisable: { - type: "boolean", - }, - escape: { - type: "object", - properties: { - taggedTemplates: { - type: "array", - items: [{ type: "string" }], - }, - methods: { - type: "array", - items: [{ type: "string" }], - }, - }, - }, - objectMatches: { - type: "array", - }, - properties: { - type: "array", - }, - variableTracing: { type: "boolean" }, - }, - additionalProperties: false, - }, - { - type: "object", - }, - ], - }, - create(context) { - const ruleHelper = new RuleHelper(context, defaultRuleChecks); - return { - CallExpression(node) { - checkCallExpression(ruleHelper, node, node.callee); - }, - ImportExpression(node) { - checkImport(ruleHelper, node); - }, - - // Tagged template expressions pass arguments in a special format we need to - // map to our existing function call logic - // foo`bar${var1}${var2}` will run as foo(['bar', ''], var1, var2) - TaggedTemplateExpression(node) { - const newCallExpr = Object.assign({}, node); - newCallExpr.callee = node.tag; - const expressions = node.quasi.expressions; - const strings = node.quasi.quasis; - newCallExpr.arguments = [strings, ...expressions]; - checkCallExpression(ruleHelper, newCallExpr, node.tag); - }, - }; +const meta = { + docs: { + description: "ESLint rule to disallow unsanitized method calls", + category: "possible-errors", + url: "https://github.com/mozilla/eslint-plugin-no-unsanitized/tree/master/docs/rules/method.md", }, }; + +module.exports = createMethodsRule({ + meta, + defaultRuleChecks, +}); diff --git a/lib/rules/parsing_method.js b/lib/rules/parsing_method.js new file mode 100644 index 0000000..6c7c3da --- /dev/null +++ b/lib/rules/parsing_method.js @@ -0,0 +1,54 @@ +/** + * @file ESLint rule to disallow unsanitized method calls to HTML parsing methods. + * @author Frederik Braun et al. + * @copyright 2015-2017 Mozilla Corporation. All rights reserved. + */ +"use strict"; + +const createMethodsRule = require("../base_method"); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +const defaultRuleChecks = { + // check first parameter to DOMParser parseFromString method. + parseFromString: { + // NOTE: look for calls on objects with 'parser' included in their name. + objectMatches: ["parser"], + properties: [0], + // Customized base message (propertyId and details.message will be appended to the custom + // message returned). + getCustomMessage(nodeCalleeCodeName) { + return `Potentially unsafe call to DOMParser parseFromString (${nodeCalleeCodeName})`; + }, + }, + + // check first parameter to Document.parseHTMLUnsafe(). + parseHTMLUnsafe: { + // NOTE: objectMatches values is internally used as case-insensitive. + objectMatches: ["document"], + properties: [0], + getCustomMessage(nodeCalleeCodeName) { + let message = "Potentially unsafe call to Document.parseHTMLUnsafe"; + if (nodeCalleeCodeName !== "Document.parseHTMLUnsafe") { + message += ` (${nodeCalleeCodeName})`; + } + return message; + }, + }, +}; + +const meta = { + docs: { + description: + "ESLint rule to disallow unsanitized method calls to HTML parsing methods", + category: "possible-errors", + url: "https://github.com/mozilla/eslint-plugin-no-unsanitized/tree/master/docs/rules/parsing_method.md", + }, +}; + +module.exports = createMethodsRule({ + meta, + defaultRuleChecks, +}); diff --git a/package.json b/package.json index df681da..82b4fa8 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ }, "scripts": { "test": "nyc mocha tests/rules/", + "test-integration": "mocha tests/integration/", "lint": "eslint . && prettier --check ." }, "files": [ diff --git a/tests/fixtures/eslint-flat-config/eslint.config.mjs b/tests/fixtures/eslint-flat-config/eslint.config.mjs new file mode 100644 index 0000000..45ac1c9 --- /dev/null +++ b/tests/fixtures/eslint-flat-config/eslint.config.mjs @@ -0,0 +1,5 @@ +import noUnsanitized from "../../../index.js"; + +const config = noUnsanitized.configs.recommended; + +export default config; diff --git a/tests/fixtures/eslint-flat-config/package.json b/tests/fixtures/eslint-flat-config/package.json new file mode 100644 index 0000000..054bc18 --- /dev/null +++ b/tests/fixtures/eslint-flat-config/package.json @@ -0,0 +1,4 @@ +{ + "name": "eslint-test", + "version": "1.0.0" +} diff --git a/tests/fixtures/eslint-flat-config/test.js b/tests/fixtures/eslint-flat-config/test.js new file mode 100644 index 0000000..4c1dc86 --- /dev/null +++ b/tests/fixtures/eslint-flat-config/test.js @@ -0,0 +1,10 @@ +/* eslint no-undef: 0, no-var: 0, no-unused-vars: 0, prefer-template: 0 */ + +// Expect property linting error. +node.innerHTML = "" + htmlInput + ""; + +// Expect method linting error. +node.insertAdjacentHTML("beforebegin", htmlString); + +// Expect parsing-method linting warning. +var doc = Document.parseHTMLUnsafe(badness); diff --git a/tests/integration/test-eslint-flat-config.js b/tests/integration/test-eslint-flat-config.js new file mode 100644 index 0000000..ec2f05b --- /dev/null +++ b/tests/integration/test-eslint-flat-config.js @@ -0,0 +1,81 @@ +const assert = require("assert"); +const { exec } = require("child_process"); +const path = require("path"); + +const { before, describe, it } = require("mocha"); +const ESLint = require("eslint").ESLint; + +const FIXTURES_DIR = path.resolve(path.join("tests", "fixtures")); +const ESLINT_BIN = path.resolve(path.join("node_modules", ".bin", "eslint")); + +const runEslint = ({ cwd, targetFile }) => { + return new Promise((resolve, reject) => { + const opts = { cwd }; + exec( + `${ESLINT_BIN} -f json "${targetFile}"`, + opts, + (error, stdout, stderr) => { + // Log and reject on unexpected errors (errors with code set to 1 are expected + // because we expect linting errors and warnings to be reported back by eslint). + if (error && error.code !== 1) { + console.log( + "Got unexpected error on executing eslint", + error + ); + if (stderr) { + console.log("stderr:\n", stderr); + } + reject(error); + return; + } + + try { + resolve(JSON.parse(stdout)); + } catch (err) { + reject(err); + } + } + ); + }); +}; + +describe("eslint-flat-config", function () { + before(function () { + // Only run these integration tests while running on eslint + // versions that support the configType flat. + if (ESLint.configType !== "flat") { + this.skip(); + } + }); + + it("loads the expected no-unsanitized recommended config", async () => { + const results = await runEslint({ + cwd: path.join(FIXTURES_DIR, "eslint-flat-config"), + targetFile: "test.js", + }); + + const expectedResults = [ + { + errorCount: 2, + warningCount: 1, + messages: [ + { ruleId: "no-unsanitized/property", severity: 2 }, + { ruleId: "no-unsanitized/method", severity: 2 }, + { ruleId: "no-unsanitized/parsing_method", severity: 1 }, + ], + }, + ]; + assert.deepEqual( + results.map((result) => ({ + errorCount: result.errorCount, + warningCount: result.warningCount, + messages: result.messages.map(({ ruleId, severity }) => ({ + ruleId, + severity, + })), + })), + expectedResults, + "Got the expected eslint errors and warnings" + ); + }); +}); diff --git a/tests/rules/parsing_method.js b/tests/rules/parsing_method.js new file mode 100644 index 0000000..0b92aad --- /dev/null +++ b/tests/rules/parsing_method.js @@ -0,0 +1,81 @@ +/** + * @file Test for no-unsanitized rule + * @author Frederik Braun et al. + * @copyright 2015-2017 Mozilla Corporation. All rights reserved + */ + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../lib/rules/parsing_method"); +const RuleTester = require("eslint").RuleTester; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const eslintTester = new RuleTester(); + +eslintTester.run("parsing_method", rule, { + // Examples of code that should not trigger the rule + // XXX this does not find z['innerHTML'] and the like. + + valid: [ + { + code: "var doc = parser.parseFromString('static string');", + }, + { + code: "var doc = Document.parseHTMLUnsafe('static string');", + }, + ], + + // Examples of code that should trigger the rule + invalid: [ + /* XXX Do NOT change the error strings below without review from freddy: + * The strings are optimized for SEO and understandability. + */ + + { + code: "var doc = domparser.parseFromString(badness);", + errors: [ + { + message: + /Potentially unsafe call to DOMParser parseFromString \(domparser\.parseFromString\) for argument 0/, + type: "CallExpression", + }, + ], + }, + // Make sure we also warn on DOMParser instance named differently (as long as `parser` is part of the object name). + { + code: "var doc = parserdom.parseFromString(badness);", + errors: [ + { + message: + /Potentially unsafe call to DOMParser parseFromString \(parserdom\.parseFromString\) for argument 0/, + type: "CallExpression", + }, + ], + }, + { + code: "var doc = Document.parseHTMLUnsafe(badness);", + errors: [ + { + message: + /Potentially unsafe call to Document.parseHTMLUnsafe for argument 0/, + type: "CallExpression", + }, + ], + }, + { + code: "var doc = SomeDocument.parseHTMLUnsafe(badness);", + errors: [ + { + message: + /Potentially unsafe call to Document.parseHTMLUnsafe \(SomeDocument\.parseHTMLUnsafe\) for argument 0/, + type: "CallExpression", + }, + ], + }, + ], +});