From 4164096c0da9e4e6abfe2b9814524384399d2c35 Mon Sep 17 00:00:00 2001
From: Andrew Eisenberg <aeisenberg@github.com>
Date: Mon, 31 May 2021 09:34:15 -0700
Subject: [PATCH] Use the version from package.json in the runner

Update the ql queries to account for change in how we look for runner

Previously, we guarded blocks of code to be run by the runner or the
action using if statements like this:

```js
if (mode === "actions") ...
```

We are no longer doing this. And now, the `unguarded-action-lib.ql`
query is out of date. This query checks that runner code does not
unintentionally access actions-only methods in the libraries.

With these changes, we now ensure that code scanning is happy.
---
 queries/unguarded-action-lib.ql | 106 +++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 21 deletions(-)

diff --git a/queries/unguarded-action-lib.ql b/queries/unguarded-action-lib.ql
index faa26d250..9a30f4361 100644
--- a/queries/unguarded-action-lib.ql
+++ b/queries/unguarded-action-lib.ql
@@ -90,17 +90,88 @@ class RunnerEntrypoint extends Function {
   }
 }
 
+/**
+ * A generic check to see if we are in actions or runner mode in a particular block of code.
+ */
+abstract class ActionsGuard extends IfStmt {
+
+  /**
+   * Get a statement block that is only executed on actions
+   */
+  abstract Stmt getActionsBlock();
+
+  /**
+    * Gets an expr that is only executed on actions
+   */
+  final Expr getAnActionsExpr() { getActionsBlock().getAChildStmt*().getAChildExpr*() = result }
+
+}
+
+/**
+ * A check of whether we are in actions mode or runner mode, based on
+ * the presense of a call to `isActions()` in the condition of an if statement.
+ */
+class IsActionsGuard extends ActionsGuard {
+  IsActionsGuard() {
+    getCondition().(CallExpr).getCalleeName() = "isActions"
+  }
+
+  /**
+   * Get the "then" block that is the "actions" path.
+   */
+  override Stmt getActionsBlock() {
+    result = getThen()
+  }
+}
+
+/**
+ * A check of whether we are in actions mode or runner mode, based on
+ * the presense of a call to `!isActions()` in the condition of an if statement.
+ */
+class NegatedIsActionsGuard extends ActionsGuard {
+  NegatedIsActionsGuard() {
+    getCondition().(LogNotExpr).getOperand().(CallExpr).getCalleeName() = "isActions"
+  }
+
+  /**
+   * Get the "else" block that is the "actions" path.
+   */
+  override Stmt getActionsBlock() {
+    result = getElse()
+  }
+}
+
+class ModeAccess extends PropAccess {
+  ModeAccess() {
+    (
+      // eg- Mode.actions
+      getBase().(Identifier).getName() = "Mode" or
+      // eg- actionUtil.Mode.actions
+      getBase().(PropAccess).getPropertyName() = "Mode"
+    ) and
+    (getPropertyName() = "actions" or getPropertyName() = "runner")
+  }
+
+  predicate isActions() {
+    getPropertyName() = "actions"
+  }
+
+  predicate isRunner() {
+    getPropertyName() = "runner"
+  }
+}
+
 /**
  * A check of whether we are in actions mode or runner mode.
  */
-class ModeGuard extends IfStmt {
+class ModeGuard extends ActionsGuard {
   ModeGuard() {
-    getCondition().(EqualityTest).getAnOperand().(StringLiteral).getValue() = "actions" or
-    getCondition().(EqualityTest).getAnOperand().(StringLiteral).getValue() = "runner"
+    getCondition().(EqualityTest).getAnOperand().(ModeAccess).isActions() or
+    getCondition().(EqualityTest).getAnOperand().(ModeAccess).isRunner()
   }
 
-  string getOperand() {
-    result = getCondition().(EqualityTest).getAnOperand().(StringLiteral).getValue()
+  ModeAccess getOperand() {
+    result = getCondition().(EqualityTest).getAnOperand()
   }
 
   predicate isPositive() {
@@ -110,21 +181,14 @@ class ModeGuard extends IfStmt {
   /**
    * Get the then or else block that is the "actions" path.
    */
-  Stmt getActionsBlock() {
-    (getOperand() = "actions" and isPositive() and result = getThen())
+  override Stmt getActionsBlock() {
+    (getOperand().isActions() and isPositive() and result = getThen())
     or
-    (getOperand() = "runner" and not isPositive() and result = getThen())
+    (getOperand().isRunner() and not isPositive() and result = getThen())
     or
-    (getOperand() = "actions" and not isPositive() and result = getElse())
+    (getOperand().isActions() and not isPositive() and result = getElse())
     or
-    (getOperand() = "runner" and isPositive() and result = getElse())
-  }
-
-  /**
-   * Get an expr that is only executed on actions
-   */
-  Expr getAnActionsExpr() {
-    getActionsBlock().getAChildStmt*().getAChildExpr*() = result
+    (getOperand().isRunner() and isPositive() and result = getElse())
   }
 }
 
@@ -133,7 +197,7 @@ class ModeGuard extends IfStmt {
  * and is not only called on actions.
  */
 Expr getAFunctionChildExpr(Function f) {
-  not exists(ModeGuard guard | guard.getAnActionsExpr() = result) and
+  not exists(ActionsGuard guard | guard.getAnActionsExpr() = result) and
   result.getContainer() = f
 }
 
@@ -145,16 +209,16 @@ Function calledBy(Function f) {
   exists(InvokeExpr invokeExpr |
     invokeExpr = getAFunctionChildExpr(f) and
     invokeExpr.getResolvedCallee() = result and
-    not exists(ModeGuard guard | guard.getAnActionsExpr() = invokeExpr)
+    not exists(ActionsGuard guard | guard.getAnActionsExpr() = invokeExpr)
   )
   or
   // Assume outer function causes inner function to be called
   (result instanceof Expr and
   result.getEnclosingContainer() = f and
-  not exists(ModeGuard guard | guard.getAnActionsExpr() = result))
+  not exists(ActionsGuard guard | guard.getAnActionsExpr() = result))
 }
 
-from VarAccess v, ActionsLibImport actionsLib, RunnerEntrypoint runnerEntry 
+from VarAccess v, ActionsLibImport actionsLib, RunnerEntrypoint runnerEntry
 where actionsLib.getAProvidedVariable() = v.getVariable()
   and getAFunctionChildExpr(calledBy*(runnerEntry)) = v
   and not (isSafeActionLibWithActionsEnvVars(actionsLib.getName()) and runnerEntry.setsActionsEnvVars())