Send version and mode to code scanning via user agent

[aeisenberg]

The initial goal of this change is to send more complete information to code scanning via the user agent. This also opens the possibility of sending the action/runner version to the CLI.

This commit changes the way the action determines if running in action
or runner mode. There is now an environment variable that is set at the
beginning of the process and elsewhere in the process, we can check to
see if the variable is set.

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• Confirm the readme has been updated if necessary.
• Confirm the changelog has been updated if necessary.

[adityasharad]

Neat! I defer to Robert on whether the mode initialisations are in the right place, but the logic looks sensible to me.

[RA80533]

The conditional branching within the updated getCodeQLBundleDownloadURL function should be tested based on the new method of determining the mode. Every occurrence in which an explicit mode was passed in now implicitly relies upon the environment in which it is called.

Should that implicit behavior be moved to a higher level of abstraction, e.g., into functions directly consumed by the end-user? There are some low-level functions that rely on the results of getCodeQLBundleDownloadURL.

[aeisenberg]

Thanks for the comment @RA80533. I added a couple of more unit tests. I feel like the code is reasonably safe now. We are sure to set the mode at the beginning of every entry point. If there is a call to get the mode and it hasn't been set yet, then that will fail.

In some ways, the logic is more complex because some low level functions now rely on a new environment variable, but the trade off is that we no longer need to pass the mode parameter around. I had originally tried keeping the mode parameter and passing it around, but then almost every function needed access to it. Using the environment variable in a single place seems like a reasonable trade-off. There are other parts of the code that also rely on environment variables, so this situation is not too different.

[RA80533]

In some ways, the logic is more complex because some low level functions now rely on a new environment variable, but the trade off is that we no longer need to pass the mode parameter around. I had originally tried keeping the mode parameter and passing it around, but then almost every function needed access to it. Using the environment variable in a single place seems like a reasonable trade-off. There are other parts of the code that also rely on environment variables, so this situation is not too different.

It is very much a case of deciding which to trade off. Environment variables become unwieldy in larger projects because they create a mathematical side effect in the flow of logic; for smaller projects, such as this one, the flow of data is easy enough to follow to the point that such dependent behavior is easy to track down.

/lgtm

[aeisenberg]

Ooooh...code scanning found a bug. Thank you code scanning.

[aeisenberg]

Note this is a code scanning error, but a false positive. We cannot have a code path accessible from the runner that will call into the core library. The if clause above prevents this from happening.

[aeisenberg]

Updated ql queries that look for runner/actions access. Could I get another look at this?

[RA80533]

Without the guard, what would be available to non-Actions runners through the core library?

EDIT: Oh, I see. The code library doesn’t support non-GitHub Actions environments.

[aeisenberg]

Yes, mostly right...there are some actions libraries that are ok to use for the runner.

codeql-action/queries/unguarded-action-lib.ql

Lines 16 to 21 in 3a474e8

	predicate isSafeActionLib(string lib) {
	lib = "@actions/http-client" or
	lib = "@actions/exec" or
	lib = "@actions/io" or
	lib.matches("@actions/exec/%")
	}

[adityasharad]

Nice. Logic makes sense and neat adjustment to the query; a few minor suggestions there.

There's probably room to combine more of the logic in that query, and use some builtin features of the CodeQL JS libraries, but we can do that separately. No need to hold up this PR.

[adityasharad]

Minor: I wonder if there's a prehook we can use to always set the mode to runner at the start of each action() call in this file. Or we can check it by query, as a follow up.

[aeisenberg]

Hmmm...it looks like there is an event mechanism for commander, but it's not quite what we need and it is internal, so I think we will need to create a query here.

[adityasharad]

You could instead declare:

abstract Stmt getActionsBlock();

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

This way each subclass needs to describe what they consider an Actions-specific statement, and you don't need to repeat the logic for an Actions-specific expression.

[aeisenberg]

Yes. That's better. Thanks.

[aeisenberg]

Wait...commander v8 is soon to be released with a hook for preAction and postAction in it. I will get this merged, and then do the update for v8.