From 085979fed9a5e24a87e4d92ee79272b59211d03f Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sun, 22 Mar 2020 06:01:29 +0900 Subject: [PATCH] Update: consider env in no-implied-eval (fixes #12733) (#12757) * Fix: consider env in no-implied-eval (fixes #12733) * fix typo, refactor to chaining * change to check only CallExpression, refactoring * check defs * checks callee * check static string value * check falsy - first argument * check empty arg * fix typo --- lib/rules/no-implied-eval.js | 184 +++++++++++++---------------- tests/lib/rules/no-implied-eval.js | 138 +++++++++++++++++++++- 2 files changed, 215 insertions(+), 107 deletions(-) diff --git a/lib/rules/no-implied-eval.js b/lib/rules/no-implied-eval.js index 46bb5d4f76e..ea5c6b8636b 100644 --- a/lib/rules/no-implied-eval.js +++ b/lib/rules/no-implied-eval.js @@ -5,6 +5,13 @@ "use strict"; +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const astUtils = require("./utils/ast-utils"); +const { getStaticValue } = require("eslint-utils"); + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -28,94 +35,97 @@ module.exports = { }, create(context) { - const CALLEE_RE = /^(setTimeout|setInterval|execScript)$/u; - - /* - * Figures out if we should inspect a given binary expression. Is a stack - * of stacks, where the first element in each substack is a CallExpression. - */ - const impliedEvalAncestorsStack = []; - - //-------------------------------------------------------------------------- - // Helpers - //-------------------------------------------------------------------------- + const EVAL_LIKE_FUNCS = Object.freeze(["setTimeout", "execScript", "setInterval"]); + const GLOBAL_CANDIDATES = Object.freeze(["global", "window"]); /** - * Get the last element of an array, without modifying arr, like pop(), but non-destructive. - * @param {Array} arr What to inspect - * @returns {*} The last element of arr - * @private + * Checks whether a node is evaluated as a string or not. + * @param {ASTNode} node A node to check. + * @returns {boolean} True if the node is evaluated as a string. */ - function last(arr) { - return arr ? arr[arr.length - 1] : null; + function isEvaluatedString(node) { + if ( + (node.type === "Literal" && typeof node.value === "string") || + node.type === "TemplateLiteral" + ) { + return true; + } + if (node.type === "BinaryExpression" && node.operator === "+") { + return isEvaluatedString(node.left) || isEvaluatedString(node.right); + } + return false; } /** - * Checks if the given MemberExpression node is a potentially implied eval identifier on window. - * @param {ASTNode} node The MemberExpression node to check. - * @returns {boolean} Whether or not the given node is potentially an implied eval. - * @private + * Checks whether a node is an Identifier node named one of the specified names. + * @param {ASTNode} node A node to check. + * @param {string[]} specifiers Array of specified name. + * @returns {boolean} True if the node is a Identifier node which has specified name. */ - function isImpliedEvalMemberExpression(node) { - const object = node.object, - property = node.property, - hasImpliedEvalName = CALLEE_RE.test(property.name) || CALLEE_RE.test(property.value); - - return object.name === "window" && hasImpliedEvalName; + function isSpecifiedIdentifier(node, specifiers) { + return node.type === "Identifier" && specifiers.includes(node.name); } /** - * Determines if a node represents a call to a potentially implied eval. - * - * This checks the callee name and that there's an argument, but not the type of the argument. - * @param {ASTNode} node The CallExpression to check. - * @returns {boolean} True if the node matches, false if not. - * @private + * Checks a given node is a MemberExpression node which has the specified name's + * property. + * @param {ASTNode} node A node to check. + * @param {string[]} specifiers Array of specified name. + * @returns {boolean} `true` if the node is a MemberExpression node which has + * the specified name's property */ - function isImpliedEvalCallExpression(node) { - const isMemberExpression = (node.callee.type === "MemberExpression"), - isIdentifier = (node.callee.type === "Identifier"), - isImpliedEvalCallee = - (isIdentifier && CALLEE_RE.test(node.callee.name)) || - (isMemberExpression && isImpliedEvalMemberExpression(node.callee)); - - return isImpliedEvalCallee && node.arguments.length; + function isSpecifiedMember(node, specifiers) { + return node.type === "MemberExpression" && specifiers.includes(astUtils.getStaticPropertyName(node)); } /** - * Checks that the parent is a direct descendent of an potential implied eval CallExpression, and if the parent is a CallExpression, that we're the first argument. - * @param {ASTNode} node The node to inspect the parent of. - * @returns {boolean} Was the parent a direct descendent, and is the child therefore potentially part of a dangerous argument? - * @private + * Reports if the `CallExpression` node has evaluated argument. + * @param {ASTNode} node A CallExpression to check. + * @returns {void} */ - function hasImpliedEvalParent(node) { + function reportImpliedEvalCallExpression(node) { + const [firstArgument] = node.arguments; - // make sure our parent is marked - return node.parent === last(last(impliedEvalAncestorsStack)) && + if (firstArgument) { + + const staticValue = getStaticValue(firstArgument, context.getScope()); + const isStaticString = staticValue && typeof staticValue.value === "string"; + const isString = isStaticString || isEvaluatedString(firstArgument); + + if (isString) { + context.report({ + node, + messageId: "impliedEval" + }); + } + } - // if our parent is a CallExpression, make sure we're the first argument - (node.parent.type !== "CallExpression" || node === node.parent.arguments[0]); } /** - * Checks if our parent is marked as part of an implied eval argument. If - * so, collapses the top of impliedEvalAncestorsStack and reports on the - * original CallExpression. - * @param {ASTNode} node The CallExpression to check. - * @returns {boolean} True if the node matches, false if not. - * @private + * Reports calls of `implied eval` via the global references. + * @param {Variable} globalVar A global variable to check. + * @returns {void} */ - function checkString(node) { - if (hasImpliedEvalParent(node)) { + function reportImpliedEvalViaGlobal(globalVar) { + const { references, name } = globalVar; - // remove the entire substack, to avoid duplicate reports - const substack = impliedEvalAncestorsStack.pop(); + references.forEach(ref => { + const identifier = ref.identifier; + let node = identifier.parent; - context.report({ - node: substack[0], - messageId: "impliedEval" - }); - } + while (isSpecifiedMember(node, [name])) { + node = node.parent; + } + + if (isSpecifiedMember(node, EVAL_LIKE_FUNCS)) { + const parent = node.parent; + + if (parent.type === "CallExpression" && parent.callee === node) { + reportImpliedEvalCallExpression(parent); + } + } + }); } //-------------------------------------------------------------------------- @@ -124,45 +134,17 @@ module.exports = { return { CallExpression(node) { - if (isImpliedEvalCallExpression(node)) { - - // call expressions create a new substack - impliedEvalAncestorsStack.push([node]); - } - }, - - "CallExpression:exit"(node) { - if (node === last(last(impliedEvalAncestorsStack))) { - - /* - * Destroys the entire sub-stack, rather than just using - * last(impliedEvalAncestorsStack).pop(), as a CallExpression is - * always the bottom of a impliedEvalAncestorsStack substack. - */ - impliedEvalAncestorsStack.pop(); - } - }, - - BinaryExpression(node) { - if (node.operator === "+" && hasImpliedEvalParent(node)) { - last(impliedEvalAncestorsStack).push(node); - } - }, - - "BinaryExpression:exit"(node) { - if (node === last(last(impliedEvalAncestorsStack))) { - last(impliedEvalAncestorsStack).pop(); - } - }, - - Literal(node) { - if (typeof node.value === "string") { - checkString(node); + if (isSpecifiedIdentifier(node.callee, EVAL_LIKE_FUNCS)) { + reportImpliedEvalCallExpression(node); } }, + "Program:exit"() { + const globalScope = context.getScope(); - TemplateLiteral(node) { - checkString(node); + GLOBAL_CANDIDATES + .map(candidate => astUtils.getVariableByName(globalScope, candidate)) + .filter(globalVar => !!globalVar && globalVar.defs.length === 0) + .forEach(reportImpliedEvalViaGlobal); } }; diff --git a/tests/lib/rules/no-implied-eval.js b/tests/lib/rules/no-implied-eval.js index a8790af3429..227e02b6bca 100644 --- a/tests/lib/rules/no-implied-eval.js +++ b/tests/lib/rules/no-implied-eval.js @@ -21,15 +21,62 @@ const ruleTester = new RuleTester(), ruleTester.run("no-implied-eval", rule, { valid: [ + "setTimeout();", + + { code: "setTimeout;", env: { browser: true } }, + { code: "setTimeout = foo;", env: { browser: true } }, + { code: "window.setTimeout;", env: { browser: true } }, + { code: "window.setTimeout = foo;", env: { browser: true } }, + { code: "window['setTimeout'];", env: { browser: true } }, + { code: "window['setTimeout'] = foo;", env: { browser: true } }, + { code: "global.setTimeout;", env: { node: true } }, + { code: "global.setTimeout = foo;", env: { node: true } }, + { code: "global['setTimeout'];", env: { node: true } }, + { code: "global['setTimeout'] = foo;", env: { node: true } }, + + "window.setTimeout('foo')", + "window.setInterval('foo')", + "window['setTimeout']('foo')", + "window['setInterval']('foo')", + + { code: "window.setTimeout('foo')", env: { node: true } }, + { code: "window.setInterval('foo')", env: { node: true } }, + { code: "window['setTimeout']('foo')", env: { node: true } }, + { code: "window['setInterval']('foo')", env: { node: true } }, + { code: "global.setTimeout('foo')", env: { browser: true } }, + { code: "global.setInterval('foo')", env: { browser: true } }, + { code: "global['setTimeout']('foo')", env: { browser: true } }, + { code: "global['setInterval']('foo')", env: { browser: true } }, + + { code: "window[`SetTimeOut`]('foo', 100);", parserOptions: { ecmaVersion: 6 }, env: { browser: true } }, + { code: "global[`SetTimeOut`]('foo', 100);", parserOptions: { ecmaVersion: 6 }, env: { node: true } }, + { code: "global[`setTimeout${foo}`]('foo', 100);", parserOptions: { ecmaVersion: 6 }, env: { browser: true } }, + { code: "global[`setTimeout${foo}`]('foo', 100);", parserOptions: { ecmaVersion: 6 }, env: { node: true } }, // normal usage - "setInterval(function() { x = 1; }, 100);", + "setTimeout(function() { x = 1; }, 100);", + "setInterval(function() { x = 1; }, 100)", + "execScript(function() { x = 1; }, 100)", + { code: "window.setTimeout(function() { x = 1; }, 100);", env: { browser: true } }, + { code: "window.setInterval(function() { x = 1; }, 100);", env: { browser: true } }, + { code: "window.execScript(function() { x = 1; }, 100);", env: { browser: true } }, + { code: "window.setTimeout(foo, 100);", env: { browser: true } }, + { code: "window.setInterval(foo, 100);", env: { browser: true } }, + { code: "window.execScript(foo, 100);", env: { browser: true } }, + { code: "global.setTimeout(function() { x = 1; }, 100);", env: { node: true } }, + { code: "global.setInterval(function() { x = 1; }, 100);", env: { node: true } }, + { code: "global.execScript(function() { x = 1; }, 100);", env: { node: true } }, + { code: "global.setTimeout(foo, 100);", env: { node: true } }, + { code: "global.setInterval(foo, 100);", env: { node: true } }, + { code: "global.execScript(foo, 100);", env: { node: true } }, // only checks on top-level statements or window.* "foo.setTimeout('hi')", // identifiers are fine "setTimeout(foo, 10)", + "setInterval(1, 10)", + "execScript(2)", // as are function expressions "setTimeout(function() {}, 10)", @@ -55,7 +102,16 @@ ruleTester.run("no-implied-eval", rule, { "setTimeout(function() { return 'foobar'; }, 10)", // https://github.com/eslint/eslint/issues/7821 - "setTimeoutFooBar('Foo Bar')" + "setTimeoutFooBar('Foo Bar')", + + { code: "foo.window.setTimeout('foo', 100);", env: { browser: true } }, + { code: "foo.global.setTimeout('foo', 100);", env: { node: true } }, + { code: "var window; window.setTimeout('foo', 100);", env: { browser: true } }, + { code: "var global; global.setTimeout('foo', 100);", env: { node: true } }, + { code: "function foo(window) { window.setTimeout('foo', 100); }", env: { browser: true } }, + { code: "function foo(global) { global.setTimeout('foo', 100); }", env: { node: true } }, + { code: "foo('', window.setTimeout);", env: { browser: true } }, + { code: "foo('', global.setTimeout);", env: { node: true } } ], invalid: [ @@ -64,20 +120,44 @@ ruleTester.run("no-implied-eval", rule, { { code: "setInterval(\"x = 1;\");", errors: [expectedError] }, { code: "execScript(\"x = 1;\");", errors: [expectedError] }, + { code: "const s = 'x=1'; setTimeout(s, 100);", parserOptions: { ecmaVersion: 6 }, errors: [expectedError] }, + { code: "setTimeout(String('x=1'), 100);", parserOptions: { ecmaVersion: 6 }, errors: [expectedError] }, + // member expressions - { code: "window.setTimeout('foo')", errors: [expectedError] }, - { code: "window.setInterval('foo')", errors: [expectedError] }, - { code: "window['setTimeout']('foo')", errors: [expectedError] }, - { code: "window['setInterval']('foo')", errors: [expectedError] }, + { code: "window.setTimeout('foo')", env: { browser: true }, errors: [expectedError] }, + { code: "window.setInterval('foo')", env: { browser: true }, errors: [expectedError] }, + { code: "window['setTimeout']('foo')", env: { browser: true }, errors: [expectedError] }, + { code: "window['setInterval']('foo')", env: { browser: true }, errors: [expectedError] }, + { code: "window[`setInterval`]('foo')", parserOptions: { ecmaVersion: 6 }, env: { browser: true }, errors: [expectedError] }, + { code: "window.window['setInterval']('foo')", env: { browser: true }, errors: [expectedError] }, + { code: "global.setTimeout('foo')", env: { node: true }, errors: [expectedError] }, + { code: "global.setInterval('foo')", env: { node: true }, errors: [expectedError] }, + { code: "global['setTimeout']('foo')", env: { node: true }, errors: [expectedError] }, + { code: "global['setInterval']('foo')", env: { node: true }, errors: [expectedError] }, + { code: "global[`setInterval`]('foo')", parserOptions: { ecmaVersion: 6 }, env: { node: true }, errors: [expectedError] }, + { code: "global.global['setInterval']('foo')", env: { node: true }, errors: [expectedError] }, // template literals { code: "setTimeout(`foo${bar}`)", parserOptions: { ecmaVersion: 6 }, errors: [expectedError] }, + { code: "window.setTimeout(`foo${bar}`)", parserOptions: { ecmaVersion: 6 }, env: { browser: true }, errors: [expectedError] }, + { code: "window.window.setTimeout(`foo${bar}`)", parserOptions: { ecmaVersion: 6 }, env: { browser: true }, errors: [expectedError] }, + { code: "global.global.setTimeout(`foo${bar}`)", parserOptions: { ecmaVersion: 6 }, env: { node: true }, errors: [expectedError] }, // string concatination { code: "setTimeout('foo' + bar)", errors: [expectedError] }, { code: "setTimeout(foo + 'bar')", errors: [expectedError] }, { code: "setTimeout(`foo` + bar)", parserOptions: { ecmaVersion: 6 }, errors: [expectedError] }, { code: "setTimeout(1 + ';' + 1)", errors: [expectedError] }, + { code: "window.setTimeout('foo' + bar)", env: { browser: true }, errors: [expectedError] }, + { code: "window.setTimeout(foo + 'bar')", env: { browser: true }, errors: [expectedError] }, + { code: "window.setTimeout(`foo` + bar)", parserOptions: { ecmaVersion: 6 }, env: { browser: true }, errors: [expectedError] }, + { code: "window.setTimeout(1 + ';' + 1)", env: { browser: true }, errors: [expectedError] }, + { code: "window.window.setTimeout(1 + ';' + 1)", env: { browser: true }, errors: [expectedError] }, + { code: "global.setTimeout('foo' + bar)", env: { node: true }, errors: [expectedError] }, + { code: "global.setTimeout(foo + 'bar')", env: { node: true }, errors: [expectedError] }, + { code: "global.setTimeout(`foo` + bar)", parserOptions: { ecmaVersion: 6 }, env: { node: true }, errors: [expectedError] }, + { code: "global.setTimeout(1 + ';' + 1)", env: { node: true }, errors: [expectedError] }, + { code: "global.global.setTimeout(1 + ';' + 1)", env: { node: true }, errors: [expectedError] }, // gives the correct node when dealing with nesting { @@ -94,6 +174,52 @@ ruleTester.run("no-implied-eval", rule, { line: 1 }, + // no error on line 2 + { + messageId: "impliedEval", + type: "CallExpression", + line: 3 + } + ] + }, + { + code: + "window.setTimeout('foo' + (function() {\n" + + " setTimeout(helper);\n" + + " window.execScript('str');\n" + + " return 'bar';\n" + + "})())", + env: { browser: true }, + errors: [ + { + messageId: "impliedEval", + type: "CallExpression", + line: 1 + }, + + // no error on line 2 + { + messageId: "impliedEval", + type: "CallExpression", + line: 3 + } + ] + }, + { + code: + "global.setTimeout('foo' + (function() {\n" + + " setTimeout(helper);\n" + + " global.execScript('str');\n" + + " return 'bar';\n" + + "})())", + env: { node: true }, + errors: [ + { + messageId: "impliedEval", + type: "CallExpression", + line: 1 + }, + // no error on line 2 { messageId: "impliedEval",