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

[pac-resolver] Remove ip dependency #281

Conversation

viktor-urbanas-qatalog
Copy link
Contributor

@viktor-urbanas-qatalog viktor-urbanas-qatalog commented Feb 9, 2024

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: #280

Copy link

changeset-bot bot commented Feb 9, 2024

🦋 Changeset detected

Latest commit: f1b4210

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

Copy link

vercel bot commented Feb 9, 2024

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

@viktor-urbanas-qatalog viktor-urbanas-qatalog force-pushed the fix/proxy-agent/ditch-ip-package branch from b421a87 to d95af6f Compare February 9, 2024 22:19
@viktor-urbanas-qatalog viktor-urbanas-qatalog changed the title fix(proxy-agent): ditch ip npm dependency fix(pac-resolver): ditch ip npm dependency Feb 9, 2024
Copy link

@lyricnz lyricnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

@dawidurbanski
Copy link

dawidurbanski commented Feb 10, 2024

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
Copy link

G-Rath commented Feb 10, 2024

I've created github/advisory-database#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
Copy link

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

@viktor-urbanas-qatalog viktor-urbanas-qatalog changed the title fix(pac-resolver): ditch ip npm dependency fix: ditch ip npm dependency Feb 12, 2024
@viktor-urbanas-qatalog viktor-urbanas-qatalog force-pushed the fix/proxy-agent/ditch-ip-package branch from be0d8d9 to f1b4210 Compare February 12, 2024 17:29
Copy link

@rodoabad rodoabad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it.

@TooTallNate TooTallNate changed the title fix: ditch ip npm dependency [pac-resolver] Remove ip dependency Feb 12, 2024
@TooTallNate TooTallNate merged commit a954da3 into TooTallNate:main Feb 12, 2024
29 checks passed
@rodoabad
Copy link

Thanks, Nate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulnerability for ip package in pac-resolver
8 participants