Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #265

erikdubbelboer · 2019-08-14T12:39:22Z

Change schemeFromURL to just split the url by :// to find the scheme.
It's not required to parse the whole URL. MySQL DSNs aren't valid URLs.

Fixes #264

Change schemeFromURL to just split the url by :// to find the scheme. It's not required to parse the whole URL. MySQL DSNs aren't valid URLs. Fixes golang-migrate#264

thaJeztah

LGTM

dhui

Thanks for the PR!

database/mysql/mysql.go

util.go

dhui · 2019-08-16T05:50:32Z

util_test.go

@@ -86,6 +86,19 @@ func TestDatabaseSchemeFromUrlSuccess(t *testing.T) {
 	}
 }

+func TestDatabaseSchemeFromUrlIssue264(t *testing.T) {


Thanks for adding this test!

It's cleaner if you could refactor TestDatabaseSchemeFromUrlSuccess to be table driven.

Also, give this test a better name. e.g. parse MySQL DSN
I think it's fine to reference the issue (use a URL) in a comment though

dhui · 2019-08-16T05:55:00Z

database/parse_test.go

@@ -139,8 +139,7 @@ func TestPasswordUnencodedReservedURLChars(t *testing.T) {
 	}{
 		{char: "!", parses: true, expectedUsername: username, expectedPassword: basePassword + "!",
 			encodedURL: schemeAndUsernameAndSep + basePassword + "%21" + urlSuffixAndSep},
-		{char: "#", parses: true, expectedUsername: "", expectedPassword: "",
-			encodedURL: schemeAndUsernameAndSep + basePassword + "#" + urlSuffixAndSep},
+		{char: "#", parses: false},


Why did these tests break? My understanding is that the hostname/port parsing was fixed. I don't see any changes to parseAuthority() in the fix: golang/go@3226f2d

I don't see where it changed either but it did: https://play.golang.org/p/gQLxfWBQnQD

Looks like these characters needed to be percent-encoded in the first place.

ABNF for authority part of an URI:

authority = [ userinfo "@" ] host [ ":" port ] userinfo = *( unreserved / pct-encoded / sub-delims / ":" ) unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~" pct-encoded = "%" HEXDIG HEXDIG sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

https://tools.ietf.org/html/rfc3986#appendix-A

dhui · 2019-08-16T06:01:11Z

database/mysql/mysql.go


-	migrationsTable := purl.Query().Get("x-migrations-table")
+	migrationsTable := config.Params["x-migrations-table"]


Using config.Params works for now, but because we're no longer pre-processing the DSN, if for some reason a x-* parameter collides with a go-sql-driver/mysql param, then we'll lose it. This is probably fine for now, but something to be wary of.

database/mysql/mysql_test.go

erikdubbelboer · 2019-08-16T10:53:53Z

@dhui I have fixed your suggestions, please check again.

dhui

Thanks for addressing the issues I raised!

dhui · 2019-08-17T05:48:14Z

util.go

-	if len(u.Scheme) == 0 {
+	i := strings.Index(url, ":")
+
+	// No : or : is the first character.


Thanks for the comment!

dhui · 2019-08-17T06:07:23Z

database/parse_test.go

@@ -139,8 +139,7 @@ func TestPasswordUnencodedReservedURLChars(t *testing.T) {
 	}{
 		{char: "!", parses: true, expectedUsername: username, expectedPassword: basePassword + "!",
 			encodedURL: schemeAndUsernameAndSep + basePassword + "%21" + urlSuffixAndSep},
-		{char: "#", parses: true, expectedUsername: "", expectedPassword: "",
-			encodedURL: schemeAndUsernameAndSep + basePassword + "#" + urlSuffixAndSep},
+		{char: "#", parses: false},


Looks like these characters needed to be percent-encoded in the first place.

ABNF for authority part of an URI:

authority = [ userinfo "@" ] host [ ":" port ] userinfo = *( unreserved / pct-encoded / sub-delims / ":" ) unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~" pct-encoded = "%" HEXDIG HEXDIG sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

https://tools.ietf.org/html/rfc3986#appendix-A

erikdubbelboer · 2019-08-17T07:21:31Z

@dhui can you please tag a new 4.5.1 release so people can upgrade their dependencies without having to change them to master?

thaJeztah · 2019-08-17T13:53:05Z

@erikdubbelboer see #264 (comment)

(trying to address that through #270)

dhui · 2019-08-17T18:08:20Z

can you please tag a new 4.5.1 release so people can upgrade their dependencies without having to change them to master?

No, not while the builds are failing. We only create releases with passing builds. In the meanwhile, you can use the master branch or an older version of Go to work around the issue.
Normally, we only merge in PRs if builds are passing, but are making an exception in this case due to the widespread breakage.

lou-lan · 2019-08-18T08:14:46Z

// Open returns a new driver instance.
func Open(url string) (Driver, error) {
	u, err := nurl.Parse(url)
	if err != nil {
		return nil, fmt.Errorf("Unable to parse URL. Did you escape all reserved URL characters? "+
			"See: https://github.com/golang-migrate/migrate#database-urls Error: %v", err)
	}
        ...
}

In master branch, golang-migrate/migrate/v4/database/driver.go file used nurl "net/url" package

Moved schemeFromURL into the database package. Also removed databaseSchemeFromURL and sourceSchemeFromURL as they were just calling schemeFromURL. Fixes golang-migrate#265 (comment)

Otherwise it will fail on MySQL DSNs. Moved schemeFromURL into the database package. Also removed databaseSchemeFromURL and sourceSchemeFromURL as they were just calling schemeFromURL. Fixes golang-migrate#265 (comment)

erikdubbelboer · 2019-08-18T09:53:39Z

@lou-lan you are right, I just created a pull request for this: #271

* Let database.Open() use schemeFromURL as well Otherwise it will fail on MySQL DSNs. Moved schemeFromURL into the database package. Also removed databaseSchemeFromURL and sourceSchemeFromURL as they were just calling schemeFromURL. Fixes #265 (comment) * Moved url functions into internal/url Also merged the test cases. * Add some database tests to improve coverage * Fix suggestions

* Let database.Open() use schemeFromURL as well Otherwise it will fail on MySQL DSNs. Moved schemeFromURL into the database package. Also removed databaseSchemeFromURL and sourceSchemeFromURL as they were just calling schemeFromURL. Fixes golang-migrate/migrate#265 (comment) * Moved url functions into internal/url Also merged the test cases. * Add some database tests to improve coverage * Fix suggestions

Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql

b5f8430

Change schemeFromURL to just split the url by :// to find the scheme. It's not required to parse the whole URL. MySQL DSNs aren't valid URLs. Fixes golang-migrate#264

justincormack previously approved these changes Aug 14, 2019

View reviewed changes

justincormack mentioned this pull request Aug 14, 2019

Bump golang 1.12.10 (CVE-2019-9512, CVE-2019-9514, CVE-2019-16276) notaryproject/notary#1485

Merged

thaJeztah previously approved these changes Aug 14, 2019

View reviewed changes

koenbollen previously approved these changes Aug 14, 2019

View reviewed changes

The mysql driver itself also used net/url.Parse

2e39eb9

erikdubbelboer dismissed stale reviews from koenbollen, thaJeztah, and justincormack via 2e39eb9 August 14, 2019 14:44

thaJeztah mentioned this pull request Aug 14, 2019

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

Closed

erikdubbelboer added 2 commits August 14, 2019 23:33

Also fix TestPasswordUnencodedReservedURLChars

4b604b8

Keep backwards compatibility with url encoded username and passwords

62106bd

zikaeroh mentioned this pull request Aug 15, 2019

Update MongoDB driver to 1.1.0, use correct import path #267

Merged

dhui requested changes Aug 16, 2019

View reviewed changes

Fix suggestions

da4d8b3

Reuse old function names

867c399

dhui approved these changes Aug 17, 2019

View reviewed changes

dhui merged commit eb7d0dd into golang-migrate:master Aug 17, 2019

erikdubbelboer mentioned this pull request Aug 18, 2019

Let database.Open() use schemeFromURL as well #271

Merged

jboalml mentioned this pull request Jan 4, 2020

Cannot connect to MySQL ory/keto#145

Closed

renovate bot mentioned this pull request May 29, 2021

fix(deps): update module github.com/golang-migrate/migrate to v4 - autoclosed advenjourney/advenjourney#20

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #265

Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #265

erikdubbelboer commented Aug 14, 2019

thaJeztah left a comment

dhui left a comment

dhui Aug 16, 2019

dhui Aug 16, 2019

erikdubbelboer Aug 16, 2019

dhui Aug 17, 2019

dhui Aug 16, 2019

erikdubbelboer commented Aug 16, 2019

dhui left a comment

dhui Aug 17, 2019

dhui Aug 17, 2019

erikdubbelboer commented Aug 17, 2019

thaJeztah commented Aug 17, 2019

dhui commented Aug 17, 2019

lou-lan commented Aug 18, 2019

erikdubbelboer commented Aug 18, 2019


		migrationsTable := purl.Query().Get("x-migrations-table")
		migrationsTable := config.Params["x-migrations-table"]

Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #265

Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql #265

Conversation

erikdubbelboer commented Aug 14, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

dhui Aug 16, 2019

Choose a reason for hiding this comment

dhui Aug 16, 2019

Choose a reason for hiding this comment

erikdubbelboer Aug 16, 2019

Choose a reason for hiding this comment

dhui Aug 17, 2019

Choose a reason for hiding this comment

dhui Aug 16, 2019

Choose a reason for hiding this comment

erikdubbelboer commented Aug 16, 2019

dhui left a comment

Choose a reason for hiding this comment

dhui Aug 17, 2019

Choose a reason for hiding this comment

dhui Aug 17, 2019

Choose a reason for hiding this comment

erikdubbelboer commented Aug 17, 2019

thaJeztah commented Aug 17, 2019

dhui commented Aug 17, 2019

lou-lan commented Aug 18, 2019

erikdubbelboer commented Aug 18, 2019