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

prevent net/url encoding the user password #69

Merged
merged 1 commit into from
Jun 29, 2018
Merged

prevent net/url encoding the user password #69

merged 1 commit into from
Jun 29, 2018

Conversation

bcho
Copy link
Contributor

@bcho bcho commented Jun 29, 2018

This is a cherry-pick fix from mattes/migrate#324. This should fix mattes/migrate#256

cc @RyanDeng


According to net/url

// A URL represents a parsed URL (technically, a URI reference).
//
// The general form represented is:
//
//	[scheme:][//[userinfo@]host][/]path[?query][#fragment]
//
// URLs that do not start with a slash after the scheme are interpreted as:
//
//	scheme:opaque[?query][#fragment]
//

If the url starts with a slash after the scheme, the url.String() func will do url encode for the userinfo. In this case, password that has non-alphanumeric char will be url encoded and mysql auth will fail.

Probably the best fix should be in Open() func of database/driver.go. However I'm using mysql and not familiar with other drivers, so make a tiny fix in the mysql driver

@coveralls
Copy link

coveralls commented Jun 29, 2018

Pull Request Test Coverage Report for Build 116

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 49.522%

Totals Coverage Status
Change from base Build 115: 0.0%
Covered Lines: 828
Relevant Lines: 1672

💛 - Coveralls

@dhui
Copy link
Member

dhui commented Jun 29, 2018

Agreed. For now, fixing every db driver is the right approach.
But the better long term change is to change the Open() method in the Driver interface to take a url.URL instead of a string but that's a backwards incompatible change.

@dhui dhui mentioned this pull request Jun 29, 2018
@dhui dhui merged commit 78d696c into golang-migrate:master Jun 29, 2018
@dhui
Copy link
Member

dhui commented Jul 24, 2018

@bcho @RyanDeng I'll probably have to revert this PR as it's breaking for users who have @ in their db username. Any encoding interplay between migrate DB drivers and the underlying DB driver will have to be dealt with in the driver. The best approach in this case may be to "re-encode" URL and make the url parts compatible with the MySQL DSN.

A future release of the MySQL driver shouldn't have the same encoding problems: go-sql-driver/mysql#591
There may always be encoding issues when using url.Parse() directly with the MySQL driver: go-sql-driver/mysql#460 (comment)

@bcho
Copy link
Contributor Author

bcho commented Jul 24, 2018 via email

dhui added a commit that referenced this pull request Jul 25, 2018
FPiety0521 pushed a commit to FPiety0521/Golang-SQL that referenced this pull request May 24, 2023
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

Successfully merging this pull request may close these issues.

MySQL driver can't handle non-alphanumeric passwords
4 participants