Refactor AST layer

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -1,53 +1,74 @@
 private import codeql.actions.ast.internal.Actions
 private import codeql.Locations
 
+/**
+ * Base class for the AST tree.
+ * Based on YamlNode from the Yaml library but making mapping values children of the mapping keys:
+ * eg: top:
+ *        key: value
+ * According to the Yaml library, both `key` and `value` are direct children of `top`
+ * This Tree implementation makes `key` child od `top` and `value` child of `key`
+ */
 class AstNode instanceof YamlNode {
-  AstNode getParentNode() {
-    if exists(YamlMapping m | m.maps(_, this))
-    then exists(YamlMapping m | m.maps(result, this))
-    else result = super.getParentNode()
-  }
-
-  AstNode getAChildNode() {
-    if this instanceof YamlMapping
-    then this.(YamlMapping).maps(result, _)
-    else
-      if this instanceof YamlCollection
-      then result = super.getChildNode(_)
-      else
-        if this instanceof YamlScalar and exists(YamlMapping m | m.maps(this, _))
-        then exists(YamlMapping m | m.maps(this, result))
-        else none()
-  }
-
-  AstNode getChildNodeByOrder(int i) {
-    result =
-      rank[i](Expression child, Location l |
-        child = this.getAChildNode() and
-        child.getLocation() = l
-      |
-        child
-        order by
-          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
-      )
-  }
-
+  AstNode getParentNode() { result = super.getParentNode() }
+
+  // AstNode getParentNode() {
+  //   if exists(YamlMapping m | m.maps(_, this))
+  //   then exists(YamlMapping m | m.maps(result, this))
+  //   else result = super.getParentNode()
+  // }
+  AstNode getAChildNode() { result = super.getAChildNode() }
+
+  // AstNode getAChildNode() {
+  //   if this instanceof YamlMapping
+  //   then this.(YamlMapping).maps(result, _)
+  //   else
+  //     if this instanceof YamlCollection
+  //     then result = super.getChildNode(_)
+  //     else
+  //       if this instanceof YamlScalar and exists(YamlMapping m | m.maps(this, _))
+  //       then exists(YamlMapping m | m.maps(this, result))
+  //       else none()
+  // }
+  // /**
+  //  * This should be getAChildNode(int i)
+  //  */
+  // AstNode getChildNodeByOrder(int i) {
+  //   result =
+  //     rank[i](Expression child, Location l |
+  //       child = this.getAChildNode() and
+  //       child.getLocation() = l
+  //     |
+  //       child
+  //       order by
+  //         l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
+  //     )
+  // }
   string toString() { result = super.toString() }
 
   string getAPrimaryQlClass() { result = super.getAPrimaryQlClass() }
 
   Location getLocation() { result = super.getLocation() }
 }
 
-class Statement extends AstNode {
-  // narrow down to something that is a statement
-  // A statement is a group of expressions and/or statements that you design to carry out a task or an action.
-  // Any statement that can return a value is automatically qualified to be used as an expression.
-}
+/**
+ * A statement is a group of expressions and/or statements that you design to carry out a task or an action.
+ * Any statement that can return a value is automatically qualified to be used as an expression.
+ */
+class Statement extends AstNode { }
+
+/**
+ * An expression is any word or group of words or symbols that is a value. In programming, an expression is a value, or anything that executes and ends up being a value.
+ */
+class Expression extends Statement { }
+
+/**
+ * A Github Actions Workflow
+ */
+class WorkflowStmt extends Statement instanceof Actions::Workflow {
+  JobStmt getAJob() { result = super.getJob(_) }
 
-class Expression extends Statement {
-  // narrow down to something that is an expression
-  // An expression is any word or group of words or symbols that is a value. In programming, an expression is a value, or anything that executes and ends up being a value.
+  JobStmt getJob(string id) { result = super.getJob(id) }
 }
 
 /**
@@ -60,54 +81,53 @@ class JobStmt extends Statement instanceof Actions::Job {
    */
   string getId() { result = super.getId() }
 
-  /** Gets the human-readable name of this job, if any, as a string. */
-  string getName() {
-    result = super.getId()
-    or
-    not exists(string s | s = super.getId()) and result = "unknown"
-  }
-
   /** Gets the step at the given index within this job. */
   StepStmt getStep(int index) { result = super.getStep(index) }
 
   /** Gets any steps that are defined within this job. */
   StepStmt getAStep() { result = super.getStep(_) }
 
+  /**
+   * Gets a needed job.
+   * eg:
+   *     - needs: [job1, job2]
+   */
   JobStmt getNeededJob() {
     exists(Actions::Needs needs |
       needs.getJob() = this and
       result = needs.getANeededJob().(JobStmt)
     )
   }
 
-  Expression getJobOutputExpr(string varName) {
-    this.(Actions::Job)
-        .lookup("outputs")
-        .(YamlMapping)
-        .maps(any(YamlScalar a | a.getValue() = varName), result)
-  }
-
-  JobOutputStmt getJobOutputStmt() { result = this.(Actions::Job).lookup("outputs") }
-
-  Statement getSuccNode(int i) {
-    result =
-      rank[i](Expression child, Location l |
-        (child = this.getAStep() or child = this.getJobOutputStmt()) and
-        l = child.getLocation()
-      |
-        child
-        order by
-          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
-      )
-  }
+  /**
+   * Gets the declaration of the outputs for the job.
+   * eg:
+   *   out1: ${steps.foo.bar}
+   *   out2: ${steps.foo.baz}
+   */
+  JobOutputStmt getOutputStmt() { result = this.(Actions::Job).lookup("outputs") }
 }
 
