"Moderate severity" dependency vulerability stemming from got (<11.8.5) module

[soyfrien]

You'll get a warning about "4 moderate severity" vulnerabilities when installing from source.

 ~  npm audit
npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.
# npm audit report

got  <11.8.5
Severity: moderate
Got allows a redirect to a UNIX socket - https://github.com/advisories/GHSA-pfrx-2q88-qq97
fix available via `npm audit fix --force`
Will install [email protected], which is a breaking change
node_modules/got
  package-json  <=6.5.0
  Depends on vulnerable versions of got
  node_modules/package-json
    latest-version  0.2.0 - 5.1.0
    Depends on vulnerable versions of package-json
    node_modules/latest-version
      update-notifier  0.2.0 - 5.1.0
      Depends on vulnerable versions of latest-version
      node_modules/update-notifier

4 moderate severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Running the audit fix does break cli.

..\source\repos\cli-microsoft365\dist\index.js:17
    const updateNotifier = require('update-notifier');
                           ^

Error [ERR_REQUIRE_ESM]: require() of ES Module ...\cli-microsoft365\node_modules\update-notifier\index.js from ..\cli-microsoft365\dist\index.js not supported.
Instead change the require of ..\cli-microsoft365\node_modules\update-notifier\index.js in ..\cli-microsoft365\dist\index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (...\cli-microsoft365\dist\index.js:17:28) {
  code: 'ERR_REQUIRE_ESM'

[garrytrinder]

Thank you for raising this @soyfrien, we are aware of the vulnerability, you can see my comment in https://github.com/pnp/cli-microsoft365/pull/3504 that we were unable to upgrade update-notifier (amongst other packages) to the latest version due to a breaking change which would resolve this issue.

The breaking change is due to the update-notifier package changing from using CommonJS to ES modules, unfortunately this is not a trivial change to the CLI but is something that we are actively investigating.

[soyfrien]

Oh wow, fascinating, I wouldn't have reason to know about the upheaval. Thank you for explaining what's going on, @garrytrinder. I just crawled back out of the rabbit hole.

At risk of sounding naive, would forking versions depended upon, applying the got patch, and then pointing the dependencies to the fork be a fix?

That is if the code in the patch can be used in the pre-ESM version of got.

Never mind, this would probably create more problems than it solves.

[garrytrinder]

At risk of sounding naive, would forking versions depended upon, applying the got patch, and then pointing the dependencies to the fork be a fix?

This is indeed a possible tactic, however I would say that we would only go down this route if one of our dependencies was not being actively maintained due to the extra overhead that maintaining an external package comes with, that is not the case in this scenario.

I have assigned this issue to myself and will use this as the first candidate ESM to integrate into our codebase incrementally, the alternative of keeping these old versions or refactoring the whole codebase to ESM are not desirable or provide great value.

Again, thank you for raising this issue, we regularly update our dependencies on a monthly cadence to ensure that we are as up to date as we can be, unfortunately some updates are more involved than others.

[waldekmastykarz]

Before we classify this as a bug, I'd suggest that we investigate the impact of the warning and if it's relevant to how CLI is being used. npm regularly reports issues with different packages, but not all of them are relevant in all cases, so understanding how applicable the reported issue is, would help us prioritize this work.

[soyfrien]

Maybe I should close this? I don’t want to track anything improperly or cause too much noise for you or the contributors, and I wouldn’t be able to help out beyond reporting what I encounter using it.

Many thanks

Sent from Proton Mail for iOS

On Mon, Sep 5, 2022 at 1:46 PM, Waldek Mastykarz @.***> wrote:

Before we classify this as a bug, I'd suggest that we investigate the impact of the warning and if it's relevant to how CLI is being used. npm regularly reports issues with different packages, but not all of them are relevant in all cases, so understanding how applicable the reported issue is, would help us prioritize this work.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[waldekmastykarz]

@soyfrien, not a problem at all. You're raising a valid concern that we should investigate and act accordingly. We appreciate your help 😊

[waldekmastykarz]

Hey @garrytrinder, are you still working on this?

[garrytrinder]

I've not been able to progress this, apologies, it slipped off my radar. I've un-assigned the issue for someone else to pick this up who may have the time to investigate.

[waldekmastykarz]

Based on detailed information of the issue in this article, since we're not exposing the URL that's passed to got, plus the user is running CLI on their own machine, I don't think that this issue is relevant to us.