refactored multiple implementations of getEnclosingTryStmt into a single predicate

Author: erik-krogh

javascript/ql/src/semmle/javascript/Stmt.qll

@@ -55,6 +55,17 @@ class Stmt extends @stmt, ExprOrStmt, Documentable {
   }
 
   override predicate isAmbient() { hasDeclareKeyword(this) or getParent().isAmbient() }
+  
+  /**
+  * Gets the `try` statement containing this statement without crossing function
+  * boundaries or other `try ` statements.
+  */
+  TryStmt getEnclosingTryStmt() {
+    getParentStmt+() = result.getBody() and
+    not exists(TryStmt mid |
+      getParentStmt+() = mid.getBody() and mid.getParentStmt+() = result.getBody()
+    )
+  }
 }
 
 /**

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -67,7 +67,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
     DataFlow::exceptionalInvocationReturnNode(pred, expr)
   |
     // Propagate out of enclosing function.
-    not exists(getEnclosingTryStmt(expr.getEnclosingStmt())) and
+    not exists(expr.getEnclosingStmt().getEnclosingTryStmt()) and
     exists(Function f |
       f = expr.getEnclosingFunction() and
       DataFlow::exceptionalFunctionReturnNode(succ, f)
@@ -76,7 +76,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
     // Propagate to enclosing try/catch.
     // To avoid false flow, we only propagate to an unguarded catch clause.
     exists(TryStmt try |
-      try = getEnclosingTryStmt(expr.getEnclosingStmt()) and
+      try = expr.getEnclosingStmt().getEnclosingTryStmt() and
       DataFlow::parameterNode(succ, try.getCatchClause().getAParameter())
     )
   )
@@ -156,19 +156,6 @@ private module CachedSteps {
   cached
   predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
 
-  /**
-   * Gets the `try` statement containing `stmt` without crossing function boundaries
-   * or other `try ` statements.
-   */
-  cached
-  TryStmt getEnclosingTryStmt(Stmt stmt) {
-    result.getBody() = stmt
-    or
-    not stmt instanceof Function and
-    not stmt = any(TryStmt try).getBody() and
-    result = getEnclosingTryStmt(stmt.getParentStmt())
-  }
-
   /**
    * Holds if there is a flow step from `pred` to `succ` through:
    * - returning a value from a function call, or

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

@@ -1,33 +1,36 @@
 /**
- * Provides a taint-tracking configuration for reasoning about cross-site 
- * scripting vulnerabilities where the taint-flow passes through a thrown 
- * exception. 
+ * Provides a taint-tracking configuration for reasoning about cross-site
+ * scripting vulnerabilities where the taint-flow passes through a thrown
+ * exception.
  */
 
 import javascript
 
 module ExceptionXss {
   import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom
-  import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom 
-  import Xss::DomBasedXss as DomBasedXss 
+  import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom
+  import Xss::DomBasedXss as DomBasedXss
   import Xss::ReflectedXss as ReflectedXSS
   import Xss::StoredXss as StoredXss
   import Xss as XSS
 
   DataFlow::ExceptionalInvocationReturnNode getCallerExceptionalReturn(Function func) {
     exists(DataFlow::InvokeNode call |
       not call.isImprecise() and
-      func = call.getACallee() and
+      func = call.getACallee(0) and
       result = call.getExceptionalReturn()
     )
   }
 
   DataFlow::Node getExceptionalSuccessor(DataFlow::Node pred) {
-    if exists(getEnclosingTryStmt(pred.asExpr().getEnclosingStmt()))
+    if exists(pred.asExpr().getEnclosingStmt().getEnclosingTryStmt())
     then
-      result = DataFlow::parameterNode(getEnclosingTryStmt(pred
+      result = DataFlow::parameterNode(pred
               .asExpr()
-              .getEnclosingStmt()).getACatchClause().getAParameter())
+              .getEnclosingStmt()
+              .getEnclosingTryStmt()
+              .getACatchClause()
+              .getAParameter())
     else result = getCallerExceptionalReturn(pred.getContainer())
   }
 
@@ -38,54 +41,51 @@ module ExceptionXss {
     node.asExpr().getEnclosingStmt() instanceof ThrowStmt
   }
 
-  TryStmt getEnclosingTryStmt(Stmt stmt) {
-    stmt.getParentStmt+() = result.getBody() and
-    not exists(TryStmt mid |
-      stmt.getParentStmt+() = mid.getBody() and mid.getParentStmt+() = result.getBody()
-    )
-  }
-  
   /**
-   * A FlowLabel representing tainted data that has not been thrown in an exception. 
+   * A FlowLabel representing tainted data that has not been thrown in an exception.
    * In the js/xss-through-exception query data-flow can only reach a sink after
-   * the data has been thrown as an exception, and data that has not been thrown 
-   * as an exception therefore has this flow label, and only this flow label, associated with it. 
+   * the data has been thrown as an exception, and data that has not been thrown
+   * as an exception therefore has this flow label, and only this flow label, associated with it.
    */
   class NotYetThrown extends DataFlow::FlowLabel {
     NotYetThrown() { this = "NotYetThrown" }
   }
 
   /**
-   * A taint-tracking configuration for reasoning about XSS with possible exceptional flow. 
-   * Flow labels are used to ensure that we only report taint-flow that has been thrown in 
-   * an exception.  
+   * A taint-tracking configuration for reasoning about XSS with possible exceptional flow.
+   * Flow labels are used to ensure that we only report taint-flow that has been thrown in
+   * an exception.
    */
   class Configuration extends TaintTracking::Configuration {
-    Configuration() { this = "ExceptionXss"}
+    Configuration() { this = "ExceptionXss" }
 
     override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
       source instanceof XSS::Shared::Source and label instanceof NotYetThrown
     }
-    
+
     override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
       sink instanceof XSS::Shared::Sink and not label instanceof NotYetThrown
     }
 
-    override predicate isSanitizer(DataFlow::Node node) {
-      node instanceof XSS::Shared::Sanitizer
-    }
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof XSS::Shared::Sanitizer }
 
-    override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
-      inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl,
+      DataFlow::FlowLabel outlbl
+    ) {
+      inlbl instanceof NotYetThrown and
+      (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
       succ = getExceptionalSuccessor(pred) and
-      (canThrowSensitiveInformation(pred) or pred = any(DataFlow::InvokeNode c).getExceptionalReturn())
+      (
+        canThrowSensitiveInformation(pred) or
+        pred = any(DataFlow::InvokeNode c).getExceptionalReturn()
+      )
       or
       // All the usual taint-flow steps apply on data-flow before it has been thrown in an exception.
-      this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown 
+      this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown
       or
-      // We taint an object deep if it happens before an exception has been thrown. 
-      inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown and
-      exists(DataFlow::PropWrite write | write.getRhs() = pred and write.getBase() = succ)
+      // We taint an object deep if it happens before an exception has been thrown.
+      inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown and exists(DataFlow::PropWrite write | write.getRhs() = pred and write.getBase() = succ)
     }
   }
 }