Skip to content

Conversation

@myleshorton
Copy link
Collaborator

@myleshorton myleshorton commented Oct 21, 2025

This pull request refactors how local/private IP addresses are detected in the BlockLocal filter, simplifying the code and removing an external dependency. The main change is the replacement of the custom iptool library with Go's built-in IP address methods, resulting in cleaner and more maintainable code.

Dependency management:

  • Removed github.com/getlantern/iptool as a direct dependency from the require block in go.mod.
  • Update github.com/getlantern/netx as a direct dependency

Code simplification and refactoring:

  • Removed the import of iptool from proxyfilters/blocklocal.go, as it is no longer needed.
  • Removed initialization of the iptool object in the BlockLocal function.
  • Replaced the use of ipt.IsPrivate(ipAddr) with Go's native !ipAddr.IP.IsGlobalUnicast() to check for non-global (local/private) IP addresses, simplifying the logic and leveraging standard library functionality.

@myleshorton myleshorton requested a review from Copilot October 21, 2025 22:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the BlockLocal filter to use Go's native IP address methods instead of the external iptool library, simplifying the codebase and reducing dependencies. The change replaces custom IP privacy detection logic with the standard library's IsGlobalUnicast() method.

Key changes:

  • Removed direct dependency on github.com/getlantern/iptool
  • Replaced ipt.IsPrivate(ipAddr) with !ipAddr.IP.IsGlobalUnicast() for detecting local/private addresses
  • Updated several indirect dependency versions in go.mod

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
proxyfilters/blocklocal.go Removed iptool import and replaced IsPrivate() call with native IsGlobalUnicast() method
go.mod Moved iptool from direct to indirect dependencies and updated version hashes for related packages

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// in the form host or host:port
if err == nil {
if ipt.IsPrivate(ipAddr) {
if !ipAddr.IP.IsGlobalUnicast() {
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

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

Using !IsGlobalUnicast() is not semantically equivalent to IsPrivate(). IsGlobalUnicast() returns false for unspecified addresses (0.0.0.0), loopback addresses, multicast addresses, and link-local addresses, but it also returns false for IPv4 broadcast addresses and other special-purpose addresses that may not be considered 'private' in the traditional RFC 1918 sense. This could inadvertently block legitimate traffic or allow traffic that should be blocked. Consider using IsPrivate() || IsLoopback() || IsLinkLocalUnicast() || IsLinkLocalMulticast() for more precise control.

Suggested change
if !ipAddr.IP.IsGlobalUnicast() {
if ipAddr.IP.IsPrivate() || ipAddr.IP.IsLoopback() || ipAddr.IP.IsLinkLocalUnicast() || ipAddr.IP.IsLinkLocalMulticast() {

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we want to allow broadcast addresses either, or multicast. Seems like a potential for amplification attacks.

@Crosse
Copy link
Contributor

Crosse commented Oct 21, 2025

I think we need IsPrivate() as well. tests

@myleshorton
Copy link
Collaborator Author

Oh very interesting. I didn't realize that some private addresses could also be considered global unicast -- I guess I always thought of the "global" part as public! Good catch.

@myleshorton myleshorton merged commit 71e7bd7 into main Oct 22, 2025
1 check passed
AGMETEOR added a commit that referenced this pull request Oct 23, 2025
AGMETEOR added a commit that referenced this pull request Oct 23, 2025
* Revert "Only forward traffic to global unicast (#660)"

This reverts commit 71e7bd7.
@Crosse Crosse deleted the myles/filter-non-public branch October 24, 2025 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants