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 security issue https://npmjs.com/advisories/781 #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nullivex
Copy link

@nullivex nullivex commented Feb 7, 2019

Should fix this, which just popped up today.

│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ node.extend                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.1.7 <2.0.0 || >= 2.0.1                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ rmdir-promise                                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ rmdir-promise > rmdir > node.flow > node.extend              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/781    

@nullivex
Copy link
Author

I spent time making sure this would be the least impactful fix to the problem. @ljharb any help here?

@nullivex
Copy link
Author

nullivex commented Mar 2, 2019

This security issue remains despite the approval for changes. Any way this can be merged? @garyyeap @ljharb @jacksctsai @ben-lin ?

@ljharb
Copy link
Member

ljharb commented Mar 3, 2019

@nullivex you'll note that the gray check means i'm not a collaborator, so pinging me isn't going to help :-)

@nullivex
Copy link
Author

nullivex commented Mar 3, 2019

My apologies, Ill leave you out of it. Initially I saw you on the Organization and thought that you may be able to contact someone with publishing privileges. Its a shame to see this vulnerability persist over incorrect usage of the package.json file.

@smamczak
Copy link

Any update on this merge?

@ljharb
Copy link
Member

ljharb commented Apr 29, 2021

To be clear, while this is a good change (everything should always prefer using ^ ranges), it's also unnecessary, because like almost every prototype pollution CVE, this one is a false positive.

rmdirr is only used here:

rmdir( target_dir, function ( err, dirs, files ){
and target_dir is
var target_dir = __dirname + '/assets/';
, which is hardcoded. Thus, the vulnerability is utterly impossible - you can't even attack yourself.

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