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

switch to audit-filter from npm package #5061

Merged
merged 4 commits into from
Nov 5, 2018
Merged

switch to audit-filter from npm package #5061

merged 4 commits into from
Nov 5, 2018

Conversation

g-k
Copy link
Contributor

@g-k g-k commented Oct 22, 2018

refs: #4948

NB: CI still fails because we haven't addressed some new advisories (they mostly look like lower priority ReDoSes).

r?

package.json Outdated
@@ -71,6 +72,7 @@
"istanbul-middleware": "0.2.2",
"mocha": "5.2.0",
"node-sass": "4.9.4",
"npm": "^6.4.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm (I believe from npm-run-all) is otherwise on 4.X, which doesn't support npm audit --json

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think the right way to do this would be to set the correct minimum npm version in the engines section, then maybe also add a config to set engine-strict to true? Might also want to bump the minimum node version in engines to the minimum for npm 6.4.1 to work; it's probably higher than 8.0.0, the current minimum.

package.json Outdated
@@ -53,6 +53,7 @@
},
"devDependencies": {
"addons-linter": "1.3.6",
"audit-filter": "^0.3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the exact version, "0.3.0"

package.json Outdated
@@ -71,6 +72,7 @@
"istanbul-middleware": "0.2.2",
"mocha": "5.2.0",
"node-sass": "4.9.4",
"npm": "^6.4.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think the right way to do this would be to set the correct minimum npm version in the engines section, then maybe also add a config to set engine-strict to true? Might also want to bump the minimum node version in engines to the minimum for npm 6.4.1 to work; it's probably higher than 8.0.0, the current minimum.

@g-k g-k force-pushed the audit-filter-from-npm branch 2 times, most recently from 76ee4c7 to 3b24171 Compare October 31, 2018 09:56
@jaredhirsch
Copy link
Member

@g-k Thanks for the update, looks good. I'll deal with the package-lock issues

@jaredhirsch
Copy link
Member

Aha. Looks like the gigantic list of integrity updates from sha512 down to sha1 is a platform difference between linux and mac. Software is hard, I guess npm/npm#17749

@jaredhirsch
Copy link
Member

Aha. Looks like the gigantic list of integrity updates from sha512 down to sha1 is a platform difference between linux and mac. Software is hard, I guess npm/npm#17749

And, in classic "new npm" style, the bug was closed with no resolution

@jaredhirsch
Copy link
Member

Actually...the sha1 and sha512 changes seem totally random, and I'm getting them when regenerating package-lock on my machine. On the latest 6.4.1 version of npm. wtf?

Why do we even bother with package-lock?

@jaredhirsch jaredhirsch force-pushed the audit-filter-from-npm branch from 3b24171 to c54a9de Compare November 1, 2018 16:25
@jaredhirsch
Copy link
Member

Hmm, still seeing package-lock.json conflicts. I guess I'll blow away the entire file and try again for a second time.

@ianb ianb dismissed jaredhirsch’s stale review November 5, 2018 17:13

Addressed in merge

@ianb
Copy link
Contributor

ianb commented Nov 5, 2018

Note test failures are due to a regression in master

@ianb ianb merged commit f05fe43 into master Nov 5, 2018
@ianb ianb deleted the audit-filter-from-npm branch November 5, 2018 17:13
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.

3 participants