facebook / facebook-nodejs-business-sdk

Node.js SDK for Meta Marketing APIs
https://developers.facebook.com/docs/business-sdk
Other
497 stars 227 forks source link

Replace Deprecated npm Package 'request' #256

Closed genlyken closed 1 year ago

genlyken commented 1 year ago

Which SDK version are you using?

16.0.2

What's the issue?

Given a new requirement to run snyk on our projects, since the current package uses request@2.88.2, I'm seeing:

Issues with no direct upgrade or patch: ✗ Server-side Request Forgery (SSRF) [Medium Severity][https://security.snyk.io/vuln/SNYK-JS-REQUEST-3361831] in request@2.88.2 introduced by facebook-nodejs-business-sdk@16.0.2 > request@2.88.2 and 1 other path(s) No upgrade or patch available

Steps/Sample code to reproduce the issue

  1. locate a package using Fb ,
  2. install snyk via https://docs.snyk.io/snyk-cli/install-the-snyk-cli
  3. run snyk test

Observed Results:

Issue as shown above.

Expected Results:

✔ Tested XX dependencies for known issues, no vulnerable paths found.

stcheng commented 1 year ago

thanks for raising this issue. since request package is no longer active, we will consider to replace the request package with alternative solutions.

genlyken commented 1 year ago

@stcheng Thanks for the update! This is the only project not passing the vulnerability checks and our clients are concerned about this. We'd love to keep using this package. Is there a timeline on making this change?

stcheng commented 1 year ago

@genlyken, thank you for providing this feedback. Although we don't currently have a concrete timeline on this topic, I'll assess how extensive the change would be to replace the 'request' package with alternatives, and I'll escalate this issue to prioritize it. I'll keep this thread updated.

genlyken commented 1 year ago

@stcheng Thanks I appreciate your quick reply! I was digging further and it doesn't look like request is being used at all. I pulled it down locally and removed the dependency and all my tests passed. Perhaps we can just remove it from the package.json?

stcheng commented 1 year ago

@genlyken oh really! because if that is the case, would you mind create a pull request? I would be glad to help review it and facilitate the progress.

genlyken commented 1 year ago

@stcheng I thought I already did the CLA thing, but I'm getting 403 when I push. I was hoping maybe you can make the change as it should just be a one line delete in the package.json

stcheng commented 1 year ago

@genlyken Sounds good. If no other issues I'll have it done by today.

stcheng commented 1 year ago

@genlyken this issue seems not that straightforward. the request package is required by request-promise which is used in http.js file(https://github.com/facebook/facebook-nodejs-business-sdk/blob/main/src/http.js#L12). Thus, some code changes are required to deprecate both request and request-promise. I'll do some further investigation here.

genlyken commented 1 year ago

@stcheng i submitted a PR (https://github.com/facebook/facebook-nodejs-business-sdk/pull/257), but I haven't figured out the CLA yet. I'm using axios and I tested this code by creating a campaign, adset and ad and uploaded a video, but there's plenty of FB code that I'm not familiar with and haven't tested. I'd love if you can take a look!

stcheng commented 1 year ago

@genlyken saw the PR. thank you! let me know if you're able to complete CLA or not.

genlyken commented 1 year ago

@stcheng I think there's an issue with my CLA where i mixed up my github accounts. Paul from meta messaged me, but I think it takes a while to clean this up.

stcheng commented 1 year ago

@genlyken any luck getting the CLA? if not, do you want me to get it addressed on behalf of you?

genlyken commented 1 year ago

@stcheng I haven't had any change in status on the CLA. I'd appreciate any steps moving forward!

stcheng commented 1 year ago

closing in favor of a2d64d93485f353ad6a3e78709c7d7fc858dcbc2