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(4.x): migrate from optimist to yargs #1662

Closed
wants to merge 2 commits into from
Closed

fix(4.x): migrate from optimist to yargs #1662

wants to merge 2 commits into from

Conversation

AviVahl
Copy link
Contributor

@AviVahl AviVahl commented Mar 19, 2020

closes #1658
closes #1661

adapted code from master to latest yargs (.option calls).

4.x:
found 188 vulnerabilities (169 low, 4 moderate, 14 high, 1 critical) in 5815 scanned packages

4.x with this PR:
found 32 vulnerabilities (17 low, 1 moderate, 13 high, 1 critical) in 5829 scanned packages

closes #1658
adapted code from master to latest yargs (`.option` calls).

```
4.x:
found 188 vulnerabilities (169 low, 4 moderate, 14 high, 1 critical) in 5815 scanned packages

4.x with this PR:
found 32 vulnerabilities (17 low, 1 moderate, 13 high, 1 critical) in 5829 scanned packages
```
@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 19, 2020

oh, I didn't add further CLI tests, but you are free to edit this PR. I'm definitely not the right person to add such tests, as I'm not a direct user of this package. It's used in many repositories I manage as an indirect dependency, so thought I'll contribute a bit by fixing the audit failures.

some indirect dependencies install @types packages which are not compatible with the older typescript. adjusted test's tsconfig to not pick these up automatically, as the actual .d.ts does not depend on these external types.
@AviVahl AviVahl changed the title fix: migrate from optimist to yargs fix(4.x): migrate from optimist to yargs Mar 19, 2020
@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 22, 2020

@wycats @nknapp any chance you've got time to review this? :)

@nknapp
Copy link
Collaborator

nknapp commented Mar 22, 2020

At the moment. Difficult. Schools are closed in Germany, which means my kids are at home. So, me and my wife, we have to supervise the kids and work in home office at the same time. At the moment I need my energy for other things. Sorry

@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 23, 2020

Appreciate the response. I wish you and your family health. Stay safe.

@JimmyBjorklund
Copy link

Any status update on this ? Would relay like to get rid of this: https://npmjs.com/advisories/1179

@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 27, 2020

Much appreciated @DiegoRBaquero
Could I bother you with a merge/release?

@nknapp
Copy link
Collaborator

nknapp commented Mar 28, 2020

I really need some tests in tasks/test-cli.js

I need to be sure that the change isn't breaking anything in the way the CLI is used.

@nknapp
Copy link
Collaborator

nknapp commented Mar 28, 2020

BTW. I don't think the security leak in optimist does create any leaks in Handlebars. It only affects the CLI, which is usually used in a very controlled environment, if at all. It's an inconvenience to get all the audit warnings, nothing more.

If you disagree, please let me know in a private email.

@aorinevo
Copy link
Contributor

@AviVahl, happy to help with merge and release.

@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 28, 2020

@aorinevo I've sent you an invite to join https://github.com/AviVahl/handlebars.js, so you can edit this branch.

@AviVahl
Copy link
Contributor Author

AviVahl commented Mar 28, 2020

It appears @aorinevo also decided to go another route, heh. Removed invitation.

@nknapp since I am not going to spend further time working on this, I am closing this PR. If anybody else wants to continue the work (add tests and whatnot), feel free to take whatever you want from here and open a new PR.

@AviVahl AviVahl closed this Mar 28, 2020
@fabb fabb mentioned this pull request Mar 29, 2020
19 tasks
@karlhorky
Copy link

I don't think the security leak in optimist does create any leaks in Handlebars. It only affects the CLI, which is usually used in a very controlled environment, if at all. It's an inconvenience to get all the audit warnings, nothing more.

This is a pretty bad take, and not one I expect from a library maintainer 😕

Security warnings may not indicate an immediate problem that will actively affect a project, true.

But if engineers start ignoring them, this leads down a dangerous path. Not only does this mean more leniency that increases the risk to projects, but it also means increased mental load when running an audit on any of your projects ("oh, we can ignore that warning... because... why was that again?").

@fabb
Copy link
Contributor

fabb commented Mar 29, 2020

I continued the work in #1666

@ErisDS
Copy link
Collaborator

ErisDS commented Mar 30, 2020

@karlhorky You've 1) misinterpreted what was said and 2) failed to follow the explicit request to respond if you disagree via email.

The point of @nknapp's comment was that there is no known security vulnerability in handlebars as a result of this warning, and therefore there's no urgent need to drop everything (like homeschooling his kids) to fix it. That's not to say it won't get fixed eventually, but that there are higher priority concerns.

There have been numerous real vulnerabilities reported over the last 12 months and @nknapp has literally dropped everything in his life to fix them immediately every single time. He has been doing a truly fantastic job maintaining handlebars singlehandedly and I tip my hat to him. I do my best to at least provide him moral support behind the scenes, because there is literally no one else helping right now and I can't do much more than I already do.

You, me and everyone else that depends on handlebars owe him gratitude and encouragement.

@karlhorky
Copy link

karlhorky commented Mar 30, 2020

@ErisDS I don't mean to discount Nils' contributions on the project - it seems you're a contributor, and so I defer to your knowledge of his work here, which it seems is formidable. So let me be clear - I appreciate all of the tireless work of the maintainers here, including Nils! It's hard to run an open source project. I'm sorry if I have caused any grief with my comment.

The point of my comment was not to say that Nils does not contribute, nor was it to say that he or anyone else should drop things that are more important (especially in these uncertain times).

My point is that this is a public forum, and words matter - more so if you are in the position of being a maintainer. The words in his comment seemed to be dismissive of the potential risks of ignoring these warnings. Because this is a public forum and he is a maintainer, this could be read by other software engineers as a reason for them to also follow similar paths, potentially for longer terms, without thinking of the risks.

This is why, after careful consideration, I decided to respond in that same public forum instead of privately with email. This was not to try to change his mind or force action at the expense of anything else, but to publicly criticize the implications of those words.

I don't mean to blame or personally attack anyone with my criticism. Now of all times, it is hard to come out with fully considered statements. But it is important what words maintainers say, and the implications of those words.

@ErisDS
Copy link
Collaborator

ErisDS commented Mar 30, 2020

Again, I don't believe that Nils intended any of what you've interpreted.

The message simply says that there's no actual vulnerability and nothing for anyone to worry about right now.

To further reinforce the intention behind those words, the reason I'm here is because Nils asked me to take over so that it does get fixed sooner rather than later.

As someone who babysits several under-maintained repos I am seeing a lot of pain from these "incorrect" security notices, because there is often more work required to fix them than there is people and time available. I appreciate the work being done to make node more secure, but when a commonly depended on module gets one of these flags, there is a huge burden placed on dependent maintainers to act swiftly.

At the same time, I don't advocate at all for ignoring the messages - I advocate for what we are seeing here - people getting stuck in and doing the work so that maintainers aren't totally overwhelmed.

@karlhorky
Copy link

I think we're all on the same side here. We all contribute to open source (I also do those pull requests that you are advocating for too), we all care about security, we all have issues dealing with the large amount of time it takes (although I'm sure that you both are under more stress in regards to open source than I am).

I think you can agree, it does matter how people feel and how things are interpreted, also when that doesn't match the original intention. The original intention does count, but not as much as the outcome.

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.

8 participants