JS: Flow through *ngFor loops

javascript/extractor/src/com/semmle/js/extractor/HTMLExtractor.java

@@ -2,6 +2,7 @@ package com.semmle.js.extractor;
 
 import java.io.File;
 import java.nio.file.Path;
+import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import com.semmle.extractor.html.HtmlPopulator;
@@ -106,14 +107,22 @@ public class HTMLExtractor implements IExtractor {
                       false /* isTypeScript */);
             } else if (isAngularTemplateAttributeName(attr.getName())) {
               // For an attribute *ngFor="let var of EXPR", start parsing at EXPR
-              int offset = attr.getName().equals("*ngFor") ? source.indexOf(" of ") + " of ".length() : 0;
+              int offset = 0;
+              if (attr.getName().equals("*ngFor")) {
+                Matcher m = ANGULAR_FOR_LOOP_DECL.matcher(source);
+                if (m.matches()) {
+                  String expr = m.group(2);
+                  offset = m.end(2) - expr.length();
+                  source = expr;
+                }
+              }
               snippetLoC =
                   extractSnippet(
                       TopLevelKind.eventHandler,
                       config.withSourceType(SourceType.ANGULAR_TEMPLATE),
                       scopeManager,
                       textualExtractor,
-                      source.substring(offset),
+                      source,
                       valueStart.getRow(),
                       valueStart.getColumn() + offset,
                       false /* isTypeScript */);
@@ -147,6 +156,8 @@ public class HTMLExtractor implements IExtractor {
         name.startsWith("*ng");
   }
 
+  private static final Pattern ANGULAR_FOR_LOOP_DECL = Pattern.compile("^ *let +(\\w+) +of(?: +|(?!\\w))(.*)");
+
   /** List of HTML attributes whose value is interpreted as JavaScript. */
   private static final Pattern JS_ATTRIBUTE =
       Pattern.compile(

javascript/ql/src/semmle/javascript/frameworks/Angular2.qll

@@ -274,6 +274,10 @@ module Angular2 {
     result.getName() = name
   }
 
+  private DataFlow::Node getAttributeValueAsNode(HTML::Attribute attrib) {
+    result = attrib.getCodeInAttribute().getChildStmt(0).(ExprStmt).getExpr().flow()
+  }
+
   /**
    * The class for an Angular component.
    */
@@ -333,7 +337,7 @@ module Angular2 {
 
     /** Gets an argument that flows into the `name` field of this component. */
     DataFlow::Node getATemplateArgument(string name) {
-      result = getATemplateInstantiation().getAttributeByName("[" + name + "]").getCodeInAttribute().getChildStmt(0).(ExprStmt).getExpr().flow()
+      result = getAttributeValueAsNode(getATemplateInstantiation().getAttributeByName("[" + name + "]"))
     }
 
     /** Gets the `templateUrl` property of the `@Component` decorator. */
@@ -415,4 +419,57 @@ module Angular2 {
       )
     }
   }
+
+  /**
+   * An attribute of form `*ngFor="let var of EXPR"`.
+   *
+   * The `EXPR` has been extracted as the sole `CodeInAttribute` top-level for this
+   * attribute. There is no AST node for the implied for-of loop.
+   */
+  private class ForLoopAttribute extends HTML::Attribute {
+    ForLoopAttribute() {
+      getName() = "*ngFor"
+    }
+
+    /** Gets a data-flow node holding the value being iterated over. */
+    DataFlow::Node getIterationDomain() {
+      result = getAttributeValueAsNode(this)
+    }
+
+    /** Gets the name of the variable holding the element of the current iteration. */
+    string getIteratorName() {
+      result = getValue().regexpCapture("^ *let (\\w+) .*", 1)
+    }
+
+    /** Gets an HTML element in which the iterator variable is in scope. */
+    HTML::Element getAnElementInScope() {
+      result.getParent*() = getElement()
+    }
+
+    /** Gets a reference to the iterator variable. */
+    DataFlow::Node getAnIteratorAccess() {
+      exists(HTML::Attribute attrib |
+        attrib = getAnElementInScope().getAnAttribute() and
+        isAngularExpressionAttribute(attrib) and
+        result = getAGlobalVarAccessInAttribute(attrib.getCodeInAttribute(), getIteratorName()).flow()
+      )
+    }
+  }
+
+  /**
+   * A taint step `array -> elem` in `*ngFor="let elem of array"`, or more precisely,
+   * a step from `array` to each access to `elem`.
+   */
+  private class ForLoopStep extends TaintTracking::AdditionalTaintStep {
+    ForLoopAttribute attrib;
+
+    ForLoopStep() {
+      this = attrib.getIterationDomain()
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = this and
+      succ = attrib.getAnIteratorAccess()
+    }
+  }
 }

javascript/ql/test/library-tests/frameworks/Angular2/foo.component.html

@@ -5,3 +5,11 @@
     [prop4]="foo | testPipe:'safe'"
     [prop5]="42 | testPipe:foo"
 ></other-component>
+
+<div *ngFor="let element of taintedArray">
+    <other-component [prop1]="element"></other-component>
+</div>
+
+<div *ngFor="let element of safeArray">
+    <other-component [prop2]="element"></other-component>
+</div>

javascript/ql/test/library-tests/frameworks/Angular2/foo.component.ts

@@ -6,8 +6,12 @@ import { Component } from "@angular/core";
 })
 export class Foo {
     foo: string;
+    taintedArray: string[];
+    safeArray: string[];
 
     constructor() {
         this.foo = source();
+        this.taintedArray = [...source()];
+        this.safeArray = ['a', 'b'];
     }
 }

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

@@ -22,6 +22,7 @@ pipeClassRef
 | TestPipe.ts:4:8:9:1 | class T ... ;\\n  }\\n} | foo.component.html:5:20:5:27 | testPipe |
 | TestPipe.ts:4:8:9:1 | class T ... ;\\n  }\\n} | foo.component.html:6:19:6:26 | testPipe |
 taintFlow
-| foo.component.ts:11:20:11:27 | source() | other.component.ts:18:48:18:57 | this.prop1 |
-| foo.component.ts:11:20:11:27 | source() | other.component.ts:21:48:21:57 | this.prop4 |
-| foo.component.ts:11:20:11:27 | source() | other.component.ts:22:48:22:57 | this.prop5 |
+| foo.component.ts:13:20:13:27 | source() | other.component.ts:18:48:18:57 | this.prop1 |
+| foo.component.ts:13:20:13:27 | source() | other.component.ts:21:48:21:57 | this.prop4 |
+| foo.component.ts:13:20:13:27 | source() | other.component.ts:22:48:22:57 | this.prop5 |
+| foo.component.ts:14:33:14:40 | source() | other.component.ts:18:48:18:57 | this.prop1 |