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

Moderate Severity: Axios CSRF Vulnerability in Versions 0.8.1 to 1.5.1 #111

Closed
fundWJ opened this issue Jan 25, 2024 · 8 comments · Fixed by #115 · May be fixed by #112
Closed

Moderate Severity: Axios CSRF Vulnerability in Versions 0.8.1 to 1.5.1 #111

fundWJ opened this issue Jan 25, 2024 · 8 comments · Fixed by #115 · May be fixed by #112
Labels
dependencies Pull requests that update a dependency file

Comments

@fundWJ
Copy link

fundWJ commented Jan 25, 2024

Hi,

We recently found this issue when updating the packages,
in npm audit we got this Moderate Severity issue:

current cloudconvert version: 2.3.5

axios  0.8.1 - 1.5.1
Severity: moderate
Axios Cross-Site Request Forgery Vulnerability - https://github.com/advisories/GHSA-wf5p-g6vw-rhxx
fix available via `npm audit fix --force`
node_modules/cloudconvert/node_modules/axios
  cloudconvert  >=2.0.0
  Depends on vulnerable versions of axios
  node_modules/cloudconvert

Would be great if this can be updated.
Thank you.

@nikos-pblworks
Copy link

+1

@josiasmontag
Copy link
Contributor

This vulnerability does not affect the SDK because we don't use axios with XSRF-TOKEN cookies or any external hosts.

Nevertheless, we should update the SDK dependencies in general.
@KnorpelSenf would be great if you had a minute for this.

@josiasmontag josiasmontag added the dependencies Pull requests that update a dependency file label Feb 2, 2024
@stefansundin
Copy link
Contributor

It would also be great if there could be a new major version that uses the built-in Node.js fetch support which has been stable since Node.js v18.

The form-data package could also be removed since there's built-in FormData in Node.js since v18 as well.

I think removing them is the best option security wise, and I don't see much reason to continue supporting Node.js versions less than v18.

@KnorpelSenf
Copy link
Contributor

Sure thing, I'll tackle this.

The form-data package could also be removed since there's built-in FormData in Node.js since v18 as well.

We need to be able to stream data. The form-data package provides a way to .append most streams and have it work automatically. Unfortunately, the web specs for FormData as implemented in Node.js force you to buffer the entire file in memory before sending it. (Who thought that this was a good idea???)

IMO we have two options:

  • keep on using form-data
  • drop down to constructing a multipart/form-data stream (I did this once)

@stefansundin is there another way to go about this?

@KnorpelSenf KnorpelSenf mentioned this issue Feb 6, 2024
@KnorpelSenf
Copy link
Contributor

Check out the work-in-progress pull request for this at #112.

I am happy to drop axios and bump to Node 16 in order to use fetch. I have two more questions about this:

  1. Should we directly go higher? Node 16 is not even maintained anymore (https://nodejs.org/en/about/previous-releases) so perhaps we should directly require version 18? Downside: breaking change for more people than needed.
  2. The WIP currently does not compile because apparently form-data has no proper integration with web fetch. Dependig on what we want to do with form-data, how should we continue with this?

@josiasmontag
Copy link
Contributor

We can directly go with Node 18 and drop support for older versions. The old SDK version is still available for older Node versions.

I have seen some workarounds to make native FormData work with streams. But I am not sure if they have some drawbacks, this needs to be tested.

@KnorpelSenf
Copy link
Contributor

I'll try that

@KnorpelSenf
Copy link
Contributor

I have updated #112 to use built-in fetch and FormData. It now also requires Node 19.8.0 or above. This enables users to use fs.openAsBlob which returns a file-based Blob instance that can be passed to FormData.

We now only require a single dependency (socket.io). I removed everything else.

This is a breaking change. Users are now unable to directly pass streams they created using fs.createReadStream. There is no way to use custom streams with the FormData from the web spec, which still blows my mind a bit. Even when browsers upload files, they use a non-specced version.

@josiasmontag if you dislike the changes in #112 then we can try to find yet a different solution.

One option is to go back to form-data from npm, but it cannot be used with built-in fetch, so we'd have to use more dependencies here again.

I am sort of tempted to write custom logic to convert any data source to a multipart/form-data stream. That way, we could

  • support streams, blobs, disk files, and anything else we want
  • only rely on built-in functionality of Node 16
  • still be free of dependencies
  • circumvent the limited functionality of FormData
  • stay on the same major version because nothing is breaking

This would come at the expense of new complexity, and I am not sure if you like to see that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants