Skip to content

Commit

Permalink
Update the ql queries to account for change in how we look for runner
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aeisenberg committed May 28, 2021
1 parent 026871f commit 8ade7ba
Showing 1 changed file with 93 additions and 14 deletions.
107 changes: 93 additions & 14 deletions queries/unguarded-action-lib.ql
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,96 @@ 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 an expr that is only executed on actions
*/
abstract Expr getAnActionsExpr();
}

/**
* 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.
*/
Stmt getActionsBlock() {
result = getThen()
}

/**
* Get an expr that is only executed on actions
*/
override 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 NegatedIsActionsGuard extends ActionsGuard {
NegatedIsActionsGuard() {
getCondition().(LogNotExpr).getOperand().(CallExpr).getCalleeName() = "isActions"
}

/**
* Get the "else" block that is the "actions" path.
*/
Stmt getActionsBlock() {
result = getElse()
}

/**
* Get an expr that is only executed on actions
*/
override Expr getAnActionsExpr() {
getActionsBlock().getAChildStmt*().getAChildExpr*() = result
}
}

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() {
Expand All @@ -111,19 +190,19 @@ class ModeGuard extends IfStmt {
* Get the then or else block that is the "actions" path.
*/
Stmt getActionsBlock() {
(getOperand() = "actions" and isPositive() and result = getThen())
(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())
(getOperand().isRunner() and isPositive() and result = getElse())
}

/**
* Get an expr that is only executed on actions
*/
Expr getAnActionsExpr() {
override Expr getAnActionsExpr() {
getActionsBlock().getAChildStmt*().getAChildExpr*() = result
}
}
Expand All @@ -133,7 +212,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
}

Expand All @@ -145,16 +224,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())
Expand Down

0 comments on commit 8ade7ba

Please sign in to comment.