Switch identifier to be taken from github_advisory_id

[mobilutz]

Background:
Two of our allowlist entries changed their npm-number 4 times in the last 48 hours!!!

This is a BREAKING CHANGE as the identifier that is used to identify advisories is being changed from numerical npm-identifier to string github-identifiers e.g. GHSA-rvg8-pwq2-xj7q

Making this backward compatible is for couple of reasons not good:

1. npm-identifiers are NOT constant
2. npmjs.com does not use the identifiers in the url attribute of the json response
3. mix of numerical and string identifiers would need to be supported (in my opinion)
4. makes this package very complex

NOTE: I bases this change not on IBM:main but quinnturner:node-12 branch so it has some changes not related to the pull-request in it. That was needed to make tests 🟢 which they are now 😄

[quinnturner]

Is there a reason that this must be backwards incompatible? I certainly prefer using the GitHub identifiers, but it would be nice (not required) to have backwards compatibility. Also, I have to refresh my memory for identifiers used for Yarn & Yarn Berry.

[quinnturner]

Note, the tests are failing because of advisory warnings in audit-ci's dependencies. I'll see if I can squash them in main for you to merge into this PR.

[mobilutz]

Is there a reason that this must be backwards incompatible? I certainly prefer using the GitHub identifiers, but it would be nice (not required) to have backwards compatibility. Also, I have to refresh my memory for identifiers used for Yarn & Yarn Berry.

I first wanted to add both possibilities to use npm-numbers AND github-numbers, but since I currently can see that the npm-numbers are being changed many many times I opted against having this.

I am firstly trying to find a solution for my company to use the non-changing github-numbers and then maybe try to find a way to make it backward compatible.

FYI on the npm-number change. Two of our allowlist entries changed the npm-number 4 times in the last 48 hours!!! The GHSA-number stayed constant!!!

[quinnturner]

If you can make the tests pass across Yarn, Yarn Berry, NPM 6, and NPM 7 with only GitHub identifiers, I will accept a non-backwards-compat PR 👍🏻

[mobilutz]

If you can make the tests pass across Yarn, Yarn Berry, NPM 6, and NPM 7 with only GitHub identifiers, I will accept a non-backwards-compat PR 👍🏻

@quinnturner The thing I could not get to work so far is the allowlist format "GHSA-UUID|example5>example4".
I will take a look at this tomorrow.

PS: I am not sure, if I can make NPM 6 work. It looks as all of the NPM requests are mocked and non are really going out, and I locally also don't have NPM 6 anymore to check how the json responses look. And I do not trust mocked responses where I manually change things 😉

[quinnturner]

I can appreciate the mocking concern. I am going to update all of the mocks with the latest live version in my other PR. If it can pass those mocks, I am satisfied!

[mobilutz]

@quinnturner I rebased my changes on your node-12 branch, and now it seems things are green.

Of course it is now harder to really see what my changes are.
Here they are: https://github.com/IBM/audit-ci/pull/217/files/0416a0e04533b199ad4553a1ebf184b57de30bad..c8cd59816ba903c892ed6200a20db7e130d9dbf2

[quinnturner]

I am still working on the base branch. The NPM 7+ algorithm has to be revised, which is taking some time since the resolution of dependencies is complicated in NPM 7+. I hope to have a solution by today or tomorrow.

[Stephen2]

Wanted to say thank you for everyone taking this on. 2nd time in a couple of days our vulns have been renumbered, and we take a hard line "Block builds" until it's resolved, so it's painful all round.

[mobilutz]

Wanted to say thank you for everyone taking this on. 2nd time in a couple of days our vulns have been renumbered, and we take a hard line "Block builds" until it's resolved, so it's painful all round.

@Stephen2 We temporarily switched off our js-auditing.

[adamb-mcr]

@mobilutz we left our js auditing in place in our CI builds so we still get logs but by omitting a severity parameter it's not a blocking step in our pipelines.

[quinnturner]

Hi there, please rebase this PR off of the latest main 👍🏻

I am also in the process of writing a codemod to help migrate users from v5 to v6, including this PR.

[mobilutz]

Hi there, please rebase this PR off of the latest main 👍🏻

@quinnturner done

I am also in the process of writing a codemod to help migrate users from v5 to v6, including this PR.

That sounds very good. If you need support, you can ping me.

PS: I was suprised as how little needed to change in the end after your node-12 upgrade together with the npm-ouputs updates!

[quinnturner]

https://github.com/quinnturner/audit-ci-codemod, working on the CI and publishing to NPM.

It reads your configuration (configurable path), then performs an in-place update:

1. Moving advisories, path-whitelist, and whitelist -> allowlist
2. Looks up existing NPM advisory IDs, resolves them to the associated GitHub advisory ID, and updates all instances of the ID in your config

[mobilutz]

@quinnturner Thanks for you feedback, I think I addressed everything.

[quinnturner]

Perfect, thank you for your efforts! I will release 6.0.0-beta.1, do some last minute checks, then follow up with 6.0.0

[quinnturner]

Released 6.0.0