Merge pull request #507 from esben-semmle/js/mixed-static-intance-this-access-inheritance

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -58,6 +58,7 @@
 | Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
 | Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. |
 | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
+| Wrong use of 'this' for static method | More results, fewer false-positive results | This rule now recognizes inherited methods. |
 
 ## Changes to QL libraries
 

javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql

@@ -10,15 +10,19 @@
 */
 import javascript
 
+/** Holds if `base` declares or inherits method `m` with the given `name`. */
+predicate hasMethod(ClassDefinition base, string name, MethodDefinition m) {
+  m = base.getMethod(name) or
+  hasMethod(base.getSuperClassDefinition(), name, m)
+}
 
 /**
 * Holds if `access` is in`fromMethod`, and it references `toMethod` through `this`.
 */
 predicate isLocalMethodAccess(PropAccess access, MethodDefinition fromMethod, MethodDefinition toMethod) {
-    fromMethod.getDeclaringClass() = toMethod.getDeclaringClass() and
+    hasMethod(fromMethod.getDeclaringClass(), access.getPropertyName(), toMethod) and
     access.getEnclosingFunction() = fromMethod.getBody() and
-    access.getBase() instanceof ThisExpr and
-    access.getPropertyName() = toMethod.getName()
+    access.getBase() instanceof ThisExpr
 }
 
 string getKind(MethodDefinition m) {

javascript/ql/src/semmle/javascript/Classes.qll

@@ -216,6 +216,13 @@ class ClassDefinition extends @classdefinition, ClassOrInterface, AST::ValueNode
     )
   }
 
+  /**
+   * Gets the definition of the super class of this class, if it can be determined.
+   */
+  ClassDefinition getSuperClassDefinition() {
+    result = getSuperClass().analyze().getAValue().(AbstractClass).getClass()
+  }
+
 }
 
 /**

javascript/ql/test/query-tests/Declarations/MixedStaticInstanceThisAccess/MixedStaticInstanceThisAccess.expected

@@ -1,2 +1,3 @@
 | instanceStatic.js:3:9:3:16 | this.baz | Access to instance method $@ from static method $@ is not possible through `this`. | instanceStatic.js:5:5:7:5 | baz(){\\n\\n    } | baz | instanceStatic.js:2:5:4:5 | static  ... K\\n    } | bar |
 | staticInstance.js:3:9:3:16 | this.baz | Access to static method $@ from instance method $@ is not possible through `this`. | staticInstance.js:5:5:6:5 | static baz(){\\n    } | baz | staticInstance.js:2:5:4:5 | bar(){\\n ... K\\n    } | bar |
+| tst.js:66:9:66:14 | this.f | Access to instance method $@ from static method $@ is not possible through `this`. | tst.js:60:5:62:5 | f() {\\n\\n    } | f | tst.js:65:5:67:5 | static  ... K\\n    } | test |

javascript/ql/test/query-tests/Declarations/MixedStaticInstanceThisAccess/tst.js

@@ -41,3 +41,28 @@ class C4 {
     }
 }
 C4.f = x;
+
+class C5_super {
+    f() {
+
+    }
+}
+class C5 extends C5_super{
+    static f() {
+
+    }
+    test() {
+        this.f; // OK
+    }
+}
+
+class C6_super {
+    f() {
+
+    }
+}
+class C6 extends C6_super{
+    static test() {
+        this.f; // NOT OK
+    }
+}