libup downgrades package-lock.json from lockfileVersion:2 to lockfileVersion:1 which seems related to this new version.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/BlockInactive/+/740295

This is becoming moderately irritating as LibUp edit-wars with developers on which version of npm to use. :-( All of CI now uses npm7.

Ack, sorry, I'll prioritize some time for it this weekend.

Compare the old npm v6 audit output:

"findings": [
  {
    "version": "5.0.15",
    "paths": [
      "angular2-highcharts>highcharts"
    ]
  },
  {
    "version": "8.2.2",
    "paths": [
      "highcharts"
    ]
  }
],
"metadata": null,
"vulnerable_versions": "<9.0.0",
"module_name": "highcharts",
"severity": "high",
"github_advisory_id": "GHSA-8j65-4pcq-xq95",
"cves": [
  "CVE-2021-29489"
],
"access": "public",
"patched_versions": ">=9.0.0",
"updated": "2021-05-06T15:44:24.000Z",
"recommendation": "Upgrade to version 9.0.0 or later",
"cwe": "CWE-79",
"found_by": null,
"deleted": null,
"id": 1002707,
"references": "- https://github.com/highcharts/highcharts/security/advisories/GHSA-8j65-4pcq-xq95\n- https://nvd.nist.gov/vuln/detail/CVE-2021-29489\n- https://github.com/advisories/GHSA-8j65-4pcq-xq95",
"created": "2021-10-07T07:31:50.547Z",
"reported_by": null,
"title": "Options structure open to XSS if passed unfiltered",
"npm_advisory_id": null,
"overview": "### Impact\nIn Highcharts versions 8 and earlier, the chart options structure was not systematically filtered for XSS vectors. The potential impact was that content from untrusted sources could execute code in the end user's browser. Especially when using the `useHTML` flag, HTML string options would be inserted unfiltered directly into the DOM. When `useHTML` was false, malicious code could be inserted by using various character replacement tricks or malformed HTML.\n\nIf your chart configuration comes from a trusted source like a static setup or pre-filtered HTML (or no markup at all in the configuration), you are not impacted.\n\n### Patches\nIn version 9, the whole rendering layer was refactored to use an DOMParser, an AST and tag and HTML allow-listing to make sure only safe content entered the DOM. In addition, prototype pollution was stopped.\n\n### Workarounds\nImplementers who are not able to upgrade may apply [DOMPurify](https://github.com/cure53/DOMPurify) recursively [to the options structure](https://jsfiddle.net/highcharts/zd3wcm5L/) to filter out malicious markup.\n\n### References\n* Details on the improved [Highcharts security](https://www.highcharts.com/docs/chart-concepts/security)\n* [The AST and TextBuilder refactoring](https://github.com/highcharts/highcharts/pull/14913)\n* [The fix for prototype pollution](https://github.com/highcharts/highcharts/pull/14884)\n\n### For more information\nIf you have any questions or comments about this advisory:\n* Visit our [support page](https://www.highcharts.com/blog/support/)\n* For more Email us at [security@highcharts.com](mailto:security@highcharts.com)\n",
"url": "https://github.com/advisories/GHSA-8j65-4pcq-xq95"

And the new, npm v7 output:

"source": 1006899,
"name": "node-fetch",
"dependency": "node-fetch",
"title": "node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor",
"url": "https://github.com/advisories/GHSA-r683-j2x4-v87g",
"severity": "high",
"range": "<2.6.7"

So we probably have to combine this with hitting https://docs.github.com/en/graphql/reference/objects#securityadvisory or something.

Change 758105 had a related patch set uploaded (by Legoktm; author: Legoktm):

[labs/libraryupgrader@master] [WIP] Switch runner to bullseye, Python 3.9 and npm 7

https://gerrit.wikimedia.org/r/758105

Change 758105 merged by jenkins-bot:

[labs/libraryupgrader@master] Switch runner to bullseye, Python 3.9 and npm 7

https://gerrit.wikimedia.org/r/758105

First patch so far: https://gerrit.wikimedia.org/r/758287 / https://libraryupgrader2.wmcloud.org/logs2/568179

It takes like 12 minutes for libup to switch from lockfileVersion1 to lockfileVersion 2. Idk why it takes so long, on my laptop it's like 10 seconds.

As far as I can tell, there's no way to actually tell what packages npm audit fix upgraded besides diffing the lock files or something. I think that's actually doable but a bit more involved than what I want to do right now.

In my testing, npm audit fix is unable to upgrade dependencies that are pinned in package.json, like it used to. To reproduce, checkout AbuseFilter at f7cc2bf0c4a602d205149ea6f167739de8cb4365, run npm audit fix --only=dev, observe that only package-lock.json is touched, not package.json. In the JSON output you see:

"@wdio/mocha-framework": {
  "name": "@wdio/mocha-framework",
  "severity": "moderate",
  "via": [
    "mocha"
  ],
  "effects": [],
  "range": "5.4.0 - 6.1.17",
  "nodes": [
    "node_modules/@wdio/mocha-framework"
  ],
  "fixAvailable": {
    "name": "@wdio/mocha-framework",
    "version": "6.11.0",
    "isSemVerMajor": false
  }
},

which suggests to me that it should've just updated package.json to point to 6.11.0 instead of 6.1.8, like it did in npm v6. Only once I change package.json to ^6.1.8 and run npm i and then npm audit fix --only=dev does it actually upgrade @wdio/mocha-framework (and its deps).

So either we have LibUp do these upgrades manually (should be straightforward, but for tomorrow), or we stop pinning in package.json and rely on the package-lock.json pins instead...which...is broken in npm v7: https://github.com/npm/cli/issues/2701. I understand why we have to move to npm v7 because of nodejs and everyone else moving, but this all is a huge functional regression :|

In T273810#7662833, @Legoktm wrote:

In my testing, npm audit fix is unable to upgrade dependencies that are pinned in package.json, like it used to. To reproduce, checkout AbuseFilter at f7cc2bf0c4a602d205149ea6f167739de8cb4365, run npm audit fix --only=dev, observe that only package-lock.json is touched, not package.json. In the JSON output you see:

"@wdio/mocha-framework": {
  "name": "@wdio/mocha-framework",
  "severity": "moderate",
  "via": [
    "mocha"
  ],
  "effects": [],
  "range": "5.4.0 - 6.1.17",
  "nodes": [
    "node_modules/@wdio/mocha-framework"
  ],
  "fixAvailable": {
    "name": "@wdio/mocha-framework",
    "version": "6.11.0",
    "isSemVerMajor": false
  }
},

which suggests to me that it should've just updated package.json to point to 6.11.0 instead of 6.1.8, like it did in npm v6. Only once I change package.json to ^6.1.8 and run npm i and then npm audit fix --only=dev does it actually upgrade @wdio/mocha-framework (and its deps).

So either we have LibUp do these upgrades manually (should be straightforward, but for tomorrow), or we stop pinning in package.json and rely on the package-lock.json pins instead...which...is broken in npm v7: https://github.com/npm/cli/issues/2701. I understand why we have to move to npm v7 because of nodejs and everyone else moving, but this all is a huge functional regression :|

If you switch to npm audit fix --only-dev --force it will upgrade to 6.11.0 (well, actually to ^6.11.0 which isn't ideal).

The amount of things enabled by --force per https://docs.npmjs.com/cli/v7/commands/npm-audit#force seems too much to do without human review.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/758596 looks...good enough!

npm:
* @wdio/mocha-framework: 6.1.8 → 6.11.0
  * https://github.com/advisories/GHSA-93q8-gq69-wqmw
* nanoid: None → 3.1.20
  * https://github.com/advisories/GHSA-qrpm-p2h7-hrv2
* sugarss: 2.0.0 → 2.0.0
  * https://github.com/advisories/GHSA-566m-qj78-rww5

The weirdness in the nanoid one is because the new fixed version has multiple versions of nanoid installed at different level of nested paths.

sugarss is because (AFAICT) npm audit is lying about fixAvailable: true when it isn't. We can either ignore the dependency if both versions are the same or keep it in. I would rather err on the side of including dependencies that may have not been upgraded rather than accidentally miss one that was.

The real solution is the lockfile differ as I mentioned earlier, but I don't think that should block this.

I'll run the canaries for all branches, spot check and then wait a few days for people to review before turning libup on again.

In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UploadWizard/+/758721 it seems like npm is changing the top-level "name" field to be the name of the current working directory :(((

In T273810#7666367, @Legoktm wrote:

In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UploadWizard/+/758721 it seems like npm is changing the top-level "name" field to be the name of the current working directory :(((

Yeah, I just spotted this; in "normal" repos this isn't a problem as it'd read from the package.json file, but we have a practice of not setting the name to make it clear the repo is not an npm-publishable thing.

Our options are either to leave it be (so all of these will switch over to 'repo') or shift our practices and set the name of the package in package.json explicitly (possibly via LibUp). Thoughts?

I think the problem with not setting "name" is that it totally varies on what you clone the repo as...LibUp uses "repo" for a predictable path name, but someone modifying a dependency locally would then change it back to "AbuseFilter" or whatever they have it checked out as, causing unnecessary dirty diffs.

So for extensions+skins, LibUp should probably just set it as basename($repo) in package.json. We already have LibUp enforcing private: true for extensions+skins, so those aren't publishable anyways.

Change 759009 had a related patch set uploaded (by Legoktm; author: Legoktm):

[labs/libraryupgrader@master] Set name in package.json for extensions and skins

https://gerrit.wikimedia.org/r/759009

Change 759009 merged by jenkins-bot:

[labs/libraryupgrader@master] Set name in package.json for extensions and skins

https://gerrit.wikimedia.org/r/759009

Well the "name" logic works, but still a bit buggy: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/759011, doesn't actually have an upgrade.

In T273810#7670031, @Legoktm wrote:

Well the "name" logic works, but still a bit buggy: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/759011, doesn't actually have an upgrade.

I don't get it, it says that there's something to fix:

"sugarss": {
  "name": "sugarss",
  "severity": "moderate",
  "via": [
    "postcss"
  ],
  "effects": [],
  "range": "<=2.0.0",
  "nodes": [
    "node_modules/sugarss"
  ],
  "fixAvailable": true
}

But postcss is:

"postcss": {
  "name": "postcss",
  "severity": "moderate",
  "via": [
    {
      "source": 1006846,
      "name": "postcss",
      "dependency": "postcss",
      "title": "Regular Expression Denial of Service in postcss",
      "url": "https://github.com/advisories/GHSA-566m-qj78-rww5",
      "severity": "moderate",
      "range": "<8.2.13"
    }
  ],
  "effects": [
    "autoprefixer",
    "postcss-less",
    "postcss-safe-parser",
    "postcss-sass",
    "postcss-scss",
    "stylelint",
    "sugarss"
  ],
  "range": "<8.2.13",
  "nodes": [
    "node_modules/postcss"
  ],
  "fixAvailable": false
},

Maybe we just ignore fixAvailable: true and look it up the "via" tree?

Even with a fix for that, the detection is still off. In https://libraryupgrader2.wmcloud.org/logs2/568236 there are supposedly two upgrades:

* meow: 3.7.0 → 3.7.0
  * https://github.com/advisories/GHSA-7p7h-4mm5-852v
* trim-newlines: 1.0.0 → 1.0.0
  * https://github.com/advisories/GHSA-7p7h-4mm5-852v

except npm audit fix doesn't have them. Honestly I have no idea how to interpret the output any further. I found https://uko.codes/dealing-with-npm-v7-audit-changes and agree with the same conclusion the author came to:

In the end, I'm left with a simple question: why? Why, oh, why did you do this, NPM developers? The previous model was much more elaborate. The real-life entities were clearly modeled as dedicated advisories or actions. There was no type-checking upon data consumption. And ultimately, the number of boxes from the output matched with the number of vulnerabilities in summary.

I tried reading the npm source code and came up with nothing. I'm pretty done with this, libup supports npm v7 as much as reasonably possible, the commit message might be wrong, and it might push patches that don't actually upgrade anything, but we already have those bugs, they'll just be different now.

Ran updates for all the "libraries", most look good, in https://gerrit.wikimedia.org/r/c/mediawiki/services/texvcjs/+/759871 the mocha update didn't make it into the commit message.

There are some problems related to eslint plugins not getting found: https://libraryupgrader2.wmcloud.org/logs2/568447

And then there are problems with dependency trees...I think these might just need to be fixed manually: https://libraryupgrader2.wmcloud.org/logs2/568445

In T273810#7686219, @Legoktm wrote:

Ran updates for all the "libraries", most look good, in https://gerrit.wikimedia.org/r/c/mediawiki/services/texvcjs/+/759871 the mocha update didn't make it into the commit message.

Fixed in https://gerrit.wikimedia.org/r/759971.

Queued up the first 75 repositories, which hits all the A extensions and a few Bs.

In T273810#7662564, @Legoktm wrote:

It takes like 12 minutes for libup to switch from lockfileVersion1 to lockfileVersion 2. Idk why it takes so long, on my laptop it's like 10 seconds.

I figured this out, LibUp has accumulated a npm cache directory that was 30GB!! I deleted it entirely and now LibUp takes a normal amount of time to do the conversion.

I queued the first 300 repos, which gets us to the Es.

For some reason in https://libraryupgrader2.wmcloud.org/logs2/568855, npm update eslint doesn't update it, it uninstalls it. Dropped that step for now, hopefully it doesn't bite us later.

Another bogus patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/761517