Merge pull request #1316 from asger-semmle/incorrect-suffix-check-fps

Approved by esben-semmle, xiemaisi

Author: semmle-qlci

change-notes/1.21/analysis-javascript.md

@@ -41,6 +41,7 @@
 | Type confusion through parameter tampering | Fewer false-positive results | This rule now recognizes additional emptiness checks. |
 | Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
 | Unreachable statement | Unreachable throws no longer give an alert | This ignores unreachable throws, as they could be intentional (for example, to placate the TS compiler). |
+| Incorrect suffix check | Fewer false-positive results | This rule now recognizes valid checks in more cases. |
 
 ## Changes to QL libraries
 

javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql

@@ -139,6 +139,16 @@ class UnsafeIndexOfComparison extends EqualityTest {
         not test.isInclusive() and
         value = -1
       )
+    ) and
+    // Check for indexOf being <0, or <=-1
+    not exists(RelationalComparison test |
+      test.getLesserOperand() = indexOf.getAnEquivalentIndexOfCall().getAUse() and
+      exists(int value | value = test.getGreaterOperand().getIntValue() |
+        value < 0
+        or
+        not test.isInclusive() and
+        value = 0
+      )
     )
   }
 

javascript/ql/test/query-tests/Security/CWE-020/tst.js

@@ -79,3 +79,13 @@ function withIndexOfCheckBad(x, y) {
 function plus(x, y) {
   return x.indexOf("." + y) === x.length - (y.length + 1); // NOT OK
 }
+
+function withIndexOfCheckLower(x, y) {
+  let index = x.indexOf(y);
+  return !(index < 0) && index === x.length - y.length - 1; // OK
+}
+
+function withIndexOfCheckLowerEq(x, y) {
+  let index = x.indexOf(y);
+  return !(index <= -1) && index === x.length - y.length - 1; // OK
+}