Security vulnerability in winston mail

[mattcollier]

If we can eliminate this, we will have a clean npm audit in some top-level applications.

┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ moment                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.19.3                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ bedrock                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ bedrock > winston-mail > emailjs > moment                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/532                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

[mattcollier]

Corrsponds to this: https://github.com/digitalbazaar/bedrock/pull/41/files

Can we just bump to 2? are we even using winston-mail?

[JSAssassin]

@mattcollier winston-mail is used here: https://github.com/digitalbazaar/bedrock/blob/master/lib/loggers.js#L187. I tested it by updating it to version 2.0.0, it did resolve the vulnerabilities and all tests passed. Is there a way to actually see it work, like sending an email?

[mattcollier]

@JSAssassin thanks for looking into this. @davidlehn I believe is most familiar with our email stack so hopefully he can answer questions you may have.

Does the winston-mail project maintain a good changelog that enumerates the breaking changes in the 2.0.0 release, if so please provide a link along with any other research you've done into the matter.

[JSAssassin]

The winston-mail project changelog doesn't say much other than emailjs has been updated in 2.0.0 release. Going through their merged PRs looks like there were some vulnerability issues with older emailjs and so it was bumped to v2.2. Digging into to it further, emailjs was reported as ReDoS vulnerable, because of its dependency on moment.js, so it dropped moment.js in its 2.2.0 update.

@davidlehn is there an actual way to test this other than bumping it up and seeing all tests pass?

[davidlehn]

I'm not sure we have the email logger enabled anywhere? The original intent was to email out 'critical' level logs. I'm fairly sure we (or at least I) tested that it worked years ago. But we may have not actively used it since. Looks like 'silent' is set in all the repos I checked.

If you want to test it, need to have a server that you can send mail through. I just have a local mail server to do that and 'localhost' is the default. There are config options to set that up in other ways. Then just log something at critical level and email should be sent.

I'm unsure how much effort should be put into this at the moment. There are perhaps better modern approaches to get the same alert effect. There may also be issues here like no flood control if a server got stuck in some critical logging loop.

[JSAssassin]

I didn't setup a mail server but did some logging to make sure that critical level logs get to the winston-mail transport and it does get there. If it's not being used right now I think it should be good for now and we can merge this synk PR.

[davidlehn]

I tried it locally with simple manual test and the newer winston-mail worked. I think the way we have it setup now is questionable. But it does work, so I merged in #41.