search

apigee-127 / bagpipes

Less code, more flow. Let's dance!
MIT License
47 stars 30 forks source link

Switch off machinepack-http to fix lodash vulnerability #42

Closed chrisbellman closed 4 years ago

chrisbellman commented 4 years ago

Thanks for upgrading lodash in this PR to fix the prototype pollution vulnerability

However, it looks like the bagpipes dependency machinepack-http still requires a pre-patched version of lodash, even in its latest release - https://github.com/sailshq/machinepack-http/blob/master/package.json .

This should be remedied per - https://nodesecurity.io/advisories/577 & https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2018-16487

Can this library switch to using a different HTTP dependency such as request (which machinepack-http uses under the hood anyway) or axios (fewer dependencies) that doesn't have any known vulnerabilities? It doesn't look too complicated to replace - https://github.com/apigee-127/bagpipes/blob/master/lib/fittings/http.js

theganyo commented 4 years ago

Have you attempted to open an issue and/or PR with https://github.com/sailshq/machinepack-http?

chrisbellman commented 4 years ago

@theganyo No. They don't have an issues tab on that repo, so I came here. It looks like I can report a bug through the related sails repo, which I'll look into.

The real dependency issue appears to be both directly in machinepack-urls as well as include-all, which is a dependency of machine, which is also a dependency of machinepack-urls, so that looked messy compared to the one file change that would be needed for bagpipes. The machinepack-http library also uses a vulnerable custom fork of lodash and seems very opinionated about it - https://github.com/sailshq/lodash :

This repo will only be updated when there are immediate, material issues affecting expected usage, like this one. Our goal is to diverge as little as possible, and to encourage the use of Lodash 4 and above whenever possible. This repo is really just for us, and anyone else who really likes Lodash 3 exactly the way it is. In other words, there will never be any new methods or options added to Lodash on this fork, and consequently there will be no minor version or major version bumps from this fork-- only patches.

I'll look into reporting these issues downstream, but it may be easier to just replace bagpipes http library with something like request, which machinepack-http wraps anyways. Do you know why machinepack-http was used in the first place instead of a more traditional library?

theganyo commented 4 years ago

Just the convenience of being able to directly feed the parameters through to a tested module and get a result. But as you said, it wouldn't be too difficult to rewrite.

chrisbellman commented 4 years ago

Opened an issue after clicking "report bug" on machinepack-http which took me to balderdashy/sails - https://github.com/balderdashy/sails/issues/6893 . Fix would require a lot of cascading updates & dependencies, so may try to fix here instead.

If I take a stab at a PR, could you review?

theganyo commented 4 years ago

Ok. But if we're going to replace this module with new custom code, we'll need to need to ensure it has a suite of tests like the original module.

lejlabecirspahic commented 4 years ago

@theganyo lodash vulnerabilities seem to be resolved in machinepack-http@8.0.0. Is there any chance to update bagpipes to use that version?

theganyo commented 4 years ago

Yes... but can you verify that the change works and doesn't break anything?

lejlabecirspahic commented 4 years ago

I updated machinepack-http to version 8.0.0 and all tests have passed (without any changes). Is that enough?

theganyo commented 4 years ago

Unfortunately, the tests are not nearly as comprehensive as I'd like. I'd like to see some verification from users that it works fine. Alternatively, I suppose we could merge and release a "beta" version...

shumailxyz commented 4 years ago

@theganyo Do you have any update on the beta release? We can test it then and share feedback here.

theganyo commented 4 years ago

@shumailxyz You got it. I updated and published bagpipes@0.2.2. Tagged as "test" for the moment. Let me know!

shumailxyz commented 4 years ago

Thanks a lot. Did you already published to npm? Can't find it under versions here: https://www.npmjs.com/package/bagpipes

theganyo commented 4 years ago

Yes. I just checked and I can see it there. Perhaps you're having a cache issue?

shumailxyz commented 4 years ago

Ah damn. Yeah, was cache issue. I see it now. Thanks.

shumailxyz commented 4 years ago

@theganyo Works fine for us. Thank you!

theganyo commented 4 years ago

Any other comments? I'll plan on moving this to the "latest" label soon.

theganyo commented 4 years ago

0.2.2 is now tagged as latest.