Skip to content

chore: fix audit-deps npm script on deeply nested inherited advisories#2808

Merged
rpl merged 1 commit intomasterfrom
chore/fix-audit-deps-on-deeply-nested-inherited-advisories
Jul 17, 2023
Merged

chore: fix audit-deps npm script on deeply nested inherited advisories#2808
rpl merged 1 commit intomasterfrom
chore/fix-audit-deps-on-deeply-nested-inherited-advisories

Conversation

@rpl
Copy link
Member

@rpl rpl commented Jul 6, 2023

While reviewing the CI jobs failure hit by most of the dependabot PR opened over the last week we noticed that it was hitting the audit-deps step of the CI job but the output of our audit-deps npm script (which parses the json output of the npm audit command) wasn't always providing the details about the security advisory associated to the reported npm dependency, e.g. like here for nyc: https://app.circleci.com/pipelines/github/mozilla/web-ext/2388/workflows/f22e2ff0-ed9c-4373-88d5-bccb0bf6869c/jobs/9630?invite=true#step-116-141

Looked into the reasons behind that, I noticed that the audit-deps script was currently assuming that the via attribute in each of the reported npm packages details was going to be either an object with the details for the security advisory or a string that is meant to be the package name from which that package is inheriting the security advisory, but it wasn't accounting for more deeply nested cases

This PR does apply a small change to recursively resolve the elements of the via properties, which as an example turns the output from the linked PR:

nyc (isDirect: true, severity: moderate, fixAvailable: true):
  undefined
    undefined undefined
    undefined
  undefined
    undefined undefined
    undefined
  ...

into the one we expected:

nyc (isDirect: true, severity: moderate, fixAvailable: true):
  https://github.com/advisories/GHSA-c2qf-rxjj-qqgw
    semver <7.5.2
    semver vulnerable to Regular Expression Denial of Service
...

@rpl rpl requested a review from willdurand July 6, 2023 18:53
@rpl rpl force-pushed the chore/fix-audit-deps-on-deeply-nested-inherited-advisories branch from ec43f8c to 6c3b372 Compare July 6, 2023 19:32
@rpl
Copy link
Member Author

rpl commented Jul 6, 2023

After fixing the issue in our audit-deps script, it becomes clear that there are two advisory that are triggering all the audit failures, in particular the semver one is the one that is hit by most of them as a transitive dependency (and based on a quick look not all the dependencies have been updated to fix their semver npm dependency):

diff --git a/.nsprc b/.nsprc
index 70c3aa7..8641594 100644
--- a/.nsprc
+++ b/.nsprc
@@ -1,6 +1,12 @@
 {
   "exceptions": [
     // See: https://github.com/mozilla/web-ext/issues/2678
-    "https://github.com/advisories/GHSA-p8p7-x288-28g6"
+    "https://github.com/advisories/GHSA-p8p7-x288-28g6",
+
+    // semver advisory
+    "https://github.com/advisories/GHSA-c2qf-rxjj-qqgw",
+
+    // word-wrap advisory
+    "https://github.com/advisories/GHSA-j8xg-fqg3-53r7"
   ]
 }

@willdurand Before considering adding these two to our nsprc and file bugs to remove the exceptions, I think that we may want to consider merging the dependabot PRs that are hitting only the audit-deps failure and then double-check how many other dependency are still make us hit the npm audit-deps because of this two advisories (and if any of them is actually part of runtime dependency which would also be hit by users of the web-ext package).

wdyt?

@willdurand
Copy link
Member

@willdurand Before considering adding these two to our nsprc and file bugs to remove the exceptions, I think that we may want to consider merging the dependabot PRs that are hitting only the audit-deps failure and then double-check how many other dependency are still make us hit the npm audit-deps because of this two advisories (and if any of them is actually part of runtime dependency which would also be hit by users of the web-ext package).

+1

@rpl rpl merged commit 97a2189 into master Jul 17, 2023
@rpl rpl deleted the chore/fix-audit-deps-on-deeply-nested-inherited-advisories branch July 17, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants