net/url: no Parse error with invalid port in IPv4 addresses (revert)

[0xmohit]

1. What version of Go are you using (go version)?

   go version go1.6 linux/amd64

2. What operating system and processor architecture are you using (go env)?

   GOARCH="amd64"
   GOBIN=""
   GOEXE=""
   GOHOSTARCH="amd64"
   GOHOSTOS="linux"
   GOOS="linux"
   GOPATH=""
   GORACE=""
   GOROOT="/usr/local/go"
   GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
   GO15VENDOREXPERIMENT="1"
   CC="gcc"
   GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
   CXX="g++"
   CGO_ENABLED="1"

3. What did you do?
   Passed URLs containing invalid port for IPv4 and IPv6 addresses. (Ran into this while digging into net/http: empty port for ":port" causes it to default to 0 #14836)
   Playground link: http://play.golang.org/p/EG38tnEGjn

4. What did you expect to see?
   Parse errors in both cases. Or consistent behavior.

5. What did you see instead?
   Parse error for IPv6 address; no error for IPv4 address.

[odeke-em]

I've taken a stab at this with CL https://go-review.googlesource.com/20903.

[gopherbot]

CL https://golang.org/cl/20903 mentions this issue.

[0xmohit]

It appears that an existing test would break if this were to be fixed so as to cause Parse error with invalid ports in IPv4 addresses and would possibly need to be changed to:

--- a/src/net/url/url_test.go
+++ b/src/net/url/url_test.go
@@ -418,10 +418,10 @@ var urltests = []URLTest{
        },
        // worst case host, still round trips
        {
-               "scheme://!$&'()*+,;=hello!:port/path",
+               "scheme://!$&'()*+,;=hello!:8080/path",
                &URL{
                        Scheme: "scheme",
-                       Host:   "!$&'()*+,;=hello!:port",
+                       Host:   "!$&'()*+,;=hello!:8080",
                        Path:   "/path",
                },
                "",

[gopherbot]

CL https://golang.org/cl/22351 mentions this issue.

[rsc]

This is wrong and should be reverted. It is perfectly fine to have non-numeric ports, except in the case of IPv6 literals because they are special in very many ways.

[bradfitz]

This is wrong and should be reverted.

@rsc, why is it wrong?

https://tools.ietf.org/html/rfc3986#section-3.2.3 says:

3.2.3.  Port

   The port subcomponent of authority is designated by an optional port
   number in decimal following the host and delimited from it by a
   single colon (":") character.

      port        = *DIGIT

   A scheme may define a default port.  For example, the "http" scheme
   defines a default port of "80", corresponding to its reserved TCP
   port number.  The type of port designated by the port number (e.g.,
   TCP, UDP, SCTP) is defined by the URI scheme.  URI producers and
   normalizers should omit the port component and its ":" delimiter if
   port is empty or if its value would be the same as that of the
   scheme's default.

except in the case of IPv6 literals because they are special in very many ways

What are those ways?

You think IPv4 and IPv6 should have different rules now how ports are parsed in Go? That seems very ... surprising.

[rsc]

In general package url does not impose structural restrictions on the authority part, because there might be things like fooscheme://whatever:baz/ at some point. The claim is that this new code is for "IPv4" but it's really for everything that's not IPv6, including plain names or other text. I've seen people using things like mysql://various(stuff:here) and that should continue working too. (We broke and reverted stuff like that once already.) It had a test (reverted in the CL) to make clear that this kind of thing was supported.

IPv6 literals are special in URLs: they introduce the special case that % can appear and does not mean the usual "introduce a %xx escape sequence", and so they need special handling. (See the big comment. The way IPv6 zone scopes were hacked into URLs is just awful.) But other structural checks are incompatible with the history here and should not be added. That's why the test was there.

Rejecting previously valid inputs is not something we should do lightly, and the CL that did it doesn't seem to understand the full scope of what the existing code did (the mistaken claims about "IPv4").

[bradfitz]

because there might be things like fooscheme://whatever:baz/ at some point

I doubt it, since that's against the RFC.

It had a test (reverted in the CL) to make clear that this kind of thing was supported.

I did see it was reverted, but the test didn't exactly make it clear what it was caring about. All it said was:

// worst case host, still round trips

... with no reference to a bug giving any backstory.

git blame points to e8be9a1 which says:

This allows mysql://a,b,c/path to continue to parse, as it did in Go 1.4 and earlier.

So if this is about breaking mysql, I don't think we break mysql with this CL, since that example didn't have an invalid port?

[rsc]

The fact is, there are many Go programs (mysql was just one public example) that use URLs that do not strictly conform to the host:numbers form. I really don't want to start breaking these programs in Go 1.7. There's no good reason to do so. Revert in https://go-review.googlesource.com/#/c/22861.

[rsc]

I explained above why IPv6 is special: it's a special case in the RFCs too.
This is now working as intended, I believe.