Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't work with npm@7 and above [work almost finished, see @next tag on npm] #34

Closed
Tracked by #60
mgdodge opened this issue Feb 9, 2021 · 75 comments
Closed
Tracked by #60
Milestone

Comments

@mgdodge
Copy link

mgdodge commented Feb 9, 2021

Using nvm to run node 14, but update npm to latest version (v 7.5.3). Project already has audit-resolve.json file. Running check-audit results in the following:

TypeError: Cannot read property 'length' of undefined
    at /home/********/node_modules/npm-audit-resolver/check.js:22:45
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
npm ERR! code 2
npm ERR! path /home/********
npm ERR! command failed
npm ERR! command sh -c check-audit

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/********/.npm/_logs/2021-02-09T00_48_37_076Z-debug.log

That log file contains the following:

0 verbose cli   '/home/********/.nvm/versions/node/v14.15.3/bin/node',
0 verbose cli   '/home/********/.nvm/versions/node/v14.15.3/bin/npm',
0 verbose cli   'run',
0 verbose cli   'check-audit'
0 verbose cli ]
1 info using [email protected]
2 info using [email protected]
3 timing config:load:defaults Completed in 1ms
4 timing config:load:file:/home/********/.nvm/versions/node/v14.15.3/lib/node_modules/npm/npmrc Completed in 1ms
5 timing config:load:builtin Completed in 1ms
6 timing config:load:cli Completed in 2ms
7 timing config:load:env Completed in 0ms
8 timing config:load:file:/home/********/.npmrc Completed in 0ms
9 timing config:load:project Completed in 1ms
10 timing config:load:file:/home/********/.npmrc Completed in 1ms
11 timing config:load:user Completed in 1ms
12 timing config:load:file:/home/********/.nvm/versions/node/v14.15.3/etc/npmrc Completed in 0ms
13 timing config:load:global Completed in 0ms
14 timing config:load:cafile Completed in 1ms
15 timing config:load:validate Completed in 0ms
16 timing config:load:setUserAgent Completed in 0ms
17 timing config:load:setEnvs Completed in 1ms
18 timing config:load Completed in 8ms
19 verbose npm-session 298dde723420661e
20 timing npm:load Completed in 17ms
21 timing command:run-script Completed in 2324ms
22 verbose stack Error: command failed
22 verbose stack     at ChildProcess.<anonymous> (/home/********/.nvm/versions/node/v14.15.3/lib/node_modules/npm/node_modules/@npmcli/promise-spawn/index.js:64:27)
22 verbose stack     at ChildProcess.emit (events.js:315:20)
22 verbose stack     at maybeClose (internal/child_process.js:1048:16)
22 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:288:5)
23 verbose pkgid ********@0.1.0
24 verbose cwd /home/********
25 verbose Linux 4.19.104-microsoft-standard
26 verbose argv "/home/********/.nvm/versions/node/v14.15.3/bin/node" "/home/********/.nvm/versions/node/v14.15.3/bin/npm" "run" "check-audit"
27 verbose node v14.15.3
28 verbose npm  v7.5.3
29 error code 2
30 error path /home/********
31 error command failed
32 error command sh -c check-audit
33 verbose exit 2

Have a variety of projects, some using check-audit in commit hooks like this, others using it in CI build hooks - really causing a lot of issues!

@naugtur
Copy link
Owner

naugtur commented Feb 9, 2021

Great to hear you're using npm-audit-resolver.
I've built it in my hobby time and am working to get it built into nom and other package managers. It's a slow moving initiative tho.

About npm7 - looks like the format of audit json changed.

@mgdodge
Copy link
Author

mgdodge commented Feb 9, 2021 via email

@naugtur
Copy link
Owner

naugtur commented Apr 8, 2021

Thanks for the link.

The output from npm7 makes some core audit-resolver features impossible.

Resolver is taking care of the dependency tree paths in which things get installed so that it reports the vulnerability as not-ignored if it finds it in a different dependency path (a new package starts using the ignored thing so maybe you no longer want to ignore it).
npm7 won't let me do that.
Gonna have to talk to npm folks...

@mgdodge
Copy link
Author

mgdodge commented Apr 8, 2021

Frustrating. Guess I'll be sticking to npm 6 for now. This utility has been invaluable to a number of projects I have been working on, so I'm not migrating them to npm 7 until I can ensure a similar audit experience.

@mgdodge
Copy link
Author

mgdodge commented Apr 8, 2021

Is it worth doing a patch-level release to update the README and tell people it won't work properly with npm 7?

@naugtur
Copy link
Owner

naugtur commented Apr 9, 2021

yup, I'll do that.

I'm also considering getting a limited version out without the path protection, but wanted to talk to npm folks first.

@naugtur
Copy link
Owner

naugtur commented Apr 9, 2021

I might have a way.
@mgdodge or anynone else - would you be willing to share a result of npm audit --json on your repositories for npm6 and npm7 for me to explore?

@mgdodge
Copy link
Author

mgdodge commented Apr 10, 2021

I'll see if I can find a project that can easily be converted to a minimal example, then create a sample repo.

@naugtur
Copy link
Owner

naugtur commented Apr 12, 2021

@mgdodge All I need is the npm audit --json output from npm6 and npm7 on your project, so if you feel comfortable sharing with me (doesn't have to be public) the list of your vulns at a point in time, It'd be enough.

@mgdodge
Copy link
Author

mgdodge commented Apr 12, 2021

Here is a gist with two example npm audit --json outputs:

https://gist.github.com/mgdodge/9d0601e76198af918f2542a5503c9f9e

The repo this is based off of is actually for a vue bug I reported a few months ago - https://github.com/mgdodge/rollup-plugin-vue-treeshake-bug-vue3. If you want/need a full checkout of the project to try running npm ci, npm update etc. against, these are from the vue2 branch of that repo.

@naugtur
Copy link
Owner

naugtur commented Apr 12, 2021

Thanks.
MY issue with output from npm7 is evident in your example. Look at y18n - in npm6 output you can find paths where the package is at. If you decide none of these affect you, you can ignore it and audit-resolver will NOT ignore it if it shows up in a new location in dependency tree.
Output from npm7 doesn't have that information, so ignoring it once will ignore it even if the vulnerable dependency gets pulled by another package that might actually call it from your production code.

Protecting against this was the main reason why audit-resolver was created. Ignoring was possible in much simpler ways.

@jakawell
Copy link

jakawell commented Apr 13, 2021

Could that information be derived from the package-lock.json file? I know that would be a lot more complex...

@naugtur
Copy link
Owner

naugtur commented Apr 13, 2021

@jakawell yes, that's the only option left, but correlating it with the right info from audit and the resolve.json file is going to be a lot of work.

@mgdodge
Copy link
Author

mgdodge commented Apr 13, 2021

Not to rain on the parade, but the package-lock.json file also underwent changes with npm7. I don't know the details, but this blog post as well as this one might provide some insight. So, not only will there be a need to support npm6 and npm7 audit json output, but possibly package-lock.json output as well. 🤷‍♂️

(Edit:) Looks like the file is backwards compatible, so that's good news. And the first linked post seems to indicate the new info maps packages "relative locations" which may or may not help in the audit process.

@joebowbeer
Copy link
Contributor

@mgdodge to be optimistic, I hope the new v7 format and hidden file make it easier for the audit resolver to work.

@naugtur what are your expectations regarding integration of the audit resolver with npm audit? Is there a statement of intent from npm or anything on the roadmap?

@azerella
Copy link

azerella commented May 8, 2021

Bumping for an update on this issue, I'm also experiencing it with npm 7

@naugtur
Copy link
Owner

naugtur commented May 8, 2021

No need for confirmation. I'll work with npm folks to see if they could expose more info from new audit, if not, I'd have to read it from the package lock. It'll take a week or two to figure it out and then the implementation.

@naugtur
Copy link
Owner

naugtur commented May 11, 2021

Update - I think I now know how to recover the missing information by re-indexing the current format. I'll give it a try.
Anyone willing to also share their npm7 audit --json results?

@mgdodge
Copy link
Author

mgdodge commented May 11, 2021

How large do you need? I can look today for a few sizes of output. With the recent lodash and hosted-git-info advisories being as widespread as they are, some audits got big real quick...

@naugtur
Copy link
Owner

naugtur commented May 11, 2021

@mgdodge well, the only limitation is what you feel comfortable sharing. It's a list of vulnerabilities in an app afterall. Send whatever, 3 examples I didn't artificially create is better than 1 :)

@bytestream
Copy link

@naugtur would adding npm@6 to this repository's package.json temporarily resolve this (until npm 7 is supported)?

I've ran into this after updating @semantic-release/npm which now requires npm@7. It breaks check-audit even though I have npm 6.14.9 installed

@naugtur
Copy link
Owner

naugtur commented May 13, 2021

Yes, installing npm 6 helps.
I first saw this issue because of semantic release. I ended up putting semanticrelease in dev dependencies and running audit from global install or npx while installing with --production

@naugtur
Copy link
Owner

naugtur commented May 27, 2021

Good news @here

I've published an early version with npm7 support. Individual package fixing is not implemented, but otherwise it should work.

Check the v3 prerelease by installing npm i npm-audit-resolver@next or npm i [email protected]

@naugtur naugtur added this to the target-v3 milestone May 27, 2021
@jamesots
Copy link

Looks like it's working for us. I tested it with npm 6 and 7 and it appears to work. I thought it was finding extra vulnerabilities in npm@7, but it turns out our scoped packages in our private repo were being reported as 00unidentified in 6, but with their actual package name in 7, so that's good!

@naugtur
Copy link
Owner

naugtur commented May 28, 2021

Yes, npm decided to stop scrambling private packages. But thanks for cross-checking. I don't have capacity to do solid testing.

Let me know whatever you notice!

@jamesots
Copy link

jamesots commented Jun 2, 2021

Because it's using a git url for audit-resolve-core, I'm unable to get the prerelease to work in Azure pipelines, as I get this error when it runs npm ci:

npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t ssh://[email protected]/naugtur/audit-resolve-core.git
npm ERR! 
npm ERR! Warning: Permanently added the RSA host key for IP address '140.82.121.4' to the list of known hosts.
npm ERR! [email protected]: Permission denied (publickey).
npm ERR! fatal: Could not read from remote repository.
npm ERR! 
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR! 
npm ERR! exited with error code: 128

mdebarros added a commit to mojaloop/central-services-metrics that referenced this issue May 18, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results and IGNORE patterns
- added useUnknownInCatchVariables config to tsconfig to support current code style
- updated README with standard auto-release information

Notes: 
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mdebarros/central-services-shared that referenced this issue May 18, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results patterns
- updated README with standard auto-release information

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.
- Added "@hapi/catbox", "@hapi/catbox-memory" to ncurc for dep:check to ignore updates due to breaking changes which should be handled by another story

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mojaloop/event-sdk that referenced this issue May 18, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results and IGNORE patterns
- added useUnknownInCatchVariables config to tsconfig to support current code style
- updated README with standard auto-release information

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mojaloop/event-sdk that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results and IGNORE patterns
- added useUnknownInCatchVariables config to tsconfig to support current code style
- updated README with standard auto-release information
- Fixed imports
- Added 'serialize-error' to ncurc for dep:check to ignore future updates - this is because v9+ only supports ESM loaders and not CJS. This will need to be addressed in a future story.
- Aligned jest config to moja standard
- Cleaned up Package.json
- Added 'prepublishOnly' script to package.json to ensure that dist is build prior to publishing
- Updated tsconfig to latest standards

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.
-  'serialize-error' version is fixed to v8.1.0, this is because v9+ only supports ESM loaders and not CJS. This will need to be addressed in a future story.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mojaloop/central-services-shared that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results patterns
- updated README with standard auto-release information
- resolved audits
- cleaned up package.json
- updated links in changelog to point correctly to mojaloop issue repo
- renamed .ncurc.json to .ncurc.js to add comment-note about the @hapi/catbox* issue as described in the notes below

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.
- Added "@hapi/catbox", "@hapi/catbox-memory" to ncurc for dep:check to ignore updates due to breaking changes which should be handled by another story

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mdebarros/central-services-health that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- removed unused files
- removed unused dependencies
- re-aligned package.json
- added replace dependency
- added IGNORE and test/result patterns to gitignore
- re-aligned lint standards config
- re-aligned tsconfig

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as npm run audit:resolve (i.e. resolve-audit) is currently not functioning. As a work-around, we need to manually run the following command npm audit fix. The npm run audit:check (i.e. check-audit) still works as expected.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mojaloop/central-services-health that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- removed unused files
- removed unused dependencies
- re-aligned package.json
- added replace dependency
- added IGNORE and test/result patterns to gitignore
- re-aligned lint standards config
- re-aligned tsconfig

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as npm run audit:resolve (i.e. resolve-audit) is currently not functioning. As a work-around, we need to manually run the following command npm audit fix. The npm run audit:check (i.e. check-audit) still works as expected.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros pushed a commit to mojaloop/central-services-stream that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- Update ci machines/slack announcements/automated releases
- Bumped dependencies
- Major bump since big dep version leap
- Removed eslint rc/ignore since library uses standard and has no ts types and as such does not need these.

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as npm run audit:resolve (i.e. resolve-audit) is currently not functioning. As a work-around, we need to manually run the following command npm audit fix. The npm run audit:check (i.e. check-audit) still works as expected.
- Primarily had to update `node-rdkafka`>=2.11 to support node 16 which one of the 3ppi services depends on.
- Tape dependency has been added to the ncurc config to ignore dep:checks as v5+ will cause tests to fail. To be addressed by a future story.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mdebarros/ml-api-adapter that referenced this issue May 19, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results and IGNORE patterns
- updated README with standard auto-release information
- Fixed imports
- Cleaned up Package.json
- updated links in CHAGNGELOG to correctly point to project repo

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.

BREAKING CHANGE: major version bump for node v16 LTS support, and re-structuring of project directories to align to core Mojaloop repositories!
mdebarros added a commit to mojaloop/ml-api-adapter that referenced this issue May 23, 2022
feat(mojaloop/#2092): upgrade nodeJS version for core services - mojaloop/project#2092
- standardised CI scripts
- fixed lint issues
- updated .nvmrc to latest LTS version
- added standard CI scripts/config to package.json: release, snapshot, standard-version, etc
- updated gitignore to include test/results and IGNORE patterns
- updated README with standard auto-release information
- Fixed imports
- Cleaned up Package.json
- updated links in CHANGELOG to correctly point to project repo
- Removed "secrets" (i.e. JWS Signing Keys) as we don't want anybody to accidentally use a "test" key. Rather have an error thrown and for people to explicitly configure their own keys. This does mean that I had to add a "dummy" (with no meaningful body) key file that is empty for unit tests to pass, and will have no impact on the docker image of the service (i.e. it will fail if JWS is enabled, and no Key is injected/configured).

Notes:
- npm-audit-resolver v3.0.0-7 is a candidate release to resolve compatibility with npm v7+ as described in naugtur/npm-audit-resolver#34. This will need to be addressed going forward as `npm run audit:resolve` (i.e. `resolve-audit`) is currently not functioning. As a work-around, we need to manually run the following command `npm audit fix`. The `npm run audit:check` (i.e. `check-audit`) still works as expected.
- 'get-port' dependency version is fixed to v5.1.1, this is because v9+ only supports ESM loaders and not CJS. This will need to be addressed in a future story.
- Helm chart mountPaths need to be updated from `/opt/ml-api-adapter` to `/opt/app` as follows:
    ```YAML
        volumeMounts:
        - mountPath: /opt/app/config
          name: <deployment-name>
        - mountPath: /opt/app/secrets <-- only required for the notification service
          name: jws-signing-key
    ```

BREAKING CHANGE: Major version bump for node v16 LTS support, re-structuring of project directories to align to core Mojaloop repositories and docker image now uses `/opt/app` instead of `/opt/ml-api-adapter` which will impact config/secret mounts.
@naugtur
Copy link
Owner

naugtur commented Jun 13, 2022

The issue with IDs should now be behind us, I'm getting ready to finish off v3 and get it published.
Who's excited? :D

@joebowbeer
Copy link
Contributor

Is npm7 still maintained? The last release (npm v7.24.2) was 1 year ago.

Given that Node 16 enters maintenance phase in 60 days, and even it ships with npm v8, is there any need to support npm v7?

@naugtur naugtur changed the title Doesn't work with npm@7 Doesn't work with npm@7 and above [work almost finished, see @next tag on npm] Aug 18, 2022
@naugtur
Copy link
Owner

naugtur commented Aug 18, 2022

Edited the title. @joebowbeer

@mgdodge
Copy link
Author

mgdodge commented Nov 4, 2022

@naugtur , with npm 9 just being released, I'm curious if they've messed with the audit format again. I think historically there's enough data here to verify things, curious if you've got an eye on that or not.

@naugtur
Copy link
Owner

naugtur commented Nov 4, 2022

My eyes were elsewhere mostly. Thanks for bringing it up.
I was hoping to release v3 for Christmas so gotta check what they did 😁

@naugtur
Copy link
Owner

naugtur commented Nov 7, 2022

@mgdodge seems to be working with npm9

@joebowbeer
Copy link
Contributor

I notice that Node 18.14.0 comes bundled with npm v9

nvm use 18.13
Now using node v18.13.0 (npm v8.19.3)
nvm use 18.14
Now using node v18.14.0 (npm v9.3.1)

The npm v9 lockfileVersion is still 2, right?

https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#lockfileversion

@naugtur
Copy link
Owner

naugtur commented Feb 9, 2023

audit resolver doesn't depend on the lockfile format, that's the whole reason behind going through the hassle of calling the cli commands - they're the most stable API surface available.

@joebowbeer
Copy link
Contributor

Can this be closed as completed?

@naugtur
Copy link
Owner

naugtur commented Feb 14, 2023

I think it finally can, with some ugly fixes for npm6 tests...

@naugtur naugtur closed this as completed Feb 14, 2023
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

No branches or pull requests