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

chore: fix npm audit woes #438

Closed
wants to merge 1 commit into from
Closed

Conversation

akx
Copy link
Contributor

@akx akx commented Mar 10, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Azure Pipelines' audit step has been broken for a while.

This PR relaxes the security step:

  • only check production dependencies
  • don't have low-level vulnerabilities break the build
  • output parseable format to avoid huge ANSI art log

Breaking Changes

None.

Additional Info

@anikethsaha
Copy link

anikethsaha commented Mar 10, 2020

Can you confirm whether its similar to this one or not ?

#414

@akx
Copy link
Contributor Author

akx commented Mar 10, 2020

@anikethsaha Not exactly. The main upgrade here is jest... however it looks like Jest 25.x requires native async support, which breaks builds on Node 6.

There are a couple of options I see here (cc @evilebottnawi):
  • Drop support/testing for Node 6.
    • It has been EOL since April 2019 or so, so I think we have no reason to support it anymore either.
    • Current versions of the plugin would continue to work, and future versions might work.
  • Change or remove the CI npm audit step. Maybe --audit-level=moderate?

I reworked this PR to only relax the audit step a little. All of the vulnerabilities we're seeing now are low-level, and via the Jest dependency.

* only check production dependencies
* don't have low-level vulnerabilities break the build
* output parseable format to avoid huge ANSI art log
@akx
Copy link
Contributor Author

akx commented Mar 10, 2020

Should be fixed now...

Turns out audit --fix has an --only=prod switch and audit without --fix has a --production switch. 🙄

@@ -21,7 +21,7 @@
"prebuild": "npm run clean",
"build": "cross-env NODE_ENV=production babel src -d dist --copy-files",
"commitlint": "commitlint --from=master",
"security": "npm audit",
"security": "npm audit --audit-level=moderate --parseable --production",
Copy link
Member

Choose a reason for hiding this comment

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

Remove all flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain? That'll make this patch a full no-op and audit will be broken still.

Copy link
Member

Choose a reason for hiding this comment

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

Acorn release new version with fixed, we don't need it anymore, just update lock file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't seem to help due to how things are pinned in Jest's JSDOM:

$ rm package-lock.json

$ npm i
added 10 packages from 84 contributors, removed 619 packages, updated 55 packages and audited 894519 packages in 18.582s
found 7 moderate severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details

$ npm audit; npm audit --parseable
=== npm audit security report ===

# Run  npm install --save-dev jest@25.1.0  to resolve 7 vulnerabilities
SEMVER WARNING: Recommended action is a potentially breaking change

[... snip ...]

found 7 moderate severity vulnerabilities in 894519 scanned packages
  7 vulnerabilities require semver-major dependency updates.


install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>@jest/reporters>jest-runtime>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>jest-runner>jest-jasmine2>jest-runtime>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>jest-runner>jest-runtime>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>jest-runtime>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>jest-runner>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>@jest/core>jest-config>jest-environment-jsdom>jsdom>acorn	Y
install	acorn	moderate	npm install --save-dev jest@25.1.0	Regular Expression Denial of Service	https://npmjs.com/advisories/1488	jest>jest-cli>jest-config>jest-environment-jsdom>jsdom>acorn	Y

$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So @evilebottnawi, what should we do?

Copy link
Member

Choose a reason for hiding this comment

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

Ignore it, we can't do something here

@akx akx mentioned this pull request Mar 10, 2020
6 tasks
@alexander-akait
Copy link
Member

Done in master, thanks for the PR

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

Successfully merging this pull request may close these issues.

3 participants