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

fix(deps): security vulnerability in yargs-parser #2566

Merged
merged 1 commit into from
May 8, 2020
Merged

fix(deps): security vulnerability in yargs-parser #2566

merged 1 commit into from
May 8, 2020

Conversation

Ionaru
Copy link
Contributor

@Ionaru Ionaru commented May 4, 2020

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

No.

Motivation / Use-Case

It fixes #2559

Additional Info

Running npm audit -production for webpack-dev-server complains about mkdirp as well, this should be looked into by the Webpack team.

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #2566 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2566   +/-   ##
=======================================
  Coverage   93.77%   93.77%           
=======================================
  Files          34       34           
  Lines        1333     1333           
  Branches      381      381           
=======================================
  Hits         1250     1250           
  Misses         81       81           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 375ab23...840233f. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @Loonride @hiroppy I think we can update this, because latest webpack-cli uses https://github.com/webpack/webpack-cli/blob/v3.3.11/package.json#L126

Copy link

@nkmdk-007 nkmdk-007 left a comment

Choose a reason for hiding this comment

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

👍

@hiroppy
Copy link
Member

hiroppy commented May 7, 2020

Need to fix CI at first.

@Ionaru
Copy link
Contributor Author

Ionaru commented May 8, 2020

@hiroppy do the CI failures on this PR have anything to do with the change I made? Tests on master fail as well, on my local machine I had 3 tests fail on master before my change and the same amount after my change.

@hiroppy
Copy link
Member

hiroppy commented May 8, 2020

@Ionaru ok, sorry. But we can't merge this PR if CI fails. Currently, we are working on v4 branch to release the new major version. V4 branch's CI is green so if you want to continue to work this PR, please change the targeted branch from master. (also actually, this change is a breaking change.)

https://github.com/webpack/webpack-dev-server/tree/v4

Thanks.

@Ionaru
Copy link
Contributor Author

Ionaru commented May 8, 2020

I don't think it's correct to have locked #2559.
This audit failure impacts many developers, pretty much everyone using Angular, and now it's locked because of "spam"? There's been no unrelated discussion on the issue, locking it prevents people from adding a 👍 as indication of priority or desire for the issue to be resolved.

I think the proper course of action is to either unlock it, or to have a Webpack member add a statement that the issue is being worked on.

@Ionaru
Copy link
Contributor Author

Ionaru commented May 8, 2020

(also actually, this change is a breaking change.)

How is it a breaking change? As far as I'm aware, the public webpack API (or CLI) does not change.

@hiroppy
Copy link
Member

hiroppy commented May 8, 2020

@Ionaru Yes, I know. So we decided to upgrade this package but we are working on v4 branch to release the new major version ASAP. I wanna say would you continue to work on your PR? If you reject this thing, I'll work on it.

@ashfordneil
Copy link

Given that this is a security fix rather than a regular PR, shouldn’t publishing it and making it available to users (angular was mentioned above, create react app also depends on this) be pushed through sooner rather than later.

While I get that normally punting things to the upcoming release makes sense, in this context it feels like you’re refusing to back-port security updates to version 3 before version 4 is even out.

@alexander-akait
Copy link
Member

@Ionaru

This audit failure impacts many developers, pretty much everyone using Angular, and now it's locked because of "spam"?

To avoid messages like: "same here", "please update" and etc

While I get that normally punting things to the upcoming release makes sense, in this context it feels like you’re refusing to back-port security updates to version 3 before version 4 is even out.

No, we want to do release, just wait other developers

Merged, but give me time for testing locally, due problem on CI

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.

yargs-parser vulnerability
6 participants