Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Put in a package-lock.json file #4617

Merged
merged 5 commits into from
Aug 8, 2018
Merged

Put in a package-lock.json file #4617

merged 5 commits into from
Aug 8, 2018

Conversation

ianb
Copy link
Contributor

@ianb ianb commented Jul 12, 2018

Per Bug 1475246, this is to keep our transitive dependencies locked

@chenba
Copy link
Collaborator

chenba commented Jul 12, 2018

CI is failing because NSP is reporting quite a number of new vulnerabilities.

@ianb
Copy link
Contributor Author

ianb commented Jul 12, 2018

Why would it be reporting new vulnerabilities? This should be all the packages we normally install. Does nsp look at package-lock for what it should check? (I didn't think it did, I recall getting errors for packages that we didn't directly require)

@jaredhirsch
Copy link
Member

This doesn't make any sense. I tried manually removing the line from the .npmrc file, re-running npm install locally, then doing ./node_modules/.bin/nsp check -o summary, with no vulnerabilities found.

@chenba
Copy link
Collaborator

chenba commented Jul 12, 2018

nsp looks at package-lock.json if that exists (in addition to package.json). And there's no option to switch that off. :-/

@jaredhirsch
Copy link
Member

I’m generating the package-lock locally (via npm install after removing the line from the .nsprc) and not seeing this error. Could someone else try?

@punamdahiya
Copy link
Contributor

Trying ./node_modules/.bin/nsp check -o summary

Report 2 vulnerabilities for me
base64url 2.0.0 >=3.0.0 firefox-screenshots@33.0.0 > jpm@1.3.1 > sign-addon@0.2.0 > jsonwebtoken@7.1.9 > jws@3.1.4 > jwa@1.1.5 > base64url@2.0.0 https://nodesecurity.io/advisories/658
base64url 2.0.0 >=3.0.0 firefox-screenshots@33.0.0 > jpm@1.3.1 > sign-addon@0.2.0 > jsonwebtoken@7.1.9 > jws@3.1.4 > base64url@2.0.0 https://nodesecurity.io/advisories/658

b/w I have npm 5.5.1 and already had .npmrc file with package-locak=false option

@chenba
Copy link
Collaborator

chenba commented Jul 12, 2018

Argh, I tried it again. First I got 13 vulnerabilities. (15 was reported in the CircleCI run for those keeping scores.) Then (+) No known vulnerabilities found consistently after. 😕

I'll run the tests again on CircleCI.

@jaredhirsch
Copy link
Member

@punamdahiya Try removing that line from your .npmrc, then re-running npm install to generate a package-lock file, and see if the reported vulnerabilities change

@punamdahiya
Copy link
Contributor

@6a68 tried deleting the entry in .npmrc and regenerating package-lock.json. It still complains about the above 2 vulnerabilities because of base64URL version 2.0.0

@chenba
Copy link
Collaborator

chenba commented Jul 25, 2018

Not only I get a different number of vulnerabilities locally than CircleCI, but even the output format is different this time.

@ianb ianb force-pushed the put-in-package-log branch 2 times, most recently from 8c7e8ff to e0f0ff3 Compare July 30, 2018 17:36
@jaredhirsch
Copy link
Member

Couple things:

  • Interestingly, specifying npm 6 in the engines part of package.json does not cause any warnings or errors if I try to install on npm 5.6.0 :-\

  • It looks like the tests are failing because they are trying to connect to the dev server's usual endpoint, localhost at port 10080, not the server-unavailable version that pings localhost port 49152.

@jaredhirsch
Copy link
Member

Other than those things, this works OK for me (with a gigantic list of npm audit errors that we ignore, for now).

@chenba
Copy link
Collaborator

chenba commented Aug 1, 2018

Are the changes to ajv, addons-linter, and svgo in package.json intentional? Otherwise it looks ready to be merged.

@chenba
Copy link
Collaborator

chenba commented Aug 1, 2018

Oh those are the results of npm audit fix. 👍

@chenba
Copy link
Collaborator

chenba commented Aug 3, 2018

Is this ready to be merged?

This doesn't change packages that require npm audit fix --force
nsp is deprecated. npm audit replaces it, but doesn't have any ability to ignore issues that we want to ignore. For now we're using npm audit || true to keep npm audit from making the build fail
@ianb
Copy link
Contributor Author

ianb commented Aug 8, 2018

I feel unclear why this does or doesn't pass checks now, but let's say it's ready to merge and go from there!

@ianb ianb merged commit 099641f into master Aug 8, 2018
@ianb ianb deleted the put-in-package-log branch August 8, 2018 17:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants