Merge branch 'main' into workflow/coverage/update

Author: smowton

Diff for: cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql

@@ -168,12 +168,7 @@ where
     strictcount(string fieldName |
       exists(Field f |
         f.getDeclaringType() = c and
-        fieldName = f.getName() and
-        // IBOutlet's are a way of building GUIs
-        // automatically out of ObjC properties.
-        // We don't want to count those for the
-        // purposes of this query.
-        not f.getType().getAnAttribute().hasName("iboutlet")
+        fieldName = f.getName()
       )
     ) and
   n > 15 and

Diff for: javascript/extractor/src/com/semmle/js/extractor/HTMLExtractor.java

@@ -186,7 +186,7 @@ private boolean isAngularTemplateAttributeName(String name) {
 
   /** Attribute names that look valid in HTML or in one of the template languages we support, like Vue and Angular. */
   private static final Pattern VALID_ATTRIBUTE_NAME =
-      Pattern.compile("[*:@]?\\[?\\(?[\\w:_\\-.]+\\]?\\)?");
+      Pattern.compile("[*:@]?\\[?\\(?[\\w:_\\-.]+\\)?\\]?");
 
   /** List of HTML attributes whose value is interpreted as JavaScript. */
   private static final Pattern JS_ATTRIBUTE =

Diff for: javascript/extractor/src/com/semmle/js/extractor/Main.java

@@ -41,7 +41,7 @@ public class Main {
    * A version identifier that should be updated every time the extractor changes in such a way that
    * it may produce different tuples for the same file under the same {@link ExtractorConfig}.
    */
-  public static final String EXTRACTOR_VERSION = "2024-10-29";
+  public static final String EXTRACTOR_VERSION = "2025-01-09";
 
   public static final Pattern NEWLINE = Pattern.compile("\n");
 

Diff for: javascript/ql/lib/semmle/javascript/DOM.qll

@@ -388,23 +388,33 @@ module DOM {
     }
   }
 
-  /**
-   * Gets a reference to a DOM event.
-   */
-  private DataFlow::SourceNode domEventSource() {
-    // e.g. <form onSubmit={e => e.target}/>
-    exists(JsxAttribute attr | attr.getName().matches("on%") |
-      result = attr.getValue().flow().getABoundFunctionValue(0).getParameter(0)
-    )
-    or
-    // node.addEventListener("submit", e => e.target)
-    result = domValueRef().getAMethodCall("addEventListener").getABoundCallbackParameter(1, 0)
-    or
-    // node.onSubmit = (e => e.target);
-    exists(DataFlow::PropWrite write | write = domValueRef().getAPropertyWrite() |
-      write.getPropertyName().matches("on%") and
-      result = write.getRhs().getAFunctionValue().getParameter(0)
-    )
+  /** A data flow node that is a source of DOM events. */
+  class DomEventSource extends DataFlow::Node instanceof DomEventSource::Range { }
+
+  /** Companion module to the `DomEventSource` class. */
+  module DomEventSource {
+    /**
+     * A data flow node that should be considered a source of DOM events.
+     */
+    abstract class Range extends DataFlow::Node { }
+
+    private class DefaultRange extends Range {
+      DefaultRange() {
+        // e.g. <form onSubmit={e => e.target}/>
+        exists(JsxAttribute attr | attr.getName().matches("on%") |
+          this = attr.getValue().flow().getABoundFunctionValue(0).getParameter(0)
+        )
+        or
+        // node.addEventListener("submit", e => e.target)
+        this = domValueRef().getAMethodCall("addEventListener").getABoundCallbackParameter(1, 0)
+        or
+        // node.onSubmit = (e => e.target);
+        exists(DataFlow::PropWrite write | write = domValueRef().getAPropertyWrite() |
+          write.getPropertyName().matches("on%") and
+          this = write.getRhs().getAFunctionValue().getParameter(0)
+        )
+      }
+    }
   }
 
   /** Gets a data flow node that refers directly to a value from the DOM. */
@@ -419,7 +429,7 @@ module DOM {
     result = domValueRef().getAMethodCall(["item", "namedItem"])
     or
     t.startInProp("target") and
-    result = domEventSource()
+    result instanceof DomEventSource
     or
     t.startInProp(DataFlow::PseudoProperties::arrayElement()) and
     result = domElementCollection()

Diff for: javascript/ql/lib/semmle/javascript/frameworks/Angular2.qll

@@ -554,4 +554,25 @@ module Angular2 {
       this = API::Node::ofType("@angular/core", "ElementRef").getMember("nativeElement").asSource()
     }
   }
+
+  /**
+   * A source of DOM events originating from the `$event` variable in an event handler installed in an Angular template.
+   */
+  private class DomEventSources extends DOM::DomEventSource::Range {
+    DomEventSources() {
+      exists(HTML::Element elm, string attributeName |
+        elm = any(ComponentClass cls).getATemplateElement() and
+        // Ignore instantiations of known element (mainly focus on native DOM elements)
+        not elm = any(ComponentClass cls).getATemplateInstantiation() and
+        not elm.getName().matches("ng-%") and
+        this =
+          elm.getAttributeByName(attributeName)
+              .getCodeInAttribute()
+              .(TemplateTopLevel)
+              .getAVariableUse("$event") and
+        attributeName.matches("(%)") and // event handler attribute
+        not attributeName.matches("(ng%)") // exclude NG events which aren't necessarily DOM events
+      )
+    }
+  }
 }

Diff for: javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll

@@ -232,6 +232,15 @@ module XssThroughDom {
         )
       }
     }
