Make unguarded-action-lib better at ignoring uses of toolcache - #423
robertbrignull merged 3 commits into
Conversation
Sorry, something went wrong.
|
This does move the setting of the env vars to earlier in the command execution so they'll be set for slightly more time but this should be fine. It's worth noting that we didn't un-set them before, so they would still be set for the majority of code. Moving the setting to the start of the runner command hopefully makes things clearer and less surprising. |
Sorry, something went wrong.
|
Yay! The results are as expected: https://github.com/github/codeql-action/pull/423/checks?check_run_id=2122028618 |
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
This looks reasonable to me.
Sorry, something went wrong.
| // program | ||
| // .command("init") | ||
| // ... | ||
| // .action(async (cmd: InitArgs) => { | ||
| // ... | ||
| // }) |
There was a problem hiding this comment.
This won't catch patterns in this form:
program
.command("init")
...
program
.action(...)
Probably not a big deal since that's not a style being used in this code base. Though I wonder if we should explicitly add a query to prevent this style? In a separate PR.
Sorry, something went wrong.
There was a problem hiding this comment.
Do you mean https://www.npmjs.com/package/commander#stand-alone-executable-subcommands? Yeah that's not a style we're currently using. Since we're now relying on it here, we could add another query to help enforce that. Alternatively I think we'll fail "safely" in that if this predicate doesn't detect the entry point correctly then it'll cause it to emit false positive alerts, which at least means we'll get notified about it and won't miss anything.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes... that pattern. Also, patterns like this:
const command = program
.command("init")
...
command
.action(...)
Just because you can write code like this, doesn't mean that you should.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh right, I missed the variable assignment so I was a bit confused. Yes I agree that would be equivalent but would throw off the query.
Sorry, something went wrong.
There was a problem hiding this comment.
As you say above, may as well merge this as it's correct right now and doesn't make thing worse. We can introduce other checks later if we want to or need to.
Sorry, something went wrong.
This hopefully makes the unguarded-action-lib query ignore uses of @acitons/tool-cache that are hopefully ok because we're setting some environment variables like it's running on actions. This is not great but it's what we're doing and we're doing it intentionally so no point having open alerts about it. They'll just potentially hide more serious alerts.
Merge / deployment checklist