Merge pull request #39 from andygrunwald/fix-url-auth-parsing

NewClient() can't handle credentials containing characters such as /

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
 

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()

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)
 	}
@@ -264,6 +264,81 @@ func TestNewClient_BasicAuth(t *testing.T) {
 	}
 }
 
+func TestNewClient_ReParseURL(t *testing.T) {
+	urls := map[string][]string{
+		"http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/[email protected]:5000/": {
+			"http://127.0.0.1:5000/", "admin", "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg",
+		},
+		"http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/[email protected]:5000/foo": {
+			"http://127.0.0.1:5000/foo", "admin", "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg",
+		},
+		"http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/[email protected]: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()