Rethink security scanning CI approach

[focusaurus]

Most of our javascript repositories are using the snyk security auditing tool in some fashion. This is often a CI check linked to github pull requests. I'm not sure this approach is the best.

Problems with snyk as a pull request CI check

• Running snyk test requires a snyk token which is a reaction-only secret and thus doesn't work when CI runs against a fork, which is the common case for community pull requests
• It fails a lot (Hi JS ecosystem!) and interrupts the flow of otherwise unrelated contributions
  • This is a blocker and in my opinion the cost (much higher barrier to community contributions, frustrating and confusing experience for new contributers) is much greater than the benefit (in theory would warn us "Hey this PR just added a dependency that has a vulnerability"). That scenario doesn't play out very commonly in my experience.
• We have a complicated set of logic to try to determine whether it's worthwhile to run snyk test or not

Alternatives to consider

• Make the CI test purely informational but remove passing snyk as a merge-blocking prereq
• Can we switch to latest npm and use npm audit without a snyk token instead?
• Can we find a different place in our SDLC/CI flow to ensure our latest dep metadata is uploaded to snyk/npm and we get alerts out of band via their emails?

CC: @rosshadden

[kieckhafer]

Looks like Snyk itself is dealing with the forked PR issue: https://github.com/snyk/snyk/pull/510

[spencern]

@focusaurus, thanks for getting this discussion started.

I've been thinking a lot about this over the past few weeks. It seems there's good momentum toward this proposal.

For several reasons, I agree with your conclusion that we should remove snyk test from our CI pipeline.

As you mention snyk fails to run during CI for all contributions from a fork. This alone is enough for me to consider this change.

The CI integration of snyk also fails to take into account whether the failure was introduced in a given pull request, causing unnecessary failure, and blocking PRs. The cost of rerunning a CI build is high, as our CI workflow takes 30 minutes or more to run.

We now have other snyk tools for auditing a PR for newly introduced vulnerabilities that were not present when we setup this CI workflow. For example, in the reaction repo, we have a GitHub check for licenses and for security, both run by snyk.

These seem to work regardless of whether a PR is from a fork or not, and are integrated via GitHub webhooks, rather than requiring a token to be shared.

We now also have better tools - such as the snyk dashboard - for ongoing monitoring of security vulnerabilities that have been introduced across all of our projects and don't have to wait for a PR to a project to learn about them.

[spencern]

@kieckhafer I think the issue may be one with CircleCI and how ENV variables (such as our token) are made available (or not) during the workflow. That issue seems to have stalled a bit

[spencern]

All considered, I'm in favor of this decision to remove snyk from our CI process and use the other tools available for auditing package vulnerabilities