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

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

Closed
0xmohit opened this issue Mar 18, 2016 · 10 comments
Closed

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

0xmohit opened this issue Mar 18, 2016 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@0xmohit
Copy link
Contributor

0xmohit commented Mar 18, 2016

  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.

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 18, 2016
@odeke-em
Copy link
Member

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

@gopherbot
Copy link

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

@0xmohit
Copy link
Contributor Author

0xmohit commented Apr 20, 2016

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

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

@rsc
Copy link
Contributor

rsc commented Apr 27, 2016

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.

@rsc rsc reopened this Apr 27, 2016
@rsc rsc changed the title net/url: no Parse error with invalid port in IPv4 addresses net/url: no Parse error with invalid port in IPv4 addresses (revert) Apr 27, 2016
@bradfitz
Copy link
Contributor

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

rsc commented Apr 27, 2016

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

bradfitz commented Apr 27, 2016

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

rsc commented May 6, 2016

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.

gopherbot pushed a commit that referenced this issue May 6, 2016
This reverts commit 9f1ccd6.

For #14860.

Change-Id: I63522a4dda8915dc8b972ae2e12495553ed65f09
Reviewed-on: https://go-review.googlesource.com/22861
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7 May 16, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 20, 2016

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

@rsc rsc closed this as completed Oct 20, 2016
@golang golang locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants