From b5f84307e892a3b0f6b3fe259347e48cc3146d41 Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Wed, 14 Aug 2019 14:35:03 +0200 Subject: [PATCH 1/6] Fix in the URL parser with go 1.12.8 and github.com/go-sql-driver/mysql 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 --- util.go | 10 ++++------ util_test.go | 13 +++++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/util.go b/util.go index 1cef03cd8..50fd1993f 100644 --- a/util.go +++ b/util.go @@ -74,15 +74,13 @@ func schemeFromURL(url string) (string, error) { return "", errEmptyURL } - u, err := nurl.Parse(url) - if err != nil { - return "", err - } - if len(u.Scheme) == 0 { + parts := strings.SplitN(url, "://", 2) + + if len(parts) < 2 || parts[0] == "" { return "", errNoScheme } - return u.Scheme, nil + return parts[0], nil } // FilterCustomQuery filters all query values starting with `x-` diff --git a/util_test.go b/util_test.go index 6543b28f8..97a0b8491 100644 --- a/util_test.go +++ b/util_test.go @@ -86,6 +86,19 @@ func TestDatabaseSchemeFromUrlSuccess(t *testing.T) { } } +func TestDatabaseSchemeFromUrlIssue264(t *testing.T) { + urlStr := "mysql://user:pass@tcp(host:1337)/db" + expected := "mysql" + + u, err := databaseSchemeFromURL(urlStr) + if err != nil { + t.Fatalf("expected no error, but received %q", err) + } + if u != expected { + t.Fatalf("expected %q, but received %q", expected, u) + } +} + func TestDatabaseSchemeFromUrlFailure(t *testing.T) { cases := []struct { name string From 2e39eb96ebfd25f0e5849e17d0cf9dad942b0ce9 Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Wed, 14 Aug 2019 16:44:18 +0200 Subject: [PATCH 2/6] The mysql driver itself also used net/url.Parse --- database/mysql/mysql.go | 47 +++++++------------------------- database/mysql/mysql_test.go | 53 ------------------------------------ 2 files changed, 10 insertions(+), 90 deletions(-) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index 96cb600c9..fdbb842e4 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -10,7 +10,6 @@ import ( "fmt" "io" "io/ioutil" - nurl "net/url" "strconv" "strings" ) @@ -21,7 +20,6 @@ import ( ) import ( - "github.com/golang-migrate/migrate/v4" "github.com/golang-migrate/migrate/v4/database" ) @@ -98,43 +96,22 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { return mx, nil } -// urlToMySQLConfig takes a net/url URL and returns a go-sql-driver/mysql Config. -// Manually sets username and password to avoid net/url from url-encoding the reserved URL characters -func urlToMySQLConfig(u nurl.URL) (*mysql.Config, error) { - origUserInfo := u.User - u.User = nil - - c, err := mysql.ParseDSN(strings.TrimPrefix(u.String(), "mysql://")) - if err != nil { - return nil, err - } - if origUserInfo != nil { - c.User = origUserInfo.Username() - if p, ok := origUserInfo.Password(); ok { - c.Passwd = p - } - } - return c, nil -} - func (m *Mysql) Open(url string) (database.Driver, error) { - purl, err := nurl.Parse(url) + config, err := mysql.ParseDSN(strings.TrimPrefix(url, "mysql://")) if err != nil { return nil, err } - q := purl.Query() - q.Set("multiStatements", "true") - purl.RawQuery = q.Encode() + config.MultiStatements = true - migrationsTable := purl.Query().Get("x-migrations-table") + migrationsTable := config.Params["x-migrations-table"] // use custom TLS? - ctls := purl.Query().Get("tls") + ctls := config.TLSConfig if len(ctls) > 0 { if _, isBool := readBool(ctls); !isBool && strings.ToLower(ctls) != "skip-verify" { rootCertPool := x509.NewCertPool() - pem, err := ioutil.ReadFile(purl.Query().Get("x-tls-ca")) + pem, err := ioutil.ReadFile(config.Params["x-tls-ca"]) if err != nil { return nil, err } @@ -144,7 +121,7 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } clientCert := make([]tls.Certificate, 0, 1) - if ccert, ckey := purl.Query().Get("x-tls-cert"), purl.Query().Get("x-tls-key"); ccert != "" || ckey != "" { + if ccert, ckey := config.Params["x-tls-cert"], config.Params["x-tls-key"]; ccert != "" || ckey != "" { if ccert == "" || ckey == "" { return nil, ErrTLSCertKeyConfig } @@ -156,8 +133,8 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } insecureSkipVerify := false - if len(purl.Query().Get("x-tls-insecure-skip-verify")) > 0 { - x, err := strconv.ParseBool(purl.Query().Get("x-tls-insecure-skip-verify")) + if len(config.Params["x-tls-insecure-skip-verify"]) > 0 { + x, err := strconv.ParseBool(config.Params["x-tls-insecure-skip-verify"]) if err != nil { return nil, err } @@ -175,17 +152,13 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } } - c, err := urlToMySQLConfig(*migrate.FilterCustomQuery(purl)) - if err != nil { - return nil, err - } - db, err := sql.Open("mysql", c.FormatDSN()) + db, err := sql.Open("mysql", config.FormatDSN()) if err != nil { return nil, err } mx, err := WithInstance(db, &Config{ - DatabaseName: purl.Path, + DatabaseName: config.DBName, MigrationsTable: migrationsTable, }) if err != nil { diff --git a/database/mysql/mysql_test.go b/database/mysql/mysql_test.go index e5f73ff2a..d8e7eca64 100644 --- a/database/mysql/mysql_test.go +++ b/database/mysql/mysql_test.go @@ -8,7 +8,6 @@ import ( "log" "github.com/golang-migrate/migrate/v4" - "net/url" "testing" ) @@ -175,55 +174,3 @@ func TestLockWorks(t *testing.T) { } }) } - -func TestURLToMySQLConfig(t *testing.T) { - testcases := []struct { - name string - urlStr string - expectedDSN string // empty string signifies that an error is expected - }{ - {name: "no user/password", urlStr: "mysql://tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "only user", urlStr: "mysql://username@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "only user - with encoded :", - urlStr: "mysql://username%3A@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username:@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "only user - with encoded @", - urlStr: "mysql://username%40@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username@@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "user/password", urlStr: "mysql://username:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - // Not supported yet: https://github.com/go-sql-driver/mysql/issues/591 - // {name: "user/password - user with encoded :", - // urlStr: "mysql://username%3A:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - // expectedDSN: "username::pasword@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "user/password - user with encoded @", - urlStr: "mysql://username%40:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username@:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "user/password - password with encoded :", - urlStr: "mysql://username:password%3A@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username:password:@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - {name: "user/password - password with encoded @", - urlStr: "mysql://username:password%40@tcp(127.0.0.1:3306)/myDB?multiStatements=true", - expectedDSN: "username:password@@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, - } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - u, err := url.Parse(tc.urlStr) - if err != nil { - t.Fatal("Failed to parse url string:", tc.urlStr, "error:", err) - } - if config, err := urlToMySQLConfig(*u); err == nil { - dsn := config.FormatDSN() - if dsn != tc.expectedDSN { - t.Error("Got unexpected DSN:", dsn, "!=", tc.expectedDSN) - } - } else { - if tc.expectedDSN != "" { - t.Error("Got unexpected error:", err, "urlStr:", tc.urlStr) - } - } - }) - } -} From 4b604b8da90d8ad0a64231ee1620f590fc3ed1da Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Wed, 14 Aug 2019 23:33:45 +0200 Subject: [PATCH 3/6] Also fix TestPasswordUnencodedReservedURLChars --- database/parse_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/database/parse_test.go b/database/parse_test.go index 6558e2528..3709a6796 100644 --- a/database/parse_test.go +++ b/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}, {char: "$", parses: true, expectedUsername: username, expectedPassword: basePassword + "$", encodedURL: schemeAndUsernameAndSep + basePassword + "$" + urlSuffixAndSep}, {char: "%", parses: false}, @@ -158,16 +157,14 @@ func TestPasswordUnencodedReservedURLChars(t *testing.T) { encodedURL: schemeAndUsernameAndSep + basePassword + "+" + urlSuffixAndSep}, {char: ",", parses: true, expectedUsername: username, expectedPassword: "password,", encodedURL: schemeAndUsernameAndSep + basePassword + "," + urlSuffixAndSep}, - {char: "/", parses: true, expectedUsername: "", expectedPassword: "", - encodedURL: schemeAndUsernameAndSep + basePassword + "/" + urlSuffixAndSep}, + {char: "/", parses: false}, {char: ":", parses: true, expectedUsername: username, expectedPassword: "password:", encodedURL: schemeAndUsernameAndSep + basePassword + "%3A" + urlSuffixAndSep}, {char: ";", parses: true, expectedUsername: username, expectedPassword: "password;", encodedURL: schemeAndUsernameAndSep + basePassword + ";" + urlSuffixAndSep}, {char: "=", parses: true, expectedUsername: username, expectedPassword: "password=", encodedURL: schemeAndUsernameAndSep + basePassword + "=" + urlSuffixAndSep}, - {char: "?", parses: true, expectedUsername: "", expectedPassword: "", - encodedURL: schemeAndUsernameAndSep + basePassword + "?" + urlSuffixAndSep}, + {char: "?", parses: false}, {char: "@", parses: true, expectedUsername: username, expectedPassword: "password@", encodedURL: schemeAndUsernameAndSep + basePassword + "%40" + urlSuffixAndSep}, {char: "[", parses: false}, From 62106bd8e77078efbca9fe8376cde0ab0b4f507d Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Wed, 14 Aug 2019 23:42:07 +0200 Subject: [PATCH 4/6] Keep backwards compatibility with url encoded username and passwords --- database/mysql/mysql.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index fdbb842e4..4c8e6a286 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "io/ioutil" + nurl "net/url" "strconv" "strings" ) @@ -104,6 +105,21 @@ func (m *Mysql) Open(url string) (database.Driver, error) { config.MultiStatements = true + // Keep backwards compatibility from when we used net/url.Parse() to parse the DSN. + // net/url.Parese() would automatically unescape it for us. + // See: https://play.golang.org/p/q9j1io-YICQ + user, err := nurl.QueryUnescape(config.User) + if err != nil { + return nil, err + } + config.User = user + + password, err := nurl.QueryUnescape(config.Passwd) + if err != nil { + return nil, err + } + config.Passwd = password + migrationsTable := config.Params["x-migrations-table"] // use custom TLS? From da4d8b3fa4cfe3109b54ae0734d0e500fa1dbe87 Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Fri, 16 Aug 2019 12:53:06 +0200 Subject: [PATCH 5/6] Fix suggestions --- database/mysql/mysql.go | 17 +++++++++---- database/mysql/mysql_test.go | 47 ++++++++++++++++++++++++++++++++++++ util.go | 7 +++--- util_test.go | 46 ++++++++++++++++++++--------------- 4 files changed, 89 insertions(+), 28 deletions(-) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index 4c8e6a286..b9f71bc7d 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -97,7 +97,7 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { return mx, nil } -func (m *Mysql) Open(url string) (database.Driver, error) { +func (m *Mysql) getConfig(url string) (*mysql.Config, error) { config, err := mysql.ParseDSN(strings.TrimPrefix(url, "mysql://")) if err != nil { return nil, err @@ -106,7 +106,7 @@ func (m *Mysql) Open(url string) (database.Driver, error) { config.MultiStatements = true // Keep backwards compatibility from when we used net/url.Parse() to parse the DSN. - // net/url.Parese() would automatically unescape it for us. + // net/url.Parse() would automatically unescape it for us. // See: https://play.golang.org/p/q9j1io-YICQ user, err := nurl.QueryUnescape(config.User) if err != nil { @@ -120,8 +120,6 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } config.Passwd = password - migrationsTable := config.Params["x-migrations-table"] - // use custom TLS? ctls := config.TLSConfig if len(ctls) > 0 { @@ -168,6 +166,15 @@ func (m *Mysql) Open(url string) (database.Driver, error) { } } + return config, nil +} + +func (m *Mysql) Open(url string) (database.Driver, error) { + config, err := m.getConfig(url) + if err != nil { + return nil, err + } + db, err := sql.Open("mysql", config.FormatDSN()) if err != nil { return nil, err @@ -175,7 +182,7 @@ func (m *Mysql) Open(url string) (database.Driver, error) { mx, err := WithInstance(db, &Config{ DatabaseName: config.DBName, - MigrationsTable: migrationsTable, + MigrationsTable: config.Params["x-migrations-table"], }) if err != nil { return nil, err diff --git a/database/mysql/mysql_test.go b/database/mysql/mysql_test.go index d8e7eca64..6c2ffc704 100644 --- a/database/mysql/mysql_test.go +++ b/database/mysql/mysql_test.go @@ -174,3 +174,50 @@ func TestLockWorks(t *testing.T) { } }) } + +func TestURLToMySQLConfig(t *testing.T) { + testcases := []struct { + name string + urlStr string + expectedDSN string // empty string signifies that an error is expected + }{ + {name: "no user/password", urlStr: "mysql://tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "only user", urlStr: "mysql://username@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "only user - with encoded :", + urlStr: "mysql://username%3A@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username:@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "only user - with encoded @", + urlStr: "mysql://username%40@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username@@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "user/password", urlStr: "mysql://username:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + // Not supported yet: https://github.com/go-sql-driver/mysql/issues/591 + // {name: "user/password - user with encoded :", + // urlStr: "mysql://username%3A:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + // expectedDSN: "username::pasword@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "user/password - user with encoded @", + urlStr: "mysql://username%40:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username@:password@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "user/password - password with encoded :", + urlStr: "mysql://username:password%3A@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username:password:@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + {name: "user/password - password with encoded @", + urlStr: "mysql://username:password%40@tcp(127.0.0.1:3306)/myDB?multiStatements=true", + expectedDSN: "username:password@@tcp(127.0.0.1:3306)/myDB?multiStatements=true"}, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + m := &Mysql{} + config, err := m.getConfig(tc.urlStr) + if err != nil { + t.Fatal("Failed to parse url string:", tc.urlStr, "error:", err) + } + dsn := config.FormatDSN() + if dsn != tc.expectedDSN { + t.Error("Got unexpected DSN:", dsn, "!=", tc.expectedDSN) + } + }) + } +} diff --git a/util.go b/util.go index 50fd1993f..ecf377391 100644 --- a/util.go +++ b/util.go @@ -74,13 +74,14 @@ func schemeFromURL(url string) (string, error) { return "", errEmptyURL } - parts := strings.SplitN(url, "://", 2) + i := strings.Index(url, ":") - if len(parts) < 2 || parts[0] == "" { + // No : or : is the first character. + if i < 1 { return "", errNoScheme } - return parts[0], nil + return url[0:i], nil } // FilterCustomQuery filters all query values starting with `x-` diff --git a/util_test.go b/util_test.go index 97a0b8491..ef395e84f 100644 --- a/util_test.go +++ b/util_test.go @@ -74,28 +74,34 @@ func TestSourceSchemeFromUrlFailure(t *testing.T) { } func TestDatabaseSchemeFromUrlSuccess(t *testing.T) { - urlStr := "protocol://path" - expected := "protocol" - - u, err := databaseSchemeFromURL(urlStr) - if err != nil { - t.Fatalf("expected no error, but received %q", err) - } - if u != expected { - t.Fatalf("expected %q, but received %q", expected, u) + cases := []struct { + name string + urlStr string + expected string + }{ + { + name: "Simple", + urlStr: "protocol://path", + expected: "protocol", + }, + { + // See issue #264 + name: "MySQLWithPort", + urlStr: "mysql://user:pass@tcp(host:1337)/db", + expected: "mysql", + }, } -} -func TestDatabaseSchemeFromUrlIssue264(t *testing.T) { - urlStr := "mysql://user:pass@tcp(host:1337)/db" - expected := "mysql" - - u, err := databaseSchemeFromURL(urlStr) - if err != nil { - t.Fatalf("expected no error, but received %q", err) - } - if u != expected { - t.Fatalf("expected %q, but received %q", expected, u) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + u, err := databaseSchemeFromURL(tc.urlStr) + if err != nil { + t.Fatalf("expected no error, but received %q", err) + } + if u != tc.expected { + t.Fatalf("expected %q, but received %q", tc.expected, u) + } + }) } } From 867c399bb90664d0b7561ce7560b6fbdaa76704d Mon Sep 17 00:00:00 2001 From: Erik Dubbelboer Date: Fri, 16 Aug 2019 12:55:32 +0200 Subject: [PATCH 6/6] Reuse old function names --- database/mysql/mysql.go | 4 ++-- database/mysql/mysql_test.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/database/mysql/mysql.go b/database/mysql/mysql.go index b9f71bc7d..5cc892a16 100644 --- a/database/mysql/mysql.go +++ b/database/mysql/mysql.go @@ -97,7 +97,7 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { return mx, nil } -func (m *Mysql) getConfig(url string) (*mysql.Config, error) { +func urlToMySQLConfig(url string) (*mysql.Config, error) { config, err := mysql.ParseDSN(strings.TrimPrefix(url, "mysql://")) if err != nil { return nil, err @@ -170,7 +170,7 @@ func (m *Mysql) getConfig(url string) (*mysql.Config, error) { } func (m *Mysql) Open(url string) (database.Driver, error) { - config, err := m.getConfig(url) + config, err := urlToMySQLConfig(url) if err != nil { return nil, err } diff --git a/database/mysql/mysql_test.go b/database/mysql/mysql_test.go index 6c2ffc704..5d6e82e8b 100644 --- a/database/mysql/mysql_test.go +++ b/database/mysql/mysql_test.go @@ -209,8 +209,7 @@ func TestURLToMySQLConfig(t *testing.T) { } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - m := &Mysql{} - config, err := m.getConfig(tc.urlStr) + config, err := urlToMySQLConfig(tc.urlStr) if err != nil { t.Fatal("Failed to parse url string:", tc.urlStr, "error:", err) }