department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
99 stars 69 forks source link

Discovery: Address Dependabot Security Alerts #12782

Closed EWashb closed 1 year ago

EWashb commented 1 year ago

Description

Dependabot vulnerability alerts were found in the repo that should be addressed. These unaddressed alerts pose security risks ranging from critical to low. Many are the result of the Claro Theme upgrade. Thankfully, the effort for fixing these is relatively low.

The following is a screenshot of some of the issues contained in the link above:

Screenshot 2023-02-27 at 8 57 45 PM

Acceptance Criteria

Team

Please check the team(s) that will do this work.

ndouglas commented 1 year ago

The lift for these is not necessarily low. The reason these do not appear as normal Dependabot pull requests is because Dependabot encountered some issue attempting to update the package.

For instance, just to pick the top one off the list:

https://github.com/department-of-veterans-affairs/va.gov-cms/security/dependabot/61

Screenshot 2023-03-09 at 8.36.31 AM.png

With a lot of these updates, the package that needs to be updated is nested somewhere back in the dependency tree and might be a dependency of multiple individual packages. This becomes an issue when these cascade into mutually-conflicting package requirements, or when packages have been de facto or de jure abandoned, or even just because the affected component(s) are so deep within the dependency tree that we don't even know what they do; we have to research just to figure out in what way a system could possibly break... and that's if the packages in question are actually used by our code, even if mediated by dozens of levels of indirection.

It should also be noted that about a number of these are updates to vagovadmin, which is no longer in use and should actually be removed (see #11011). That doesn't actually reduce the lift much of the individual items, since they have correspondences in vagovclaro.

Furthermore, most or all of these vulnerabilities exist in code that is executed only when the CMS theme is built, and is not exposed even to the editors and admins of the CMS. That is not to say that we should not remedy these issues. These are visible and it's better to address a vulnerability than to not address it. But we cannot determine the actual risk to the organization posed by a given vulnerability based simply on the label attached by Dependabot.

I don't like the idea of changing 21 -- or 15, or 10, or however many issues there actually are -- at once, as part of a single PR. We are not going to be able to deeply and meaningfully test the effects of each of these changes, not individually, and certainly not collectively. In my opinion, we should split this out to make later forensics easier should one update or another incur some unexpected change.

In my opinion, we should:

edmund-dunn commented 1 year ago

Recommended steps:

  1. Create Epic for updating and retooling theme build tools
  2. Add #11011 as the first ticket in epic; this would eliminate roughly half of the alerts
  3. Need to replace gulp with npm scripts; gulp hasn't had a release for four years and has introduced some vulnerabilities