+/**
+ * Declaration of the outputs for the job.
+ * eg:
+ *   out1: ${steps.foo.bar}
+ *   out2: ${steps.foo.baz}
+ */
 class JobOutputStmt extends Statement instanceof YamlMapping {
   JobStmt job;
 
   JobOutputStmt() { job.(YamlMapping).lookup("outputs") = this }
 
-  StepOutputAccessExpr getSuccNode(int i) { result = this.(YamlMapping).getValueNode(i) }
+  YamlMapping asYamlMapping() { result = this }
+
+  /**
+   * Gets a specific value expression
+   * eg: ${steps.foo.bar}
+   */
+  Expression getOutputExpr(string id) {
+    this.(YamlMapping).maps(any(YamlScalar s | s.getValue() = id), result)
+  }
 }
 
 /**
@@ -116,15 +136,7 @@ class JobOutputStmt extends Statement instanceof YamlMapping {
 class StepStmt extends Statement instanceof Actions::Step {
   string getId() { result = super.getId() }
 
-  string getName() {
-    result = super.getId()
-    or
-    not exists(string s | s = super.getId()) and result = "unknown"
-  }
-
   JobStmt getJob() { result = super.getJob() }
-
-  abstract AstNode getSuccNode(int i);
 }
 
 /**
@@ -145,57 +157,23 @@ class UsesExpr extends StepStmt, Expression {
       result = with.lookup(key)
     )
   }
-
-  Expression getArgumentByOrder(int i) {
-    exists(Actions::With with |
-      with.getStep() = uses.getStep() and
-      result =
-        rank[i](Expression child, Location l |
-          child = with.lookup(_) and l = child.getLocation()
-        |
-          child
-          order by
-            l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
-        )
-    )
-  }
-
-  Expression getAnArgument() {
-    exists(Actions::With with |
-      with.getStep() = this and
-      result = with.lookup(_)
-    )
-  }
-
-  override AstNode getSuccNode(int i) { result = this.getArgumentByOrder(i) }
 }
 
 /**
- * An argument passed to a UsesExpr.
+ * A Run step represents the evaluation of a provided script
  */
