TooTallNate / proxy-agents

Node.js HTTP Proxy Agents Monorepo
https://proxy-agents.n8.io
871 stars 229 forks source link

[pac-resolver] Remove `ip` dependency #281

Closed viktor-urbanas-qatalog closed 4 months ago

viktor-urbanas-qatalog commented 4 months ago

There is a high severity vulnerability in the https://github.com/indutny/node-ip package

Unfortunately this package was updated long time ago and seems to be dead

This PR aims to fix the aforementioned issue by getting rid of the node-ip dependency in favour of using copied parts of code which are used in this lib

Fixes: https://github.com/TooTallNate/proxy-agents/issues/280

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: f1b42101177e0df8f7ee50c02e2cf2250c3f875a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------ | ----- | | pac-resolver | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
proxy-agents ✅ Ready (Inspect) Visit Preview Feb 12, 2024 8:57am
dawidurbanski commented 4 months ago

It's great to see this change. Should be ok since only affected functions are ip.isPrivate and ip.isPublic and this package is not relying on them at all.

But, the whole proxy-agent will still depend on the ip package through socks-proxy-agent:

│ └─┬ socks-proxy-agent 8.0.2
          │   └─┬ socks 2.7.1
          │     └── ip 2.0.0

The alert states that all versions affected are <= 1.1.8, and that there are no patched versions while the newest version is 2.0.0.

This means technically 2.0.0 is still vulnerable, but it won't trigger the advisory audit issue.

Since we know this package does not use any of the affected functions (isPublic or isPrivate), we could just bump ip package to 2.0.0, to get rid of the warning instead of moving the code into this repo.

G-Rath commented 4 months ago

I've created https://github.com/github/advisory-database/pull/3504 updating the advisory to include v2, and Josh has said they'll get it removed from socks.

This PR should be fine to land in parallel since it's a separate change, reduced the advisory count overall, and hopefully the socks fix will be non-breaking meaning no further changes will be needed for these packages (people will just be able to npm update socks).

wvanderdeijl commented 4 months ago

socks has released version 2.7.3 that removes the ip dependency: https://github.com/JoshGlazebrook/socks/releases/tag/2.7.3

rodoabad commented 4 months ago

Thanks, Nate!