Refactor Input/Outpts

ql/lib/codeql/actions/Ast.qll

@@ -2,7 +2,7 @@ private import codeql.actions.ast.internal.Actions
 private import codeql.Locations
 
 /**
- * Base class for thejAST tree. Based on YamlNode from the Yaml library.
+ * Base class for the AST tree. Based on YamlNode from the Yaml library.
  */
 class AstNode instanceof YamlNode {
   AstNode getParentNode() { result = super.getParentNode() }
@@ -23,7 +23,7 @@ class AstNode instanceof YamlNode {
   /**
    * Gets a environment variable expression by name in the scope of the current node.
    */
-  EnvExpr getEnvExpr(string name) {
+  Expression getEnvExpr(string name) {
     exists(Actions::Env env |
       env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
     |
@@ -42,9 +42,18 @@ class AstNode instanceof YamlNode {
 class CompositeAction extends AstNode instanceof Actions::CompositeAction {
   Runs getRuns() { result = super.getRuns() }
 
-  Inputs getInputs() { result = this.(YamlMapping).lookup("inputs") }
-
   Outputs getOutputs() { result = this.(YamlMapping).lookup("outputs") }
+
+  Expression getAnOutputExpr() { result = this.getOutputs().getAnOutputExpr() }
+
+  Expression getOutputExpr(string name) { result = this.getOutputs().getOutputExpr(name) }
+
+  Input getAnInput() { this.(YamlMapping).lookup("inputs").(YamlMapping).maps(result, _) }
+
+  Input getInput(string name) {
+    this.(YamlMapping).lookup("inputs").(YamlMapping).maps(result, _) and
+    result.(YamlString).getValue() = name
+  }
 }
 
 class Runs extends AstNode instanceof Actions::Runs {
@@ -81,23 +90,24 @@ class ReusableWorkflow extends Workflow {
 
   ReusableWorkflow() { this.(Actions::Workflow).getOn().getNode("workflow_call") = workflow_call }
 
-  Inputs getInputs() { result = workflow_call.(YamlMapping).lookup("inputs") }
-
   Outputs getOutputs() { result = workflow_call.(YamlMapping).lookup("outputs") }
+
+  Expression getAnOutputExpr() { result = this.getOutputs().getAnOutputExpr() }
+
+  Expression getOutputExpr(string name) { result = this.getOutputs().getOutputExpr(name) }
+
+  Input getAnInput() { workflow_call.(YamlMapping).lookup("inputs").(YamlMapping).maps(result, _) }
+
+  Input getInput(string name) {
+    workflow_call.(YamlMapping).lookup("inputs").(YamlMapping).maps(result, _) and
+    result.(YamlString).getValue() = name
+  }
 }
 
-class Inputs extends AstNode instanceof YamlMapping {
+class Input extends AstNode {
   YamlMapping parent;
 
-  Inputs() { parent.lookup("inputs") = this }
-
-  /**
-   * Gets a specific input expression (YamlMapping) by name.
-   */
-  InputExpr getInputExpr(string name) {
-    result.(YamlString).getValue() = name and
-    this.(YamlMapping).maps(result, _)
-  }
+  Input() { parent.lookup("inputs").(YamlMapping).maps(this, _) }
 }
 
 class Outputs extends AstNode instanceof YamlMapping {
@@ -106,9 +116,17 @@ class Outputs extends AstNode instanceof YamlMapping {
   Outputs() { parent.lookup("outputs") = this }
 
   /**
-   * Gets a specific output expression (YamlMapping) by name.
+   * Gets an output expression.
    */
-  OutputExpr getOutputExpr(string name) {
+  Expression getAnOutputExpr() {
+    this.(YamlMapping).lookup(_).(YamlMapping).lookup("value") = result or
+    this.(YamlMapping).lookup(_) = result
+  }
+
+  /**
+   * Gets a specific output expression by name.
+   */
+  Expression getOutputExpr(string name) {
     this.(YamlMapping).lookup(name).(YamlMapping).lookup("value") = result or
     this.(YamlMapping).lookup(name) = result
   }
@@ -130,7 +148,7 @@ class Strategy extends AstNode instanceof YamlMapping {
   /**
    * Gets a specific matric expression (YamlMapping) by name.
    */
-  MatrixVariableExpr getMatrixVariableExpr(string name) {
+  Expression getMatrixVariableExpr(string name) {
     this.(YamlMapping).lookup("matrix").(YamlMapping).lookup(name) = result
   }
 
@@ -318,41 +336,40 @@ class Run extends Step {
   string getScript() { result = scriptExpr.getValue() }
 }
 
-/**
- * An AST node associated with a Reusable Workflow input.
- */
-class InputExpr extends AstNode {
-  InputExpr() { exists(Inputs inputs | inputs.(YamlMapping).maps(this, _)) }
-}
-
-/**
- * An AST node holding an Env var value.
- */
-class EnvExpr extends AstNode {
-  EnvExpr() { exists(Actions::Env env | env.(YamlMapping).lookup(_) = this) }
-}
-
-/**
- * An AST node holding a job or workflow output var.
- */
-class OutputExpr extends AstNode {
-  OutputExpr() {
-    exists(Outputs outputs |
-      outputs.(YamlMapping).lookup(_).(YamlMapping).lookup("value") = this or
-      outputs.(YamlMapping).lookup(_) = this
-    )
-  }
-}
-
-/**
- * An AST node holding a matrix var.
- */
-class MatrixVariableExpr extends AstNode {
-  MatrixVariableExpr() {
-    exists(Strategy outputs | outputs.(YamlMapping).lookup("matrix").(YamlMapping).lookup(_) = this)
-  }
-}
-
+// /**
+//  * An AST node associated with a Reusable Workflow input.
+//  */
+// class InputExpr extends AstNode {
+//   InputExpr() { exists(Inputs inputs | inputs.(YamlMapping).maps(this, _)) }
+// }
+//
+// /**
+//  * An AST node holding an Env var value.
+//  */
+// class EnvExpr extends AstNode {
+//   EnvExpr() { exists(Actions::Env env | env.(YamlMapping).lookup(_) = this) }
+// }
+//
+// /**
+//  * An AST node holding a job or workflow output var.
+//  */
+// class OutputExpr extends AstNode {
+//   OutputExpr() {
+//     exists(Outputs outputs |
+//       outputs.(YamlMapping).lookup(_).(YamlMapping).lookup("value") = this or
+//       outputs.(YamlMapping).lookup(_) = this
+//     )
+//   }
+// }
+//
+// /**
+//  * An AST node holding a matrix var.
+//  */
+// class MatrixVariableExpr extends AstNode {
+//   MatrixVariableExpr() {
+//     exists(Strategy outputs | outputs.(YamlMapping).lookup("matrix").(YamlMapping).lookup(_) = this)
+//   }
+// }
 /**
  * Evaluation of a workflow expression ${{}}.
  */
@@ -508,9 +525,9 @@ class InputsExpression extends ContextExpression {
   override AstNode getTarget() {
     result.getLocation().getFile() = this.getLocation().getFile() and
     (
-      exists(ReusableWorkflow w | w.getInputs().getInputExpr(fieldName) = result)
+      exists(ReusableWorkflow w | w.getInput(fieldName) = result)
       or
-      exists(CompositeAction a | a.getInputs().getInputExpr(fieldName) = result)
+      exists(CompositeAction a | a.getInput(fieldName) = result)
     )
   }
 }

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

@@ -148,8 +148,8 @@ private class CompositeActionTree extends StandardPreOrderTree instanceof Compos
     result =
       rank[i](AstNode child, Location l |
         (
-          child = this.(CompositeAction).getInputs() or
-          child = this.(CompositeAction).getOutputs() or
+          child = this.(CompositeAction).getAnInput() or
+          child = this.(CompositeAction).getAnOutputExpr() or
           child = this.(CompositeAction).getRuns()
         ) and
         l = child.getLocation()
@@ -172,10 +172,10 @@ private class WorkflowTree extends StandardPreOrderTree instanceof Workflow {
       result =
         rank[i](AstNode child, Location l |
           (
-            child = this.(ReusableWorkflow).getInputs() or
-            child = this.(ReusableWorkflow).getOutputs() or
-            child = this.(ReusableWorkflow).getStrategy() or
-            child = this.(ReusableWorkflow).getAJob()
+            child = this.(ReusableWorkflow).getAJob() or
+            child = this.(ReusableWorkflow).getAnInput() or
+            child = this.(ReusableWorkflow).getAnOutputExpr() or
+            child = this.(ReusableWorkflow).getStrategy()
           ) and
           l = child.getLocation()
         |
@@ -199,19 +199,6 @@ private class WorkflowTree extends StandardPreOrderTree instanceof Workflow {
   }
 }
 
-private class InputsTree extends StandardPreOrderTree instanceof Inputs {
-  override ControlFlowTree getChildNode(int i) {
-    result =
-      rank[i](AstNode child, Location l |
-        child = super.getInputExpr(_) and l = child.getLocation()
-      |
-        child
-        order by
-          l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString()
-      )
-  }
-}
-
 private class OutputsTree extends StandardPreOrderTree instanceof Outputs {
   override ControlFlowTree getChildNode(int i) {
     result =
@@ -287,14 +274,13 @@ private class RunTree extends StandardPreOrderTree instanceof Run {
 
 private class UsesLeaf extends LeafTree instanceof Uses { }
 
-private class InputExprTree extends LeafTree instanceof InputExpr { }
-
-private class OutputExprTree extends LeafTree instanceof OutputExpr { }
-
-private class MatrixVariableExprTree extends LeafTree instanceof MatrixVariableExpr { }
-
-private class EnvExprTree extends LeafTree instanceof EnvExpr { }
+private class InputTree extends LeafTree instanceof Input { }
 
+// private class OutputExprTree extends LeafTree instanceof OutputExpr { }
+//
+// private class MatrixVariableExprTree extends LeafTree instanceof MatrixVariableExpr { }
+//
+// private class EnvExprTree extends LeafTree instanceof EnvExpr { }
 private class ExprAccessTree extends LeafTree instanceof ContextExpression { }
 
 private class AstNodeLeaf extends LeafTree instanceof Expression { }

ql/lib/codeql/actions/dataflow/FlowSources.qll

@@ -160,7 +160,7 @@ private class ExternallyDefinedSource extends RemoteFlowSource {
 private class CompositeActionInputSource extends RemoteFlowSource {
   CompositeAction c;
 
-  CompositeActionInputSource() { c.getInputs().getInputExpr(_) = this.asExpr() }
+  CompositeActionInputSource() { c.getAnInput() = this.asExpr() }
 
   override string getSourceType() { result = "Composite action input" }
 

ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll

@@ -54,8 +54,13 @@ DataFlowType getNodeType(Node node) { any() }
 predicate nodeIsHidden(Node node) { none() }
 
 class DataFlowExpr extends Cfg::Node {
-  DataFlowExpr() { any() }
-  //DataFlowExpr() { this.getAstNode() instanceof Expression }
+  DataFlowExpr() {
+    this.getAstNode() instanceof Expression or
+    this.getAstNode() instanceof Uses or
+    this.getAstNode() instanceof Run or
+    this.getAstNode() instanceof Outputs or
+    this.getAstNode() instanceof Input
+  }
 }
 
 /**
@@ -150,7 +155,7 @@ ContentApprox getContentApprox(Content c) { result = c }
  * Made a string to match the ArgumentPosition type.
  */
 class ParameterPosition extends string {
-  ParameterPosition() { exists(any(ReusableWorkflow w).getInputs().getInputExpr(this)) }
+  ParameterPosition() { exists(any(ReusableWorkflow w).getInput(this)) }
 }
 
 /**

ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll

@@ -48,22 +48,19 @@ class ExprNode extends Node, TExprNode {
  * Reusable workflow input nodes
  */
 class ParameterNode extends ExprNode {
-  private InputExpr input;
+  private Input input;
 
-  ParameterNode() {
-    this.asExpr() = input and
-    input = any(Inputs s).getInputExpr(_)
-  }
+  ParameterNode() { this.asExpr() = input }
 
   predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-    input = c.(ReusableWorkflow).getInputs().getInputExpr(pos)
+    input = c.(ReusableWorkflow).getInput(pos)
   }
 
   override string toString() { result = "input " + input.toString() }
 
   override Location getLocation() { result = input.getLocation() }
 
-  InputExpr getInputExpr() { result = input }
+  Input getInput() { result = input }
 }
 
 /**

ql/src/Security/CWE-020/CompositeActionsSinks.ql

@@ -25,7 +25,7 @@ private class ExpressionInjectionSink extends DataFlow::Node {
 
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    exists(CompositeAction c | c.getInputs().getInputExpr(_) = source.asExpr())
+    exists(CompositeAction c | c.getAnInput() = source.asExpr())
   }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof ExpressionInjectionSink }

ql/src/Security/CWE-020/CompositeActionsSources.ql

@@ -24,7 +24,7 @@ private module MyConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(CompositeAction c | c.getOutputs().getOutputExpr(_) = sink.asExpr())
+    exists(CompositeAction c | c.getAnOutputExpr() = sink.asExpr())
   }
 
   predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {

ql/src/Security/CWE-020/CompositeActionsSummaries.ql

@@ -18,11 +18,11 @@ import codeql.actions.dataflow.ExternalFlow
 
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    exists(CompositeAction c | c.getInputs().getInputExpr(_) = source.asExpr())
+    exists(CompositeAction c | c.getAnInput() = source.asExpr())
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(CompositeAction c | c.getOutputs().getOutputExpr(_) = sink.asExpr())
+    exists(CompositeAction c | c.getAnOutputExpr() = sink.asExpr())
   }
 }
 

ql/src/Security/CWE-020/ReusableWorkflowsSinks.ql

@@ -25,7 +25,7 @@ private class ExpressionInjectionSink extends DataFlow::Node {
 
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    exists(ReusableWorkflow w | w.getInputs().getInputExpr(_) = source.asExpr())
+    exists(ReusableWorkflow w | w.getAnInput() = source.asExpr())
   }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof ExpressionInjectionSink }

ql/src/Security/CWE-020/ReusableWorkflowsSources.ql

@@ -24,7 +24,7 @@ private module MyConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(ReusableWorkflow w | w.getOutputs().getOutputExpr(_) = sink.asExpr())
+    exists(ReusableWorkflow w | w.getAnOutputExpr() = sink.asExpr())
   }
 
   predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {

ql/src/Security/CWE-020/ReusableWorkflowsSummaries.ql

@@ -18,11 +18,11 @@ import codeql.actions.dataflow.ExternalFlow
 
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    exists(ReusableWorkflow w | w.getInputs().getInputExpr(_) = source.asExpr())
+    exists(ReusableWorkflow w | w.getAnInput() = source.asExpr())
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(ReusableWorkflow w | w.getOutputs().getOutputExpr(_) = sink.asExpr())
+    exists(ReusableWorkflow w | w.getAnOutputExpr() = sink.asExpr())
   }
 }
 

ql/test/library-tests/test.ql

@@ -29,13 +29,9 @@ query predicate runStepChildren(Run run, AstNode child) { child.getParentNode()
 
 query predicate parentNodes(AstNode child, AstNode parent) { child.getParentNode() = parent }
 
-query predicate cfgNodes(Cfg::Node n) {
-  n.getLocation().getFile().getBaseName() = "argus_case_study.yml"
-} //any() }
+query predicate cfgNodes(Cfg::Node n) { n.getLocation().getFile().getBaseName() = "test.yml" } //any() }
 
-query predicate dfNodes(DataFlow::Node e) {
-  e.getLocation().getFile().getBaseName() = "argus_case_study.yml"
-} //any() }
+query predicate dfNodes(DataFlow::Node e) { e.getLocation().getFile().getBaseName() = "test.yml" } //any() }
 
 query predicate exprNodes(DataFlow::Node e) { any() }