+
+    /**
+     * An object containing input values from an Angular form, accessed through an `NgForm` object.
+     */
+    class AngularFormSource extends Source {
+      AngularFormSource() {
+        this = API::Node::ofType("@angular/forms", "NgForm").getMember("value").asSource()
+      }
+    }
   }
 
   /**
@@ -261,4 +270,16 @@ module XssThroughDom {
       this = getSelectionCall(DataFlow::TypeTracker::end()).getAMethodCall("toString")
     }
   }
+
+  /**
+   * A source of DOM input originating from an Angular two-way data binding.
+   */
+  private class AngularNgModelSource extends Source {
+    AngularNgModelSource() {
+      exists(Angular2::ComponentClass component, string fieldName |
+        fieldName = component.getATemplateElement().getAttributeByName("[(ngModel)]").getValue() and
+        this = component.getFieldInputNode(fieldName)
+      )
+    }
+  }
 }

Diff for: javascript/ql/src/change-notes/2025-01-09-angular2-xss-through-dom.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* The `js/xss-through-dom` query now recognises sources of DOM input originating from Angular templates.

Diff for: javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected

@@ -1,4 +1,6 @@
 edges
+| angular.ts:12:5:12:23 | field: string = ""; | angular.ts:33:24:33:33 | this.field | provenance |  |
+| angular.ts:29:24:29:33 | form.value | angular.ts:29:24:29:37 | form.value.foo | provenance |  |
 | forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values | provenance |  |
 | forms.js:9:31:9:36 | values | forms.js:9:31:9:40 | values.foo | provenance |  |
 | forms.js:11:24:11:29 | values | forms.js:12:31:12:36 | values | provenance |  |
@@ -42,6 +44,12 @@ edges
 | xss-through-dom.js:154:25:154:27 | msg | xss-through-dom.js:155:27:155:29 | msg | provenance |  |
 | xss-through-dom.js:159:34:159:52 | $("textarea").val() | xss-through-dom.js:154:25:154:27 | msg | provenance |  |
 nodes
+| angular.ts:12:5:12:23 | field: string = ""; | semmle.label | field: string = ""; |
+| angular.ts:16:24:16:41 | event.target.value | semmle.label | event.target.value |
+| angular.ts:20:24:20:35 | target.value | semmle.label | target.value |
+| angular.ts:29:24:29:33 | form.value | semmle.label | form.value |
+| angular.ts:29:24:29:37 | form.value.foo | semmle.label | form.value.foo |
+| angular.ts:33:24:33:33 | this.field | semmle.label | this.field |
 | forms.js:8:23:8:28 | values | semmle.label | values |
 | forms.js:9:31:9:36 | values | semmle.label | values |
 | forms.js:9:31:9:40 | values.foo | semmle.label | values.foo |
