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

Update npm packages to mitigate vulnerabilities #51

Merged
merged 3 commits into from
Jun 13, 2019

Conversation

kb0rg
Copy link
Contributor

@kb0rg kb0rg commented Jun 4, 2019

This PR updates most of the packages recommended by npm

found 10 moderate severity vulnerabilities in 316 scanned packages
  run `npm audit fix` to fix 3 of them.
  7 vulnerabilities require manual review. See the full report for details.

The 1 remaining vulnerability is dependent on an update to the github-tools github-api package, which should be landing in the near future -- but which should not block this PR.

TODO:

  • determine whether this is useful/ necessary
  • determine whether the remaining 7 vulnerabilities should be patched
  • manually update as many of the remaining 7 as recommended
  • possibly wait until github updates their package and get it all dealt with at once?
  • test that nothing breaks (? how ?)

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

The following 3 vulnerabilities were addressed in 8db9cf

kborg: ~/dev/heimdall [WIP-update-packages-maybe] $ npm audit
                                                                                
                       === npm audit security report ===                        
                                                                                
# Run  npm install github-api@3.2.0  to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ axios                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ github-api                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ github-api > axios                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/880                       │
└───────────────┴──────────────────────────────────────────────────────────────┘


# Run  npm update lodash --depth 6  to resolve 2 vulnerabilities
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ cheerio                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ cheerio > lodash                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/782                       │
└───────────────┴──────────────────────────────────────────────────────────────┘


┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > form-data > async >    │
│               │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/782                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

The following 7 vulnerabilities require manual review

┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ hoek                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ > 4.2.0 < 5.0.0 || >= 5.0.3                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > hawk > boom > hoek     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/566                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ hoek                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ > 4.2.0 < 5.0.0 || >= 5.0.3                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > hawk > cryptiles >     │
│               │ boom > hoek                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/566                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ hoek                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ > 4.2.0 < 5.0.0 || >= 5.0.3                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > hawk > hoek            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/566                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ hoek                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ > 4.2.0 < 5.0.0 || >= 5.0.3                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > hawk > sntp > hoek     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/566                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Remote Memory Exposure                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ request                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.68.0                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/309                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ hawk                                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.1.3 < 4.0.0 || >=4.1.1                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > hawk                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/77                        │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Memory Exposure                                              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ tunnel-agent                                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=0.6.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ hubot-flowdock                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ hubot-flowdock > flowdock > request > tunnel-agent           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/598                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

@kb0rg kb0rg requested review from sthompson22 and Shadowfiend June 4, 2019 20:09
@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

I'm still getting my footing with the world of npm, I'm not sure what's the usual handling of audit warnings like this?

I'm going to dig in and read the advisories to try to form some opinions of my own, but I'd also love to hear opinions on what does/ doesn't really matter to us in this regard.

eg: Can can this whole PR if it doesn't seem necessary.

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

Notes:

The 7 vulnerabilities listed above boil down to only 4 packages needing updates (one of them appears 4 times as a dependency of other packages).

Questions:

  1. These all seem like potentially-serious vulnerabilities, but I don't know how likely we are to be exposed to any of them. Any guidelines on how to assess that? Or.. Is it best practice to just go ahead and follow the npm recommendations?

  2. None of these packages are directly referenced in package.json since they are all installed as dependencies of other packages... I thought package-lock.json was generally used as a generated file, not one we edit ourselves. I guess this is where the "require manual review" note comes in. Is it ok put the version info into package-lock.json or should I add them to package.json?
    (nevermind, this seems to be the way to go)

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

After replacing the standard hubot-flowdock package with hubot-reload-flowdock we're down to only one vulnerability: axios which is needed by github-api

kborg: ~/dev/heimdall [update-npm-packages] $ 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      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ axios                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=0.18.1                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ github-api                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ github-api > axios                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/880                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
found 1 moderate severity vulnerability in 379 scanned packages
  1 vulnerability requires manual review. See the full report for details.

We're currently using the most current version of github-api -- might there be unexpected consequences of updating axios past the version that github-api specifies?

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 4, 2019

NOTE this is marked WIP because I have outstanding questions (see above comments), would love 👀 on it.

@Shadowfiend
Copy link
Contributor

This is super-useful and necessary, really appreciate your picking it up. Most of these probably don't hit us too hard as we're running this on the server, but the axios one might, and we're definitely in better safe than sorry land.