-class ArgumentExpr extends Expression {
-  UsesExpr uses;
-
-  ArgumentExpr() { this = uses.getAnArgument() }
-}
-
-/**
- * A Run step represents a call to an inline script or executable on the runner machine.
- */
-class RunExpr extends StepStmt {
+class RunExpr extends StepStmt, Expression {
   Actions::Run scriptExpr;
 
   RunExpr() { scriptExpr.getStep() = this }
 
   Expression getScriptExpr() { result = scriptExpr }
 
   string getScript() { result = scriptExpr.getValue() }
-
-  override AstNode getSuccNode(int i) { result = this.getScriptExpr() and i = 0 }
 }
 
 /**
- * A YAML string containing a workflow expression.
+ * Evaluation of a workflow expression ${{}}.
  */
 class ExprAccessExpr extends Expression instanceof YamlString {
   string expr;
@@ -208,7 +186,7 @@ class ExprAccessExpr extends Expression instanceof YamlString {
 }
 
 /**
- * A ExprAccessExpr where the expression references a step output.
+ * A ExprAccessExpr where the expression evaluated is a step output read.
  * eg: `${{ steps.changed-files.outputs.all_changed_files }}`
  */
 class StepOutputAccessExpr extends ExprAccessExpr {
@@ -230,7 +208,7 @@ class StepOutputAccessExpr extends ExprAccessExpr {
 }
 
 /**
- * A ExprAccessExpr where the expression references a job output.
+ * A ExprAccessExpr where the expression evaluated is a job output read.
  * eg: `${{ needs.job1.outputs.foo}}`
  */
 class JobOutputAccessExpr extends ExprAccessExpr {
@@ -250,7 +228,7 @@ class JobOutputAccessExpr extends ExprAccessExpr {
     exists(JobStmt job |
       job.getId() = jobId and
       job.getLocation().getFile() = this.getLocation().getFile() and
-      job.getJobOutputExpr(varName) = result
+      job.getOutputStmt().getOutputExpr(varName) = result
     )
   }
 }

ql/lib/codeql/actions/controlflow/internal/Cfg.qll

@@ -139,31 +139,40 @@ private import CfgImpl
 private import Completion
 private import CfgScope
 
-// Trees are what end up creating Cfg::Node objects and therefore DataFlow::Node objects.
-// Its also required that there is parent/child relationships between nodes so orphans nodes will not appear as either Cfg::Node or DataFlow::Node.
-// For example
-// - ArgumentExpr should be children of UsesExpr, and UsesExpr should be children of StepStmt.
-// TODO: We need to make VarAccess expressions part ot the tree as they are currently orphans
-private class CfgNodeTree extends StandardPreOrderTree instanceof AstNode {
-  override AstNode getChildNode(int i) { result = super.getChildNodeByOrder(i) }
+private class JobTree extends StandardPreOrderTree instanceof JobStmt {
+  override ControlFlowTree getChildNode(int i) {
+    result =
+      rank[i](Expression child, Location l |
+        (child = super.getAStep() or child = super.getOutputStmt()) and
+        l = child.getLocation()
+      |
+        child
+        order by
+          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
+      )
+  }
+}
+
+private class JobOutputTree extends StandardPreOrderTree instanceof JobOutputStmt {
+  override ControlFlowTree getChildNode(int i) { result = super.asYamlMapping().getValueNode(i) }
+}
+
+private class UsesTree extends StandardPreOrderTree instanceof UsesExpr {
+  override ControlFlowTree getChildNode(int i) {
+    result =
+      rank[i](Expression child, Location l |
+        child = super.getArgument(_) and l = child.getLocation()
+      |
+        child
+        order by
+          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
+      )
+  }
+}
+
+private class RunTree extends StandardPreOrderTree instanceof RunExpr {
+  override ControlFlowTree getChildNode(int i) { result = super.getScriptExpr() and i = 0 }
 }
-// private class JobStmtTree extends StandardPreOrderTree instanceof JobStmt {
-//   override ControlFlowTree getChildNode(int i) { result = super.getSuccNode(i) }
-// }
-//
-// private class StepStmtTree extends StandardPreOrderTree instanceof StepStmt {
-//   override ControlFlowTree getChildNode(int i) { result = super.getSuccNode(i) }
-// }
-//
-// private class JobOutputTree extends StandardPreOrderTree instanceof JobOutputStmt {
-//   override ControlFlowTree getChildNode(int i) { result = super.getSuccNode(i) }
-// }
-//
-// // TODO: Do we need this or we can just care about the ExprAccessExpr
-// private class ArgumentTree extends LeafTree instanceof ArgumentExpr { }
-//
-// private class ExprAccessTree extends LeafTree instanceof ExprAccessExpr { }
-//
-// private class StepOutputAccessTree extends LeafTree instanceof StepOutputAccessExpr { }
-//
-// private class JobOutputAccessTree extends LeafTree instanceof JobOutputAccessExpr { }
+
+private class ExprAccessTree extends LeafTree instanceof ExprAccessExpr { }
+