JS: Enhance isDomProperty to check for getAPropertyRead on DOM nodes

Author: Napalys

javascript/ql/lib/Expressions/ExprHasNoEffect.qll

@@ -129,6 +129,39 @@ predicate noSideEffects(Expr e) {
   )
 }
 
+/**
+ * Holds if `e` contains a DOM property access in a context where it would
+ * be evaluated for side effects.
+ */
+predicate containsDomPropertyAccess(Expr e) {
+  isDomProperty(e.(PropAccess).getPropertyName())
+  or
+  exists(LogicalBinaryExpr logical | logical = e |
+    containsDomPropertyAccess(logical.getLeftOperand()) or
+    containsDomPropertyAccess(logical.getRightOperand())
+  )
+  or
+  exists(SeqExpr seq | seq = e | containsDomPropertyAccess(seq.getAnOperand()))
+  or
+  exists(ParExpr paren | paren = e | containsDomPropertyAccess(paren.getExpression()))
+  or
+  exists(ConditionalExpr cond | cond = e | containsDomPropertyAccess(cond.getCondition()))
+}
+
+/**
+ * Holds if `e` is an expression that might appear useless but contains
+ * DOM side effects that make it meaningful.
+ */
+predicate hasHiddenDomSideEffects(Expr e) {
+  (
+    e instanceof LogicalBinaryExpr or
+    e instanceof SeqExpr or
+    e instanceof ParExpr or
+    e instanceof BinaryExpr
+  ) and
+  containsDomPropertyAccess(e)
+}
+
 /**
  * Holds if the expression `e` should be reported as having no effect.
  */
@@ -145,6 +178,7 @@ predicate hasNoEffect(Expr e) {
   not isDeclaration(e) and
   // exclude DOM properties, which sometimes have magical auto-update properties
   not isDomProperty(e.(PropAccess).getPropertyName()) and
+  not hasHiddenDomSideEffects(e) and
   // exclude xUnit.js annotations
   not e instanceof XUnitAnnotation and
   // exclude common patterns that are most likely intentional

javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected

@@ -1,11 +1,4 @@
-| dom.js:2:5:2:30 | a.clien ... ientTop | This expression has no effect. |
-| dom.js:2:5:2:50 | a.clien ...  === !0 | This expression has no effect. |
 | dom.js:2:33:2:50 | a.clientTop === !0 | This expression has no effect. |
-| dom.js:3:5:3:20 | a && a.clientTop | This expression has no effect. |
-| dom.js:4:5:4:28 | a.clien ... ientTop | This expression has no effect. |
-| dom.js:5:18:5:43 | a.clien ... ientTop | This expression has no effect. |
-| dom.js:6:18:6:63 | b && (b ... entTop) | This expression has no effect. |
-| dom.js:6:23:6:63 | (b.clie ... entTop) | This expression has no effect. |
 | try.js:22:9:22:26 | x.ordinaryProperty | This expression has no effect. |
 | tst2.js:2:4:2:4 | 0 | This expression has no effect. |
 | tst.js:3:1:3:2 | 23 | This expression has no effect. |

javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/dom.js

@@ -1,7 +1,7 @@
 function f(){
     a.clientTop && a.clientTop, a.clientTop === !0; // $Alert
-    a && a.clientTop; // $SPURIOUS:Alert
-    a.clientTop, a.clientTop; // $SPURIOUS:Alert
-    if(a) return a.clientTop && a.clientTop, a.clientTop === !0 // $SPURIOUS:Alert
-    if(b) return b && (b.clientTop, b.clientTop && b.clientTop), null // $SPURIOUS:Alert
+    a && a.clientTop;
+    a.clientTop, a.clientTop;
+    if(a) return a.clientTop && a.clientTop, a.clientTop === !0;
+    if(b) return b && (b.clientTop, b.clientTop && b.clientTop), null;
 }