changes based on review feedback

Author: erik-krogh

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -303,7 +303,6 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    *
    * INTERNAL: this predicate should only be used from within `blocks(boolean, Expr)`.
    */
-  pragma[noinline,nomagic]
   predicate internalBlocks(DataFlow::Node nd, string label) {
     // 1) `nd` is a use of a refinement node that blocks its input variable
     exists(SsaRefinementNode ref, boolean outcome |

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

@@ -5,7 +5,6 @@
  */
 
 import javascript
-import Statements.UselessConditional
 
 module ExceptionXss {
   import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom
@@ -48,7 +47,7 @@ module ExceptionXss {
       node.asExpr().getEnclosingStmt() instanceof ThrowStmt
       or
       exists(DataFlow::PropRef prop |
-        node.getEnclosingExpr() = prop.getPropertyNameExpr() and
+        node = DataFlow::valueNode(prop.getPropertyNameExpr()) and
         isNullOrUndefined(prop.getBase().analyze().getAType())
       )
     )
@@ -70,15 +69,15 @@ module ExceptionXss {
    */
   class Callback extends DataFlow::FunctionNode {
     DataFlow::ParameterNode errorParameter;
+    IfStmt ifStmt;
 
     Callback() {
       exists(DataFlow::CallNode call | call.getLastArgument().getAFunctionValue() = this) and
       this.getNumParameter() = 2 and
       errorParameter = this.getParameter(0) and
-      exists(Expr errorCheck |
-        isExplicitConditional(this.getFunction().getBodyStmt(0), errorCheck) and
-        errorParameter.flowsTo(DataFlow::valueNode(errorCheck))
-      )
+      ifStmt = this.getFunction().getBodyStmt(0) and 
+      errorParameter.flowsToExpr(ifStmt.getCondition()) and
+      not ifStmt.getThen().getBasicBlock().getASuccessor*() = this.getFunction().getBodyStmt(1).getBasicBlock()
     }
 
     DataFlow::Node getErrorParam() { result = errorParameter }

javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js

@@ -174,4 +174,13 @@ app.get('/user/:id', function(req, res) {
 	new Promise(resolve => inner(foo, resolve)).catch((e) => {
       $('myId').html(e); // NOT OK
     });
-})();
+})();
+
+app.get('/user/:id', function(req, res) {
+  unknown(req.params.id, (error, res) => {
+    if (error) {
+	  $('myId').html(error); // OK (falls through to the next statement) 
+	}
+	$('myId').html(res); // OK (for now?)
+  });
+});