@@ -124,6 +132,10 @@ nodes
 | xss-through-dom.js:159:34:159:52 | $("textarea").val() | semmle.label | $("textarea").val() |
 subpaths
 #select
+| angular.ts:16:24:16:41 | event.target.value | angular.ts:16:24:16:41 | event.target.value | angular.ts:16:24:16:41 | event.target.value | $@ is reinterpreted as HTML without escaping meta-characters. | angular.ts:16:24:16:41 | event.target.value | DOM text |
+| angular.ts:20:24:20:35 | target.value | angular.ts:20:24:20:35 | target.value | angular.ts:20:24:20:35 | target.value | $@ is reinterpreted as HTML without escaping meta-characters. | angular.ts:20:24:20:35 | target.value | DOM text |
+| angular.ts:29:24:29:37 | form.value.foo | angular.ts:29:24:29:33 | form.value | angular.ts:29:24:29:37 | form.value.foo | $@ is reinterpreted as HTML without escaping meta-characters. | angular.ts:29:24:29:33 | form.value | DOM text |
+| angular.ts:33:24:33:33 | this.field | angular.ts:12:5:12:23 | field: string = ""; | angular.ts:33:24:33:33 | this.field | $@ is reinterpreted as HTML without escaping meta-characters. | angular.ts:12:5:12:23 | field: string = ""; | DOM text |
 | forms.js:9:31:9:40 | values.foo | forms.js:8:23:8:28 | values | forms.js:9:31:9:40 | values.foo | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:8:23:8:28 | values | DOM text |
 | forms.js:12:31:12:40 | values.bar | forms.js:11:24:11:29 | values | forms.js:12:31:12:40 | values.bar | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:11:24:11:29 | values | DOM text |
 | forms.js:25:23:25:34 | values.email | forms.js:24:15:24:20 | values | forms.js:25:23:25:34 | values.email | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:24:15:24:20 | values | DOM text |

Diff for: javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/angular.ts

@@ -0,0 +1,36 @@
+import { Component } from "@angular/core";
+import { NgForm } from "@angular/forms";
+
+@Component({
+    template: `
+        <input type="text" (input)="setInput1($event)"></input>
+        <input type="text" (input)="setInput2($event.target)"></input>
+        <input type="text" [(ngModel)]="field"></input>
+    `
+})
+export class Foo {
+    field: string = "";
+    safeField: string = "";
+
+    setInput1(event) {
+        document.write(event.target.value); // NOT OK
+    }
+
+    setInput2(target) {
+        document.write(target.value); // NOT OK
+    }
+
+    setOtherInput(e) {
+        document.write(e.target.value); // OK
+        document.write(e.value); // OK
+    }
+
+    blah(form: NgForm) {
+        document.write(form.value.foo); // NOT OK
+    }
+
+    useField() {
+        document.write(this.field); // NOT OK
+        document.write(this.safeField); // OK
+    }
+}

Diff for: shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

@@ -2154,8 +2154,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
         pragma[nomagic]
         private predicate storeStepFwd(NodeEx node1, Ap ap1, Content c, NodeEx node2, Ap ap2) {
           fwdFlowStore(node1, _, ap1, _, c, _, _, node2, _, _, _) and
-          ap2 = apCons(c, ap1) and
-          readStepFwd(_, ap2, c, _, _)
+          readStepFwd(_, ap2, c, _, ap1)
         }
 
         pragma[nomagic]
@@ -4393,6 +4392,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
       Typ getTyp(DataFlowType t) { result = t }
 
       bindingset[c, tail]
+      pragma[inline_late]
       Ap apCons(Content c, Ap tail) { result.isCons(c, tail) }
 
       class ApHeadContent = Content;
@@ -4462,6 +4462,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
       abstract Content getHead();
 
       /** Holds if this is a representation of `head` followed by `tail`. */
+      pragma[nomagic]
       abstract predicate isCons(Content head, AccessPath tail);
 
       /** Gets the front of this access path. */