JS: address comments

Author: asger-semmle

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

@@ -8,7 +8,7 @@
 
         The <code>indexOf</code> and <code>lastIndexOf</code> methods are sometimes used to check
         if a substring occurs at a certain position in a string. However, if the returned index
-        is compared to an expression that might evaluate to -1, the check can may pass in some
+        is compared to an expression that might evaluate to -1, the check may pass in some
         cases where the substring was not found at all.
 
         </p>
@@ -26,7 +26,7 @@
 
             Use <code>String.prototype.endsWith</code> if it is available.
             Otherwise, explicitly handle the -1 case, either by checking the relative
-            lengths of the strings, or check if the returned index is -1.
+            lengths of the strings, or by checking if the returned index is -1.
 
         </p>
 
@@ -47,7 +47,7 @@
 
             However, if <code>y</code> is one character longer than <code>x</code>, the right-hand side
             <code>x.length - y.length</code> becomes -1, which then equals the return value
-            of <code>lastIndexOf</code>. This will make the test pass, evne though <code>x</code> does not
+            of <code>lastIndexOf</code>. This will make the test pass, even though <code>x</code> does not
             end with <code>y</code>.
 
         </p>

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

@@ -36,10 +36,17 @@ class IndexOfCall extends DataFlow::MethodCallNode {
     result.getArgument(0).getALocalSource() = this.getArgument(0).getALocalSource() and
     result.getMethodName() = this.getMethodName()
   }
+
+  /**
+   * Gets an expression that refers to the return value of this call. 
+   */
+  Expr getAUse() {
+    this.flowsToExpr(result)
+  }
 }
 
 /**
- * Gets a source of the given string value.
+ * Gets a source of the given string value, or one of its operands if it is a concatenation.
  */
 DataFlow::SourceNode getStringSource(DataFlow::Node node) {
   result = node.getALocalSource()
@@ -65,7 +72,7 @@ class LiteralLengthExpr extends DotExpr {
 }
 
 /**
- * Holds if `node` is derived from the length of the given `indexOf`-operand.
+ * Holds if `length` is derived from the length of the given `indexOf`-operand.
  */
 predicate isDerivedFromLength(DataFlow::Node length, DataFlow::Node operand) {
   exists (IndexOfCall call | operand = call.getAnOperand() |
@@ -84,9 +91,7 @@ predicate isDerivedFromLength(DataFlow::Node length, DataFlow::Node operand) {
       length = lengthExpr.flow())
   )
   or
-  exists (DataFlow::Node mid |
-    isDerivedFromLength(mid, operand) and
-    length = mid.getASuccessor())
+  isDerivedFromLength(length.getAPredecessor(), operand)
   or
   exists (SubExpr sub |
     isDerivedFromLength(sub.getAnOperand().flow(), operand) and
@@ -101,7 +106,7 @@ class UnsafeIndexOfComparison extends EqualityTest {
   DataFlow::Node testedValue;
 
   UnsafeIndexOfComparison() {
-    hasOperands(indexOf.asExpr(), testedValue.asExpr()) and
+    hasOperands(indexOf.getAUse(), testedValue.asExpr()) and
     isDerivedFromLength(testedValue, indexOf.getReceiver()) and
     isDerivedFromLength(testedValue, indexOf.getArgument(0)) and
 
@@ -118,13 +123,13 @@ class UnsafeIndexOfComparison extends EqualityTest {
 
     // Check for indexOf being -1
     not exists (EqualityTest test, Expr minusOne |
-      test.hasOperands(indexOf.getAnEquivalentIndexOfCall().asExpr(), minusOne) and
+      test.hasOperands(indexOf.getAnEquivalentIndexOfCall().getAUse(), minusOne) and
       minusOne.getIntValue() = -1
     ) and
 
     // Check for indexOf being >1, or >=0, etc
     not exists (RelationalComparison test |
-      test.getGreaterOperand() = indexOf.getAnEquivalentIndexOfCall().asExpr() and
+      test.getGreaterOperand() = indexOf.getAnEquivalentIndexOfCall().getAUse() and
       exists (int value | value = test.getLesserOperand().getIntValue() |
         value >= 0
         or

javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected

@@ -7,3 +7,4 @@
 | tst.js:39:10:39:49 | x.lastI ... .length | This suffix check is missing a length comparison to correctly handle lastIndexOf returning -1. |
 | tst.js:55:32:55:71 | x.index ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
 | tst.js:67:32:67:71 | x.index ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| tst.js:76:25:76:57 | index = ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |

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

@@ -70,3 +70,8 @@ function indexOfCheckBad(x, y) {
 function endsWithSlash(x) {
   return x.indexOf("/") === x.length - 1; // OK - even though it also matches the empty string
 }
+
+function withIndexOfCheckBad(x, y) {
+  let index = x.indexOf(y);
+  return index !== 0 && index === x.length - y.length - 1; // NOT OK
+}