twilio / twilio-node

Node.js helper library
MIT License
1.37k stars 495 forks source link

[Vulnerability] patch axios vulnerability CVE 2023 45857 #973

Closed yangsu-ab closed 8 months ago

yangsu-ab commented 8 months ago

Issue Summary

Current version of axios used by the library has a vulnerability. https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459

I see a few PRs open for this already, but it is not being addressed. https://github.com/twilio/twilio-node/pull/971 https://github.com/twilio/twilio-node/pull/970

Steps to Reproduce

  1. Install latest twilio-node dependency
  2. Run snyk/github security scan

Code Snippet

Exception/Log

Issues with no direct upgrade or patch:
  ✗ Cross-site Request Forgery (CSRF) [High Severity][https://security.snyk.io/vuln/SNYK-JS-AXIOS-6032459] in axios@0.26.1
    introduced by twilio@4.18.1 > axios@0.26.1
  This issue was fixed in versions: 1.6.0

Technical details:

saghaulor commented 8 months ago

Given that this is an API client and that there is a warning indicating not to use this in front end apps which means there are no cookies involved, and that CSRF is a front end vuln related to cookie handling, I'm pretty sure this vulnerability does not affect this library in any meaningful way.

From the readme https://github.com/twilio/twilio-node/blob/main/README.md?plain=1#L28-L29

Do not use this Node.js library in a front-end application. Doing so can expose your Twilio credentials to end-users as part of the bundled HTML/JavaScript sent to their browser.

yangsu-ab commented 8 months ago

Given that this is an API client and that there is a warning indicating not to use this in front end apps which means there are no cookies involved, and that CSRF is a front end vuln related to cookie handling, I'm pretty sure this vulnerability does not affect this library in any meaningful way.

From the readme https://github.com/twilio/twilio-node/blob/main/README.md?plain=1#L28-L29

Do not use this Node.js library in a front-end application. Doing so can expose your Twilio credentials to end-users as part of the bundled HTML/JavaScript sent to their browser.

while i agree this MIGHT not impact the client in any meaningful way, as of now. but

  1. there is no way to tell if the vulnerability will impact the future releases. anything could happen
  2. letting a vulnerability stay in the code intentionally causes all sorts of issues for consumers of the library. i.e. broken pipelines, setting up rules to ignore particular scan result etc.
  3. what downside is there to patch the library? there is already PR created.
mfulton26 commented 8 months ago

Upgrading to the latest version of axios or moving to use fetch API built into Node 18+ has its own benefits.

Node 14 & 16 are both end of life releases. Removing axios and using fetch instead means one less package to depend on, have potential security issues from, etc.

tiwarishubham635 commented 8 months ago

Hi! We have merged #971 addressing this issue. Thanks!

saghaulor commented 8 months ago

Upgrading to the latest version of axios or moving to use fetch API built into Node 18+ has its own benefits.

Node 14 & 16 are both end of life releases. Removing axios and using fetch instead means one less package to depend on, have potential security issues from, etc.

This is a great point. Hopefully the team will consider replacing Axios and using the built in fetch API.

saghaulor commented 8 months ago

Given that this is an API client and that there is a warning indicating not to use this in front end apps which means there are no cookies involved, and that CSRF is a front end vuln related to cookie handling, I'm pretty sure this vulnerability does not affect this library in any meaningful way. From the readme https://github.com/twilio/twilio-node/blob/main/README.md?plain=1#L28-L29

Do not use this Node.js library in a front-end application. Doing so can expose your Twilio credentials to end-users as part of the bundled HTML/JavaScript sent to their browser.

while i agree this MIGHT not impact the client in any meaningful way, as of now. but

  1. there is no way to tell if the vulnerability will impact the future releases. anything could happen
  2. letting a vulnerability stay in the code intentionally causes all sorts of issues for consumers of the library. i.e. broken pipelines, setting up rules to ignore particular scan result etc.
  3. what downside is there to patch the library? there is already PR created.

Oh, I agree on all points. I was merely trying to contextualize the vuln as often people will see a vuln and make quick judgements. I was trying to help anyone who doesn't fully understand the issue to understand that there isn't a security risk. However, to your point, there are other reasons to upgrade. Thankfully the fix was merged!