naugtur / npm-audit-resolver

Apache License 2.0
121 stars 28 forks source link

fix did not fix #12

Closed joebowbeer closed 2 years ago

joebowbeer commented 5 years ago

When I was trying to fix the recent js-yaml issue, the npm update invoked by npm-audit-resolver claims to have succeeded but I had to run npm audit fix to actually fix the issue.

Steps:

I installed npm-audit-resolver as a devDependency and added a audit-resolver script to package.json that runs resolve-audit.

npm run audit-resolver
> resolve-audit

>>>> npm audit --json 
>>>> exit: 1
Total of 2 actions to process

--------------------------------------------------
 js-yaml needs your attention.

[ moderate ] Denial of Service
 vulnerable versions <3.13.0 found in:
 -  devDependencies: tslint>js-yaml
_
 f) fix with npm update js-yaml --depth 2
 d) show more details and ask me again
 r) remind me in 24h
 i) ignore paths
 del) Remove all listed dependency paths
 s) Skip this
 q) Quit
What would you like to do? f
Fixing!
>>>> npm update js-yaml --depth 2
>>>> exit: 0

--------------------------------------------------
 js-yaml needs your attention.

[ moderate ] Denial of Service
 vulnerable versions <3.13.0 found in:
 -  devDependencies: mocha>js-yaml
_
 ?) investigate
 d) show more details and ask me again
 r) remind me in 24h
 i) ignore paths
 del) Remove all listed dependency paths
 s) Skip this
 q) Quit
What would you like to do? i
done.

However, npm audit (and check-audit) still complains about the first problem and npm audit fix is needed to fix it.

npm audit
# Run  npm update js-yaml --depth 2  to resolve 1 vulnerability
found 2 moderate severity vulnerabilities in 1945 scanned packages
  run `npm audit fix` to fix 1 of them.
  1 vulnerability requires manual review. See the full report for details.

Fixing:

npm audit fix
added 1 package from 4 contributors and updated 1 package in 2.497s
fixed 1 of 2 vulnerabilities in 1945 scanned packages
  1 vulnerability required manual review and could not be updated

Verifying:

npm run audit-checker
> check-audit

>>>> npm audit --json 
>>>> exit: 1
Total of 1 actions to process
skipping js-yaml issue based on audit-resolv.json
audit ok.
naugtur commented 5 years ago

Thanks for a very thorough report. If you could also provide the version number of npm-audit-resolver you used I should be able to reproduce.

joebowbeer commented 5 years ago

npm-audit-resolver@1.5.0

naugtur commented 5 years ago

I don't see how it could have happened yet. Could you share your package-lock from before and after the working fix?

joebowbeer commented 5 years ago

It is not easy to return to this state but if I encounter something similar I'll try to isolate a repro.

My workaround now is to run audit fix first to let it do what it can before running resolve-audit.

naugtur commented 5 years ago

Ok, no worries.

I was hoping you had package lock file committed to git so the broken version would be easy to find in history.

naugtur commented 5 years ago

I just reproduced it in one of my repos. Note that if you run npm audit and then take the command it prints as a fix for the single issue it doesn't fix the issue, but npm audit fix does.

That means if resolver only gets the output of audit -> not much I can do. Let's ask some npm people.

houldsg commented 3 years ago

I just came across this. I think it's related to https://github.com/npm/cli/issues/394

When "fix" runs npm update lodash --depth 2, the audit-resolve.json is created and marks them as fixed but there is no change to the package-lock.json

This is the simplest package-lock.json repro i could make

{
  "name": "x",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "async": {
      "version": "2.6.0",
      "resolved": "https://registry.npmjs.org/async/-/async-2.6.0.tgz",
      "integrity": "sha512-xAfGg1/NTLBBKlHFmnd7PlmUW9KhVQIUuSrYem9xzFUZy13ScvtyGGejaae9iAVRiRq9+Cx7DPFaAAhCpyxyPw==",
      "requires": {
        "lodash": "^4.14.0"
      },
      "dependencies": {
        "lodash": {
          "version": "4.17.20",
          "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",
          "integrity": "sha512-PlhdFcillOINfeV7Ni6oF1TAEayyZBoZ8bcshTHqOYJYlrqzRK5hagpagky5o4HfCzzd1TRkXPMFq6cKk9rGmA=="
        }
      }
    }
  }
}
mcintyre94 commented 3 years ago

I think this must be an npm bug - I can repro the above, but just running npm update lodash --depth 2 with no npm-audit-resolver involved has the same issue.

naugtur commented 3 years ago

npm7 no longer produces individual fix commands.

naugtur commented 2 years ago

Closing this, no more reasonable options to support fixing and marking as fixed.