net/url: mysql://signer@tcp(mysql:3306)/notarysigner no longer parses after 1.12.8

[thaJeztah]

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

$ go version
1.12.8 or 1.11.13

Does this issue reproduce with the latest release?

Yes

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

Not relevant

What did you do?

Updated to Go 1.12.8

What did you expect to see?

CI to pass after updating to Go 1.12.8 😅 notaryproject/notary#1485

What did you see instead?

error: parse mysql://signer@tcp(mysql:3306)/notarysigner: invalid port ":3306)" after host

This looks a similar regression as #12023

t.b.h., I don't know if such DSN's should be parseable by net/url (#12023 (comment)), but it's a change in behaviour, so I thought I'd report it

Reproducer: https://play.golang.org/p/cPiVWDRZ3G4

Looks like it only fails if there's a port specified:

package main

import (
	"fmt"
	"net/url"
)

func main() {
	_, err := url.Parse("mysql://boulder@tcp(localhost)/boulder_test?parseTime=true")
	if err != nil {
		fmt.Println(err)
	}
	_, err = url.Parse("mysql://boulder@tcp(localhost:3306)/boulder_test?parseTime=true")
	if err != nil {
		fmt.Println(err)
	}
}

Produces:

parse mysql://boulder@tcp(localhost:3306)/boulder_test?parseTime=true: invalid port ":3306)" after host

[thaJeztah]

/cc @rsc @mikioh

[tmthrgd]

This was caused by CL 189258 fixing #29098.

There’s actually an existing test covering exactly this case (as part of TestParseErrors), but it’s not actually testing anything. In particular, this line is only logging errors and not failing the test—it should be t.Errorf.

The change was intentional, but this might have been overlooked. The review comments on the CL indicate an okay-ness with breaking here.

/cc @FiloSottile

[thaJeztah]

In particular, this line is only logging errors and not failing the test—it should be t.Errorf.

That's "interesting" 😅

Thanks; yes, reading the old discussion #12023 (comment), it seems like parsing DSN's with net/url is just bad use (I see the issue I was running into is being worked on; golang-migrate/migrate#265)

Is this something to be called out in the release notes as a breaking change?

(Feel free to close this one if this was intentional, and the test is ok as-is; just thought it was good to bring it up here in case it was not intentional)

[FiloSottile]

Thanks for reporting this and sorry for the disruption. Indeed, this is unfortunate but I'm afraid necessary. DSNs like that are not well-formed URIs per RFC 3986, which is what net/url targets. All things being equal we will prefer backwards compatibility to standard compliance, but it became clear that allowing malformed ports was having a security impact on applications that assumed RFC 3986 compliance, so unless we break the world here we should stay on the stricter behavior.

I mentioned this change in the golang-announce email, and had I noticed the test failure I would have opened an issue with github.com/go-sql-driver/mysql in advance. If somebody on this issue would like to open an issue there, I'd appreciate it, otherwise I'll look into which of their APIs are affected and let them know this evening.

I will leave this issue open to fix the test.

[FiloSottile]

The good news is that github.com/go-sql-driver/mysql itself does not use net/url.Parse, so is unaffected. If other tools like github.com/golang-migrate/migrate try to parse the DSN before passing it to github.com/go-sql-driver/mysql they will fail though.

[ernado]

The result of parsing STUN URI with invalid port like stun:example.org:someString was changed too.
Relying on

u.Host = u.Opaque
host, rawPort := u.Hostname(), u.Port()

was my mistake. Now I'm getting example.org:port as a hostname.

[gopherbot]

Change https://golang.org/cl/191966 mentions this issue: net/url: fail TestParseErrors test when getting an unwanted error

[thaJeztah]

Oh! Thought I left a reply, but apparently forgot 😓

Thanks for reporting this and sorry for the disruption. Indeed, this is unfortunate but I'm afraid necessary. DSNs like that are not well-formed URIs per RFC 3986, which is what net/url targets.

👍 Thanks for confirming that; I did a cursory look at the RFC, but one quickly goes down the rabbit hole when reading those (could've missed an errata), so I wasn't 100% sure. 🤗,

If somebody on this issue would like to open an issue there, I'd appreciate it, otherwise I'll look into which of their APIs are affected and let them know this evening.

I opened a ticket in that repository, and it was fixed there, so all is well 👍

I will leave this issue open to fix the test.

👍

Thanks!

[stefanb]

#33876 fixes the URL parsing tests to detect this issue, introduced by 61bb56a, a partial fix for #29098.