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

driver.New broken on go 1.11.13+, 1.12.8+, 1.13 #8

Open
adamhassel opened this issue Oct 18, 2019 · 7 comments
Open

driver.New broken on go 1.11.13+, 1.12.8+, 1.13 #8

adamhassel opened this issue Oct 18, 2019 · 7 comments

Comments

@adamhassel
Copy link
Contributor

Due to changes in the net/url package's Parse function as a consequence of CVE-2019-14809 (https://groups.google.com/forum/#!topic/golang-announce/65QixT3tcmg), driver.New will fail to parse connection strings containing parentheses.

E.g. mysql://root:@tcp(localhost:3306)/dbname, mysql://root:@(db.example.com:3306)/dbname, which are both valid MySQL connection strings, will fail with invalid port ":3306)" after host.

https://play.golang.org/p/T4NUSAQGzpS

Compare:

$ cat parse.go 
package main

import (
	"fmt"
	"net/url"
)

func main() {
	fmt.Println(url.Parse("mysql://root:@tcp(localhost:3306)/dbname"))
}

$ go version
go version go1.11.11 linux/amd64

$ go run parse.go 
mysql://root:@tcp(localhost:3306)/dbname <nil>
@gravis
Copy link
Member

gravis commented Oct 21, 2019

We spotted that a few days ago. Unfortunately, that also means a lot of code rewrite since we use this method for all drivers:

u, err := neturl.Parse(url)

Contributions are welcome :)

@adamhassel
Copy link
Contributor Author

adamhassel commented Oct 25, 2019

Well, since the assumption that a connection string can be parsed as a URL has turned out to not hold, I suggest simply dropping that bit, extract the scheme manually and rely on failing on the getDriver call instead.

So something like (quick and dirty, can probably be done prettier):

// New returns Driver and calls Initialize on it.
func New(url string) (Driver, error) {
	re := regexp.MustCompile(`(?m)^(\w+)://`)
	match := re.FindStringSubmatch(url)
	if len(match) < 2 {
		return nil, fmt.Errorf("no driver found in connection string") // or whatever
	}

	drv := getDriver(match[1])
	if drv == nil {
		return nil, fmt.Errorf("driver '%s' not found", match[1])
	}
	return drv.new(url)
}

@adamhassel
Copy link
Contributor Author

I have a PR ready, but need push access.

@gravis
Copy link
Member

gravis commented Oct 25, 2019

@adamhassel great! You don't actually need push access, you can create a fork and create a Pull Request. This is the classic workflow on GitHub or GitLab. Thanks

@adamhassel
Copy link
Contributor Author

@gravis, sure. Used to working on branches :)

Anyhow, PR #9

@adamhassel
Copy link
Contributor Author

Now even with passing tests, for the first time in ... ever, as far as I can see.

@adamhassel
Copy link
Contributor Author

@gravis ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants