JS: Use VariableOrThis in variable capture as well

Author: asgerf

javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll

@@ -1,6 +1,7 @@
 private import javascript
 private import semmle.javascript.frameworks.data.internal.ApiGraphModels as ApiGraphModels
 private import semmle.javascript.dataflow.internal.FlowSummaryPrivate as FlowSummaryPrivate
+private import semmle.javascript.dataflow.internal.VariableOrThis
 private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
 
 module Private {
@@ -75,7 +76,7 @@ module Private {
     MkIteratorError() or
     MkPromiseValue() or
     MkPromiseError() or
-    MkCapturedContent(LocalVariable v) { v.isCaptured() }
+    MkCapturedContent(LocalVariableOrThis v) { v.isCaptured() }
 
   cached
   newtype TContentSet =
@@ -163,7 +164,7 @@ module Public {
     int asArrayIndex() { result = this.asPropertyName().(PropertyName).asArrayIndex() }
 
     /** Gets the captured variable represented by this content, if any. */
-    LocalVariable asCapturedVariable() { this = MkCapturedContent(result) }
+    LocalVariableOrThis asCapturedVariable() { this = MkCapturedContent(result) }
 
     /** Holds if this represents values stored at an unknown array index. */
     predicate isUnknownArrayElement() { this = MkArrayElementUnknown() }

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -1022,7 +1022,7 @@ private predicate isBlockedLegacyNode(Node node) {
   // Note that some variables, such as top-level variables, are still modelled with these nodes (which will result in jump steps).
   exists(LocalVariable variable |
     node = TCapturedVariableNode(variable) and
-    variable instanceof VariableCaptureConfig::CapturedVariable
+    variable = any(VariableCaptureConfig::CapturedVariable v).asLocalVariable()
   )
   or
   legacyBarrier(node)
@@ -1230,7 +1230,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
     c = ContentSet::arrayElement()
   )
   or
-  exists(LocalVariable variable |
+  exists(LocalVariableOrThis variable |
     VariableCaptureOutput::readStep(getClosureNode(node1), variable, getClosureNode(node2)) and
     c.asSingleton() = MkCapturedContent(variable)
   )
@@ -1349,7 +1349,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
     c = ContentSet::promiseValue()
   )
   or
-  exists(LocalVariable variable |
+  exists(LocalVariableOrThis variable |
     VariableCaptureOutput::storeStep(getClosureNode(node1), variable, getClosureNode(node2)) and
     c.asSingleton() = MkCapturedContent(variable)
   )

javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll

@@ -1,5 +1,6 @@
 private import javascript as js
 private import semmle.javascript.dataflow.internal.DataFlowNode
+private import semmle.javascript.dataflow.internal.VariableOrThis
 private import codeql.dataflow.VariableCapture
 private import semmle.javascript.dataflow.internal.sharedlib.DataFlowImplCommon as DataFlowImplCommon
 
@@ -51,7 +52,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
     )
   }
 
-  class CapturedVariable extends js::LocalVariable {
+  class CapturedVariable extends LocalVariableOrThis {
     CapturedVariable() {
       DataFlowImplCommon::forceCachingInSameStage() and
       this.isCaptured() and
@@ -63,7 +64,9 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
 
   additional predicate captures(js::Function fun, CapturedVariable variable) {
     (
-      variable.getAnAccess().getContainer().getFunctionBoundary() = fun
+      variable.asLocalVariable().getAnAccess().getContainer().getFunctionBoundary() = fun
+      or
+      variable.getAThisUse().getUseContainer() = fun
       or
       exists(js::Function inner |
         captures(inner, variable) and
@@ -122,7 +125,8 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
   private predicate isCapturedByOwnInitializer(js::VariableDeclarator decl) {
     exists(js::Function function |
       function = getACapturingFunctionInTree(decl.getInit()) and
-      captures(function, decl.getBindingPattern().(js::VarDecl).getVariable())
+      captures(function,
+        LocalVariableOrThis::variable(decl.getBindingPattern().(js::VarDecl).getVariable()))
     )
   }
 
@@ -141,7 +145,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
   }
 
   class CapturedParameter extends CapturedVariable {
-    CapturedParameter() { this.isParameter() }
+    CapturedParameter() { this.asLocalVariable().isParameter() or exists(this.asThisContainer()) }
   }
 
   class Expr extends js::AST::ValueNode {
@@ -152,10 +156,10 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
     }
   }
 
-  class VariableRead extends Expr instanceof js::VarAccess, js::RValue {
+  class VariableRead extends Expr instanceof js::ControlFlowNode {
     private CapturedVariable variable;
 
-    VariableRead() { this = variable.getAnAccess() }
+    VariableRead() { this = variable.getAUse() }
 
     CapturedVariable getVariable() { result = variable }
   }
@@ -178,7 +182,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
   private newtype TVariableWrite =
     MkExplicitVariableWrite(js::VarRef pattern) {
       exists(js::DataFlow::lvalueNodeInternal(pattern)) and
-      pattern.getVariable() instanceof CapturedVariable
+      any(CapturedVariable v).asLocalVariable() = pattern.getVariable()
     } or
     MkImplicitVariableInit(CapturedVariable v) { not v instanceof CapturedParameter }
 
@@ -200,7 +204,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
 
     ExplicitVariableWrite() { this = MkExplicitVariableWrite(pattern) }
 
-    override CapturedVariable getVariable() { result = pattern.getVariable() }
+    override CapturedVariable getVariable() { result.asLocalVariable() = pattern.getVariable() }
 
     override string toString() { result = pattern.toString() }
 
@@ -248,7 +252,9 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
     override predicate hasCfgNode(BasicBlock bb, int i) {
       // 'i' would normally be bound to 0, but we lower it to -1 so FunctionDeclStmts can be evaluated
       // at index 0.
-      any(js::SsaImplicitInit def).definesAt(bb, _, variable) and i = -1
+      any(js::SsaImplicitInit def).definesAt(bb, _, variable.asLocalVariable()) and i = -1
+      or
+      bb.(js::EntryBasicBlock).getContainer() = variable.asThisContainer() and i = -1
     }
   }
 
@@ -266,7 +272,13 @@ module VariableCaptureOutput = Flow<js::DbLocation, VariableCaptureConfig>;
 js::DataFlow::Node getNodeFromClosureNode(VariableCaptureOutput::ClosureNode node) {
   result = TValueNode(node.(VariableCaptureOutput::ExprNode).getExpr())
   or
-  result = TValueNode(node.(VariableCaptureOutput::ParameterNode).getParameter().getADeclaration()) // TODO: is this subsumed by the ExprNode case?
+  result =
+    TValueNode(node.(VariableCaptureOutput::ParameterNode)
+          .getParameter()
+          .asLocalVariable()
+          .getADeclaration()) // TODO: is this subsumed by the ExprNode case?
+  or
+  result = TThisNode(node.(VariableCaptureOutput::ParameterNode).getParameter().asThisContainer())
   or
   result = TExprPostUpdateNode(node.(VariableCaptureOutput::ExprPostUpdateNode).getExpr())
   or

javascript/ql/test/library-tests/TripleDot/useuse.js

@@ -75,21 +75,22 @@ function t6() {
                 this.y = y;
             });
 
-            sink(this.x); // $ MISSING: hasValueFlow=t6.1
-            sink(this.y); // $ MISSING: hasValueFlow=t6.1
+            sink(this.x); // $ hasValueFlow=t6.1
+            sink(this.y); // $ hasValueFlow=t6.2
 
             invoke(() => {
-                sink(this.x); // $ MISSING: hasValueFlow=t6.1
-                sink(this.y); // $ MISSING: hasValueFlow=t6.2
+                sink(this.x); // $ hasValueFlow=t6.1
+                sink(this.y); // $ hasValueFlow=t6.2
             });
 
             this.methodLike = function() {
-                sink(this.x); // $ MISSING: hasValueFlow=t6.1
-                sink(this.y); // $ MISSING: hasValueFlow=t6.2
+                sink(this.x); // $ hasValueFlow=t6.1
+                sink(this.y); // $ hasValueFlow=t6.2
             }
         }
     }
     const c = new C(source('t6.1'), source('t6.2'));
     sink(c.x); // $ hasValueFlow=t6.1
-    sink(c.y); // $ MISSING: hasValueFlow=t6.2
+    sink(c.y); // $ hasValueFlow=t6.2
+    c.methodLike();
 }