I would say, go ahead and bump axios in the lock file and let's keep an eye out for a newer version of github-api at https://github.com/github-tools/github/releases .

Though it's also been a while since the last release there, so we'll see if that happens.

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 7, 2019

Though it's also been a while since the last release there, so we'll see if that happens.

...interesting. Looking at the releases page you linked, it looks like it has indeed been a while. but on its npm package page it was updated 2 months ago. I guess they missed updating a tag or something (I haven't used the releases feature in GH, not sure how that would fall out of sync)

The good news is that there is a PR open that fixes the axios issue -- once it passes tests. And there is [more recent discussion] of related issues. [Edited to remove link that wasn't really related except very peripherally)

I'll open an issue to track that.

@Shadowfiend
Copy link
Contributor

If you run npm audit in this latest state does everything look good?

kb0rg added 3 commits June 7, 2019 11:02
All of the remaining vulnerabilities from last audit stemmed from
request
Updating request package still left a lot of vulnerabilities, and
since ultimately request is a dependency of hubot-flowdock it seemed
wise to go one more level up

The last update to hubot-flowdock was a year ago, reload has fixed
some dependencies since then
@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 7, 2019

If you run npm audit in this latest state does everything look good?

no, I get the same result (sorry I commented on that - but in flowdock instead of here):

but the same vulnerability remains listed in the audit

kborg: ~/dev/heimdall [update-npm-packages] $ 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      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ axios                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=0.18.1                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ github-api                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ github-api > axios                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/880                       │
└───────────────┴──────────────────────────────────────────────────────────────┘
found 1 moderate severity vulnerability in 384 scanned packages
  1 vulnerability requires manual review. See the full report for details.

although... now that I have links to the updated axios tar and sha (I wasn't sure how to get those) - maybe I can paste them into the requirements section of the github dependencies?

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 7, 2019

It may be better to just wait on this a couple days and see if the github api gets bumped up: looks like they are actively dealing with it (comment 3 hours ago, closing out one PR in favor of another. The open PR isn't passing tests yet but I'm guessing if they've selected which PR to go with, they are working on it)

@Shadowfiend
Copy link
Contributor

Yeah, after reading the npm audit help, it looks like we're SOL unless GitHub updates their plugin, or we fork it, update it, and publish to npm. Fun!

For now let's hold off. I'm not convinced we'll see quick movement, looks like the person who closed the PR you linked was the author of that PR and doesn't work at GitHub; however, maybe we get lucky ;) You may also want to try prodding GitHub on Twitter if this is still sitting around in a few days.

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 10, 2019

looks like the person who closed the PR you linked was the author of that PR and doesn't work at GitHub

shoooooot good catch, I was not looking closely enough at that.

oo wait -- now that I'm looking more closely, it looks like that user has been given write access to the repo (!!!) so we may see some progress after all 🤞

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 10, 2019

also worth noting

adding an issue to look into replacing this with Octokit

@Shadowfiend
Copy link
Contributor

Goodness. And also github-tools has no relationship to GitHub lol.

Moving to octokit is definitely the thing to do, nice find!

@kb0rg kb0rg force-pushed the update-npm-packages branch from 36711de to 6071efe Compare June 12, 2019 01:51
@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 12, 2019

I dropped the last commit (the one with failed the attempt to manually update axios)

@Shadowfiend
Copy link
Contributor

Cool. How we feeling about dropping the WIP?

@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 12, 2019

How we feeling about dropping the WIP?

you know... I was thinking about it yesterday, then hesitated -- but should have commented here about why:
I'm wondering about the last item on the TODO checklist:

test that nothing breaks (? how ?)

any thoughts on that? or do we just, um, QA-via-deploy 😬 😁 ?

(I don't exactly think "add test suite" is within scope here, but wondering if there are other ways to smoke-test while running locally?)

@kb0rg kb0rg changed the title [WIP] Update npm packages Update npm packages to mitigate vulnerabilities Jun 12, 2019
@kb0rg
Copy link
Contributor Author

kb0rg commented Jun 12, 2019

Following up on offline chat about these questions:
while "QA-via-deploy" is not the ideal approach in general, we decided it is ok in this case.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO 🤞

@Shadowfiend Shadowfiend merged commit f1cb0dd into master Jun 13, 2019
@Shadowfiend Shadowfiend deleted the update-npm-packages branch June 13, 2019 20:00
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