From 5376d14e915ab22bfc81bcbd38c779b181a9ff72 Mon Sep 17 00:00:00 2001 From: Frederik Braun Date: Tue, 11 Oct 2022 13:12:51 +0200 Subject: [PATCH 1/3] Fix #207: Chase right-hand side of AssignmentExpression in normalizeMethodCall --- lib/ruleHelper.js | 3 +++ tests/rules/method.js | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/ruleHelper.js b/lib/ruleHelper.js index e711bc4..03f012a 100644 --- a/lib/ruleHelper.js +++ b/lib/ruleHelper.js @@ -256,6 +256,9 @@ RuleHelper.prototype = { case "ArrowFunctionExpression": methodName = ""; break; + case "AssignmentExpression": + methodName = this.normalizeMethodCall(node.right); + break; case "Import": methodName = "import"; break; diff --git a/tests/rules/method.js b/tests/rules/method.js index ca038d8..f284226 100644 --- a/tests/rules/method.js +++ b/tests/rules/method.js @@ -360,7 +360,10 @@ eslintTester.run("method", rule, { }, { code: "x.setHTML(evil, { sanitizer: new Sanitizer()})" - } + }, + { + code: "(info.current = type)(child_ctx)", + }, ], // Examples of code that should trigger the rule From 5f0b4a421e2e9feb409fa20f1e0a46ec0e674c92 Mon Sep 17 00:00:00 2001 From: Frederik Braun Date: Fri, 14 Oct 2022 09:17:12 +0200 Subject: [PATCH 2/3] Add more test case variations --- tests/rules/method.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/rules/method.js b/tests/rules/method.js index f284226..964d37a 100644 --- a/tests/rules/method.js +++ b/tests/rules/method.js @@ -364,6 +364,9 @@ eslintTester.run("method", rule, { { code: "(info.current = type)(child_ctx)", }, + { + code: "(info.current = n.insertAdjacentHTML)('beforebegin', 'innocent')", + } ], // Examples of code that should trigger the rule @@ -949,6 +952,14 @@ eslintTester.run("method", rule, { ], parserOptions: { ecmaVersion: 6 } }, - + { + code: "(info.current = n.insertAdjacentHTML)('beforebegin', c)", + errors: [ + { + message: /Unsafe call to n.insertAdjacentHTML for argument 1/, + type: "CallExpression" + } + ], + }, ] }); From e17643310820ed65b72e3510b67b132f8e62aa1b Mon Sep 17 00:00:00 2001 From: Frederik Braun Date: Tue, 1 Nov 2022 14:49:41 +0100 Subject: [PATCH 3/3] wip: issue #210 and normalizeMethodcall --- lib/ruleHelper.js | 28 +++++++++++++++++++++++- tests/rules/method.js | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/lib/ruleHelper.js b/lib/ruleHelper.js index 03f012a..a859ce8 100644 --- a/lib/ruleHelper.js +++ b/lib/ruleHelper.js @@ -243,6 +243,19 @@ RuleHelper.prototype = { normalizeMethodCall(node) { let methodName; let objectName; + + // operators that allow us to only inspect the right side of the assignment: + const RIGHT_OPERATORS = ["="]; + + // operators where we can skip analysis: + // e.g., there's no point in doing a check if + // this ends up doing a mathematical operation. The result is not going to be a callable + const MATH_OPERATORS = ["+=", "-=", "*=", "/=", "%=", "**=", "<<=", ">>=", + ">>>=", "&=", "|=", "^="]; + + // operators where we don"t know the result of a logical expression, which may result in the code executing the left or the right part of the assignmentcan check the left bit: + const LOGICAL_OPERATOS = ["||=", "&&=", "??="]; + switch (node.type) { case "Identifier": methodName = node.name; @@ -257,7 +270,20 @@ RuleHelper.prototype = { methodName = ""; break; case "AssignmentExpression": - methodName = this.normalizeMethodCall(node.right); + if (RIGHT_OPERATORS.includes(node.operator)) { + methodName = this.normalizeMethodCall(node.right); + } else if (MATH_OPERATORS.includes(node.operator)) { + methodName = ""; + break; + } else if (LOGICAL_OPERATOS.includes(node.operator)) { + // Issue #210: oh, no! we have two methods names we need to check, but this function + // may only return one. it's not immediately clear how to reconcile that with existing callers of normalizeMethodCall. + methodName = ""; + } + else { + // this is the forcing function for us to complain about and implement support if new JS operators come up. + this.reportUnsupported(node, "Unexpected callable", `unexpected assignment with operator '${node.operator}' in normalizeMethodCall`); + } break; case "Import": methodName = "import"; diff --git a/tests/rules/method.js b/tests/rules/method.js index 964d37a..2ff01a1 100644 --- a/tests/rules/method.js +++ b/tests/rules/method.js @@ -366,7 +366,14 @@ eslintTester.run("method", rule, { }, { code: "(info.current = n.insertAdjacentHTML)('beforebegin', 'innocent')", + }, + { + code: "(false ||= n.insertAdjacentHTML)('beforebegin', 'innocent')", + }, + { + code: "(n.insertAdjacentHTML &&= false)('beforebegin', 'innocent')", } + ], // Examples of code that should trigger the rule @@ -961,5 +968,49 @@ eslintTester.run("method", rule, { } ], }, + { + + // The issue with this testcase is, that it might not actually lead to a call to insertAdjacentHTML. + code: "(false ||= n.insertAdjacentHTML)('beforebegin', evil)", + errors: [ + { + message: /Unsafe call to n.insertAdjacentHTML for argument 1/, + type: "CallExpression" + } + ], + }, + { + + // The issue with this testcase is, that it might not actually lead to a call to insertAdjacentHTML. + code: "(n.insertAdjacentHTML ||= false)('beforebegin', evil)", + errors: [ + { + message: /Unsafe call to n.insertAdjacentHTML for argument 1/, + type: "CallExpression" + } + ], + }, + { + + // The issue with this testcase is, that it might not actually lead to a call to insertAdjacentHTML. + code: "(false &&= n.insertAdjacentHTML)('beforebegin', evil)", + errors: [ + { + message: /Unsafe call to n.insertAdjacentHTML for argument 1/, + type: "CallExpression" + } + ], + }, + { + + // The issue with this testcase is, that it might not actually lead to a call to insertAdjacentHTML. + code: "(n.insertAdjacentHTML &&= false)('beforebegin', evil)", + errors: [ + { + message: /Unsafe call to n.insertAdjacentHTML for argument 1/, + type: "CallExpression" + } + ], + } ] });