Send version and mode to code scanning via user agent by aeisenberg · Pull Request #516 · github/codeql-action · GitHub

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.

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

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.

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.

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

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

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.

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

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.

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/%") 
        
           }

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.

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.

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.

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.

Yes. That's better. Thanks.

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.

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.

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.

aeisenberg marked this pull request as draft May 20, 2021 22:21

aeisenberg force-pushed the aeisenberg/update-changelog-on-release branch 12 times, most recently from 1efbebf to 800a951 Compare May 21, 2021 18:04

aeisenberg changed the title [WIP] Add the isAction function Send version and mode to code scanning via user agent May 21, 2021

aeisenberg force-pushed the aeisenberg/user-agent branch from 0606d50 to e0c15c8 Compare May 21, 2021 19:52

aeisenberg mentioned this pull request May 21, 2021

Set the CODEQL_WRAPPER env var #496

Closed

2 tasks

aeisenberg marked this pull request as ready for review May 21, 2021 20:44

aeisenberg changed the base branch from aeisenberg/update-changelog-on-release to main May 21, 2021 20:45

aeisenberg requested review from adityasharad and robertbrignull May 21, 2021 20:45

adityasharad reviewed May 21, 2021

View reviewed changes

Comment thread src/api-client.ts Outdated

RA80533 suggested changes May 22, 2021

View reviewed changes

Comment thread src/actions-util.ts Outdated

RA80533 reviewed May 22, 2021

View reviewed changes

Comment thread src/actions-util.ts Outdated

aeisenberg force-pushed the aeisenberg/user-agent branch from ac8ab32 to a20890d Compare May 26, 2021 23:01

aeisenberg force-pushed the aeisenberg/user-agent branch from a20890d to e5734f0 Compare May 27, 2021 23:03

aeisenberg force-pushed the aeisenberg/user-agent branch from e5734f0 to c824362 Compare May 27, 2021 23:16

aeisenberg force-pushed the aeisenberg/user-agent branch from c824362 to af01d94 Compare May 28, 2021 18:47

aeisenberg force-pushed the aeisenberg/user-agent branch 2 times, most recently from c646c93 to 026871f Compare May 28, 2021 19:11

aeisenberg commented May 28, 2021

View reviewed changes

aeisenberg force-pushed the aeisenberg/user-agent branch from 3a474e8 to 8ade7ba Compare May 28, 2021 20:43

adityasharad approved these changes May 29, 2021

View reviewed changes

aeisenberg added 3 commits May 31, 2021 09:03

Send the version and mode with the user agent

4758879

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.

Use the version from package.json in the runner

1a4cdd3

aeisenberg force-pushed the aeisenberg/user-agent branch from 8ade7ba to 1a4cdd3 Compare May 31, 2021 16:35

aeisenberg enabled auto-merge May 31, 2021 16:44

aeisenberg merged commit ca94508 into main May 31, 2021

aeisenberg deleted the aeisenberg/user-agent branch May 31, 2021 16:49

github-actions Bot mentioned this pull request Jun 7, 2021

Merge main into v1 #549

Merged

5 tasks

+              export function setMode(mode: Mode) {
+                // avoid accessing actions core when in runner mode
+                if (mode === Mode.actions) {
+                  core.exportVariable(CODEQL_RUN_MODE_ENV_VAR, mode);

                   "(Advanced, windows-only) Inject a windows tracer of this process into a parent process <number> levels up."
                 )
                 .action(async (cmd: InitArgs) => {
+                  setMode(Mode.runner);

+                /**
+                 * Get an expr that is only executed on actions
+                 */
+                abstract Expr getAnActionsExpr();

Send version and mode to code scanning via user agent by aeisenberg · Pull Request #516 · github/codeql-action · GitHub

Send version and mode to code scanning via user agent - #516

Send version and mode to code scanning via user agent#516aeisenberg merged 3 commits intomaingithub/codeql-action:mainfrom aeisenberg/user-agentgithub/codeql-action:aeisenberg/user-agentCopy head branch name to clipboard

Conversation

aeisenberg commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RA80533 commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeisenberg commented May 27, 2021

Uh oh!

RA80533 commented May 27, 2021

Uh oh!

aeisenberg commented May 28, 2021

Uh oh!

aeisenberg May 28, 2021

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented May 28, 2021

Uh oh!

RA80533 commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeisenberg commented May 28, 2021

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

adityasharad May 28, 2021

Choose a reason for hiding this comment

Uh oh!

aeisenberg May 31, 2021

Choose a reason for hiding this comment

Uh oh!

adityasharad May 29, 2021

Choose a reason for hiding this comment

Uh oh!

aeisenberg May 31, 2021

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented May 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Send version and mode to code scanning via user agent#516
aeisenberg merged 3 commits into
mainfrom
aeisenberg/user-agent

aeisenberg commented May 20, 2021 •
edited
Loading

RA80533 commented May 26, 2021 •
edited
Loading

RA80533 commented May 28, 2021 •
edited
Loading