Merge pull request github#16136 from asgerf/js/instance-to-subclasses

JS: Make getInstance() propagate to subclasses

Author: asgerf

docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst

@@ -478,7 +478,7 @@ The following components are supported:
 - **Element** selects an element of an array, iterator, or set object.
 - **MapValue** selects a value of a map object.
 - **Awaited** selects the value of a promise.
-- **Instance** selects instances of a class.
+- **Instance** selects instances of a class, including instances of its subclasses.
 - **Fuzzy** selects all values that are derived from the current value through a combination of the other operations described in this list.
   For example, this can be used to find all values that appear to originate from a particular package. This can be useful for finding method calls
   from a known package, but where the receiver type is not known or is difficult to model.

javascript/ql/lib/semmle/javascript/ApiGraphs.qll

@@ -241,15 +241,23 @@ module API {
     }
 
     /**
-     * Gets a node representing an instance of this API component, that is, an object whose
-     * constructor is the function represented by this node.
+     * Gets a node representing an instance of the class represented by this node.
+     * This includes instances of subclasses.
      *
-     * For example, if this node represents a use of some class `A`, then there might be a node
-     * representing instances of `A`, typically corresponding to expressions `new A()` at the
-     * source level.
+     * For example:
+     * ```js
+     * import { C } from "foo";
+     *
+     * new C(); // API::moduleImport("foo").getMember("C").getInstance()
+     *
+     * class D extends C {
+     *   m() {
+     *     this; // API::moduleImport("foo").getMember("C").getInstance()
+     *  }
+     * }
      *
-     * This predicate may have multiple results when there are multiple constructor calls invoking this API component.
-     * Consider using `getAnInstantiation()` if there is a need to distinguish between individual constructor calls.
+     * new D(); // API::moduleImport("foo").getMember("C").getInstance()
+     * ```
      */
     cached
     Node getInstance() {
@@ -891,6 +899,17 @@ module API {
       (propDesc = Promises::errorProp() or propDesc = "")
     }
 
+    pragma[nomagic]
+    private DataFlow::ClassNode getALocalSubclass(DataFlow::SourceNode node) {
+      result.getASuperClassNode().getALocalSource() = node
+    }
+
+    bindingset[node]
+    pragma[inline_late]
+    private DataFlow::ClassNode getALocalSubclassFwd(DataFlow::SourceNode node) {
+      result = getALocalSubclass(node)
+    }
+
     /**
      * Holds if `ref` is a use of a node that should have an incoming edge from `base` labeled
      * `lbl` in the API graph.
@@ -927,6 +946,15 @@ module API {
           or
           lbl = Label::forwardingFunction() and
           DataFlow::functionForwardingStep(pred.getALocalUse(), ref)
+          or
+          exists(DataFlow::ClassNode cls |
+            lbl = Label::instance() and
+            cls = getALocalSubclassFwd(pred).getADirectSubClass*()
+          |
+            ref = cls.getAReceiverNode()
+            or
+            ref = cls.getAClassReference().getAnInstantiation()
+          )
         )
         or
         exists(DataFlow::Node def, DataFlow::FunctionNode fn |

javascript/ql/src/change-notes/2024-04-08-instance-to-subclasses.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* `API::Node#getInstance()` now includes instances of subclasses, include transitive subclasses.
+  The same changes applies to uses of the `Instance` token in data extensions.

javascript/ql/test/library-tests/frameworks/data/test.expected

@@ -74,6 +74,10 @@ taintFlow
 | test.js:249:28:249:35 | source() | test.js:249:28:249:35 | source() |
 | test.js:252:15:252:22 | source() | test.js:252:15:252:22 | source() |
 | test.js:254:32:254:39 | source() | test.js:254:32:254:39 | source() |
+| test.js:262:10:262:31 | this.ba ... ource() | test.js:262:10:262:31 | this.ba ... ource() |
+| test.js:265:6:265:39 | new MyS ... ource() | test.js:265:6:265:39 | new MyS ... ource() |
+| test.js:269:10:269:31 | this.ba ... ource() | test.js:269:10:269:31 | this.ba ... ource() |
+| test.js:272:6:272:40 | new MyS ... ource() | test.js:272:6:272:40 | new MyS ... ource() |
 isSink
 | test.js:54:18:54:25 | source() | test-sink |
 | test.js:55:22:55:29 | source() | test-sink |

javascript/ql/test/library-tests/frameworks/data/test.js

@@ -256,3 +256,17 @@ function fuzzy() {
   fuzzyCall(source()); // OK - does not come from 'testlib'
   require('blah').fuzzyCall(source()); // OK - does not come from 'testlib'
 }
+
+class MySubclass extends testlib.BaseClass {
+  foo() {
+    sink(this.baseclassSource()); // NOT OK
+  }
+}
+sink(new MySubclass().baseclassSource()); // NOT OK
+
+class MySubclass2 extends MySubclass {
+  foo2() {
+    sink(this.baseclassSource()); // NOT OK
+  }
+}
+sink(new MySubclass2().baseclassSource()); // NOT OK

javascript/ql/test/library-tests/frameworks/data/test.ql

@@ -80,6 +80,7 @@ class Sources extends ModelInput::SourceModelCsv {
         "testlib;Member[ParamDecoratorSource].DecoratedParameter;test-source",
         "testlib;Member[MethodDecorator].DecoratedMember.Parameter[0];test-source",
         "testlib;Member[MethodDecoratorWithArgs].ReturnValue.DecoratedMember.Parameter[0];test-source",
+        "testlib;Member[BaseClass].Instance.Member[baseclassSource].ReturnValue;test-source",
       ]
   }
 }