Separate out security checks from test:static

[deviantintegral]

There can be good reasons to merge PRs that have open security issues:

1. There is a related upgrade that needs to be merged and tested first before upgrading the insecure package.
2. The site has some other issue going on concurrently ("the home page is down!").
3. If test:static is called from a pre-commit hook, it may be that another developer is handling the security update concurrently in a separate PR.

Let's remove test:security from test:static. On the one hand, I think it could be good to add test:security as a nightly CI job. However, with dependabot or renovate, that may not be useful.

[justafish]

Whilst Renovate/Dependabot will pick up a security issue in the main branch, it doesn't tell you if the package you've just added to your PR contains a security issue (which has happened to me many times with node packages). I can understand it might be annoying for scenario #2 where time is of the essence, but isn't that more an issue of the required status checks you've enabled that's preventing it from being merged? Maybe a compromise here would be having a file of known security issues that can be skipped (as we've done in a previous project we both worked on 😄 )

[deviantintegral]

I think a file of known issues to skip is good. It would be a small bump in development on open PRs ("why did my PR all of a sudden start failing"), but also one that's easy enough to fix.

[hawkeyetwolf]

Do you have any particular format in mind (json?) or example for what the "file of known issues" would look like? Thanks! 🙏🏻

[justafish]

Here's an example pulled from a project, but open to other suggestions!

# Format:
#
#  # Package name
#  # Title
#  # Reason ignored
#  Advisory URL

#####################
# Yarn Dependencies #
#####################

# url-regex
# Regex Denial of Service
# Only used for testing tools
https://www.npmjs.com/advisories/1550

#########################
# Composer Dependencies #
#########################

#!/bin/sh

yarn audit --level moderate
RESULT=$?

VULNERABLE=false
if [ "${RESULT}" != 0 ] && [ -f security-known-issues ]; then
  ADVISORIES=$(yarn audit --level moderate --json | jq -r 'select(.type=="auditAdvisory") | .data.advisory.url')
  for ADVISORY in ${ADVISORIES}; do
      grep -x -q ${ADVISORY} security-known-issues
      if [ $? ]; then
        echo "Known vulnerability: ${ADVISORY}"
      else
        echo "Unknown vulnerability: ${ADVISORY}"
        VULNERABLE=true
      fi
  done

  if [ "${VULNERABLE}" = false ]; then
    echo
    echo "No unknown vulnerabilities found."
    exit 0
  fi
  echo
  echo "New security vulnerabilities were found. If they can be safely ignored (e.g. for build tools) then add the advisory URL as a new line to security-known-issues."
fi

exit ${RESULT}

[deviantintegral]

Things I like:

1. Simple - in practice, just URLs and comments.
2. Flexible - would presumably support any URL.

I wonder:

1. does every vulnerability shown have a URL that's easily parsable? I assume so, but worth checking.
2. Should these be explicitly scoped to the directory of the package file? In other words, a separate file would exist inside of a Drupal theme for its package.json.
3. Do we want something structured in YAML (ugh, but it's standard) so if we need to add more keys later we can?

• I suppose this could allow for a single security file, where we had an outer level that pointed to the file each ignore went to.

4. Should there be an "expiry date", even optional? For example, let's say there's a Drupal update, and I'm just wanting to unblock my completely unrelated PR. It could be nice to have the warning re-surface after a week or two in case it's somehow forgotten.
5. Are we sure there isn't any sort of common format we can rely on that would be surfaced automatically to dependabot or other reports? It would be great if this could feed into those automatically, reducing email noise.

[davereid]

We tried to bundle "checking security along with linting" on a previous project and found it was really undesirable. Ideally, security checks are a separate workflow so that it can be marked as not required and won't block existing PRs until the security issue is resolved.

Things like composer audit failing on unsupported packages like happened on #478 would be nice to cause failures everywhere while it gets solved upstream.

[davereid]

Do we have a direction on where we'd like to continue with this discussed on Monday?

[deviantintegral]

Dave and I had a chance to discuss this today. We would like to separate out the security jobs and recommend they run on a daily schedule. In the future, it would be OK if we added a job that looked specifically at changed dependencies in the lock file and then failed a PR, but that's certainly more work and this issue has come up on multiple projects. So, let's get the easy thing done first!

[davereid]

Getting started on this afternoon after getting the Composer Lock Diff changes ready for review today.

[davereid]

Continuing work on this and expect to have at least a draft PR ready for initial review today.

[davereid]

Hmm, I thought there would be more to this but there doesn't seem to be a generic scaffolded workflow for static tests that Drainpipe provides. For example, https://github.com/Lullabot/lullabot.com-d8/blob/main/.github/workflows/TestStatic.yml seems to exist by itself.

[davereid]

PR ready for review at #558

[justafish]

We discussed the issues with the various solutions today:

• If we have a separate job that can chew up minutes
• We would like to see when adding a new package to a project whether or not it introduces a security issue
• Renovate doesn't mark Drupal updates with the security label, so they don't always get opened in a timely manor if you have throttling set on Renovate

We decided to go with the solution further up in the comments and make that work for composer #112 (comment)
It seems that local-security-checker and composer audit are checking the same databases, with the difference being that local security checker is a standalone go binary and so doesn't need a PHP environment to run in