From a7116402b55bbe0f4d17ff7e1dc16fb75bc35ddb Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 11 Sep 2025 13:48:17 +0200 Subject: [PATCH 01/26] wip --- javascript/ql/lib/semmle/javascript/AMD.qll | 44 +- .../ql/lib/semmle/javascript/NodeJS.qll | 45 +- .../javascript/dataflow/AbstractValues.qll | 4 +- .../javascript/dataflow/TypeInference.qll | 1 - .../dataflow/internal/AbstractValuesImpl.qll | 8 +- .../internal/InterModuleTypeInference.qll | 433 ------------------ 6 files changed, 42 insertions(+), 493 deletions(-) delete mode 100644 javascript/ql/lib/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll diff --git a/javascript/ql/lib/semmle/javascript/AMD.qll b/javascript/ql/lib/semmle/javascript/AMD.qll index 4828aff27cc4..996e2ea3fdfb 100644 --- a/javascript/ql/lib/semmle/javascript/AMD.qll +++ b/javascript/ql/lib/semmle/javascript/AMD.qll @@ -191,30 +191,12 @@ class AmdModuleDefinition extends CallExpr instanceof AmdModuleDefinition::Range } /** + * DEPRECATED. Abstract values are no longer used to track module exports. + * * Gets an abstract value representing one or more values that may flow * into this module's `module.exports` property. */ - DefiniteAbstractValue getAModuleExportsValue() { - result = [this.getAnImplicitExportsValue(), this.getAnExplicitExportsValue()] - } - - pragma[noinline, nomagic] - private AbstractValue getAnImplicitExportsValue() { - // implicit exports: anything that is returned from the factory function - result = this.getModuleExpr().analyze().getAValue() - } - - pragma[noinline] - private AbstractValue getAnExplicitExportsValue() { - // explicit exports: anything assigned to `module.exports` - exists(AbstractProperty moduleExports, AmdModule m | - this = m.getDefine() and - moduleExports.getBase().(AbstractModuleObject).getModule() = m and - moduleExports.getPropertyName() = "exports" - | - result = moduleExports.getAValue() - ) - } + deprecated DefiniteAbstractValue getAModuleExportsValue() { none() } /** * Gets a call to `require` inside this module. @@ -357,21 +339,19 @@ class AmdModule extends Module { AmdModuleDefinition getDefine() { amdModuleTopLevel(result, this) } override DataFlow::Node getAnExportedValue(string name) { - exists(DataFlow::PropWrite pwn | result = pwn.getRhs() | - pwn.getBase().analyze().getAValue() = this.getDefine().getAModuleExportsValue() and - name = pwn.getPropertyName() - ) + none() // TODO: AMD getAnExportedValue } override DataFlow::Node getABulkExportedNode() { + // TODO: AMD getABulkExportedNode // Assigned to `module.exports` via the factory's `module` parameter - exists(AbstractModuleObject m, DataFlow::PropWrite write | - m.getModule() = this and - write.getPropertyName() = "exports" and - write.getBase().analyze().getAValue() = m and - result = write.getRhs() - ) - or + // exists(AbstractModuleObject m, DataFlow::PropWrite write | + // m.getModule() = this and + // write.getPropertyName() = "exports" and + // write.getBase().analyze().getAValue() = m and + // result = write.getRhs() + // ) + // or // Returned from factory function result = this.getDefine().getModuleExpr().flow() } diff --git a/javascript/ql/lib/semmle/javascript/NodeJS.qll b/javascript/ql/lib/semmle/javascript/NodeJS.qll index 7e9e2fdea904..30c706b9da39 100644 --- a/javascript/ql/lib/semmle/javascript/NodeJS.qll +++ b/javascript/ql/lib/semmle/javascript/NodeJS.qll @@ -37,14 +37,25 @@ class NodeModule extends Module { * into this module's `module.exports` property. */ pragma[noinline] - DefiniteAbstractValue getAModuleExportsValue() { - result = this.getAModuleExportsProperty().getAValue() + deprecated DefiniteAbstractValue getAModuleExportsValue() { none() } + + /** + * Gets the `SourceNode` corresponding to the value of `module`. + */ + private DataFlow::SourceNode getModuleSourceNode() { + result = DataFlow::ssaDefinitionNode(Ssa::implicitInit(this.getModuleVariable())) } - pragma[noinline] - private AbstractProperty getAModuleExportsProperty() { - result.getBase().(AbstractModuleObject).getModule() = this and - result.getPropertyName() = "exports" + /** + * Gets a `SourceNode` corresponding to the initial value of `module.exports` or + * anything assigned to `module.exports`. + */ + private DataFlow::SourceNode getExportsSourceNode() { + result = DataFlow::ssaDefinitionNode(Ssa::implicitInit(this.getExportsVariable())) + or + result = this.getModuleSourceNode().getAPropertyWrite("exports").getRhs().getALocalSource() + or + result = this.getModuleSourceNode().getAPropertyRead("exports") } /** @@ -52,9 +63,8 @@ class NodeModule extends Module { * For performance this predicate only computes relevant expressions (in `getAModuleExportsCandidate`). * So if using this predicate - consider expanding the list of relevant expressions. */ - DataFlow::AnalyzedNode getAModuleExportsNode() { - result = getAModuleExportsCandidate() and - result.getAValue() = this.getAModuleExportsValue() + deprecated DataFlow::AnalyzedNode getAModuleExportsNode() { + result = this.getExportsSourceNode().getALocalUse() } /** Gets a symbol exported by this module. */ @@ -64,21 +74,15 @@ class NodeModule extends Module { result = this.getAnImplicitlyExportedSymbol() or // getters and the like. - exists(DataFlow::PropWrite pwn | - pwn.getBase() = this.getAModuleExportsNode() and - result = pwn.getPropertyName() - ) + result = this.getExportsSourceNode().getAPropertyWrite().getPropertyName() } override DataFlow::Node getAnExportedValue(string name) { // a property write whose base is `exports` or `module.exports` - exists(DataFlow::PropWrite pwn | result = pwn.getRhs() | - pwn.getBase() = this.getAModuleExportsNode() and - name = pwn.getPropertyName() - ) + result = this.getExportsSourceNode().getAPropertyWrite(name).getRhs() or // a re-export using spread-operator. E.g. `const foo = require("./foo"); module.exports = {bar: bar, ...foo};` - exists(ObjectExpr obj | obj = this.getAModuleExportsNode().asExpr() | + exists(ObjectExpr obj | obj = this.getExportsSourceNode().asExpr() | result = obj.getAProperty() .(SpreadProperty) @@ -99,16 +103,15 @@ class NodeModule extends Module { // } exists(DynamicPropertyAccess::EnumeratedPropName read, Import imp, DataFlow::PropWrite write | read.getSourceObject().getALocalSource().asExpr() = imp and + write = this.getExportsSourceNode().getAPropertyWrite() and getASourceProp(read) = write.getRhs() and - write.getBase() = this.getAModuleExportsNode() and write.getPropertyNameExpr().flow().getImmediatePredecessor*() = read and result = imp.getImportedModule().getAnExportedValue(name) ) or // an externs definition (where appropriate) exists(PropAccess pacc | result = DataFlow::valueNode(pacc) | - pacc.getBase() = this.getAModuleExportsNode().asExpr() and - name = pacc.getPropertyName() and + pacc = this.getExportsSourceNode().getAPropertyRead(name).asExpr() and this.isExterns() and exists(pacc.getDocumentation()) ) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/AbstractValues.qll b/javascript/ql/lib/semmle/javascript/dataflow/AbstractValues.qll index 41509516cc16..9d474e39219d 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/AbstractValues.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/AbstractValues.qll @@ -390,7 +390,7 @@ class AbstractGlobalObject extends DefiniteAbstractValue, TAbstractGlobalObject /** * An abstract value representing a CommonJS `module` object. */ -class AbstractModuleObject extends DefiniteAbstractValue, TAbstractModuleObject { +deprecated class AbstractModuleObject extends DefiniteAbstractValue, TAbstractModuleObject { /** Gets the module whose `module` object this abstract value represents. */ Module getModule() { this = TAbstractModuleObject(result) } @@ -414,7 +414,7 @@ class AbstractModuleObject extends DefiniteAbstractValue, TAbstractModuleObject /** * An abstract value representing a CommonJS `exports` object. */ -class AbstractExportsObject extends DefiniteAbstractValue, TAbstractExportsObject { +deprecated class AbstractExportsObject extends DefiniteAbstractValue, TAbstractExportsObject { /** Gets the module whose `exports` object this abstract value represents. */ Module getModule() { this = TAbstractExportsObject(result) } diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll index 32ad78eb2c6d..53fab7371c2a 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll @@ -30,7 +30,6 @@ import AbstractValues private import InferredTypes private import Refinements import internal.BasicExprTypeInference -import internal.InterModuleTypeInference import internal.InterProceduralTypeInference import internal.PropertyTypeInference import internal.VariableTypeInference diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractValuesImpl.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractValuesImpl.qll index 6bcef1dc4128..31e8c1ac9ed7 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractValuesImpl.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractValuesImpl.qll @@ -43,10 +43,10 @@ newtype TAbstractValue = TAbstractArguments(Function f) { exists(f.getArgumentsVariable().getAnAccess()) } or /** An abstract representation of the global object. */ TAbstractGlobalObject() or - /** An abstract representation of a `module` object. */ - TAbstractModuleObject(Module m) or - /** An abstract representation of a `exports` object. */ - TAbstractExportsObject(Module m) or + /** DEPRECATED. This abstract value is no longer tracked. */ + deprecated TAbstractModuleObject(Module m) { none() } or + /** DEPRECATED. This abstract value is no longer tracked. */ + deprecated TAbstractExportsObject(Module m) { none() } or /** An abstract representation of all objects arising from an object literal expression. */ TAbstractObjectLiteral(ObjectExpr oe) or /** diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll deleted file mode 100644 index 3134d76d8f16..000000000000 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll +++ /dev/null @@ -1,433 +0,0 @@ -/** - * INTERNAL: Do not use directly; use `semmle.javascript.dataflow.TypeInference` instead. - * - * Provides classes implementing type inference across imports. - */ - -private import javascript -private import AbstractValuesImpl -private import semmle.javascript.dataflow.InferredTypes -private import AbstractPropertiesImpl - -/** - * Flow analysis for ECMAScript 2015 imports as variable definitions. - */ -private class AnalyzedImportSpecifier extends AnalyzedVarDef, @import_specifier { - ImportDeclaration id; - - AnalyzedImportSpecifier() { - this = id.getASpecifier() and - exists(id.resolveImportedPath()) and - not this.(ImportSpecifier).isTypeOnly() - } - - override DataFlow::AnalyzedNode getRhs() { result.(AnalyzedImport).getImportSpecifier() = this } - - override predicate isIncomplete(DataFlow::Incompleteness cause) { - // mark as incomplete if the import could rely on the lookup path - mayDependOnLookupPath(id.getImportedPathString()) and - cause = "import" - or - // mark as incomplete if we cannot fully analyze this import - exists(Module m | m = id.resolveImportedPath() | - mayDynamicallyComputeExports(m) - or - incompleteExport(m, this.(ImportSpecifier).getImportedName()) - ) and - cause = "import" - } -} - -/** - * Holds if resolving `path` may depend on the lookup path, that is, - * it does not start with `.` or `/`. - */ -bindingset[path] -private predicate mayDependOnLookupPath(string path) { - exists(string firstChar | firstChar = path.charAt(0) | firstChar != "." and firstChar != "/") -} - -/** - * Holds if `m` may dynamically compute its exports. - */ -private predicate mayDynamicallyComputeExports(Module m) { - // if `m` accesses its `module` or `exports` variable, we conservatively assume the worst; - // in particular, this makes all imports from CommonJS modules indefinite - exists(Variable v, VarAccess va | v.getName() = "module" or v.getName() = "exports" | - va = v.getAnAccess() and - ( - v = m.getScope().getAVariable() - or - // be conservative in case our heuristics for detecting Node.js modules fail - v instanceof GlobalVariable and va.getTopLevel() = m - ) - ) - or - // AMD modules can export arbitrary objects, so an import is essentially a property read - // and hence must be considered indefinite - m instanceof AmdModule - or - // `m` re-exports all exports of some other module that dynamically computes its exports - exists(BulkReExportDeclaration rexp | rexp = m.(ES2015Module).getAnExport() | - mayDynamicallyComputeExports(rexp.getReExportedModule()) - ) -} - -/** - * Holds if `x` is imported from `m`, possibly through a chain of re-exports. - */ -private predicate relevantExport(ES2015Module m, string x) { - exists(ImportDeclaration id | - id.getImportedModule() = m and - x = id.getASpecifier().getImportedName() - ) - or - exists(ReExportDeclaration rexp | - rexp.getReExportedModule() = m and - reExportsAs(rexp, x, _) - ) -} - -/** - * Holds if `rexp` re-exports `x` as `y`. - */ -private predicate reExportsAs(ReExportDeclaration rexp, string x, string y) { - relevantExport(rexp.getEnclosingModule(), y) and - ( - exists(ExportSpecifier spec | spec = rexp.(SelectiveReExportDeclaration).getASpecifier() | - x = spec.getLocalName() and - y = spec.getExportedName() - ) - or - rexp instanceof BulkReExportDeclaration and - x = y - ) -} - -/** - * Holds if `m` re-exports `y`, but we cannot fully analyze this export. - */ -private predicate incompleteExport(ES2015Module m, string y) { - exists(ReExportDeclaration rexp | rexp = m.getAnExport() | - exists(string x | reExportsAs(rexp, x, y) | - // path resolution could rely on lookup path - mayDependOnLookupPath(rexp.getImportedPath().getStringValue()) - or - // unresolvable path - not exists(rexp.getReExportedModule()) - or - exists(Module n | n = rexp.getReExportedModule() | - // re-export from CommonJS/AMD - mayDynamicallyComputeExports(n) - or - // recursively incomplete - incompleteExport(n, x) - ) - ) - or - // namespace re-export - exists(ExportNamespaceSpecifier ns | - ns.getExportDeclaration() = rexp and - ns.getExportedName() = y - ) - ) -} - -/** - * Flow analysis for import specifiers, interpreted as implicit reads of - * properties of the `module.exports` object of the imported module. - */ -private class AnalyzedImport extends AnalyzedPropertyRead, DataFlow::ValueNode { - Module imported; - override ImportSpecifier astNode; - - AnalyzedImport() { - exists(ImportDeclaration id | - astNode = id.getASpecifier() and - not astNode.isTypeOnly() and - imported = id.getImportedModule() - ) - } - - /** Gets the import specifier being analyzed. */ - ImportSpecifier getImportSpecifier() { result = astNode } - - override predicate reads(AbstractValue base, string propName) { - exists(AbstractProperty exports | - exports = MkAbstractProperty(TAbstractModuleObject(imported), "exports") - | - base = exports.getALocalValue() and - propName = astNode.getImportedName() - ) - or - // when importing CommonJS/AMD modules from ES2015, `module.exports` appears - // as the default export - ( - not imported instanceof ES2015Module - or - // CommonJS/AMD module generated by TypeScript compiler - imported.getAStmt() instanceof ExportAssignDeclaration - ) and - astNode.getImportedName() = "default" and - base = TAbstractModuleObject(imported) and - propName = "exports" - } -} - -/** - * Flow analysis for namespace imports. - */ -private class AnalyzedNamespaceImport extends AnalyzedImport { - override ImportNamespaceSpecifier astNode; - - override predicate reads(AbstractValue base, string propName) { - base = TAbstractModuleObject(imported) and - propName = "exports" - } -} - -/** - * Flow analysis for namespace imports. - */ -private class AnalyzedDestructuredImport extends AnalyzedPropertyRead { - Module imported; - - AnalyzedDestructuredImport() { - exists(ImportDeclaration id | - this = DataFlow::destructuredModuleImportNode(id) and - imported = id.getImportedModule() - ) - } - - override predicate reads(AbstractValue base, string propName) { - base = TAbstractModuleObject(imported) and - propName = "exports" - } -} - -/** - * A call to `require`, interpreted as an implicit read of - * the `module.exports` property of the imported module. - */ -class AnalyzedRequireCall extends AnalyzedPropertyRead, DataFlow::ValueNode { - Module required; - - AnalyzedRequireCall() { required = astNode.(Require).getImportedModule() } - - override predicate reads(AbstractValue base, string propName) { - base = TAbstractModuleObject(required) and - propName = "exports" - } -} - -/** - * A special TypeScript `require` call in an import-assignment, - * interpreted as an implicit of the `module.exports` property of the imported module. - */ -class AnalyzedExternalModuleReference extends AnalyzedPropertyRead, DataFlow::ValueNode { - Module required; - - AnalyzedExternalModuleReference() { - required = astNode.(ExternalModuleReference).getImportedModule() - } - - override predicate reads(AbstractValue base, string propName) { - base = TAbstractModuleObject(required) and - propName = "exports" - } -} - -/** - * Flow analysis for AMD exports. - */ -private class AnalyzedAmdExport extends AnalyzedPropertyWrite, DataFlow::ValueNode { - AmdModule amd; - - AnalyzedAmdExport() { astNode = amd.getDefine().getModuleExpr() } - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractModuleObject(amd) and - propName = "exports" and - source = this - } -} - -/** - * Flow analysis for AMD imports, interpreted as an implicit read of - * the `module.exports` property of the imported module. - */ -private class AnalyzedAmdImport extends AnalyzedPropertyRead, DataFlow::Node { - Module required; - - AnalyzedAmdImport() { - exists(AmdModule amd, Expr dep | - exists(Parameter p | - amd.getDefine().dependencyParameter(dep, p) and - this = DataFlow::parameterNode(p) - ) - or - exists(CallExpr requireCall | - requireCall = amd.getDefine().getARequireCall() and - dep = requireCall.getAnArgument() and - this = requireCall.flow() - ) - | - required = dep.(Import).getImportedModule() - ) - } - - override predicate reads(AbstractValue base, string propName) { - base = TAbstractModuleObject(required) and - propName = "exports" - } -} - -/** - * Flow analysis for parameters corresponding to AMD imports. - */ -private class AnalyzedAmdParameter extends AnalyzedVarDef, @var_decl { - AnalyzedAmdImport imp; - - AnalyzedAmdParameter() { imp = DataFlow::parameterNode(this) } - - override AbstractValue getAnRhsValue() { result = imp.getALocalValue() } -} - -/** - * Flow analysis for exports that export a single value. - */ -private class AnalyzedValueExport extends AnalyzedPropertyWrite, DataFlow::ValueNode { - ExportDeclaration export; - string name; - - AnalyzedValueExport() { this = export.getSourceNode(name) } - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractExportsObject(export.getEnclosingModule()) and - propName = name and - source = export.getSourceNode(name).analyze() - } -} - -/** - * Flow analysis for exports that export a binding. - */ -private class AnalyzedVariableExport extends AnalyzedPropertyWrite, DataFlow::ValueNode { - ExportDeclaration export; - string name; - AnalyzedVarDef varDef; - - AnalyzedVariableExport() { - export.exportsAs(varDef.getAVariable(), name) and - astNode = varDef.getTarget() - } - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractExportsObject(export.getEnclosingModule()) and - propName = name and - ( - source = varDef.getSource().analyze() - or - varDef.getTarget() instanceof DestructuringPattern and - source = export.getSourceNode(propName) - ) - } - - override predicate writesValue(AbstractValue baseVal, string propName, AbstractValue val) { - baseVal = TAbstractExportsObject(export.getEnclosingModule()) and - propName = name and - val = varDef.getAnAssignedValue() - } -} - -/** - * Flow analysis for default exports. - */ -private class AnalyzedDefaultExportDeclaration extends AnalyzedValueExport { - override ExportDefaultDeclaration export; - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - super.writes(baseVal, propName, source) - or - // some (presumably historic) transpilers treat `export default foo` as `module.exports = foo`, - // so allow that semantics, too, but only if there isn't a named export in the same module. - exists(Module m | - super.writes(TAbstractExportsObject(m), "default", source) and - baseVal = TAbstractModuleObject(m) and - propName = "exports" and - not m.(ES2015Module).getAnExport() instanceof ExportNamedDeclaration - ) - } -} - -/** - * Flow analysis for TypeScript export assignments. - */ -private class AnalyzedExportAssign extends AnalyzedPropertyWrite, DataFlow::ValueNode { - ExportAssignDeclaration exportAssign; - - AnalyzedExportAssign() { astNode = exportAssign.getExpression() } - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractModuleObject(exportAssign.getTopLevel()) and - propName = "exports" and - source = this - } -} - -/** - * Flow analysis for assignments to the `exports` variable in a Closure module. - */ -private class AnalyzedClosureExportAssign extends AnalyzedPropertyWrite, DataFlow::ValueNode { - override AssignExpr astNode; - - AnalyzedClosureExportAssign() { - astNode.getLhs() = any(Closure::ClosureModule mod).getExportsVariable().getAReference() - } - - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractModuleObject(astNode.getTopLevel()) and - propName = "exports" and - source = astNode.getRhs().flow() - } -} - -/** - * Read of a global access path exported by a Closure library. - * - * This adds a direct flow edge to the assigned value. - */ -private class AnalyzedClosureGlobalAccessPath extends AnalyzedNode, AnalyzedPropertyRead { - string accessPath; - - AnalyzedClosureGlobalAccessPath() { - accessPath = Closure::getClosureNamespaceFromSourceNode(this) - } - - override predicate reads(AbstractValue base, string propName) { - exists(Closure::ClosureModule mod | - mod.getClosureNamespace() = accessPath and - base = TAbstractModuleObject(mod) and - propName = "exports" - ) - } -} - -/** - * A namespace export declaration analyzed as a property write. - */ -private class AnalyzedExportNamespaceSpecifier extends AnalyzedPropertyWrite, DataFlow::ValueNode { - override ExportNamespaceSpecifier astNode; - ReExportDeclaration decl; - - AnalyzedExportNamespaceSpecifier() { - decl = astNode.getExportDeclaration() and - not decl.isTypeOnly() - } - - override predicate writesValue(AbstractValue baseVal, string propName, AbstractValue value) { - baseVal = TAbstractExportsObject(this.getTopLevel()) and - propName = astNode.getExportedName() and - value = TAbstractExportsObject(decl.getReExportedModule()) - } -} From 43ef82bc96b82a0b378aaf07792978e78339706c Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 11 Sep 2025 13:58:09 +0200 Subject: [PATCH 02/26] JS: Deprecate getAnExportedSymbol() and DubiousImport.ql getAnExportedSymbol() has a complex implementation that I'd rather not want to migrate given that it's only used by a dead query. --- .../query-suite/not_included_in_qls.expected | 1 - .../ql/lib/semmle/javascript/Modules.qll | 4 +- .../ql/lib/semmle/javascript/NodeJS.qll | 48 ------------- javascript/ql/src/NodeJS/DubiousImport.qhelp | 47 ------------- javascript/ql/src/NodeJS/DubiousImport.ql | 68 ------------------- .../test/library-tests/NodeJS/tests.expected | 8 --- .../ql/test/library-tests/NodeJS/tests.ql | 4 -- 7 files changed, 2 insertions(+), 178 deletions(-) delete mode 100644 javascript/ql/src/NodeJS/DubiousImport.qhelp delete mode 100644 javascript/ql/src/NodeJS/DubiousImport.ql 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 8b0330b708ba..28577c4503e5 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 30c706b9da39..4688339dcbe0 100644 --- a/javascript/ql/lib/semmle/javascript/NodeJS.qll +++ b/javascript/ql/lib/semmle/javascript/NodeJS.qll @@ -67,16 +67,6 @@ class NodeModule extends Module { result = this.getExportsSourceNode().getALocalUse() } - /** Gets a symbol exported by this module. */ - override string getAnExportedSymbol() { - result = super.getAnExportedSymbol() - or - result = this.getAnImplicitlyExportedSymbol() - or - // getters and the like. - result = this.getExportsSourceNode().getAPropertyWrite().getPropertyName() - } - override DataFlow::Node getAnExportedValue(string name) { // a property write whose base is `exports` or `module.exports` result = this.getExportsSourceNode().getAPropertyWrite(name).getRhs() @@ -126,29 +116,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() | @@ -184,21 +151,6 @@ private DataFlow::SourceNode getASourceProp(DynamicPropertyAccess::EnumeratedPro ) } -/** - * Gets an expression that syntactically could be a alias for `module.exports`. - * This predicate exists to reduce the size of `getAModuleExportsNode`, - * while keeping all the tuples that could be relevant in later computations. - */ -pragma[noinline] -private DataFlow::Node getAModuleExportsCandidate() { - // A bit of manual magic - result = any(DataFlow::PropWrite w).getBase() - or - result = DataFlow::valueNode(any(PropAccess p | exists(p.getPropertyName())).getBase()) - or - result = DataFlow::valueNode(any(ObjectExpr obj)) -} - /** * Holds if `nodeModules` is a folder of the form `/node_modules`, where * `` is a (not necessarily proper) prefix of `f` and does not end in `/node_modules`, 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) { From 6eec05e564fea6a13842a97d9e5518c8f4ef1fab Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 11 Sep 2025 14:09:56 +0200 Subject: [PATCH 03/26] Update NodeJS.qll --- javascript/ql/lib/semmle/javascript/NodeJS.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/NodeJS.qll b/javascript/ql/lib/semmle/javascript/NodeJS.qll index 4688339dcbe0..ae1ef8438301 100644 --- a/javascript/ql/lib/semmle/javascript/NodeJS.qll +++ b/javascript/ql/lib/semmle/javascript/NodeJS.qll @@ -53,6 +53,8 @@ class NodeModule extends Module { private DataFlow::SourceNode getExportsSourceNode() { result = DataFlow::ssaDefinitionNode(Ssa::implicitInit(this.getExportsVariable())) or + result = DataFlow::thisNode(this) // `this` is an alias for `module.exports` + or result = this.getModuleSourceNode().getAPropertyWrite("exports").getRhs().getALocalSource() or result = this.getModuleSourceNode().getAPropertyRead("exports") From d6a331d5da45a20a6ed4587c1c83234e3025b808 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 11 Sep 2025 14:17:47 +0200 Subject: [PATCH 04/26] wip --- .../lib/semmle/javascript/PackageExports.qll | 3 +- .../javascript/dataflow/TypeInference.qll | 4 ++- .../internal/AbstractPropertiesImpl.qll | 13 +------- .../dataflow/internal/FlowSteps.qll | 33 ++++--------------- .../internal/InterProceduralTypeInference.qll | 13 -------- .../internal/VariableTypeInference.qll | 26 --------------- .../javascript/internal/NameResolution.qll | 4 +++ 7 files changed, 15 insertions(+), 81 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/PackageExports.qll b/javascript/ql/lib/semmle/javascript/PackageExports.qll index b4032980688f..826e815d4c5c 100644 --- a/javascript/ql/lib/semmle/javascript/PackageExports.qll +++ b/javascript/ql/lib/semmle/javascript/PackageExports.qll @@ -237,8 +237,7 @@ private DataFlow::Node getAnExportFromModule(Module mod) { // exports saved to the global object result = DataFlow::globalObjectRef().getAPropertyWrite().getRhs() and result.getTopLevel() = mod - or - result.analyze().getAValue() = TAbstractModuleObject(mod) + // TODO: perhaps rely on name resolution here? } /** diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll index 53fab7371c2a..58021952a224 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TypeInference.qll @@ -169,6 +169,8 @@ class AnalyzedNode extends DataFlow::Node { class AnalyzedValueNode extends AnalyzedNode, DataFlow::ValueNode { } /** + * DEPRECATED. Type inference is no longer used for reasoning about module exports. + * * A module for which analysis results are available. * * The type inference supports AMD, CommonJS and ES2015 modules. All three @@ -177,7 +179,7 @@ class AnalyzedValueNode extends AnalyzedNode, DataFlow::ValueNode { } * exports are modeled as property writes on `module.exports`, and imports * as property reads on any potential value of `module.exports`. */ -class AnalyzedModule extends TopLevel instanceof Module { +deprecated class AnalyzedModule extends TopLevel instanceof Module { /** Gets the name of this module. */ string getName() { result = super.getName() } diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractPropertiesImpl.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractPropertiesImpl.qll index 6d86e7ea3f25..8d41dd52d091 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractPropertiesImpl.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/AbstractPropertiesImpl.qll @@ -30,13 +30,6 @@ newtype TAbstractProperty = * of the concrete objects represented by `baseVal`. */ AbstractValue getAnInitialPropertyValue(DefiniteAbstractValue baseVal, string propertyName) { - // initially, `module.exports === exports` - exists(Module m | - baseVal = TAbstractModuleObject(m) and - propertyName = "exports" and - result = TAbstractExportsObject(m) - ) - or // class members result = getAnInitialMemberValue(getMember(baseVal, propertyName)) or @@ -77,11 +70,7 @@ private AbstractValue getAnInitialMemberValue(MemberDefinition m) { * Holds if `baseVal` is an abstract value whose properties we track for the purposes * of `getALocalValue`. */ -predicate shouldAlwaysTrackProperties(AbstractValue baseVal) { - baseVal instanceof AbstractModuleObject or - baseVal instanceof AbstractExportsObject or - baseVal instanceof AbstractCallable -} +predicate shouldAlwaysTrackProperties(AbstractValue baseVal) { baseVal instanceof AbstractCallable } /** Holds if `baseVal` is an abstract value whose properties we track. */ predicate shouldTrackProperties(AbstractValue baseVal) { diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll index 1711faa4adeb..e9abe8909636 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -8,16 +8,7 @@ import javascript deprecated import semmle.javascript.dataflow.Configuration import semmle.javascript.dataflow.internal.CallGraphs private import semmle.javascript.internal.CachedStages - -/** - * Holds if flow should be tracked through properties of `obj`. - * - * Flow is tracked through `module` and `module.exports` objects. - */ -predicate shouldTrackProperties(AbstractValue obj) { - obj instanceof AbstractExportsObject or - obj instanceof AbstractModuleObject -} +private import semmle.javascript.internal.NameResolution /** * Holds if `source` corresponds to an expression returned by `f`, and @@ -337,28 +328,16 @@ private module CachedSteps { ) } - /** - * Holds if there is an assignment to property `prop` of an object represented by `obj` - * with right hand side `rhs` somewhere, and properties of `obj` should be tracked. - */ - pragma[noinline] - private predicate trackedPropertyWrite(AbstractValue obj, string prop, DataFlow::Node rhs) { - exists(AnalyzedPropertyWrite pw | - pw.writes(obj, prop, rhs) and - shouldTrackProperties(obj) and - // avoid introducing spurious global flow - not pw.baseIsIncomplete("global") - ) - } - /** * Holds if there is a flow step from `pred` to `succ` through an object property. */ cached predicate propertyFlowStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(AbstractValue obj, string prop | - trackedPropertyWrite(obj, prop, pred) and - succ.(AnalyzedPropertyRead).reads(obj, prop) + // TODO: Ensure name resolution has good enough support for NodeJS and AMD + exists(NameResolution::Node node1, NameResolution::Node node2 | + NameResolution::ValueFlow::resolvedReadStep(node1, node2) and + pred = DataFlow::valueNode(node1) and + succ = DataFlow::valueNode(node2) ) } diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll index 80de3cb64fdd..02d4ca08d0a4 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll @@ -45,19 +45,6 @@ private class AnalyzedThisInBoundFunction extends AnalyzedThisExpr { } } -/** - * Flow analysis for `this` expressions in node modules. - * - * These expressions are assumed to refer to the `module.exports` object. - */ -private class AnalyzedThisAsModuleExports extends DataFlow::AnalyzedNode, DataFlow::ThisNode { - NodeModule m; - - AnalyzedThisAsModuleExports() { m = this.getBindingContainer() } - - override AbstractValue getALocalValue() { result = TAbstractExportsObject(m) } -} - /** * Flow analysis for `this` expressions inside a function that is instantiated. * diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll index bbbd967fda80..f2a67ffd8fd7 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableTypeInference.qll @@ -159,28 +159,6 @@ private class AnalyzedRestParameter extends AnalyzedValueNode { override AbstractValue getALocalValue() { result = TAbstractOtherObject() } } -/** - * Flow analysis for `module` and `exports` parameters of AMD modules. - */ -private class AnalyzedAmdParameter extends AnalyzedVarDef { - AbstractValue implicitInitVal; - - AnalyzedAmdParameter() { - exists(AmdModule m, AmdModuleDefinition mdef | mdef = m.getDefine() | - this = mdef.getModuleParameter() and - implicitInitVal = TAbstractModuleObject(m) - or - this = mdef.getExportsParameter() and - implicitInitVal = TAbstractExportsObject(m) - ) - } - - override AbstractValue getAnAssignedValue() { - result = super.getAnAssignedValue() or - result = implicitInitVal - } -} - /** * An SSA definitions that has been analyzed. */ @@ -355,10 +333,6 @@ private predicate nodeBuiltins(Variable var, AbstractValue av) { exists(Module m, string name | var = m.getScope().getVariable(name) | name = "require" and av = TIndefiniteAbstractValue("heap") or - name = "module" and av = TAbstractModuleObject(m) - or - name = "exports" and av = TAbstractExportsObject(m) - or name = "arguments" and av = TAbstractOtherObject() or (name = "__filename" or name = "__dirname") and diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index b25c98fd693b..f5a591d643e9 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -386,6 +386,10 @@ module NameResolution { node2.(JSDocLocalTypeAccess).getALexicalName() = var ) or + resolvedReadStep(node1, node2) + } + + predicate resolvedReadStep(Node node1, Node node2) { exists(Node base, string name, ModuleLike mod | readStep(base, name, node2) and base = trackModule(mod) and From fd67e547d0af114c76afd042a98afed2e343033a Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 11 Sep 2025 15:57:56 +0200 Subject: [PATCH 05/26] wip --- .../javascript/internal/NameResolution.qll | 54 +++++++++++++++++++ .../CallGraphs/AnnotatedTest/Test.expected | 1 + 2 files changed, 55 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index f5a591d643e9..7b8299104458 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -356,6 +356,8 @@ module NameResolution { name = "$$promise-content" and result = imprt.getImportedPathExpr() ) + or + storeToExports(result, mod, name) } /** @@ -373,6 +375,58 @@ module NameResolution { value = target.getAnAssignedExpr().(ObjectExpr).getPropertyByName(prop).getInit() } + pragma[nomagic] + private GlobalVarAccess globalAccess(Module mod, string name) { + result.getName() = name and + result.getTopLevel() = mod and + name = ["exports", "module"] // manually restrict size of predicate + } + + private Node moduleObjectRef(Module mod) { + result = mod.getScope().getVariable("module").getAnAccess() + or + result = globalAccess(mod, "module") + or + commonStep(moduleObjectRef(mod), result) + } + + private Node exportsObjectRhs(Module mod) { + exists(AssignExpr assign | + assign.getLhs().(PropAccess).accesses(moduleObjectRef(mod), "exports") and + result = assign.getRhs() + ) + or + commonStep(result, exportsObjectRhs(mod)) + } + + private Node exportsObjectAlias(Module mod) { + result = mod.getScope().getVariable("exports").getAnAccess() + or + result = globalAccess(mod, "exports") + or + result.(ThisExpr).getBindingContainer() = mod and + mod instanceof NodeModule + or + readStep(moduleObjectRef(mod), "exports", result) + or + result = exportsObjectRhs(mod) + or + commonStep(exportsObjectAlias(mod), result) + } + + private predicate storeToExports(Node node, Module mod, string name) { + exists(AssignExpr assign | + node = assign.getRhs() and + assign.getLhs().(PropAccess).accesses(exportsObjectAlias(mod), name) + ) + or + exists(Property prop | + node = prop.getInit() and + name = prop.getName() and + prop.getObjectExpr() = exportsObjectAlias(mod) + ) + } + /** Steps that only apply for this configuration. */ private predicate specificStep(Node node1, Node node2) { exists(LexicalName var | S::isRelevantVariable(var) | diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected index 0abd563b4193..8dd60d4c6d93 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected @@ -2,6 +2,7 @@ spuriousCallee missingCallee | constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | | constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | +| reExportLibClient.js:4:1:4:6 | ns.f() | lib.js:3:2:3:14 | function() {} | -1 | calls | badAnnotation accessorCall | accessors.js:12:1:12:5 | obj.f | accessors.js:5:8:5:12 | () {} | From fe3c5811707e6767886708b65b4d489fb8b537df Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 12 Sep 2025 13:30:00 +0200 Subject: [PATCH 06/26] wip --- .../javascript/internal/NameResolution.qll | 20 ++++++++++++++++++- .../EndpointNaming/EndpointNaming.expected | 9 +++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 7b8299104458..f0e2a0be7900 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -250,6 +250,17 @@ module NameResolution { ) } + /** + * A step that treats imports of form `import foo from "somewhere"` as a namespace import, + * to support some non-standard compilers. + */ + private predicate defaultImportInteropStep(Node node1, Node node2) { + exists(ImportDefaultSpecifier spec | + node1 = spec.getImportDeclaration().getImportedPathExpr() and + node2 = spec.getLocal() + ) + } + private signature module TypeResolutionInputSig { /** * Holds if flow is permitted through the given variable. @@ -488,7 +499,14 @@ module NameResolution { /** * Gets a node to which the given module flows. */ - predicate trackModule = ValueFlow::TrackNode::track/1; + Node trackModule(ModuleLike mod) { + result = mod + or + ValueFlow::step(trackModule(mod), result) + or + defaultImportInteropStep(trackModule(mod), result) and + not mod.(ES2015Module).hasBothNamedAndDefaultExports() + } predicate trackClassValue = ValueFlow::TrackNode::track/1; diff --git a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected index d2c34c887cc7..cf823b84e89a 100644 --- a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected +++ b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected @@ -1,4 +1,13 @@ testFailures +| pack3/lib.js:1:16:1:33 | | Unexpected result: name=(pack3).libFunction.default | +| pack3/lib.js:1:35:1:118 | // $ na ... unction | Missing result: alias=(pack3).libFunction.default==(pack3).libFunction | +| pack3/lib.js:1:35:1:118 | // $ na ... unction | Missing result: name=(pack3).libFunction | +| pack8/foo.js:1:1:1:12 | | Unexpected result: name=(pack8).Foo.Foo | +| pack8/foo.js:1:14:1:34 | // $ na ... k8).Foo | Missing result: name=(pack8).Foo | +| pack8/foo.js:4:26:4:28 | | Unexpected result: alias=(pack8).Foo.default==(pack8).Foo.Foo | +| pack8/foo.js:4:31:4:73 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.default==(pack8).Foo | +| pack8/foo.js:5:27:5:65 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.Foo==(pack8).Foo | +| pack10/foo.js:1:34:1:55 | // $ na ... 10).Foo | Missing result: name=(pack10).Foo | ambiguousPreferredPredecessor | pack2/lib.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getInstance() | | pack2/lib.js:8:22:8:34 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getMember("foo") | From 8a376576812443bd448f1bb7135a5c0cfcb7a752 Mon Sep 17 00:00:00 2001 From: Asger F Date: Sat, 13 Sep 2025 22:01:31 +0200 Subject: [PATCH 07/26] more --- .../javascript/dataflow/internal/FlowSteps.qll | 12 +++++++++++- .../semmle/javascript/internal/NameResolution.qll | 6 ++++++ .../EndpointNaming/EndpointNaming.expected | 4 ---- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll index e9abe8909636..70770d444971 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -337,7 +337,17 @@ private module CachedSteps { exists(NameResolution::Node node1, NameResolution::Node node2 | NameResolution::ValueFlow::resolvedReadStep(node1, node2) and pred = DataFlow::valueNode(node1) and - succ = DataFlow::valueNode(node2) + succ = getNodeFromNameResolutionNode(node2) + ) + } + + pragma[inline] + private DataFlow::Node getNodeFromNameResolutionNode(NameResolution::Node node) { + result = DataFlow::valueNode(node) + or + exists(ImportSpecifier spec | + node = spec.getLocal() and + result = DataFlow::valueNode(spec) ) } diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index f0e2a0be7900..d4a7ede59178 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -339,6 +339,12 @@ module NameResolution { S::isRelevantVariable(result) ) or + exists(ExportDefaultDeclaration exprt | + mod = exprt.getContainer() and + name = "default" and + result = exprt.getOperand() + ) + or exists(ExportNamespaceSpecifier spec | result = spec and mod = spec.getContainer() and diff --git a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected index cf823b84e89a..dc5a333e085b 100644 --- a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected +++ b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected @@ -1,13 +1,9 @@ testFailures -| pack3/lib.js:1:16:1:33 | | Unexpected result: name=(pack3).libFunction.default | -| pack3/lib.js:1:35:1:118 | // $ na ... unction | Missing result: alias=(pack3).libFunction.default==(pack3).libFunction | -| pack3/lib.js:1:35:1:118 | // $ na ... unction | Missing result: name=(pack3).libFunction | | pack8/foo.js:1:1:1:12 | | Unexpected result: name=(pack8).Foo.Foo | | pack8/foo.js:1:14:1:34 | // $ na ... k8).Foo | Missing result: name=(pack8).Foo | | pack8/foo.js:4:26:4:28 | | Unexpected result: alias=(pack8).Foo.default==(pack8).Foo.Foo | | pack8/foo.js:4:31:4:73 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.default==(pack8).Foo | | pack8/foo.js:5:27:5:65 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.Foo==(pack8).Foo | -| pack10/foo.js:1:34:1:55 | // $ na ... 10).Foo | Missing result: name=(pack10).Foo | ambiguousPreferredPredecessor | pack2/lib.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getInstance() | | pack2/lib.js:8:22:8:34 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getMember("foo") | From ae28fba111550ef246acef7c9a6a92e83490b4a4 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 16 Sep 2025 08:56:12 +0200 Subject: [PATCH 08/26] wip --- .../javascript/internal/NameResolution.qll | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index d4a7ede59178..9970b9961016 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -73,7 +73,7 @@ module NameResolution { * * May also include some type-specific steps in cases where this is harmless when tracking values. */ - private predicate commonStep(Node node1, Node node2) { + private predicate commonStep1(Node node1, Node node2) { // Import paths are part of the graph and has an incoming edge from the imported module, if found. // This ensures we can also use the PathExpr as a source when working with external (unresolved) modules. exists(Import imprt | @@ -184,6 +184,13 @@ module NameResolution { ) } + pragma[inline] + private predicate commonStep(Node node1, Node node2) { + commonStep1(node1, node2) + or + node2 = ValueFlow::exportsObjectRhs(node1) + } + /** * Holds if there is a read from `node1` to `node2` that accesses the member `name`. */ @@ -404,18 +411,18 @@ module NameResolution { or result = globalAccess(mod, "module") or - commonStep(moduleObjectRef(mod), result) + commonStep1(moduleObjectRef(mod), result) } - private Node exportsObjectRhs(Module mod) { + Node exportsObjectRhs(Module mod) { exists(AssignExpr assign | assign.getLhs().(PropAccess).accesses(moduleObjectRef(mod), "exports") and result = assign.getRhs() ) - or - commonStep(result, exportsObjectRhs(mod)) } + private Node exportsObjectRhsPred(Module mod) { commonStep1*(result, exportsObjectRhs(mod)) } + private Node exportsObjectAlias(Module mod) { result = mod.getScope().getVariable("exports").getAnAccess() or @@ -426,7 +433,7 @@ module NameResolution { or readStep(moduleObjectRef(mod), "exports", result) or - result = exportsObjectRhs(mod) + result = exportsObjectRhsPred(mod) or commonStep(exportsObjectAlias(mod), result) } From 03bc3344391c0db39f374fc29cafcbb512f2c576 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 17 Sep 2025 11:06:28 +0200 Subject: [PATCH 09/26] JS: More stuff --- .../dataflow/internal/FlowSteps.qll | 6 + .../javascript/internal/NameResolution.qll | 119 ++++++++++-------- .../EndpointNaming/EndpointNaming.expected | 5 - 3 files changed, 71 insertions(+), 59 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll index 70770d444971..e82ebee59118 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -339,6 +339,12 @@ private module CachedSteps { pred = DataFlow::valueNode(node1) and succ = getNodeFromNameResolutionNode(node2) ) + or + exists(Import imprt, Module mod | + imprt.getImportedModule() = mod and + pred = DataFlow::valueNode(NameResolution::getModuleBulkExport(mod)) and + succ = imprt.getImportedModuleNodeStrict() + ) } pragma[inline] diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 9970b9961016..c76e89d056b4 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -188,7 +188,7 @@ module NameResolution { private predicate commonStep(Node node1, Node node2) { commonStep1(node1, node2) or - node2 = ValueFlow::exportsObjectRhs(node1) + node2 = exportsObjectRhs(node1) } /** @@ -216,7 +216,8 @@ module NameResolution { exists(PropAccess access | node1 = access.getBase() and name = access.getPropertyName() and - node2 = access + node2 = access and + access instanceof RValue ) or exists(ObjectPattern pattern | @@ -268,6 +269,66 @@ module NameResolution { ) } + pragma[nomagic] + private GlobalVarAccess globalAccess(Module mod, string name) { + result.getName() = name and + result.getTopLevel() = mod and + name = ["exports", "module"] // manually restrict size of predicate + } + + /** Gets a reference to the CommonJS `module` object within the given module. */ + private Node moduleObjectRef(Module mod) { + result = mod.getScope().getVariable("module").getAnAccess() + or + result = globalAccess(mod, "module") + or + commonStep1(moduleObjectRef(mod), result) + } + + /** Gets the right-hand side of an assignment to `module.exports` within the given module. */ + private Node exportsObjectRhs(Module mod) { + exists(AssignExpr assign | + assign.getLhs().(PropAccess).accesses(moduleObjectRef(mod), "exports") and + result = assign.getRhs() + ) + } + + /** Gets a node that is bulk-exported from the given module. */ + Node getModuleBulkExport(ModuleLike mod) { result = exportsObjectRhs(mod) } + + /** Gets a node that flows to `module.exports` within the given module. */ + private Node exportsObjectRhsPred(Module mod) { commonStep1*(result, exportsObjectRhs(mod)) } + + /** Gets a node that is an alias for `module.exports` within the given module. */ + private Node exportsObjectAlias(Module mod) { + result = mod.getScope().getVariable("exports").getAnAccess() + or + result = globalAccess(mod, "exports") + or + result.(ThisExpr).getBindingContainer() = mod and + mod instanceof NodeModule + or + readStep(moduleObjectRef(mod), "exports", result) + or + result = exportsObjectRhsPred(mod) + or + commonStep(exportsObjectAlias(mod), result) + } + + /** Holds if `node` is stored into `module.exports.` within the given module. */ + private predicate storeToExports(Node node, Module mod, string name) { + exists(AssignExpr assign | + node = assign.getRhs() and + assign.getLhs().(PropAccess).accesses(exportsObjectAlias(mod), name) + ) + or + exists(Property prop | + node = prop.getInit() and + name = prop.getName() and + prop.getObjectExpr() = exportsObjectAlias(mod) + ) + } + private signature module TypeResolutionInputSig { /** * Holds if flow is permitted through the given variable. @@ -399,58 +460,6 @@ module NameResolution { value = target.getAnAssignedExpr().(ObjectExpr).getPropertyByName(prop).getInit() } - pragma[nomagic] - private GlobalVarAccess globalAccess(Module mod, string name) { - result.getName() = name and - result.getTopLevel() = mod and - name = ["exports", "module"] // manually restrict size of predicate - } - - private Node moduleObjectRef(Module mod) { - result = mod.getScope().getVariable("module").getAnAccess() - or - result = globalAccess(mod, "module") - or - commonStep1(moduleObjectRef(mod), result) - } - - Node exportsObjectRhs(Module mod) { - exists(AssignExpr assign | - assign.getLhs().(PropAccess).accesses(moduleObjectRef(mod), "exports") and - result = assign.getRhs() - ) - } - - private Node exportsObjectRhsPred(Module mod) { commonStep1*(result, exportsObjectRhs(mod)) } - - private Node exportsObjectAlias(Module mod) { - result = mod.getScope().getVariable("exports").getAnAccess() - or - result = globalAccess(mod, "exports") - or - result.(ThisExpr).getBindingContainer() = mod and - mod instanceof NodeModule - or - readStep(moduleObjectRef(mod), "exports", result) - or - result = exportsObjectRhsPred(mod) - or - commonStep(exportsObjectAlias(mod), result) - } - - private predicate storeToExports(Node node, Module mod, string name) { - exists(AssignExpr assign | - node = assign.getRhs() and - assign.getLhs().(PropAccess).accesses(exportsObjectAlias(mod), name) - ) - or - exists(Property prop | - node = prop.getInit() and - name = prop.getName() and - prop.getObjectExpr() = exportsObjectAlias(mod) - ) - } - /** Steps that only apply for this configuration. */ private predicate specificStep(Node node1, Node node2) { exists(LexicalName var | S::isRelevantVariable(var) | @@ -519,6 +528,8 @@ module NameResolution { or defaultImportInteropStep(trackModule(mod), result) and not mod.(ES2015Module).hasBothNamedAndDefaultExports() + or + result = exportsObjectAlias(mod) } predicate trackClassValue = ValueFlow::TrackNode::track/1; diff --git a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected index dc5a333e085b..d2c34c887cc7 100644 --- a/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected +++ b/javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected @@ -1,9 +1,4 @@ testFailures -| pack8/foo.js:1:1:1:12 | | Unexpected result: name=(pack8).Foo.Foo | -| pack8/foo.js:1:14:1:34 | // $ na ... k8).Foo | Missing result: name=(pack8).Foo | -| pack8/foo.js:4:26:4:28 | | Unexpected result: alias=(pack8).Foo.default==(pack8).Foo.Foo | -| pack8/foo.js:4:31:4:73 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.default==(pack8).Foo | -| pack8/foo.js:5:27:5:65 | // $ al ... k8).Foo | Missing result: alias=(pack8).Foo.Foo==(pack8).Foo | ambiguousPreferredPredecessor | pack2/lib.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getInstance() | | pack2/lib.js:8:22:8:34 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getMember("foo") | From f5db4f0f1c07bdad076e52d998b5ded7deddefcb Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 09:15:29 +0200 Subject: [PATCH 10/26] JS: Fix for export declarations --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index c76e89d056b4..edeae180cbbc 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -81,6 +81,12 @@ module NameResolution { node2 = imprt.getImportedPathExpr() ) or + // Same as above, but for re-export declarations + exists(ReExportDeclaration exprt | + node1 = exprt.getReExportedModule() and + node2 = exprt.getImportedPath() + ) + or exists(ImportNamespaceSpecifier spec | node1 = spec.getImportDeclaration().getImportedPathExpr() and node2 = spec.getLocal() From ab5acbf11ef262f3a295ea19268d84c2d96c6200 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 09:19:51 +0200 Subject: [PATCH 11/26] JS: Use name resolution in call graph construction --- .../ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll | 3 +++ .../test/library-tests/CallGraphs/AnnotatedTest/Test.expected | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index cc4c883381ea..2e80653ac133 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -5,6 +5,7 @@ private import javascript private import semmle.javascript.dataflow.internal.StepSummary private import semmle.javascript.dataflow.internal.PreCallGraphStep +private import semmle.javascript.internal.NameResolution cached module CallGraph { @@ -12,6 +13,8 @@ module CallGraph { cached Function getAFunctionValue(AnalyzedNode node) { result = node.getAValue().(AbstractCallable).getFunction() + or + node = DataFlow::valueNode(NameResolution::trackFunctionValue(result)) } /** Holds if the type inferred for `node` is indefinite due to global flow. */ diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected index 8dd60d4c6d93..0abd563b4193 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected @@ -2,7 +2,6 @@ spuriousCallee missingCallee | constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | | constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | -| reExportLibClient.js:4:1:4:6 | ns.f() | lib.js:3:2:3:14 | function() {} | -1 | calls | badAnnotation accessorCall | accessors.js:12:1:12:5 | obj.f | accessors.js:5:8:5:12 | () {} | From 9868d61a70030b810dee383d6344e5783e2ea444 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 10:00:07 +0200 Subject: [PATCH 12/26] JS: Add toDataFlowNode() and use more consistently --- .../javascript/dataflow/internal/CallGraphs.qll | 2 +- .../javascript/dataflow/internal/FlowSteps.qll | 16 +++------------- .../javascript/internal/NameResolution.qll | 10 ++++++++++ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index 2e80653ac133..5e902f4fb10d 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -14,7 +14,7 @@ module CallGraph { Function getAFunctionValue(AnalyzedNode node) { result = node.getAValue().(AbstractCallable).getFunction() or - node = DataFlow::valueNode(NameResolution::trackFunctionValue(result)) + node = NameResolution::trackFunctionValue(result).toDataFlowNode().(DataFlow::SourceNode) } /** Holds if the type inferred for `node` is indefinite due to global flow. */ diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll index e82ebee59118..ff0b4175f96e 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -336,27 +336,17 @@ private module CachedSteps { // TODO: Ensure name resolution has good enough support for NodeJS and AMD exists(NameResolution::Node node1, NameResolution::Node node2 | NameResolution::ValueFlow::resolvedReadStep(node1, node2) and - pred = DataFlow::valueNode(node1) and - succ = getNodeFromNameResolutionNode(node2) + pred = node1.toDataFlowNode() and + succ = node2.toDataFlowNode() ) or exists(Import imprt, Module mod | imprt.getImportedModule() = mod and - pred = DataFlow::valueNode(NameResolution::getModuleBulkExport(mod)) and + pred = NameResolution::getModuleBulkExport(mod).toDataFlowNode() and succ = imprt.getImportedModuleNodeStrict() ) } - pragma[inline] - private DataFlow::Node getNodeFromNameResolutionNode(NameResolution::Node node) { - result = DataFlow::valueNode(node) - or - exists(ImportSpecifier spec | - node = spec.getLocal() and - result = DataFlow::valueNode(spec) - ) - } - /** * Gets a node whose value is assigned to `gv` in `f`. */ diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index edeae180cbbc..0ba5f5ecdbe3 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -34,6 +34,16 @@ module NameResolution { or result = this.(JSDocTypeExpr).getLocation() } + + DataFlow::Node toDataFlowNode() { + result = DataFlow::valueNode(this) + or + // TODO: refactor graph to avoid the need for this + exists(ImportSpecifier spec | + this = spec.getLocal() and + result = DataFlow::valueNode(spec) + ) + } } private signature predicate nodeSig(Node node); From 54e8b9fac77e7a7d39bf80e9e2b0a2d69d549b99 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 12:10:22 +0200 Subject: [PATCH 13/26] Correct map named exports to a data flow node --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 0ba5f5ecdbe3..5c9daa170091 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -43,6 +43,11 @@ module NameResolution { this = spec.getLocal() and result = DataFlow::valueNode(spec) ) + or + exists(ExportDeclaration exprt, string name | + exprt.exportsAs(this, name) and + result = exprt.getSourceNode(name) + ) } } From b04157b25a1845d6b287056e5f8f8e99656bb30e Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 13:48:00 +0200 Subject: [PATCH 14/26] JS: FIx a test case Namespace imports never refer to the default export --- .../ql/test/library-tests/Closure/tests/importFromEs6.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/test/library-tests/Closure/tests/importFromEs6.js b/javascript/ql/test/library-tests/Closure/tests/importFromEs6.js index 5e380984817e..775f3cd264b0 100644 --- a/javascript/ql/test/library-tests/Closure/tests/importFromEs6.js +++ b/javascript/ql/test/library-tests/Closure/tests/importFromEs6.js @@ -1,10 +1,10 @@ // ES6 imports can import files by name, as long as they are modules import * as googModule from './googModule'; -import * as googModuleDefault from './googModuleDefault'; +import googModuleDefault from './googModuleDefault'; import * as es6Module from './es6Module'; -import * as es6ModuleDefault from './es6ModuleDefault'; +import es6ModuleDefault from './es6ModuleDefault'; es6Module.fun(); es6ModuleDefault(); From ddd65d44d9bf2e76345167103db966cee28787b3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 14:04:36 +0200 Subject: [PATCH 15/26] Fix node1,node2 being swapped when using exportsObjectRhs in commonStep --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 5c9daa170091..33eaefd66dc9 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -209,7 +209,10 @@ module NameResolution { private predicate commonStep(Node node1, Node node2) { commonStep1(node1, node2) or - node2 = exportsObjectRhs(node1) + exists(ModuleLike mod | + node1 = exportsObjectRhs(mod) and + node2 = mod + ) } /** From c19ca7818b8612d417b3869cfb702c9d6e16ef6f Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 14:05:21 +0200 Subject: [PATCH 16/26] JS: Replace Closure's storeToVariable predicate with more exportsObjectRhs tuples --- .../javascript/internal/NameResolution.qll | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 33eaefd66dc9..7a48dd0c3a28 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -315,6 +315,8 @@ module NameResolution { assign.getLhs().(PropAccess).accesses(moduleObjectRef(mod), "exports") and result = assign.getRhs() ) + or + result = mod.(Closure::ClosureModule).getExportsVariable().getAnAssignedExpr() } /** Gets a node that is bulk-exported from the given module. */ @@ -458,8 +460,6 @@ module NameResolution { result = enum.getMemberByName(name).getIdentifier() ) or - storeToVariable(result, name, mod.(Closure::ClosureModule).getExportsVariable()) - or exists(DynamicImportExpr imprt | mod = imprt and name = "$$promise-content" and @@ -469,21 +469,6 @@ module NameResolution { storeToExports(result, mod, name) } - /** - * Holds if `value` is stored in `target.prop`. Only needs to recognise assignments - * that are also recognised by JSDoc tooling such as the Closure compiler. - */ - private predicate storeToVariable(Expr value, string prop, LocalVariableLike target) { - exists(AssignExpr assign | - // target.name = value - assign.getLhs().(PropAccess).accesses(target.getAnAccess(), prop) and - value = assign.getRhs() - ) - or - // target = { name: value } - value = target.getAnAssignedExpr().(ObjectExpr).getPropertyByName(prop).getInit() - } - /** Steps that only apply for this configuration. */ private predicate specificStep(Node node1, Node node2) { exists(LexicalName var | S::isRelevantVariable(var) | From bf38400f13514076b230e08dbc4a9b05f0cfd374 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 14:06:04 +0200 Subject: [PATCH 17/26] JS: Treat defaultImportInteropStep as a common step Default import interop is now used by default, and then modules are only allowed to use it in restricted cases. Previously, only modules could use it (in restricted cases). --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 7a48dd0c3a28..e6b092f35949 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -203,6 +203,8 @@ module NameResolution { node1 = fun.getArgument(i) and node2 = fun.getParameter(i) ) + or + defaultImportInteropStep(node1, node2) } pragma[inline] @@ -533,7 +535,11 @@ module NameResolution { Node trackModule(ModuleLike mod) { result = mod or - ValueFlow::step(trackModule(mod), result) + exists(Node prev | + prev = trackModule(mod) and + ValueFlow::step(prev, result) and + not defaultImportInteropStep(prev, result) + ) or defaultImportInteropStep(trackModule(mod), result) and not mod.(ES2015Module).hasBothNamedAndDefaultExports() From 31aebc770016cc6bfea4a1cf8be497c1d2f492b1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 21:10:44 +0200 Subject: [PATCH 18/26] JS: Improve handling of Closure --- .../ql/lib/semmle/javascript/GlobalAccessPaths.qll | 6 ++++++ .../javascript/dataflow/internal/CallGraphs.qll | 7 ++++++- .../lib/semmle/javascript/internal/NameResolution.qll | 11 ++++++----- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 4a461961f8af..684b98993ae4 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -272,6 +272,12 @@ module AccessPath { result = decl.getIdentifier().(GlobalVarDecl).getName() and root.isGlobal() ) + or + exists(Closure::ClosureModule mod | + node = mod.getExportsVariable().getAnAssignedExpr().flow() and + root.isGlobal() and + result = mod.getClosureNamespace() + ) } /** A module for computing an access to a variable that happens after a property has been written onto it */ diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index 5e902f4fb10d..1f5395911bc0 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -14,7 +14,12 @@ module CallGraph { Function getAFunctionValue(AnalyzedNode node) { result = node.getAValue().(AbstractCallable).getFunction() or - node = NameResolution::trackFunctionValue(result).toDataFlowNode().(DataFlow::SourceNode) + node = NameResolution::trackFunctionValue(result).toDataFlowNode() + or + exists(DataFlow::Node pred | + AccessPath::step(pred, node) and + result = getAFunctionValue(pred) + ) } /** Holds if the type inferred for `node` is indefinite due to global flow. */ diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index e6b092f35949..d48c51d9a93c 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -194,11 +194,6 @@ module NameResolution { node2 = req ) or - exists(Closure::ClosureModule mod | - node1 = mod.getExportsVariable() and - node2 = mod - ) - or exists(ImmediatelyInvokedFunctionExpr fun, int i | node1 = fun.getArgument(i) and node2 = fun.getParameter(i) @@ -319,6 +314,12 @@ module NameResolution { ) or result = mod.(Closure::ClosureModule).getExportsVariable().getAnAssignedExpr() + or + exists(ExportDefaultDeclaration exprt | + mod instanceof Closure::ClosureModule and + exprt.getContainer() = mod and + result = exprt.getOperand() + ) } /** Gets a node that is bulk-exported from the given module. */ From 01b1e8ab65d7d743df3fe29c8c8c0b06dec8bf6d Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 21:52:24 +0200 Subject: [PATCH 19/26] JS: Improve Closure support in GlobalAccessPaths --- .../lib/semmle/javascript/GlobalAccessPaths.qll | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll index 684b98993ae4..ce79212a38a2 100644 --- a/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll +++ b/javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll @@ -273,10 +273,21 @@ module AccessPath { root.isGlobal() ) or - exists(Closure::ClosureModule mod | + exists(Closure::ClosureModule mod | root.isGlobal() | node = mod.getExportsVariable().getAnAssignedExpr().flow() and - root.isGlobal() and result = mod.getClosureNamespace() + or + exists(ExportNamedDeclaration decl, string name | + decl.getContainer() = mod and + node = decl.getSourceNode(name) and + result = join(mod.getClosureNamespace(), name) + ) + or + exists(ExportDefaultDeclaration decl | + decl.getContainer() = mod and + node = DataFlow::valueNode(decl.getOperand()) and + result = mod.getClosureNamespace() + ) ) } From cc98de51c1f32eee94730fa61a9f7176e913c38c Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 18 Sep 2025 22:43:09 +0200 Subject: [PATCH 20/26] JS: Handle require() and the like --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index d48c51d9a93c..a359abcdf99d 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -102,6 +102,13 @@ module NameResolution { node2 = exprt.getImportedPath() ) or + // For imports like `require()` that are just expressions evaluating to the imported module. + exists(Import imprt | + imprt.getImportedModuleNode() = DataFlow::valueNode(imprt) and + node1 = imprt.getImportedPathExpr() and + node2 = imprt + ) + or exists(ImportNamespaceSpecifier spec | node1 = spec.getImportDeclaration().getImportedPathExpr() and node2 = spec.getLocal() From d4ead6402c37bafb4accee4f0da3230829b1e8f2 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 19 Sep 2025 09:22:44 +0200 Subject: [PATCH 21/26] JS: Refactor toDataFlowNode --- .../javascript/dataflow/internal/CallGraphs.qll | 2 +- .../javascript/dataflow/internal/FlowSteps.qll | 6 +++--- .../javascript/internal/NameResolution.qll | 16 +++++++++------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index 1f5395911bc0..244a8088790d 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -14,7 +14,7 @@ module CallGraph { Function getAFunctionValue(AnalyzedNode node) { result = node.getAValue().(AbstractCallable).getFunction() or - node = NameResolution::trackFunctionValue(result).toDataFlowNode() + node = NameResolution::trackFunctionValue(result).toDataFlowNodeOut() or exists(DataFlow::Node pred | AccessPath::step(pred, node) and diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll index ff0b4175f96e..79181ee91386 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -336,13 +336,13 @@ private module CachedSteps { // TODO: Ensure name resolution has good enough support for NodeJS and AMD exists(NameResolution::Node node1, NameResolution::Node node2 | NameResolution::ValueFlow::resolvedReadStep(node1, node2) and - pred = node1.toDataFlowNode() and - succ = node2.toDataFlowNode() + pred = node1.toDataFlowNodeIn() and + succ = node2.toDataFlowNodeOut() ) or exists(Import imprt, Module mod | imprt.getImportedModule() = mod and - pred = NameResolution::getModuleBulkExport(mod).toDataFlowNode() and + pred = NameResolution::getModuleBulkExport(mod).toDataFlowNodeIn() and succ = imprt.getImportedModuleNodeStrict() ) } diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index a359abcdf99d..25e6bfebc60c 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -35,19 +35,21 @@ module NameResolution { result = this.(JSDocTypeExpr).getLocation() } - DataFlow::Node toDataFlowNode() { + DataFlow::Node toDataFlowNodeIn() { result = DataFlow::valueNode(this) or - // TODO: refactor graph to avoid the need for this + result = DataFlow::valueNode(this.(Variable).getAnAssignedExpr()) + } + + DataFlow::Node toDataFlowNodeOut() { + result = DataFlow::valueNode(this) + or + result = DataFlow::valueNode(this.(Variable).getAnAccess()) + or exists(ImportSpecifier spec | this = spec.getLocal() and result = DataFlow::valueNode(spec) ) - or - exists(ExportDeclaration exprt, string name | - exprt.exportsAs(this, name) and - result = exprt.getSourceNode(name) - ) } } From 3146007f4e47c73ff496978b8afbfa0b4d4dc105 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 19 Sep 2025 09:33:24 +0200 Subject: [PATCH 22/26] More cases in toDataFlowNodeIn --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 25e6bfebc60c..4695cd7d73d1 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -39,6 +39,10 @@ module NameResolution { result = DataFlow::valueNode(this) or result = DataFlow::valueNode(this.(Variable).getAnAssignedExpr()) + or + result = DataFlow::valueNode(any(ClassDefinition def | this = def.getVariable())) + or + result = DataFlow::valueNode(any(FunctionDeclStmt def | this = def.getVariable())) } DataFlow::Node toDataFlowNodeOut() { From 69563a74d91e87c8f75e0753fd6c4e4ddb30e486 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 19 Sep 2025 12:47:27 +0200 Subject: [PATCH 23/26] JS: Remove restriction around effectively-constant variables --- .../lib/semmle/javascript/internal/NameResolution.qll | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 4695cd7d73d1..77209ce6c463 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -378,19 +378,10 @@ module NameResolution { predicate isRelevantVariable(LexicalName var); } - /** - * A local variable with exactly one definition, not counting implicit initialization. - */ - private class EffectivelyConstantVariable extends LocalVariableLike { - EffectivelyConstantVariable() { - count(SsaExplicitDefinition ssa | ssa.getSourceVariable() = this) <= 1 // count may be zero if ambient - } - } - /** Configuration for propagating values and namespaces */ private module ValueConfig implements TypeResolutionInputSig { predicate isRelevantVariable(LexicalName var) { - var instanceof EffectivelyConstantVariable + var instanceof LocalVariableLike or // We merge the namespace and value declaration spaces as it seems there is // no need to distinguish them in practice. From 04fbb8c73f5cef7b2156cbbd98f86d533309b4a3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 22 Sep 2025 00:05:55 +0200 Subject: [PATCH 24/26] JS: Do alias analysis is CG construction --- .../dataflow/internal/CallGraphs.qll | 78 +++++++++++++++++-- 1 file changed, 72 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index 244a8088790d..373d77f41d75 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -5,20 +5,86 @@ private import javascript private import semmle.javascript.dataflow.internal.StepSummary private import semmle.javascript.dataflow.internal.PreCallGraphStep +private import semmle.javascript.dataflow.internal.FlowSteps private import semmle.javascript.internal.NameResolution cached module CallGraph { + pragma[nomagic] + private predicate shouldBackTrack(DataFlow::SourceNode node) { + exists(DataFlow::Node rhs | rhs = node.getAPropertyWrite().getRhs().getALocalSource() | + track(_) = rhs + ) + } + + pragma[nomagic] + private DataFlow::SourceNode backtrackStoreTarget() { + shouldBackTrack(result) + or + AccessPath::step(result.getALocalUse(), backtrackStoreTarget()) + or + propertyFlowStep(result.getALocalUse(), backtrackStoreTarget()) + or + storeReadStep(result, backtrackStoreTarget()) + } + + pragma[nomagic] + private predicate shouldTrack(DataFlow::SourceNode node) { + node instanceof DataFlow::FunctionNode + or + node instanceof DataFlow::ClassNode + or + ( + node instanceof DataFlow::ObjectLiteralNode or + node instanceof DataFlow::InvokeNode or + node instanceof DataFlow::ArrayLiteralNode + ) and + backtrackStoreTarget() = node + } + + pragma[nomagic] + private predicate deepStore(DataFlow::SourceNode node1, string prop, DataFlow::SourceNode node2) { + track(node2).getAPropertyWrite(prop).getRhs().getALocalSource() = node1 + } + + pragma[nomagic] + private predicate deepRead(DataFlow::SourceNode node1, string prop, DataFlow::SourceNode node2) { + track(node1).getAPropertyRead(prop) = node2 + } + + pragma[nomagic] + private predicate storeReadStep(DataFlow::SourceNode node1, DataFlow::SourceNode node2) { + exists(DataFlow::SourceNode base, string name | + deepStore(node1, name, base) and + deepRead(base, name, node2) + ) + } + + pragma[nomagic] + private DataFlow::SourceNode track(DataFlow::SourceNode source) { + shouldTrack(source) and + ( + result = source + or + AccessPath::step(track(source).getALocalUse(), result) + or + propertyFlowStep(track(source).getALocalUse(), result) + or + storeReadStep(track(source), result) + ) + } + /** Gets the function referenced by `node`, as determined by the type inference. */ cached Function getAFunctionValue(AnalyzedNode node) { - result = node.getAValue().(AbstractCallable).getFunction() - or - node = NameResolution::trackFunctionValue(result).toDataFlowNodeOut() + exists(DataFlow::FunctionNode func | + result = func.getFunction() and + track(func).getALocalUse() = node + ) or - exists(DataFlow::Node pred | - AccessPath::step(pred, node) and - result = getAFunctionValue(pred) + exists(DataFlow::ClassNode cls | + result = cls.getConstructor().getFunction() and + track(cls).getALocalUse() = node ) } From 2d1f2db4af4aa30dc608cd9d7bb6513eaeec5b30 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 22 Sep 2025 08:58:56 +0200 Subject: [PATCH 25/26] JS: Track AMD exports --- .../ql/lib/semmle/javascript/internal/NameResolution.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll index 77209ce6c463..4cd2b78ac0a7 100644 --- a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll +++ b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll @@ -333,6 +333,8 @@ module NameResolution { exprt.getContainer() = mod and result = exprt.getOperand() ) + or + result = mod.(AmdModule).getDefine().getFactoryFunction().getAReturnedExpr() } /** Gets a node that is bulk-exported from the given module. */ @@ -352,6 +354,8 @@ module NameResolution { or readStep(moduleObjectRef(mod), "exports", result) or + result = mod.(AmdModule).getDefine().getExportsParameter() + or result = exportsObjectRhsPred(mod) or commonStep(exportsObjectAlias(mod), result) From 47e8a22a60cd08135ae3509636d52b9442997519 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 22 Sep 2025 08:59:51 +0200 Subject: [PATCH 26/26] More CG tweaks --- .../dataflow/internal/CallGraphs.qll | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll index 373d77f41d75..9ccef8fd5682 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -18,14 +18,29 @@ module CallGraph { } pragma[nomagic] - private DataFlow::SourceNode backtrackStoreTarget() { - shouldBackTrack(result) + private predicate methodHostToReceiverStep(DataFlow::SourceNode node1, DataFlow::SourceNode node2) { + exists(DataFlow::PropWrite write | + node1 = write.getBase().getALocalSource() and + node2 = write.getRhs().getALocalSource().(DataFlow::FunctionNode).getReceiver() + ) + } + + pragma[inline] + private predicate step(DataFlow::SourceNode node1, DataFlow::SourceNode node2) { + AccessPath::step(node1.getALocalUse(), node2) or - AccessPath::step(result.getALocalUse(), backtrackStoreTarget()) + propertyFlowStep(node1.getALocalUse(), node2) or - propertyFlowStep(result.getALocalUse(), backtrackStoreTarget()) + storeReadStep(node1, node2) or - storeReadStep(result, backtrackStoreTarget()) + methodHostToReceiverStep(node1, node2) + } + + pragma[nomagic] + private DataFlow::SourceNode backtrackStoreTarget() { + shouldBackTrack(result) + or + step(result, backtrackStoreTarget()) } pragma[nomagic] @@ -63,15 +78,9 @@ module CallGraph { pragma[nomagic] private DataFlow::SourceNode track(DataFlow::SourceNode source) { shouldTrack(source) and - ( - result = source - or - AccessPath::step(track(source).getALocalUse(), result) - or - propertyFlowStep(track(source).getALocalUse(), result) - or - storeReadStep(track(source), result) - ) + result = source + or + step(track(source), result) } /** Gets the function referenced by `node`, as determined by the type inference. */