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

Use Promise.all() #16

Closed
wants to merge 5 commits into from
Closed

Use Promise.all() #16

wants to merge 5 commits into from

Conversation

imaman
Copy link

@imaman imaman commented Feb 7, 2019

Initial motivation for this PR was https://npmjs.com/advisories/781 .

This security issue can also be solved by other means (the two PRs opened on node.flow: dreamerslab/node.flow#4, dreamerslab/node.flow#5), I believe that migrating the code in here to using Promises is a course of action which will benefit the long term maintainability of this repo (as Promises are the de-facto standard for managing async computations).

In this PR I tried to be as conservative as possible: I just made the minimal change that could work. If you feel that it is OK for this code to use newer constructs (async/await, arrow functions) please let me know. I'd be happy to do this porting as well.

$ npm audit
                                                                                
                       === npm audit security report ===                        
                                                                                
┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ node.extend                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.1.7 <2.0.0 || >= 2.0.1                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ node.flow                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ node.flow > node.extend                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/781                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

@imaman imaman changed the title Use Promise.all() to overcome https://npmjs.com/advisories/781 Use Promise.all() Feb 7, 2019
@nullivex
Copy link

nullivex commented Mar 9, 2019

I hope that one of these get accepted. Or the repository ownership should be transferred.

@imaman Good work!

@nullivex
Copy link

nullivex commented Apr 2, 2019

I didnt see any movement here so went ahead and forked this and applied the PR.

See this release:
https://github.com/nullivex/rmdir2

https://www.npmjs.com/package/rmdir2

@nullivex
Copy link

nullivex commented Apr 2, 2019

I tested this release working great for me.

@imaman imaman closed this Jan 10, 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.

2 participants