Merge pull request #229 from github/hvitved/api-graphs/remove-mk-module

API graphs: Remove `MkModule`

Author: hvitved

ql/src/codeql_ruby/ApiGraphs.qll

@@ -198,13 +198,9 @@ module API {
   }
 
   /** A node corresponding to the use of an API component. */
-  class Use extends Node, Impl::TUse {
+  class Use extends Node, Impl::MkUse {
     override string toString() {
-      exists(string type |
-        this = Impl::MkUse(_) and type = "Use "
-        or
-        this = Impl::MkModule(_) and type = "ModuleUse "
-      |
+      exists(string type | this = Impl::MkUse(_) and type = "Use " |
         result = type + getPath()
         or
         not exists(this.getPath()) and result = type + "with no path"
@@ -216,27 +212,15 @@ module API {
   Root root() { any() }
 
   /**
-   * Gets a node corresponding to an import of module `m`.
+   * Gets a node corresponding to a top-level member `m` (typically a module).
+   *
+   * This is equivalent to `root().getAMember("m")`.
    *
    * Note: You should only use this predicate for top level modules or classes. If you want nodes corresponding to a nested module or class,
    * you should use `.getMember` on the parent module/class. For example, for nodes corresponding to the class `Gem::Version`,
-   * use `moduleImport("Gem").getMember("Version")`.
+   * use `getTopLevelMember("Gem").getMember("Version")`.
    */
-  Node moduleImport(string m) {
-    result = Impl::MkModule(m) and not m.matches("%::%")
-    or
-    result = Impl::MkUse(unresolvedModuleReference(m))
-  }
-
-  DataFlow::Node unresolvedModuleReference(string m) {
-    exists(ConstantReadAccess iexpr, DataFlow::Node node |
-      not exists(resolveScopeExpr(iexpr)) and
-      not exists(iexpr.getScopeExpr()) and
-      m = iexpr.getName() and
-      result = node and
-      node.asExpr().getExpr() = iexpr
-    )
-  }
+  Node getTopLevelMember(string m) { result = root().getMember(m) }
 
   /**
    * Provides the actual implementation of API graphs, cached for performance.
@@ -266,23 +250,29 @@ module API {
     newtype TApiNode =
       /** The root of the API graph. */
       MkRoot() or
-      /** An abstract representative for imports of the module called `name`. */
-      MkModule(string name) { exists(TResolved(name)) } or
       /** A use of an API member at the node `nd`. */
-      MkUse(DataFlow::Node nd) {
-        use(_, _, nd)
-        or
-        nd = unresolvedModuleReference(_)
-      }
-
-    class TUse = MkModule or MkUse;
+      MkUse(DataFlow::Node nd) { use(_, _, nd) }
 
     /**
      * Holds if `ref` is a use of a node that should have an incoming edge from `base` labeled
      * `lbl` in the API graph.
      */
     cached
     predicate use(TApiNode base, string lbl, DataFlow::Node ref) {
+      base = MkRoot() and
+      exists(string name, ExprNodes::ConstantAccessCfgNode access, ConstantReadAccess read |
+        access = ref.asExpr() and
+        lbl = Label::member(read.getName()) and
+        read = access.getExpr()
+      |
+        TResolved(name) = resolveScopeExpr(read) and
+        not name.matches("%::%")
+        or
+        name = read.getName() and
+        not exists(resolveScopeExpr(read)) and
+        not exists(read.getScopeExpr())
+      )
+      or
       exists(DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode pred |
         // First, we find a predecessor of the node `ref` that we want to determine. The predecessor
         // is any node that is a type-tracked use of a data flow node (`src`), which is itself a
@@ -331,16 +321,7 @@ module API {
      * Holds if `ref` is a use of node `nd`.
      */
     cached
-    predicate use(TApiNode nd, DataFlow::Node ref) {
-      exists(string name, ExprNodes::ConstantAccessCfgNode access, ConstantReadAccess read |
-        access = ref.asExpr() and
-        nd = MkModule(name) and
-        read = access.getExpr() and
-        TResolved(name) = resolveScopeExpr(read)
-      )
-      or
-      nd = MkUse(ref)
-    }
+    predicate use(TApiNode nd, DataFlow::Node ref) { nd = MkUse(ref) }
 
     /**
      * Gets a data-flow node to which `src`, which is a use of an API-graph node, flows.
@@ -370,14 +351,6 @@ module API {
      */
     cached
     predicate edge(TApiNode pred, string lbl, TApiNode succ) {
-      /* There's an edge from the root node for each imported module. */
-      exists(string m |
-        pred = MkRoot() and
-        lbl = Label::mod(m)
-      |
-        succ = moduleImport(m)
-      )
-      or
       /* Every node that is a use of an API component is itself added to the API graph. */
       exists(DataFlow::LocalSourceNode ref |
         use(pred, lbl, ref) and
@@ -397,11 +370,6 @@ module API {
 }
 
 private module Label {
-  /** Gets the edge label for the module `m`. */
-  bindingset[m]
-  bindingset[result]
-  string mod(string m) { result = "moduleImport(\"" + m + "\")" }
-
   /** Gets the `member` edge label for member `m`. */
   bindingset[m]
   bindingset[result]

ql/src/queries/security/cwe-732/WeakFilePermissions.ql

@@ -49,7 +49,7 @@ class PermissionArgument extends DataFlow::Node {
 
   PermissionArgument() {
     exists(string methodName |
-      call = API::moduleImport(["File", "FileUtils"]).getAMethodCall(methodName)
+      call = API::getTopLevelMember(["File", "FileUtils"]).getAMethodCall(methodName)
     |
       methodName in ["chmod", "chmod_R", "lchmod"] and this = call.getArgument(0)
       or

ql/test/library-tests/dataflow/api-graphs/test1.rb

@@ -1,13 +1,24 @@
-MyModule #$ use=moduleImport("MyModule")
-print MyModule.foo #$ use=moduleImport("MyModule").getReturn("foo")
-Kernel.print(e) #$ use=moduleImport("Kernel").getReturn("print")
-Object::Kernel #$ use=moduleImport("Kernel")
-Object::Kernel.print(e)  #$ use=moduleImport("Kernel").getReturn("print")
+MyModule #$ use=getMember("MyModule")
+print MyModule.foo #$ use=getMember("MyModule").getReturn("foo")
+Kernel.print(e) #$ use=getMember("Kernel").getReturn("print")
+Object::Kernel #$ use=getMember("Kernel")
+Object::Kernel.print(e)  #$ use=getMember("Kernel").getReturn("print")
 begin
-    print MyModule.bar #$ use=moduleImport("MyModule").getReturn("bar")
-    raise AttributeError #$ use=moduleImport("AttributeError")
-rescue AttributeError => e #$ use=moduleImport("AttributeError")
-    Kernel.print(e)  #$ use=moduleImport("Kernel").getReturn("print")
+    print MyModule.bar #$ use=getMember("MyModule").getReturn("bar")
+    raise AttributeError #$ use=getMember("AttributeError")
+rescue AttributeError => e #$ use=getMember("AttributeError")
+    Kernel.print(e)  #$ use=getMember("Kernel").getReturn("print")
 end
-Unknown.new.run #$ use=moduleImport("Unknown").instance.getReturn("run")
-Foo::Bar::Baz #$ use=moduleImport("Foo").getMember("Bar").getMember("Baz")
+Unknown.new.run #$ use=getMember("Unknown").instance.getReturn("run")
+Foo::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz")
+
+Const = [1, 2, 3]
+Const.each do |c| #$ use=getMember("Const").getReturn("each")
+    puts c
+end
+
+foo = Foo #$ use=getMember("Foo")
+foo::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz")
+
+FooAlias = Foo #$ use=getMember("Foo")
+FooAlias::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz")