search

crowdin / ota-client-js

JavaScript client library for Crowdin Over-The-Air Content Delivery
https://crowdin.github.io/ota-client-js
MIT License
70 stars 11 forks source link

Axios error and vulnerability #86

Closed otavio-bn closed 2 days ago

otavio-bn commented 2 weeks ago

Hi, we recently introduced the OTA SDK in one of our projects to speed up the release of new strings. We had some problems but the big one was related to Axios. The first one was the Cannot read properties of undefined (reading 'create') error and we fixed it by providing our client using the httpClient option.

While this solution worked, we are now facing another problem. Snyk pointed out that the current Axios version being used suffers from this vulnerability: https://security.snyk.io/vuln/SNYK-JS-AXIOS-7361793.

Since we see that the httpClient is used twice in the code for performing 2 get operations, we would like to know if maybe it makes sense to remove this dependency. It seems like we can achieve the same effect by only using plain fetch.

We use Crowdin a lot and we can contribute to this improvement. We thought about not using the library, but it seems like contributing is the best path to follow. Given that, we want to know what you all think about this suggestion.

Thanks

andrii-bodnar commented 2 weeks ago

Hi @otavio-bn, thanks for reporting this!

I have just released a new version with the vulnerability fixes - 1.1.3, could you please try it?

otavio-bn commented 2 weeks ago

Hi @andrii-bodnar, thanks so much for replying so fast and fixing the problem! This will definitely work for us! But we would also like to ask if it makes sense to have axios as optional since if you provide httpClient it will not be used. Does it make sense to have it as optional dependencies?

andrii-bodnar commented 2 weeks ago

I was thinking about making it a peerDependency but not sure about that since it is still required in case the developer doesn't use the custom httpClient

otavio-bn commented 2 weeks ago

I see your point. The only problem we see is having something unused on the bundle. Would it be beneficial to swap Axios for a Fetch? I mean, looks like the usage of Axios is very basic, only for get operation.

andrii-bodnar commented 2 weeks ago

@yevheniyJ what do you think about this?

otavio-bn commented 2 weeks ago

Hey, just to follow up. Adding as peer will fix the problem for sure. We question this because maybe we can remove Axios and improve the library size :)

yevheniyJ commented 4 days ago

The only downside here is that fetch is only supported in Node starting from v18. So if we remove axios some apps might fail due to missing fetch in their runtimes. @andrii-bodnar we can think about major release where we remove axios

andrii-bodnar commented 3 days ago

@yevheniyJ thanks for looking at this!

Since Node v16 is no longer maintained, I think it's okay to require Node v18 as a minimal version and release a new major version without Axios.

I'm only concerned that the fetch API became stable in Node v21 and is marked as experimental in Node v18. Do you see any problems here?

yevheniyJ commented 3 days ago

@andrii-bodnar No, it still should all work. fetch is available in global scope by default so we should be good

andrii-bodnar commented 2 days ago

@otavio-bn available in 2.0.0.

otavio-bn commented 14 hours ago

Thank you everyone! You are amazing!