Merge branch 'master' of https://github.com/github/codeql into pr/erik-krogh/3566

Author: erik-krogh

change-notes/1.25/analysis-javascript.md

@@ -3,8 +3,10 @@
 ## General improvements
 
 * Support for the following frameworks and libraries has been improved:
+  - [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
   - [bluebird](http://bluebirdjs.com/)
   - [express](https://www.npmjs.com/package/express)
+  - [fastify](https://www.npmjs.com/package/fastify)
   - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
@@ -13,12 +15,11 @@
   - [mssql](https://www.npmjs.com/package/mssql)
   - [mysql](https://www.npmjs.com/package/mysql)
   - [pg](https://www.npmjs.com/package/pg)
-  - [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
   - [sequelize](https://www.npmjs.com/package/sequelize)
   - [spanner](https://www.npmjs.com/package/spanner)
   - [sqlite](https://www.npmjs.com/package/sqlite)
-  - [ssh2](https://www.npmjs.com/package/ssh2)
   - [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
+  - [ssh2](https://www.npmjs.com/package/ssh2)
 
 * TypeScript 3.9 is now supported.
 
@@ -35,41 +36,42 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
-| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
-| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
-| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
-| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
-| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe patterns of constructing HTML. |
+| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
+| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe patterns of constructing HTML. |
+| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
+| Expression has no effect (`js/useless-expression`) | Fewer results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
+| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
 | Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
-| Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
-| Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
-| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes more coding patterns that are vulnerable to prototype pollution. |
+| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
+| Unknown directive (`js/unknown-directive`) | Fewer results | This query no longer flags directives generated by the Babel compiler. |
+| Unused property (`js/unused-property`) | Fewer results | This query no longer flags properties of objects that are operands of `yield` expressions. |
 | Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |
-| Unused property (`js/unused-property`) | Less results | This query no longer flags properties of objects that are operands of `yield` expressions. |
 
 The following low-precision queries are no longer run by default on LGTM (their results already were not displayed):
 
   - `js/angular/dead-event-listener`
   - `js/angular/unused-dependency`
-  - `js/conflicting-html-attribute`
-  - `js/useless-assignment-to-global`
-  - `js/too-many-parameters`
-  - `js/unused-property`
   - `js/bitwise-sign-check`
   - `js/comparison-of-identical-expressions`
-  - `js/misspelled-identifier`
+  - `js/conflicting-html-attribute`
+  - `js/ignored-setter-parameter`
   - `js/jsdoc/malformed-param-tag`
-  - `js/jsdoc/unknown-parameter`
   - `js/jsdoc/missing-parameter`
-  - `js/omitted-array-element`
-  - `js/ignored-setter-parameter`
+  - `js/jsdoc/unknown-parameter`
   - `js/json-in-javascript-file`
+  - `js/misspelled-identifier`
+  - `js/nested-loops-with-same-variable`
   - `js/node/cyclic-import`
   - `js/node/unused-npm-dependency`
-  - `js/single-run-loop`
-  - `js/nested-loops-with-same-variable`
+  - `js/omitted-array-element`
   - `js/return-outside-function`
+  - `js/single-run-loop`
+  - `js/too-many-parameters`
+  - `js/unused-property`
+  - `js/useless-assignment-to-global`
 
 ## Changes to libraries
 
@@ -79,3 +81,4 @@ The following low-precision queries are no longer run by default on LGTM (their
   - `Parameter.flow()` now gets the correct data flow node for a parameter. Previously this had a result, but the node was disconnected from the data flow graph.
   - `ParameterNode.asExpr()` and `.getAstNode()` now gets the parameter's AST node, whereas previously it had no result.
   - `Expr.flow()` now has a more meaningful result for destructuring patterns. Previously this node was disconnected from the data flow graph. Now it represents the values being destructured by the pattern.
+* The global data-flow and taint-tracking libraries now model indirect parameter accesses through the `arguments` object in some cases, which may lead to additional results from some of the security queries, particularly "Prototype pollution in utility function".

cpp/ql/src/external/DefectFilter.qll

@@ -1,31 +1,52 @@
+/** Provides a class for working with defect query results stored in dashboard databases. */
+
 import cpp
 
+/**
+ * Holds if `id` in the opaque identifier of a result reported by query `queryPath`,
+ * such that `message` is the associated message and the location of the result spans
+ * column `startcolumn` of line `startline` to column `endcolumn` of line `endline`
+ * in file `filepath`.
+ *
+ * For more information, see [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+ */
 external predicate defectResults(
   int id, string queryPath, string file, int startline, int startcol, int endline, int endcol,
   string message
 );
 
+/**
+ * A defect query result stored in a dashboard database.
+ */
 class DefectResult extends int {
   DefectResult() { defectResults(this, _, _, _, _, _, _, _) }
 
+  /** Gets the path of the query that reported the result. */
   string getQueryPath() { defectResults(this, result, _, _, _, _, _, _) }
 
+  /** Gets the file in which this query result was reported. */
   File getFile() {
     exists(string path |
       defectResults(this, _, path, _, _, _, _, _) and result.getAbsolutePath() = path
     )
   }
 
+  /** Gets the line on which the location of this query result starts. */
   int getStartLine() { defectResults(this, _, _, result, _, _, _, _) }
 
+  /** Gets the column on which the location of this query result starts. */
   int getStartColumn() { defectResults(this, _, _, _, result, _, _, _) }
 
+  /** Gets the line on which the location of this query result ends. */
   int getEndLine() { defectResults(this, _, _, _, _, result, _, _) }
 
+  /** Gets the column on which the location of this query result ends. */
   int getEndColumn() { defectResults(this, _, _, _, _, _, result, _) }
 
+  /** Gets the message associated with this query result. */
   string getMessage() { defectResults(this, _, _, _, _, _, _, result) }
 
+  /** Gets the URL corresponding to the location of this query result. */
   string getURL() {
     result =
       "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn() + ":" +

cpp/ql/src/external/ExternalArtifact.qll

@@ -1,26 +1,45 @@
+/**
+ * Provides classes for working with external data.
+ */
+
 import cpp
 
+/**
+ * An external data item.
+ */
 class ExternalData extends @externalDataElement {
+  /** Gets the path of the file this data was loaded from. */
   string getDataPath() { externalData(this, result, _, _) }
 
+  /**
+   * Gets the path of the file this data was loaded from, with its
+   * extension replaced by `.ql`.
+   */
   string getQueryPath() { result = getDataPath().regexpReplaceAll("\\.[^.]*$", ".ql") }
 
+  /** Gets the number of fields in this data item. */
   int getNumFields() { result = 1 + max(int i | externalData(this, _, i, _) | i) }
 
-  string getField(int index) { externalData(this, _, index, result) }
+  /** Gets the value of the `i`th field of this data item. */
+  string getField(int i) { externalData(this, _, i, result) }
 
-  int getFieldAsInt(int index) { result = getField(index).toInt() }
+  /** Gets the integer value of the `i`th field of this data item. */
+  int getFieldAsInt(int i) { result = getField(i).toInt() }
 
-  float getFieldAsFloat(int index) { result = getField(index).toFloat() }
+  /** Gets the floating-point value of the `i`th field of this data item. */
+  float getFieldAsFloat(int i) { result = getField(i).toFloat() }
 
-  date getFieldAsDate(int index) { result = getField(index).toDate() }
+  /** Gets the value of the `i`th field of this data item, interpreted as a date. */
+  date getFieldAsDate(int i) { result = getField(i).toDate() }
 
+  /** Gets a textual representation of this data item. */
   string toString() { result = getQueryPath() + ": " + buildTupleString(0) }
 
-  private string buildTupleString(int start) {
-    start = getNumFields() - 1 and result = getField(start)
+  /** Gets a textual representation of this data item, starting with the `n`th field. */
+  private string buildTupleString(int n) {
+    n = getNumFields() - 1 and result = getField(n)
     or
-    start < getNumFields() - 1 and result = getField(start) + "," + buildTupleString(start + 1)
+    n < getNumFields() - 1 and result = getField(n) + "," + buildTupleString(n + 1)
   }
 }
 
@@ -33,7 +52,9 @@ class DefectExternalData extends ExternalData {
     this.getNumFields() = 2
   }
 
+  /** Gets the URL associated with this data item. */
   string getURL() { result = getField(0) }
 
+  /** Gets the message associated with this data item. */
   string getMessage() { result = getField(1) }
 }

cpp/ql/src/external/MetricFilter.qll

@@ -1,31 +1,58 @@
+/** Provides a class for working with metric query results stored in dashboard databases. */
+
 import cpp
 
+/**
+ * Holds if `id` in the opaque identifier of a result reported by query `queryPath`,
+ * such that `value` is the reported metric value and the location of the result spans
+ * column `startcolumn` of line `startline` to column `endcolumn` of line `endline`
+ * in file `filepath`.
+ *
+ * For more information, see [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+ */
 external predicate metricResults(
   int id, string queryPath, string file, int startline, int startcol, int endline, int endcol,
   float value
 );
 
+/**
+ * A metric query result stored in a dashboard database.
+ */
 class MetricResult extends int {
   MetricResult() { metricResults(this, _, _, _, _, _, _, _) }
 
+  /** Gets the path of the query that reported the result. */
   string getQueryPath() { metricResults(this, result, _, _, _, _, _, _) }
 
+  /** Gets the file in which this query result was reported. */
   File getFile() {
     exists(string path |
       metricResults(this, _, path, _, _, _, _, _) and result.getAbsolutePath() = path
     )
   }
 
+  /** Gets the line on which the location of this query result starts. */
   int getStartLine() { metricResults(this, _, _, result, _, _, _, _) }
 
+  /** Gets the column on which the location of this query result starts. */
   int getStartColumn() { metricResults(this, _, _, _, result, _, _, _) }
 
+  /** Gets the line on which the location of this query result ends. */
   int getEndLine() { metricResults(this, _, _, _, _, result, _, _) }
 
+  /** Gets the column on which the location of this query result ends. */
   int getEndColumn() { metricResults(this, _, _, _, _, _, result, _) }
 
+  /**
+   * Holds if there is a `Location` entity whose location is the same as
+   * the location of this query result.
+   */
   predicate hasMatchingLocation() { exists(this.getMatchingLocation()) }
 
+  /**
+   * Gets the `Location` entity whose location is the same as the location
+   * of this query result.
+   */
   Location getMatchingLocation() {
     result.getFile() = this.getFile() and
     result.getStartLine() = this.getStartLine() and
@@ -34,8 +61,10 @@ class MetricResult extends int {
     result.getEndColumn() = this.getEndColumn()
   }
 
+  /** Gets the value associated with this query result. */
   float getValue() { metricResults(this, _, _, _, _, _, _, result) }
 
+  /** Gets the URL corresponding to the location of this query result. */
   string getURL() {
     result =
       "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn() + ":" +

cpp/ql/src/semmle/code/cpp/Declaration.qll

@@ -98,7 +98,12 @@ class Declaration extends Locatable, @declaration {
     this.hasQualifiedName(namespaceQualifier, "", baseName)
   }
 
-  override string toString() { result = this.getName() }
+  /**
+   * Gets a description of this `Declaration` for display purposes.
+   */
+  string getDescription() { result = this.getName() }
+
+  final override string toString() { result = this.getDescription() }
 
   /**
    * Gets the name of this declaration.

cpp/ql/src/semmle/code/cpp/Namespace.qll

@@ -79,7 +79,10 @@ class Namespace extends NameQualifyingElement, @namespace {
   /** Gets the metric namespace. */
   MetricNamespace getMetrics() { result = this }
 
-  override string toString() { result = this.getQualifiedName() }
+  /** Gets a version of the `QualifiedName` that is more suitable for display purposes. */
+  string getFriendlyName() { result = this.getQualifiedName() }
+
+  final override string toString() { result = getFriendlyName() }
 
   /** Gets a declaration of (part of) this namespace. */
   NamespaceDeclarationEntry getADeclarationEntry() { result.getNamespace() = this }
@@ -104,7 +107,7 @@ class NamespaceDeclarationEntry extends Locatable, @namespace_decl {
     namespace_decls(underlyingElement(this), unresolveElement(result), _, _)
   }
 
-  override string toString() { result = this.getNamespace().toString() }
+  override string toString() { result = this.getNamespace().getFriendlyName() }
 
   /**
    * Gets the location of the token preceding the namespace declaration
@@ -150,7 +153,7 @@ class UsingDeclarationEntry extends UsingEntry {
    */
   Declaration getDeclaration() { usings(underlyingElement(this), unresolveElement(result), _) }
 
-  override string toString() { result = "using " + this.getDeclaration().toString() }
+  override string toString() { result = "using " + this.getDeclaration().getDescription() }
 }
 
 /**
@@ -169,7 +172,7 @@ class UsingDirectiveEntry extends UsingEntry {
    */
   Namespace getNamespace() { usings(underlyingElement(this), unresolveElement(result), _) }
 
-  override string toString() { result = "using namespace " + this.getNamespace().toString() }
+  override string toString() { result = "using namespace " + this.getNamespace().getFriendlyName() }
 }
 
 /**
@@ -204,7 +207,7 @@ class GlobalNamespace extends Namespace {
    */
   deprecated string getFullName() { result = this.getName() }
 
-  override string toString() { result = "(global namespace)" }
+  override string getFriendlyName() { result = "(global namespace)" }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/Variable.qll

@@ -260,24 +260,33 @@ class ParameterDeclarationEntry extends VariableDeclarationEntry {
    */
   int getIndex() { param_decl_bind(underlyingElement(this), result, _) }
 
+  private string getAnonymousParameterDescription() {
+    not exists(getName()) and
+    exists(string idx |
+      idx =
+        ((getIndex() + 1).toString() + "th")
+            .replaceAll("1th", "1st")
+            .replaceAll("2th", "2nd")
+            .replaceAll("3th", "3rd")
+            .replaceAll("11st", "11th")
+            .replaceAll("12nd", "12th")
+            .replaceAll("13rd", "13th") and
+      if exists(getCanonicalName())
+      then result = "declaration of " + getCanonicalName() + " as anonymous " + idx + " parameter"
+      else result = "declaration of " + idx + " parameter"
+    )
+  }
+
   override string toString() {
-    if exists(getName())
-    then result = super.toString()
-    else
-      exists(string idx |
-        idx =
-          ((getIndex() + 1).toString() + "th")
-              .replaceAll("1th", "1st")
-              .replaceAll("2th", "2nd")
-              .replaceAll("3th", "3rd")
-              .replaceAll("11st", "11th")
-              .replaceAll("12nd", "12th")
-              .replaceAll("13rd", "13th")
-      |
-        if exists(getCanonicalName())
-        then result = "declaration of " + getCanonicalName() + " as anonymous " + idx + " parameter"
-        else result = "declaration of " + idx + " parameter"
-      )
+    isDefinition() and
+    result = "definition of " + getName()
+    or
+    not isDefinition() and
+    if getName() = getCanonicalName()
+    then result = "declaration of " + getName()
+    else result = "declaration of " + getCanonicalName() + " as " + getName()
+    or
+    result = getAnonymousParameterDescription()
   }
 
   /**