From ba033b018dc3290fd3bcdb75219b260a19328a79 Mon Sep 17 00:00:00 2001 From: Oliver Palmer Date: Mon, 4 Sep 2017 16:29:05 -0400 Subject: [PATCH 1/3] fix NewClient() url parsing for passwords containing / or other url characters Depending on the contents of the username and password the default url.Parse may not work. The below is an example URL that would end up being parsed incorrectly with url.Parse: http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@localhost:38607 So instead of depending on url.Parse we'll try using a regular expression first to match a specific pattern instead. This won't be 100% reliable in all cases which is why we fall back onto using url.Parse anyway. --- gerrit.go | 36 ++++++++++++++++++++---- gerrit_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/gerrit.go b/gerrit.go index ad92caa..9282d1f 100644 --- a/gerrit.go +++ b/gerrit.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "reflect" + "regexp" "strings" "github.com/google/go-querystring/query" @@ -65,6 +66,12 @@ var ( // credentials didn't allow us to query account information using digest, basic or cookie // auth. ErrAuthenticationFailed = errors.New("failed to authenticate using the provided credentials") + + // ReParseURL is used to parse the url provided to NewClient(). This + // regular expression contains five groups which capture the scheme, + // username, password, hostname and port. If we parse the url with this + // regular expression + ReParseURL = regexp.MustCompile(`^(http|https)://(.+):(.+)@(.+):(\d+)(.*)$`) ) // NewClient returns a new Gerrit API client. The gerritURL argument has to be the @@ -87,16 +94,35 @@ func NewClient(endpoint string, httpClient *http.Client) (*Client, error) { return nil, ErrNoInstanceGiven } + hasAuth := false + username := "" + password := "" + + // Depending on the contents of the username and password the default + // url.Parse may not work. The below is an example URL that + // would end up being parsed incorrectly with url.Parse: + // http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@localhost:38607 + // So instead of depending on url.Parse we'll try using a regular expression + // first to match a specific pattern. If that ends up working we modify + // the incoming endpoint to remove the username and password so the rest + // of this function will run as expected. + submatches := ReParseURL.FindAllStringSubmatch(endpoint, -1) + if len(submatches) > 0 && len(submatches[0]) > 5 { + submatch := submatches[0] + username = submatch[2] + password = submatch[3] + endpoint = fmt.Sprintf( + "%s://%s:%s%s", submatch[1], submatch[4], submatch[5], submatch[6]) + hasAuth = true + } + baseURL, err := url.Parse(endpoint) if err != nil { return nil, err } - // Username and/or password provided as part of the url. - - hasAuth := false - username := "" - password := "" + // Note, if we retrieved the URL and password using the regular + // expression above then the below code will do nothing. if baseURL.User != nil { username = baseURL.User.Username() parsedPassword, haspassword := baseURL.User.Password() diff --git a/gerrit_test.go b/gerrit_test.go index 0bf3b85..05c2cab 100644 --- a/gerrit_test.go +++ b/gerrit_test.go @@ -264,6 +264,82 @@ func TestNewClient_BasicAuth(t *testing.T) { } } +func TestNewClient_ReParseURL(t *testing.T) { + urls := map[string][]string{ + "http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@127.0.0.1:5000/": { + "http://127.0.0.1:5000/", "admin", "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg", + }, + "http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@127.0.0.1:5000/foo": { + "http://127.0.0.1:5000/foo", "admin", "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg", + }, + "http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@127.0.0.1:5000": { + "http://127.0.0.1:5000", "admin", "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg", + }, + "https://admin:foo/bar@localhost:5": { + "https://localhost:5", "admin", "foo/bar", + }, + } + for input, expectations := range urls { + submatches := gerrit.ReParseURL.FindAllStringSubmatch(input, -1) + submatch := submatches[0] + username := submatch[2] + password := submatch[3] + endpoint := fmt.Sprintf( + "%s://%s:%s%s", submatch[1], submatch[4], submatch[5], submatch[6]) + if endpoint != expectations[0] { + t.Errorf("%s != %s", expectations[0], endpoint) + } + if username != expectations[1] { + t.Errorf("%s != %s", expectations[1], username) + } + if password != expectations[2] { + t.Errorf("%s != %s", expectations[2], password) + } + + } +} + +func TestNewClient_BasicAuth_PasswordWithSlashes(t *testing.T) { + setup() + defer teardown() + + account := gerrit.AccountInfo{ + AccountID: 100000, + Name: "test", + Email: "test@localhost", + Username: "test"} + hits := 0 + + testMux.HandleFunc("/a/accounts/self", func(w http.ResponseWriter, r *http.Request) { + hits++ + switch hits { + case 1: + writeresponse(t, w, nil, http.StatusUnauthorized) + case 2: + // The second request should be a basic auth request if the first request, which is for + // digest based auth, fails. + if !strings.HasPrefix(r.Header.Get("Authorization"), "Basic ") { + t.Error("Missing 'Basic ' prefix") + } + writeresponse(t, w, account, http.StatusOK) + case 3: + t.Error("Did not expect another request") + } + }) + + serverURL := fmt.Sprintf( + "http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@%s", + testServer.Listener.Addr().String()) + client, err := gerrit.NewClient(serverURL, nil) + if err != nil { + t.Error(err) + } + if !client.Authentication.HasAuth() { + t.Error("Expected HasAuth() == true") + } +} + + func TestNewClient_CookieAuth(t *testing.T) { setup() defer teardown() From 52a27d66a867c4a7559a44d62d5673d008a0237a Mon Sep 17 00:00:00 2001 From: Oliver Palmer Date: Mon, 4 Sep 2017 16:34:29 -0400 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3136e68..c4d5799 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,11 @@ first. For more complete details see * Go 1.5 has been removed from testing on Travis. The linters introduced in 0.4.0 do not support this version, Go 1.5 is lacking security updates and most Linux distros have moved beyond Go 1.5 now. +* Add Go 1.9 to the Travis matrix. +* Fixed an issue where urls containing certain characters in the credentials + could cause NewClient() to use an invalid url. Something like `/`, which + Gerrit could use for generated passwords, for example would break url.Parse's + expectations. ### 0.3.0 From 28a926595d33b4662a481db2a830434ee1721915 Mon Sep 17 00:00:00 2001 From: Oliver Palmer Date: Mon, 4 Sep 2017 16:38:49 -0400 Subject: [PATCH 3/3] lint fix --- gerrit_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gerrit_test.go b/gerrit_test.go index 05c2cab..502dd48 100644 --- a/gerrit_test.go +++ b/gerrit_test.go @@ -81,7 +81,7 @@ func testMethod(t *testing.T, r *http.Request, want string) { } } -func testRequestURL(t *testing.T, r *http.Request, want string) { +func testRequestURL(t *testing.T, r *http.Request, want string) { // nolint: unparam if got := r.URL.String(); got != want { t.Errorf("Request URL: %v, want %v", got, want) } @@ -339,7 +339,6 @@ func TestNewClient_BasicAuth_PasswordWithSlashes(t *testing.T) { } } - func TestNewClient_CookieAuth(t *testing.T) { setup() defer teardown()