JS: Handle wrapper functions more gracefully

Author: asgerf

javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll

@@ -347,26 +347,6 @@ private predicate functionHasNameCandidate(
   nameFromExterns(function, package, name, badness)
 }
 
-private predicate functionHasPrimaryName(
-  DataFlow::FunctionNode function, string package, string name, int badness
-) {
-  badness = min(int b | functionHasNameCandidate(function, _, _, b) | b) and
-  package = min(string p | functionHasNameCandidate(function, p, _, badness) | p) and
-  name =
-    min(string n |
-      functionHasNameCandidate(function, package, n, badness)
-    |
-      n order by n.length(), n
-    )
-}
-
-/**
- * Holds if `(package, name)` is the primary name for the given `function`.
- */
-predicate functionHasPrimaryName(DataFlow::FunctionNode function, string package, string name) {
-  functionHasPrimaryName(function, package, name, _)
-}
-
 private predicate sourceNodeHasNameCandidate(
   DataFlow::SourceNode node, string package, string name, int badness
 ) {
@@ -387,6 +367,29 @@ private predicate sourceNodeHasPrimaryName(
     min(string n | sourceNodeHasNameCandidate(node, package, n, badness) | n order by n.length(), n)
 }
 
+/**
+ * Holds if `node` is a function or a call that returns a function.
+ */
+private predicate isFunctionSource(DataFlow::SourceNode node) {
+  node instanceof DataFlow::FunctionNode
+  or
+  node instanceof DataFlow::InvokeNode and
+  exists(node.getABoundFunctionValue(_)) and
+  // `getASinkNode` steps through imports (but not other calls) so exclude calls that are imports (i.e. require calls)
+  // as we want to get as close to the source as possible.
+  not node instanceof DataFlow::ModuleImportNode
+}
+
+/**
+ * Holds if `(package, name)` is the primary name for the given `function`.
+ *
+ * The `function` node may be an actual function expression, or a call site from which a function is returned.
+ */
+predicate functionHasPrimaryName(DataFlow::SourceNode function, string package, string name) {
+  sourceNodeHasPrimaryName(function, package, name, _) and
+  isFunctionSource(function)
+}
+
 private predicate sinkHasSourceName(API::Node sink, string package, string name, int badness) {
   exists(DataFlow::SourceNode source |
     sink = getASinkNode(source) and

javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.expected

@@ -2,12 +2,6 @@ testFailures
 | pack11/index.ts:33:1:33:16 |  | Unexpected result: method=(pack11).C3.privateField |
 | pack11/index.ts:33:18:33:69 | // $ me ... ng.name | Missing result:method=(pack11).C3.publicField.really.long.name |
 | pack11/index.ts:41:23:41:24 |  | Unexpected result: alias=(pack11).C3.publicField.really.long.name==(pack11).C3.privateField |
-| pack12/index.js:2:12:2:21 |  | Unexpected result: method=(pack12).f1 |
-| pack12/index.js:6:28:6:50 | // $ me ... k12).f1 | Missing result:method=(pack12).f1 |
-| pack12/index.js:7:19:7:25 |  | Unexpected result: alias=(pack12).f2==(pack12).f1 |
-| pack12/index.js:7:28:7:50 | // $ me ... k12).f2 | Missing result:method=(pack12).f2 |
-| pack12/index.js:10:19:10:25 |  | Unexpected result: alias=(pack12).g1==(pack12).f1 |
-| pack12/index.js:10:28:10:50 | // $ me ... k12).g1 | Missing result:method=(pack12).g1 |
 failures
 ambiguousPreferredPredecessor
 | pack2/lib.js:1:1:3:1 | def moduleImport("pack2").getMember("exports").getMember("lib").getMember("LibClass").getInstance() |

javascript/ql/test/library-tests/EndpointNaming/EndpointNaming.ql

@@ -5,6 +5,10 @@ import semmle.javascript.endpoints.EndpointNaming as EndpointNaming
 import testUtilities.InlineExpectationsTest
 import EndpointNaming::Debug
 
+private predicate isIgnored(DataFlow::FunctionNode function) {
+  function.getFunction() = any(ConstructorDeclaration decl | decl.isSynthetic()).getBody()
+}
+
 module TestConfig implements TestSig {
   string getARelevantTag() { result = ["instance", "class", "method", "alias"] }
 
@@ -21,11 +25,12 @@ module TestConfig implements TestSig {
         EndpointNaming::classInstanceHasPrimaryName(cls, package, name)
       )
       or
-      exists(DataFlow::FunctionNode function |
-        not function.getFunction() = any(ConstructorDeclaration decl | decl.isSynthetic()).getBody() and
-        location = function.getFunction().getLocation() and
+      exists(DataFlow::SourceNode function |
+        not isIgnored(function) and
+        location = function.getAstNode().getLocation() and
         tag = "method" and
-        EndpointNaming::functionHasPrimaryName(function, package, name)
+        EndpointNaming::functionHasPrimaryName(function, package, name) and
+        not function instanceof DataFlow::ClassNode // reported with class tag
       )
     )
     or