Incorrect handling of IPv6 addresses in some configurations

[benguild]

Hi, I noticed that this library (along with others) assumes that the port number is always provided in http.Request.RemoteAddr

... Because this is an incorrect assumption, this breaks IPv6 on such installations where the port number is NOT provided: https://play.golang.org/p/oFZhZ6BCf3

Code to demonstrate issue

package main

import (
	"fmt"
	"strings"
)

func ipAddrFromRemoteAddr(s string) string {
	idx := strings.LastIndex(s, ":")
	if idx == -1 {
		return s
	}
	return s[:idx]
}

func main() {
	fmt.Println("OK:", ipAddrFromRemoteAddr("127.0.0.1"))
	fmt.Println("MISSING FINAL BLOCK:", ipAddrFromRemoteAddr("fe80:0000:0000:0000:34cb:9850:4868:9d2c"))
	fmt.Println("MISSING FINAL BLOCK:", ipAddrFromRemoteAddr("fe80::34cb:9850:4868:9d2c"))

}

Result:

OK: 127.0.0.1
MISSING FINAL BLOCK: fe80:0000:0000:0000:34cb:9850:4868
MISSING FINAL BLOCK: fe80::34cb:9850:4868

It's probably necessary to validate the IPv6 address first in order to detect if the port is not there. There is actually a pretty bad bug since a lot of App Engine deployments might be using this as-is and Google has no intention of changing this "portless" behavior.

[benguild]

An alternative solution (besides commenting out that method to strip out what's after the colon) ... might be to allow the user to provide a BOOL variable in the configuration to say the port is not included.

[benguild]

The fix in f2c85b5 isn't going to work: https://play.golang.org/p/ugnk527u-b

As seen here, net.SplitHostPort will return an error if it's passed a "RemoteAddr" without a port that's IPv6:

address 2001:0db8:0000:0042:0000:8a2e:0370:7334: too many colons in address

[benguild]

I'm a bit surprised that you decided to ignore the error from that method, as well. That's a really bad idea.

[didip]

What about this?

// 1. Cover the basic use cases for both ipv4 and ipv6
ip, _, err := net.SplitHostPort(r.RemoteAddr)
if err != nil {
	// 2. Upon error, just return the remote addr.
	return r.RemoteAddr
}
return ip

This will cover ipv4, ipv6 with port, and ipv6 without port.

[benguild]

Looking at the code for the method: https://golang.org/src/net/ipsock.go?s=4746:4812#L145 ... It appears that might work.

Although it seems a bit delicate considering that new errors could be introduced, currently the two "unexpected" errors I see at first glance are:

• missingPort = "missing port in address"
• tooManyColons = "too many colons in address"

... Those seem to account for the two cases that the port is missing from either address format?

[benguild]

For comparison, here's how Gin handles this on App Engine and the like: https://github.com/gin-gonic/gin/blob/9a4ecc87d6f8272b8e2450f9c0ab12d3e814521f/context.go#L514

[benguild]

... Would it be possible to just pass in the IP somehow externally?

[didip]

I can cover the app engine as well.

If you want to pass in IP, you can already do so by using X-Real-IP

[benguild]

I mean as a struct parameter.

[didip]

Do you always expect the same ip addresses?

[benguild]

No, just thinking aloud about ideas. For example, if there is already code that the user has to get the IP for a request based on their configuration, it would be helpful to override.

[didip]

I see, a function hook seems like a decent idea to let user pick up IP address however they want.

I need to think about it a bit more. In the mean time, I pushed the ipv6 change under v3.0.2.

[benguild]

Created: didip/tollbooth_gin#4