Merge pull request #20399 from asgerf/js/default-interop2

JS: Refactor handling of ambiguous default imports

javascript/ql/lib/semmle/javascript/ApiGraphs.qll

@@ -822,7 +822,7 @@ module API {
           or
           // special case: from `require('m')` to an export of `prop` in `m`
           exists(Import imp, Module m, string prop |
-            pred = imp.getImportedModuleNode() and
+            pred = imp.getImportedModuleNodeStrict() and
             m = imp.getImportedModule() and
             lbl = Label::member(prop) and
             rhs = m.getAnExportedValue(prop)
@@ -1337,7 +1337,7 @@ module API {
       result = nd.getALocalSource()
       or
       // additional backwards step from `require('m')` to `exports` or `module.exports` in m
-      exists(Import imp | imp.getImportedModuleNode() = trackDefNode(nd, t.continue()) |
+      exists(Import imp | imp.getImportedModuleNodeStrict() = trackDefNode(nd, t.continue()) |
         result = DataFlow::exportsVarNode(imp.getImportedModule())
         or
         result = DataFlow::moduleVarNode(imp.getImportedModule()).getAPropertyRead("exports")

javascript/ql/lib/semmle/javascript/ES2015Modules.qll

@@ -137,17 +137,26 @@ class ImportDeclaration extends Stmt, Import, @import_declaration {
       is instanceof ImportNamespaceSpecifier and
       count(this.getASpecifier()) = 1
       or
-      // For compatibility with the non-standard implementation of default imports,
-      // treat default imports as namespace imports in cases where it can't cause ambiguity
-      // between named exports and the properties of a default-exported object.
-      not this.getImportedModule().(ES2015Module).hasBothNamedAndDefaultExports() and
-      is.getImportedName() = "default"
+      result = this.getAmbiguousDefaultImportNode()
     )
     or
     // `import { createServer } from 'http'`
     result = DataFlow::destructuredModuleImportNode(this)
   }
 
+  /**
+   * Gets the data flow node corresponding to the `foo` in `import foo from "somewhere"`.
+   *
+   * This refers to the default import, but some non-standard compilers will treat it as a namespace
+   * import. In order to support both interpretations, it is considered an "ambiguous default import".
+   *
+   * Note that renamed default imports, such as `import { default as foo } from "somewhere"`,
+   * are not considered ambiguous, and will not be reported by this predicate.
+   */
+  DataFlow::Node getAmbiguousDefaultImportNode() {
+    result = DataFlow::valueNode(this.getASpecifier().(ImportDefaultSpecifier))
+  }
+
   /** Holds if this is declared with the `type` keyword, so it only imports types. */
   predicate isTypeOnly() { has_type_keyword(this) }
 

javascript/ql/lib/semmle/javascript/Modules.qll

@@ -179,7 +179,42 @@ abstract class Import extends AstNode {
   }
 
   /**
-   * Gets the data flow node that the default import of this import is available at.
+   * Gets the data flow node corresponding to the imported module.
+   *
+   * For example:
+   * ```js
+   * // ES2015 style
+   * import * as foo from "fs"; // Gets the node for `foo`
+   * import { readSync } from "fs"; // Gets a node representing the destructured import
+   *
+   * // CommonJS style
+   * require("fs"); // Gets the return value
+   *
+   * // AMD style
+   * define(["fs"], function(fs) { // Gets the node for the `fs` parameter
+   * });
+   * ```
+   *
+   * For default imports, this gets two nodes: the default import node, and a node representing the imported module:
+   * ```js
+   * import foo from "fs"; // gets both `foo` and a node representing the imported module
+   * ```
+   * This behaviour is to support non-standard compilers that treat default imports
+   * as namespace imports. Use `getImportedModuleNodeStrict()` to avoid this behaviour in cases
+   * where it would cause ambiguous data flow.
    */
   abstract DataFlow::Node getImportedModuleNode();
+
+  /**
+   * Gets the same as `getImportedModuleNode()` except ambiguous default imports are excluded
+   * in cases where it would cause ambiguity between named exports and properties
+   * of a default export.
+   */
+  final DataFlow::Node getImportedModuleNodeStrict() {
+    result = this.getImportedModuleNode() and
+    not (
+      result = this.(ImportDeclaration).getAmbiguousDefaultImportNode() and
+      this.getImportedModule().(ES2015Module).hasBothNamedAndDefaultExports()
+    )
+  }
 }

javascript/ql/lib/semmle/javascript/dataflow/Portals.qll

@@ -11,6 +11,7 @@
  *
  * The API of this library is not stable yet and may change.
  */
+deprecated module;
 
 import javascript
 

javascript/ql/test/library-tests/Portals/PortalEntry.ql

@@ -1,5 +0,0 @@
-import javascript
-import semmle.javascript.dataflow.Portals
-
-from Portal p, boolean escapes
-select p, p.getAnEntryNode(escapes), escapes

javascript/ql/test/library-tests/Portals/PortalExit.ql

@@ -1,5 +0,0 @@
-import javascript
-import semmle.javascript.dataflow.Portals
-
-from Portal p, boolean isRemote
-select p, p.getAnExitNode(isRemote), isRemote

javascript/ql/test/library-tests/Portals/src/bluebird/index.js

@@ -1,9 +0,0 @@
-function Promise(exec) {
-  this.exec = exec;
-}
-
-Promise.prototype.then = function(fulfilled, rejected) {
-  rejected(null);
-};
-
-exports.Promise = Promise;

javascript/ql/test/library-tests/Portals/src/bluebird/package.json

@@ -1,3 +0,0 @@
-{
-    "name": "bluebird"
-}

javascript/ql/test/library-tests/Portals/src/bluebird/tst.js

@@ -1,2 +0,0 @@
-var Promise= require('./index').Promise;
-var p = new Promise();

javascript/ql/test/library-tests/Portals/src/cyclic/index.js

@@ -1,10 +0,0 @@
-function foo(cb) {
-  cb(foo);
-  return foo;
-}
-foo.f00 = foo;
-
-foo(foo);
-foo(foo());
-
-exports.foo = foo;

javascript/ql/test/library-tests/Portals/src/cyclic/package.json

@@ -1,3 +0,0 @@
-{
-  "name": "cyclic"
-}

javascript/ql/test/library-tests/Portals/src/m1/index.js

@@ -1 +0,0 @@
-module.exports = (x) => x;

javascript/ql/test/library-tests/Portals/src/m1/package.json

@@ -1,3 +0,0 @@
-{
-  "name": "m1"
-}

javascript/ql/test/library-tests/Portals/src/m2/main.js

@@ -1,16 +0,0 @@
-export function foo(p) {
-  console.log(p.x.y);
-  p.z = "hi";
-}
-
-export default class {
-  constructor(name) {
-    this.name = name;
-  }
-
-  m(x) {
-    console.log(x + " " + this.name);
-  }
-
-  static s(y) { return y; }
-};

javascript/ql/test/library-tests/Portals/src/m2/package.json

@@ -1,4 +0,0 @@
-{
-  "name": "m2",
-  "main": "main.js"
-}

javascript/ql/test/library-tests/Portals/src/m3/index.js

@@ -1,3 +0,0 @@
-var m1 = require("m1");
-
-module.exports = function() { console.log(m1("Hello, world!")); };

javascript/ql/test/library-tests/Portals/src/m3/package.json

@@ -1,8 +0,0 @@
-{
-  "name": "client",
-  "private": true,
-  "dependencies": {
-    "m1": "*",
-    "m2": "*"
-  }
-}

javascript/ql/test/library-tests/Portals/src/m3/tst.js

@@ -1 +0,0 @@
-require(".")();

javascript/ql/test/library-tests/Portals/src/m3/tst2.js

@@ -1,5 +0,0 @@
-import { foo } from "m2";
-var o = {
-  y: "?"
-};
-foo({ x: o });

javascript/ql/test/library-tests/Portals/src/m3/tst3.js

@@ -1,5 +0,0 @@
-import A from "m2";
-A.m("hi");
-A.s("there");
-new A("me").m("hi");
-new A("me").s("there");

javascript/ql/test/library-tests/Portals/src/m4/index.js

@@ -1 +0,0 @@
-exports.foo = function(x) {};

javascript/ql/test/library-tests/Portals/src/m4/package.json

@@ -1,3 +0,0 @@
-{
-  "name": "m4"
-}

javascript/ql/test/library-tests/Portals/src/m5/index.js

@@ -1,6 +0,0 @@
-const fs = require("fs"),
-    base64 = require("base-64/base64.js");
-
-module.exports.readBase64 = function (f) {
-    return base64.encode(String(fs.readFileSync(f)));
-};

javascript/ql/test/library-tests/Portals/src/m5/package.json

@@ -1,6 +0,0 @@
-{
-    "name": "m5",
-    "dependencies": {
-        "base-64": "*"
-    }
-}