diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 814572c5def..e2f044d6e25 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -15,9 +15,10 @@ | **Query** | **Tags** | **Purpose** | |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | -| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | -| Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | +| Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | +| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | +| Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/config/suites/javascript/correctness-more b/javascript/config/suites/javascript/correctness-more index 4cdb56ddaf1..10aa1dcf796 100644 --- a/javascript/config/suites/javascript/correctness-more +++ b/javascript/config/suites/javascript/correctness-more @@ -5,6 +5,7 @@ + semmlecode-javascript-queries/Expressions/SuspiciousInvocation.ql: /Correctness/Expressions + semmlecode-javascript-queries/Expressions/SuspiciousPropAccess.ql: /Correctness/Expressions + semmlecode-javascript-queries/Expressions/UnboundEventHandlerReceiver.ql: /Correctness/Expressions ++ semmlecode-javascript-queries/Expressions/UnclearOperatorPrecedence.ql: /Correctness/Expressions + semmlecode-javascript-queries/Expressions/UnknownDirective.ql: /Correctness/Expressions + semmlecode-javascript-queries/Expressions/WhitespaceContradictsPrecedence.ql: /Correctness/Expressions + semmlecode-javascript-queries/LanguageFeatures/IllegalInvocation.ql: /Correctness/Language Features diff --git a/javascript/ql/src/Expressions/UnclearOperatorPrecedence.qhelp b/javascript/ql/src/Expressions/UnclearOperatorPrecedence.qhelp new file mode 100644 index 00000000000..1601fe08461 --- /dev/null +++ b/javascript/ql/src/Expressions/UnclearOperatorPrecedence.qhelp @@ -0,0 +1,46 @@ + + + + +

+Nested expressions that rely on less well-known operator precedence rules can be +hard to read and understand. They could even indicate a bug where the author of the +code misunderstood the precedence rules. +

+ + + +

+Use parentheses or additional whitespace to clarify grouping. +

+ + + +

+Consider the following snippet of code: +

+ + + +

+It might look like this tests whether x and y have any bits in +common, but in fact == binds more tightly than &, so the test +is equivalent to x & (y == 0). +

+ +

+If this is the intended interpretation, parentheses should be used to clarify this. You could +also consider adding extra whitespace around & or removing whitespace +around == to make it visually apparent that it binds less tightly: +x & y==0. +

+

+Probably the best approach in this case, though, would be to use the && +operator instead to clarify the intended interpretation: x && y == 0. +

+ + + +
  • Mozilla Developer Network, Operator precedence.
    • +     +     diff --git a/javascript/ql/src/Expressions/UnclearOperatorPrecedence.ql b/javascript/ql/src/Expressions/UnclearOperatorPrecedence.ql new file mode 100644 index 00000000000..7438835d794 --- /dev/null +++ b/javascript/ql/src/Expressions/UnclearOperatorPrecedence.ql @@ -0,0 +1,29 @@ +/** + * @name Unclear precedence of nested operators + * @description Nested expressions involving binary bitwise operators and comparisons are easy + * to misunderstand without additional disambiguating parentheses or whitespace. + * @kind problem + * @problem.severity recommendation + * @id js/unclear-operator-precedence + * @tags maintainability + * correctness + * statistical + * non-attributable + * external/cwe/cwe-783 + * @precision very-high + */ + +import javascript + +from BitwiseBinaryExpr bit, Comparison rel, Expr other +where bit.hasOperands(rel, other) and + // only flag if whitespace doesn't clarify the nesting (note that if `bit` has less + // whitespace than `rel`, it will be reported by `js/whitespace-contradicts-precedence`) + bit.getWhitespaceAroundOperator() = rel.getWhitespaceAroundOperator() and + // don't flag if the other operand is itself a comparison, + // since the nesting tends to be visually more obvious in such cases + not other instanceof Comparison and + // don't flag occurrences in minified code + not rel.getTopLevel().isMinified() +select rel, "The '" + rel.getOperator() + "' operator binds more tightly than " + + "'" + bit.getOperator() + "', which may not be obvious in this case." \ No newline at end of file diff --git a/javascript/ql/src/Expressions/examples/UnclearOperatorPrecedence.js b/javascript/ql/src/Expressions/examples/UnclearOperatorPrecedence.js new file mode 100644 index 00000000000..679708e4f18 --- /dev/null +++ b/javascript/ql/src/Expressions/examples/UnclearOperatorPrecedence.js @@ -0,0 +1,3 @@ +if (x & y == 0) { + // ... +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.expected b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.expected new file mode 100644 index 00000000000..7a5678eb017 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.expected @@ -0,0 +1,3 @@ +| tst.js:1:9:1:17 | 0x0A != 0 | The '!=' operator binds more tightly than '&', which may not be obvious in this case. | +| tst.js:6:1:6:7 | x !== y | The '!==' operator binds more tightly than '&', which may not be obvious in this case. | +| tst.js:10:3:10:6 | b==c | The '==' operator binds more tightly than '&', which may not be obvious in this case. | diff --git a/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.qlref b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.qlref new file mode 100644 index 00000000000..5a9a25e1118 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.qlref @@ -0,0 +1 @@ +Expressions/UnclearOperatorPrecedence.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.js b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.js new file mode 100644 index 00000000000..5490b0b4232 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.js @@ -0,0 +1,10 @@ +x.f() & 0x0A != 0; // NOT OK +x.f() & (0x0A != 0); // OK +x.f() & 0x0A != 0; // OK +x.f() & 0x0A!=0; // OK + +x !== y & 1; // NOT OK + +x > 0 & x < 10; // OK + +a&b==c; // NOT OK diff --git a/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.min.js b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.min.js new file mode 100644 index 00000000000..800b7b08df2 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.min.js @@ -0,0 +1 @@ +a&b==c; // OK (minified file)