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: upgrade yargs to latest version #314

Closed
wants to merge 1 commit into from

Conversation

sturdynut
Copy link

This change should fix a security vulnerability (mem v^1.1.0) from the version of yargs used by this project. Here is the warning we are getting from our repo that uses eslint-find-rules:

image

Here's the chain of dependencies that lead to the insecure package.

  • eslint-find-rules → yargs (v8.0.1)
  • yargs (v8.0.1) → os-locale (v2.0.0)
  • os-locale (v2.0.0) → mem (v^1.1.0")

I ran tests after upgrading and seemed ok, however there are quite a few breaking changes between yargs v8.0.1 and the current v14.2.0, so i've listed them below for your reference.

Breaking Changes

since v8.0.1

  • we now only officially support yargs.$0 parameter and discourage direct access to yargs.parsed
  • previously to this fix methods like yargs.getOptions() contained the state of the last command to execute.
  • do not allow additional positionals in strict mode
  • options with leading '+' or '0' now parse as strings
  • dropping Node 6 which hits end of life in April 2019
  • see yargs-parser@12.0.0 CHANGELOG
  • we now warn if the yargs stanza package.json is used.
  • Options absent from argv (not set via CLI argument) are now absent from the parsed result object rather than being set with undefined
  • drop Node 4 from testing matrix, such that we'll gradually start drifting away from supporting Node 4.
  • yargs-parser does not populate 'false' when boolean flag is not passed
  • tests that assert against help output will need to be updated
  • requiresArg now has significantly different error output, matching nargs.
  • .usage() no longer accepts an options object as the second argument. It can instead be used as an * alias for configuring a default command.
    previously hidden options were simply implied using a falsy description
  • help command now only executes if it's the last positional in argv._
  • version() and help() are now enabled by default, and show up in help output; the implicit help command can no longer be enabled/disabled independently from the help command itself (which can now be disabled).
  • parse() now behaves as an alias for .argv, unless a parseCallback is provided.

@ljharb
Copy link
Collaborator

ljharb commented Oct 19, 2019

supporting node 4 seems important while we support eslint versions that support node 4.

@AdrieanKhisbe
Copy link

Having also stumbled on the mem security altert, I was thinking about doing a Pull Request for a package refreshing of dependencies.
But before I saw this PR by @sturdynut that does what I need.

I understand @ljharb, but wouldn't it possible to make a major version dropping at least the node 4 and 6 that are EOL for long.

I can give a hand if needed.

cc @sarbbottam

@ljharb
Copy link
Collaborator

ljharb commented Feb 21, 2020

Of course it’s possible - the question is, is it desirable, given that this isn’t a particularly problematic CVE?

Separately, EOL status is irrelevant; tools should support what people use.

@AdrieanKhisbe
Copy link

AdrieanKhisbe commented Feb 21, 2020

@ljharb I Totally agree your points
Indeed it's noise, this mem CVE is for sure not a problematic vulnerability, even more in that context. We can live with that :)

However we can also fix it, and took the opportunity to bump many libraries of eslint-find-rules (5 out of 7 of prod dependencies can have a major update available)


About your concern about breaking support for existing user, I think it can be addressed:
When a major is released, it won't break anything for existing user until they update, and They would just do it when then need, and if the meet the EOL condition for next major.
The only long term impact on existing user I see, it's that it will deprive user locked on 4, 6 the eventual new feature that might be added in the feature. (unless a compatible backport is possible and made)
In fact many development node libraries (and even eslintplugins more specifically) have a similar policy in practice: for instance ava, and eslint-plugin-unicorn have drop former 8 LTS on their latest major.


Tell me what your mind about this. =)
(hoping I have not been too verbose 🙊)

AdrieanKhisbe added a commit to CoorpAcademy/eslint-plugin-coorpacademy that referenced this pull request Mar 16, 2020
Only vulnerability staying is the eslint-find-rules plugin related one:
sarbbottam/eslint-find-rules#314 (comment)
AdrieanKhisbe added a commit to CoorpAcademy/eslint-plugin-coorpacademy that referenced this pull request Mar 16, 2020
…nter 🌬️

Minor refresh PR with no expected change to current plugin behavior:
- Scoped bump dependencies (excluding major we can upgrade too)
- Npm audit fix to get ride of most vuln but [one not fixable right now, but not a real issue](sarbbottam/eslint-find-rules#314)
@NextNebula
Copy link

NextNebula commented Jun 8, 2020

I think you should follow the supported node versions of eslint. If users need older node support, they can use a older version of this plugin. eslint 6 dropped support for node 6 (https://eslint.org/docs/user-guide/migrating-to-6.0.0) and eslint 7 dropped support for node 8 (https://eslint.org/blog/2020/05/eslint-v7.0.0-released).

Having support for very old node versions while eslint doesn't even support them is less important then all users of this plugin having 2 npm audit messages.

@ljharb
Copy link
Collaborator

ljharb commented Sep 6, 2020

@NextNebula we do, but we support down to eslint 3, so we support whatever nodes any of eslint 3-7 support.

npm audit messages are almost always false positives; they're unimportant.

@ljharb
Copy link
Collaborator

ljharb commented Nov 8, 2021

Closed by 1e940a9 / #337.

@ljharb ljharb closed this Nov 8, 2021
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.

4 participants