pofider / phantom-html-to-pdf

Highly scalable html to pdf conversion using phantom workers
MIT License
159 stars 33 forks source link

Node Security Project failing #69

Closed codecvlt closed 7 years ago

codecvlt commented 7 years ago

┌───────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│               │ Remote Memory Exposure                                                                                              │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Name          │ request                                                                                                             │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ CVSS          │ 5.3 (Medium)                                                                                                        │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Installed     │ 2.42.0                                                                                                              │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Vulnerable    │ >=2.2.6 <2.47.0 || >2.51.0 <=2.67.0                                                                                 │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Patched       │ >=2.68.0                                                                                                            │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Path          │ XXXXXXX > phantom-html-to-pdf@0.5.4 > phantomjs@1.9.17 > request@2.42.0                                           │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ More Info     │ https://nodesecurity.io/advisories/309                                                                              │
└───────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│               │ Regular Expression Denial of Service                                                                                │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Name          │ hawk                                                                                                                │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ CVSS          │ 5.3 (Medium)                                                                                                        │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Installed     │ 1.1.1                                                                                                               │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Vulnerable    │ < 3.1.3  || >= 4.0.0 <4.1.1                                                                                         │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Patched       │ >=3.1.3 < 4.0.0 || >=4.1.1                                                                                          │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Path          │ XXXXXX > phantom-html-to-pdf@0.5.4 > phantomjs@1.9.17 > request@2.42.0 > hawk@1.1.1                              │
├───────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ More Info     │ https://nodesecurity.io/advisories/77                                                                               │
└───────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘```
codecvlt commented 7 years ago

Updating the default phantomjs version to 2xx should fix this.

bjrmatos commented 7 years ago

both vulnerabilities does not affect the security of the system for the use case that this package was built, the npm phantomjs package is only using the request package to install a phantomjs binary, it is not used while an application is running, it is only used when installing dependencies so i don't see any vulnerability here.

updating to phantomjs version 2 will only remove these warnings of the NSP but there is no other gain here.. also we don't want to make phantom v2 executable the default because phantom v2 has many rendering issues compared to phantom 1.9.8 (the most stable) executable.

if you are just worried about those warnings you can easily install phantomjs@2.x.x in your machine, the NSP warning still will be present but your system won't be using the phantom 1.9.8 executable.

codecvlt commented 7 years ago

Fair enough. Problem right now is your latest version in npm still lists phantomjs as a main dependency. I see in your master branch you made it an optionalDependency. Pointing my version to your master branch removes the NSP warning. When do you think you will release a new npm version which reflects what you have on master branch?

codecvlt commented 7 years ago

Weird, looks like 0.5.4 does have phantomjs as optional. My bad.

codecvlt commented 7 years ago

Hmm, though when installed, the package.json for 0.5.4 does have phantomjs in the "dependencies" section.

bjrmatos commented 7 years ago

Hmm, though when installed, the package.json for 0.5.4 does have phantomjs in the "dependencies" section.

yes, that is the way that npm works, optionalDependencies are always installed, the only difference is that if some optional dependency fails to install then the whole installation won't fail, it will just ignore the optional dependency.

i understand that seeing those warnings can be frustrating, so a solution can be to move the phantomjs dependency from optionalDependencies to peerDependencies, but i'm not sure if everyone will be ok with that, so let's see.. (cc @pofider)

codecvlt commented 7 years ago

Makes sense. It's ok. I've added this package to my ignore list since it shouldn't be exploitable in my use case.

Cheers, and thanks for the quick responses!

pofider commented 7 years ago

I believe the explanation @bjrmatos has done is sufficient. I don't plan to do big changes like moving the phantomjs dependency out at this moment.