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

thaJeztah · 2019-08-14T11:18:16Z

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

The text was updated successfully, but these errors were encountered:

thaJeztah · 2019-08-14T11:20:17Z

/cc @rsc @mikioh

tmthrgd · 2019-08-14T12:25:29Z

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 · 2019-08-14T16:46:40Z

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 · 2019-08-14T17:51:28Z

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 · 2019-08-14T20:08:02Z

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 · 2019-08-14T22:56:30Z

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.

The TestParseErrors test function was not strict with unwanted errors received from url.Parse(). It was not failing in such cases, now it does. Updates golang#33646 and golang#29098

gopherbot · 2019-08-27T18:12:06Z

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

thaJeztah · 2019-08-27T18:20:14Z

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 · 2019-08-27T18:35:45Z

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

The TestParseErrors test function was not strict with unwanted errors received from url.Parse(). It was not failing in such cases, now it does Fixes golang#33646 Updates golang#29098 Change-Id: I069521093e2bff8b1fcd41ffd3f9799f3108bc61 GitHub-Last-Rev: e6844c5 GitHub-Pull-Request: golang#33876 Reviewed-on: https://go-review.googlesource.com/c/go/+/191966 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>

agnivade changed the title ~~Go 1.11.8/1.12.13 regression: net/url: mysql://signer@tcp(mysql:3306)/notarysigner no longer parses~~ net/url: mysql://signer@tcp(mysql:3306)/notarysigner no longer parses after 1.12.8 Aug 14, 2019

agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 14, 2019

FiloSottile self-assigned this Aug 14, 2019

FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 14, 2019

FiloSottile added this to the Go1.14 milestone Aug 14, 2019

gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 14, 2019

thaJeztah mentioned this issue Aug 14, 2019

Incompatibility in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql golang-migrate/migrate#264

Closed

mmailhos mentioned this issue Aug 26, 2019

url.URL does not support DSN format anymore amacneil/dbmate#86

Closed

stefanb mentioned this issue Aug 27, 2019

net/url: fail TestParseErrors test when getting an unwanted error #33876

Closed

stefanb added a commit to stefanb/go that referenced this issue Aug 27, 2019

Adjusted test expectations as per golang#33646

53beeb5

gopherbot closed this as completed in cc6feab Aug 27, 2019

gravis mentioned this issue Sep 28, 2019

handle SSLOption. db-journey/cassandra-driver#10

Merged

wayjam mentioned this issue Nov 6, 2019

Fix in url parser after go 1.12.8 phogolabs/prana#2

Merged

5 tasks

This was referenced Feb 29, 2020

fix(sqlcon): Parse DSN manually instead of using url.Parse ory/x#119

Merged

fix(driver): Extract scheme from DSN using sqlcon.GetDriverName ory/keto#156

Merged

golang locked and limited conversation to collaborators Aug 26, 2020

gopherbot added the FrozenDueToAge label Aug 26, 2020

rsc unassigned FiloSottile Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thaJeztah commented Aug 14, 2019 •

edited

thaJeztah commented Aug 14, 2019

tmthrgd commented Aug 14, 2019

thaJeztah commented Aug 14, 2019

FiloSottile commented Aug 14, 2019

FiloSottile commented Aug 14, 2019

ernado commented Aug 14, 2019 •

edited

gopherbot commented Aug 27, 2019

thaJeztah commented Aug 27, 2019

stefanb commented Aug 27, 2019 •

edited

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

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

Comments

thaJeztah commented Aug 14, 2019 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

thaJeztah commented Aug 14, 2019

tmthrgd commented Aug 14, 2019

thaJeztah commented Aug 14, 2019

FiloSottile commented Aug 14, 2019

FiloSottile commented Aug 14, 2019

ernado commented Aug 14, 2019 • edited

gopherbot commented Aug 27, 2019

thaJeztah commented Aug 27, 2019

stefanb commented Aug 27, 2019 • edited

thaJeztah commented Aug 14, 2019 •

edited

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

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

ernado commented Aug 14, 2019 •

edited

stefanb commented Aug 27, 2019 •

edited