From b44442662d40c5ef1fe6c5b64443767f4e4d036f Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 4 Sep 2025 12:35:24 +0200 Subject: [PATCH 1/4] JS: Refactor default import interop --- .../ql/lib/semmle/javascript/ApiGraphs.qll | 6 ++- .../lib/semmle/javascript/ES2015Modules.qll | 18 ++++--- .../ql/lib/semmle/javascript/Modules.qll | 53 ++++++++++++++++++- 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 3bb04f2686be..8f0a9047498d 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/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.getImportedModuleNodeIfUnambiguous() and m = imp.getImportedModule() and lbl = Label::member(prop) and rhs = m.getAnExportedValue(prop) @@ -1337,7 +1337,9 @@ 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.getImportedModuleNodeIfUnambiguous() = trackDefNode(nd, t.continue()) + | result = DataFlow::exportsVarNode(imp.getImportedModule()) or result = DataFlow::moduleVarNode(imp.getImportedModule()).getAPropertyRead("exports") diff --git a/javascript/ql/lib/semmle/javascript/ES2015Modules.qll b/javascript/ql/lib/semmle/javascript/ES2015Modules.qll index e7534449f55b..1d0e73bfe724 100644 --- a/javascript/ql/lib/semmle/javascript/ES2015Modules.qll +++ b/javascript/ql/lib/semmle/javascript/ES2015Modules.qll @@ -130,18 +130,18 @@ class ImportDeclaration extends Stmt, Import, @import_declaration { override DataFlow::Node getImportedModuleNode() { // `import * as http from 'http'` or `import http from `http`' + not exists(DataFlow::destructuredModuleImportNode(this)) and exists(ImportSpecifier is | is = this.getASpecifier() and result = DataFlow::valueNode(is) | - is instanceof ImportNamespaceSpecifier and - count(this.getASpecifier()) = 1 + is instanceof ImportNamespaceSpecifier 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" + // treat default imports as namespace imports. In cases where it causes ambiguity + // between named exports and the properties of a default-exported object, the caller + // of `getImportedModuleNode()` must check `isDefaultImport()` and correct the behaviour. + this.hasOnlyDefaultImport() ) or // `import { createServer } from 'http'` @@ -152,6 +152,12 @@ class ImportDeclaration extends Stmt, Import, @import_declaration { predicate isTypeOnly() { has_type_keyword(this) } override string getAPrimaryQlClass() { result = "ImportDeclaration" } + + private predicate hasOnlyDefaultImport() { + unique( | | this.getASpecifier()) instanceof ImportDefaultSpecifier + } + + override predicate isDefaultImport() { this.hasOnlyDefaultImport() } } /** A literal path expression appearing in an `import` declaration. */ diff --git a/javascript/ql/lib/semmle/javascript/Modules.qll b/javascript/ql/lib/semmle/javascript/Modules.qll index 3ddd5132d05d..a22062214563 100644 --- a/javascript/ql/lib/semmle/javascript/Modules.qll +++ b/javascript/ql/lib/semmle/javascript/Modules.qll @@ -179,7 +179,58 @@ 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 referring to imported module object. + * + * For example: + * ```js + * // ES2015 style + * import * as foo from "bar"; // gets the node for `foo` + * import foo from "bar"; // gets the node for `foo` (see note on default imports below) + * + * // CommonJS style + * require("bar"); // gets the node for the `require` call + * + * // AMD style + * define(["bar"], function(foo) { // gets the node for the `foo` parameter + * }) + * ``` + * + * For statements of form `import foo from "bar'`, this gives the node corresponding to `foo`. + * Technically this should refer to the export binding named `"default"`, not the whole module, but for compatibility with non-standard + * interpretations of default imports, this node is usually treated as also referring to the whole module. + * If this behaviour is not wanted, use `isDefaultImport()` to handle that case differently. */ abstract DataFlow::Node getImportedModuleNode(); + + /** + * Holds of the result of `getImportedModuleNode` actually refers to the export binding named `"default"`, + * as opposed an object whose properties correspond to the export bindings of the imported module. + * + * For compatibility with non-standard interpretations of `default` imports, the default + * import is usually returned by `getImportedModuleNode()`. If such behaviour is not wanted, + * this predicate can be used to handle that case differently. + * + * For example, `getImportedModuleNode()` returns `foo` in both of these imports, but `isDefaultImport()` + * only holds for the first one: + * ```js + * import foo from "bar"; + * import * as foo from "bar"; + * ``` + */ + predicate isDefaultImport() { none() } + + /** + * Gets the same as `getImportedModuleNode()` expect this has no result for default imports when the target module + * has both default and named exports. + * + * This is to avoid ambiguity between named export bindings and the properties of the default-exported object. + */ + pragma[nomagic] + final DataFlow::Node getImportedModuleNodeIfUnambiguous() { + if + this.isDefaultImport() and + this.getImportedModule().(ES2015Module).hasBothNamedAndDefaultExports() + then none() + else result = this.getImportedModuleNode() + } } From 52dedb6507a7d39f333df6ef72feb69edda8be09 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 4 Sep 2025 14:02:39 +0200 Subject: [PATCH 2/4] Apply spell checking fixes from copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- javascript/ql/lib/semmle/javascript/Modules.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Modules.qll b/javascript/ql/lib/semmle/javascript/Modules.qll index a22062214563..26174dc80bb4 100644 --- a/javascript/ql/lib/semmle/javascript/Modules.qll +++ b/javascript/ql/lib/semmle/javascript/Modules.qll @@ -203,7 +203,7 @@ abstract class Import extends AstNode { abstract DataFlow::Node getImportedModuleNode(); /** - * Holds of the result of `getImportedModuleNode` actually refers to the export binding named `"default"`, + * Holds if the result of `getImportedModuleNode` actually refers to the export binding named `"default"`, * as opposed an object whose properties correspond to the export bindings of the imported module. * * For compatibility with non-standard interpretations of `default` imports, the default @@ -220,7 +220,7 @@ abstract class Import extends AstNode { predicate isDefaultImport() { none() } /** - * Gets the same as `getImportedModuleNode()` expect this has no result for default imports when the target module + * Gets the same as `getImportedModuleNode()` except this has no result for default imports when the target module * has both default and named exports. * * This is to avoid ambiguity between named export bindings and the properties of the default-exported object. From 9d38a3a7401bac09b63790a768323203da3f7774 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 4 Sep 2025 14:03:26 +0200 Subject: [PATCH 3/4] JS: Fix QL4QL spell checking errors --- javascript/ql/lib/semmle/javascript/ES2015Modules.qll | 2 +- javascript/ql/lib/semmle/javascript/Modules.qll | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ES2015Modules.qll b/javascript/ql/lib/semmle/javascript/ES2015Modules.qll index 1d0e73bfe724..5dbd91ef980b 100644 --- a/javascript/ql/lib/semmle/javascript/ES2015Modules.qll +++ b/javascript/ql/lib/semmle/javascript/ES2015Modules.qll @@ -140,7 +140,7 @@ class ImportDeclaration extends Stmt, Import, @import_declaration { // For compatibility with the non-standard implementation of default imports, // treat default imports as namespace imports. In cases where it causes ambiguity // between named exports and the properties of a default-exported object, the caller - // of `getImportedModuleNode()` must check `isDefaultImport()` and correct the behaviour. + // of `getImportedModuleNode()` must check `isDefaultImport()` and correct the behavior. this.hasOnlyDefaultImport() ) or diff --git a/javascript/ql/lib/semmle/javascript/Modules.qll b/javascript/ql/lib/semmle/javascript/Modules.qll index 26174dc80bb4..87a39434c545 100644 --- a/javascript/ql/lib/semmle/javascript/Modules.qll +++ b/javascript/ql/lib/semmle/javascript/Modules.qll @@ -198,7 +198,7 @@ abstract class Import extends AstNode { * For statements of form `import foo from "bar'`, this gives the node corresponding to `foo`. * Technically this should refer to the export binding named `"default"`, not the whole module, but for compatibility with non-standard * interpretations of default imports, this node is usually treated as also referring to the whole module. - * If this behaviour is not wanted, use `isDefaultImport()` to handle that case differently. + * If this behavior is not wanted, use `isDefaultImport()` to handle that case differently. */ abstract DataFlow::Node getImportedModuleNode(); @@ -207,7 +207,7 @@ abstract class Import extends AstNode { * as opposed an object whose properties correspond to the export bindings of the imported module. * * For compatibility with non-standard interpretations of `default` imports, the default - * import is usually returned by `getImportedModuleNode()`. If such behaviour is not wanted, + * import is usually returned by `getImportedModuleNode()`. If such behavior is not wanted, * this predicate can be used to handle that case differently. * * For example, `getImportedModuleNode()` returns `foo` in both of these imports, but `isDefaultImport()` From d3d3fc28b0d13a88df4adb04dce578d4b2b97f31 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 4 Sep 2025 14:04:52 +0200 Subject: [PATCH 4/4] JS: Use conjunction instead of if none() --- javascript/ql/lib/semmle/javascript/Modules.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Modules.qll b/javascript/ql/lib/semmle/javascript/Modules.qll index 87a39434c545..e4b8941e98c6 100644 --- a/javascript/ql/lib/semmle/javascript/Modules.qll +++ b/javascript/ql/lib/semmle/javascript/Modules.qll @@ -227,10 +227,10 @@ abstract class Import extends AstNode { */ pragma[nomagic] final DataFlow::Node getImportedModuleNodeIfUnambiguous() { - if + result = this.getImportedModuleNode() and + not ( this.isDefaultImport() and this.getImportedModule().(ES2015Module).hasBothNamedAndDefaultExports() - then none() - else result = this.getImportedModuleNode() + ) } }