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

Please fix security issues #530

Closed
waynebrantley opened this issue Jul 26, 2018 · 5 comments
Closed

Please fix security issues #530

waynebrantley opened this issue Jul 26, 2018 · 5 comments
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@waynebrantley
Copy link

This project has several vulnerabilities in the dependencies are reported by npm audit.

package - node-http-signature

The biggest is caused by this project's dependency on node-http-signature which depends on several items with vulnerabilities.

  • sshpk has a high vulnerability (denial of service).

This was reported an a PR requested (but not merged) several months ago. Appears to be no maintainer.

TritonDataCenter/node-http-signature#70
https://nodesecurity.io/advisories?search=sshpk&version=1.13.1

Would request you replace the dependency or fork to resolve.

package - request

Another vulnerability is on hoek which is used by used by hawk, which is used by request.
If you update request - they removed the dependency on hawk, which will fix this.
https://nodesecurity.io/advisories?search=hoek&version=4.2.0

Additionally, would it be possibly to make npm audit part of the deployment/release process of this repository so future vulnerabilities can be mitigated sooner?

@funkyboy
Copy link
Contributor

@waynebrantley thanks for reporting this. I merged this PR but I still see occurrences of hoek. Will get back to this.

@waynebrantley
Copy link
Author

@funkyboy Thanks for getting on this.
What about sshpk and stringstream?
Also, can you able to make npm audit part of your release process?

@funkyboy
Copy link
Contributor

@waynebrantley will chip them away one at a time. I will update this issue as I progress.

@mattheworiordan
Copy link
Member

Also, can you able to make npm audit part of your release process?

@waynebrantley please create a separate issue for this. We will certainly consider it in future.

@waynebrantley in addition:

sshpk has a high vulnerability (denial of service).

None of our code paths rely on this. So yes this should be fixed, but in browsers this is not used, and in Node versions of this lib, this vulnerability doe not impact any of our users as far as I can tell.

Another vulnerability is on hoek which is used by used by hawk, which is used by request.

Sure, but this is an authentication scheme we do not support. So unless someone explicitly uses the request module by hacking into the privates of our library, this too is not a security issue that can effect our users at present.

@funkyboy it would be good to upgrade these dependencies for hygiene reasons, but this is a low priority issue it seems.

@mattheworiordan mattheworiordan added bug Something isn't working. It's clear that this does need to be fixed. low-priority labels Jul 31, 2018
@SimonWoolf
Copy link
Member

Thanks for getting on this. What about sshpk and stringstream?

All of the audit failures currently reported against ably-js are with dev dependencies. That is, code that is only used as part of our development or testing infrastructure, not that is actually used when running ably-js.

For example, ably-js currently depends on two versions of the request module; 2.87.0 for production use, and 2.79.0 which is required by a dev dependency. Only the latter depends on hawk and stringstream; the version actually used by ably-js does not.

Eliminating security issues in dev/test code, while it would be nice, is often not practical: our testing stuff needs to work on very old platforms (we support back to IE8), and updating to newer versions would in some cases conflict with this. But again, that has absolutely no effect on the security of ably-js as used.

Also, can you able to make npm audit part of your release process?

Sure, pending npm/npm#20564 to avoid these sort of false positives. #551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

No branches or pull requests

4 participants