diff --git a/javascript/ql/integration-tests/query-suite/not_included_in_qls.expected b/javascript/ql/integration-tests/query-suite/not_included_in_qls.expected index fa52a97a4e4a..cb3ab8646b33 100644 --- a/javascript/ql/integration-tests/query-suite/not_included_in_qls.expected +++ b/javascript/ql/integration-tests/query-suite/not_included_in_qls.expected @@ -44,7 +44,6 @@ ql/javascript/ql/src/Metrics/FUseOfES6.ql ql/javascript/ql/src/Metrics/FunCyclomaticComplexity.ql ql/javascript/ql/src/Metrics/FunLinesOfCode.ql ql/javascript/ql/src/NodeJS/CyclicImport.ql -ql/javascript/ql/src/NodeJS/DubiousImport.ql ql/javascript/ql/src/NodeJS/UnresolvableImport.ql ql/javascript/ql/src/NodeJS/UnusedDependency.ql ql/javascript/ql/src/Performance/NonLocalForIn.ql diff --git a/javascript/ql/lib/semmle/javascript/Modules.qll b/javascript/ql/lib/semmle/javascript/Modules.qll index 3ddd5132d05d..74ba5adfe61c 100644 --- a/javascript/ql/lib/semmle/javascript/Modules.qll +++ b/javascript/ql/lib/semmle/javascript/Modules.qll @@ -25,8 +25,8 @@ abstract class Module extends TopLevel { /** Gets a module from which this module imports. */ Module getAnImportedModule() { result = this.getAnImport().getImportedModule() } - /** Gets a symbol exported by this module. */ - string getAnExportedSymbol() { exists(this.getAnExportedValue(result)) } + /** DEPRECATED. Use `exists(getAnExportedValue(name))` instead. */ + deprecated string getAnExportedSymbol() { exists(this.getAnExportedValue(result)) } /** * Get a value that is explicitly exported from this module with under `name`. diff --git a/javascript/ql/lib/semmle/javascript/NodeJS.qll b/javascript/ql/lib/semmle/javascript/NodeJS.qll index 7e9e2fdea904..0fb9ae5ce0fc 100644 --- a/javascript/ql/lib/semmle/javascript/NodeJS.qll +++ b/javascript/ql/lib/semmle/javascript/NodeJS.qll @@ -57,19 +57,6 @@ class NodeModule extends Module { result.getAValue() = this.getAModuleExportsValue() } - /** Gets a symbol exported by this module. */ - override string getAnExportedSymbol() { - result = super.getAnExportedSymbol() - or - result = this.getAnImplicitlyExportedSymbol() - or - // getters and the like. - exists(DataFlow::PropWrite pwn | - pwn.getBase() = this.getAModuleExportsNode() and - result = pwn.getPropertyName() - ) - } - override DataFlow::Node getAnExportedValue(string name) { // a property write whose base is `exports` or `module.exports` exists(DataFlow::PropWrite pwn | result = pwn.getRhs() | @@ -123,29 +110,6 @@ class NodeModule extends Module { ) } - /** Gets a symbol that the module object inherits from its prototypes. */ - private string getAnImplicitlyExportedSymbol() { - exists(ExternalConstructor ec | ec = this.getPrototypeOfExportedExpr() | - result = ec.getAMember().getName() - or - ec instanceof FunctionExternal and result = "prototype" - or - ec instanceof ArrayExternal and - exists(NumberLiteral nl | result = nl.getValue() and exists(result.toInt())) - ) - } - - /** Gets an externs declaration of the prototype object of a value exported by this module. */ - private ExternalConstructor getPrototypeOfExportedExpr() { - exists(AbstractValue exported | exported = this.getAModuleExportsValue() | - result instanceof ObjectExternal - or - exported instanceof AbstractFunction and result instanceof FunctionExternal - or - exported instanceof AbstractOtherObject and result instanceof ArrayExternal - ) - } - deprecated override predicate searchRoot(PathExpr path, Folder searchRoot, int priority) { path.getEnclosingModule() = this and exists(string pathval | pathval = path.getValue() | diff --git a/javascript/ql/src/NodeJS/DubiousImport.qhelp b/javascript/ql/src/NodeJS/DubiousImport.qhelp deleted file mode 100644 index db3a39b2b3b7..000000000000 --- a/javascript/ql/src/NodeJS/DubiousImport.qhelp +++ /dev/null @@ -1,47 +0,0 @@ - - - -

-Since JavaScript is a dynamically typed language, module imports in node.js are not statically checked -for correctness: calls to require simply return an object containing all the exports of -the imported module, and accessing a member that was not, in fact, exported, yields -undefined. This is most likely unintentional and usually indicates a bug. -

- -
- - -

-Examine the import in question and determine the correct name of the symbol to import. -

- -
- - -

-In the following example, module point.js exports the function Point by -assigning it to module.exports. The client module client.js tries to -import it by reading from the Point property, but since this property does not exist -the result will be undefined, and the new invocation will fail. -

- - - -

-Instead of reading the Point property, client.js should directly use -the result of the require call: -

- - - -
- - - -
  • Node.js Manual: Modules.
  • - - -
    -
    diff --git a/javascript/ql/src/NodeJS/DubiousImport.ql b/javascript/ql/src/NodeJS/DubiousImport.ql deleted file mode 100644 index 97ed336e22bc..000000000000 --- a/javascript/ql/src/NodeJS/DubiousImport.ql +++ /dev/null @@ -1,68 +0,0 @@ -/** - * @name Dubious import - * @description Importing a symbol from a module that does not export it most likely indicates a bug. - * @kind problem - * @problem.severity warning - * @id js/node/import-without-export - * @tags reliability - * maintainability - * frameworks/node.js - * @precision low - */ - -import javascript - -/** Holds if `m` is likely to have exports that are not picked up by the analysis. */ -predicate hasUntrackedExports(NodeModule m) { - // look for assignments of the form `module.exports[p] = ...`, where we cannot - // determine the name of the exported property being assigned - exists(DataFlow::PropWrite pwn | - pwn.getBase().analyze().getAValue() = m.getAModuleExportsValue() and - not exists(pwn.getPropertyName()) - ) - or - // look for assignments of the form `module.exports = exp` where `exp` is indefinite - exists(AbstractModuleObject am, AnalyzedPropertyWrite apw, DataFlow::AnalyzedNode exp | - am.getModule() = m and - apw.writes(am, "exports", exp) and - exp.getAValue().isIndefinite(_) - ) - or - // look for function calls of the form `f(module.exports)` - exists(InvokeExpr invk | invk.getAnArgument().analyze().getAValue() = m.getAModuleExportsValue()) -} - -/** - * Holds if there is an assignment anywhere defining `prop` on the result of - * a `require` import of module `m`. - */ -predicate propDefinedOnRequire(NodeModule m, string prop) { - exists(DataFlow::ModuleImportNode imp | - imp.asExpr().(Require).getImportedModule() = m and - exists(imp.getAPropertyWrite(prop)) - ) -} - -/** - * Holds if the base expression of `pacc` could refer to the result of - * a `require` import of module `m`. - */ -predicate propAccessOn(PropAccess pacc, NodeModule m) { - exists(DataFlow::ModuleImportNode imp | - imp.asExpr().(Require).getImportedModule() = m and - imp.flowsToExpr(pacc.getBase()) - ) -} - -from NodeModule m, PropAccess pacc, string prop -where - propAccessOn(pacc, m) and - count(NodeModule mm | propAccessOn(pacc, mm)) = 1 and - prop = pacc.getPropertyName() and - // m doesn't export 'prop' - not prop = m.getAnExportedSymbol() and - // 'prop' isn't otherwise defined on m - not propDefinedOnRequire(m, prop) and - // m doesn't use complicated exports - not hasUntrackedExports(m) -select pacc, "Module $@ does not export symbol " + prop + ".", m, m.getName() diff --git a/javascript/ql/test/library-tests/NodeJS/tests.expected b/javascript/ql/test/library-tests/NodeJS/tests.expected index 964b784cf93f..f66d996465ce 100644 --- a/javascript/ql/test/library-tests/NodeJS/tests.expected +++ b/javascript/ql/test/library-tests/NodeJS/tests.expected @@ -1,11 +1,3 @@ -module_getAnExportedSymbol -| b.js:1:1:8:0 | | sneaky | -| d.js:1:1:7:15 | | baz | -| reexport/a.js:1:1:3:1 | | foo | -| reexport/b.js:1:1:6:1 | | bar | -| reexport/b.js:1:1:6:1 | | foo | -| sub/c.js:1:1:4:0 | | foo | -| sub/f.js:1:1:4:17 | | bar | module_getAnImport | a.js:1:1:14:0 | | a.js:1:9:1:22 | require('./b') | | a.js:1:1:14:0 | | a.js:2:7:2:19 | require('fs') | diff --git a/javascript/ql/test/library-tests/NodeJS/tests.ql b/javascript/ql/test/library-tests/NodeJS/tests.ql index f5e0f58dc467..27f5bd7da7fc 100644 --- a/javascript/ql/test/library-tests/NodeJS/tests.ql +++ b/javascript/ql/test/library-tests/NodeJS/tests.ql @@ -1,9 +1,5 @@ import javascript -query predicate module_getAnExportedSymbol(NodeModule m, string symbol) { - symbol = m.getAnExportedSymbol() -} - query predicate module_getAnImport(NodeModule m, Import imp) { imp = m.getAnImport() } query predicate module_getAnImportedModule(NodeModule m, Module mod) {