Topic: discuss npm-audit-resolver needs

[darcyclarke]

• As per @naugtur's comment/response last week I'm creating an issue to ensure we track this item & that the agenda will get updated accordingly

[naugtur]

The main idea behind ignoring issues using audit-resolver is the precision with which it works. The vulnerability is ignored not on the package level, but a specific path in dependency tree and if the same vulnerable package surfaces elsewhere - potentially as a dependency of a dependency where it does get called in production code. In that case the warning needs to come back.

npm7's audit output is not structured the way it used to in npm6, but it contains almost all of the necessary information. I can reindex it to produce paths in dependency tree that audit-resolver uses. The only thing that's missing is deduplication.

Example:
a depends on b which depends on c
c is vulnerable
npm7 lists all a,b and c on the top level and the references in .via let me produce the chains:

a>b>c
b>c
c

But I don't need all of them.
If only a is a direct dependency, I need the a>b>c in audit-resolve file.
If both a and b are installed, I need a>b>c AND b>c.
There's no way to tell which items in npm7 audit output are direct dependencies so I have to run npm ls --depth=0 --json to get that information.

ASK1: An indicatoin of thta fact with a simple boolean or .depth field would do a lot to help.
I thought of using the "nodes" field, but it seems with deduplication they often land at the top.

Current audit doesn't seem to have enough information available in it to construct a valid fix command - install or update, dev dependency or not, depth, that's missing from the audit output in npm7 and audit-resolver doesn't have a command to run to fix an individual issue and record the fact it was fixed.

ASK2: add a .command field to fixAvailable or share/expose the algorithm with which npm audit fix decides what to do.

ASK3: tell me how to check it or add a field explaining if it's a production or dev dependency

[naugtur]

@darcyclarke Sorry I didn't make it for the previous meeting. Can we get it in tomorrow?

[G-Rath]

audit-app has the same problem - for npm 7 we now have to take ignores as <advisory>|<dependency> instead of <advisory>|<path to dependency in tree> which is giving up the whole point of the package (the precision vs having you ignore whole advisories blindly).

iirc I thought there was an issue with the .via chain that meant it didn't actually provide a complete enough chain, but that could be me miss-remembering (thinking about it, I think it was the issue you described @naugtur)

[naugtur]

@G-Rath I'm done implementing the reindexing that lets me go back to using <advisory>|<path to dependency in tree> but to make it work I need to run and process npm ls or get the ASK1 to happen. .via provides a complete chain, but no information about completeness of the chain.
You could reuse my implementation pulling npm-audit-resolver as a dependency when I'm done working on current version.

[G-Rath]

@naugtur I've been working on implementing using npm list into audit-app, which works quite well except I realised today that npm list requires node_modules as it loads the actual tree rather than the virtual one.

Just wanted to let you know in case you didn't already know that, as for me this is currently a pretty big blocker since its not feasible to require a successful npm install.

I've proposed implementing a --virtual flag to have npm list use the virtual tree instead, but I'm not sure how feasible that is (based on the npm test suite, it might fall over in other cases like circular dependencies, but I've not had a chance to properly explore and confirm that; but 🤞 it'll be fine).

Alternatively of course this wouldn't be an issue if the v7 audit results included the version and path information like v6 did 🤷

[naugtur]

Thanks for letting me know. It didn't come up in my testing.

On the most recent npm RFC meeting we reviewed what resolver depends on and I got some pointers that help address my asks.

I found where I want to add it in arborist, but don't know how to figure out it's a top level dependency yet. Anyway, I'll add a boolean field denoting if an item is a direct dependency and that eliminates the need for npm ls.

[G-Rath]

@naugtur if the only information you're missing is if something is a top level dependency, could you not just read the package.json for that?

[naugtur]

I'd need to implement all the npm features re. finding the right package.json location. With npmrc and workspaces etc.

[G-Rath]

Isn't that something you already have to do to call npm audit, since you've got to point it at the package.json (+ it's lock) you wish to audit?

[G-Rath]

btw, I've had a closer re-look into the audit output of v7, and it doesn't seem to be properly mappable to a full and complete set of dependency paths with via & co alone: I've got a project which pulls in ws via jest and webpack-plugin-serve which is reported by npm audit as vulnerable, but the audit output does not include either of those packages - it just has ws as vulnerable with an empty effects array.

IMO this all points to the need to have a formalised format that audit data includes to ensure that this data is just available to us as part of the audit, rather than having to try and piece it back together after the fact.

For me, the critical missing properties are which versions of the vulnerable packages are actually installed (which has to be known by the auditor in order to be able to order in the first place), and the full path to those packages in the dependency tree (which again the auditor should know already).

[naugtur]

What you're describing seems more like a bug than missing feature TBH.

[G-Rath]

I agree 🙂