models/mirror: unescape credentials at read (#4014)

If we save credentials already escaped, 'url.QueryEscape' still
escapes it and makes the credentials become incorrect.

models/mirror.go

@@ -71,6 +71,46 @@ func (m *Mirror) ScheduleNextUpdate() {
 	m.NextUpdate = time.Now().Add(time.Duration(m.Interval) * time.Hour)
 }
 
+// findPasswordInMirrorAddress returns start (inclusive) and end index (exclusive)
+// of password portion of credentials in given mirror address.
+// It returns a boolean value to indicate whether password portion is found.
+func findPasswordInMirrorAddress(addr string) (start int, end int, found bool) {
+	// Find end of credentials (start of path)
+	end = strings.LastIndex(addr, "@")
+	if end == -1 {
+		return -1, -1, false
+	}
+
+	// Find delimiter of credentials (end of username)
+	start = strings.Index(addr, "://")
+	if start == -1 {
+		return -1, -1, false
+	}
+	start += 3
+	delim := strings.Index(addr[start:], ":")
+	if delim == -1 {
+		return -1, -1, false
+	}
+	delim += 1
+
+	if start+delim >= end {
+		return -1, -1, false // No password portion presented
+	}
+
+	return start + delim, end, true
+}
+
+// unescapeMirrorCredentials returns mirror address with unescaped credentials.
+func unescapeMirrorCredentials(addr string) string {
+	start, end, found := findPasswordInMirrorAddress(addr)
+	if !found {
+		return addr
+	}
+
+	password, _ := url.QueryUnescape(addr[start:end])
+	return addr[:start] + password + addr[end:]
+}
+
 func (m *Mirror) readAddress() {
 	if len(m.address) > 0 {
 		return
@@ -81,7 +121,7 @@ func (m *Mirror) readAddress() {
 		log.Error(2, "Load: %v", err)
 		return
 	}
-	m.address = cfg.Section("remote \"origin\"").Key("url").Value()
+	m.address = unescapeMirrorCredentials(cfg.Section("remote \"origin\"").Key("url").Value())
 }
 
 // HandleCloneUserCredentials replaces user credentials from HTTP/HTTPS URL
@@ -122,29 +162,12 @@ func (m *Mirror) FullAddress() string {
 
 // escapeCredentials returns mirror address with escaped credentials.
 func escapeMirrorCredentials(addr string) string {
-	// Find end of credentials (start of path)
+	start, end, found := findPasswordInMirrorAddress(addr)
-	end := strings.LastIndex(addr, "@")
+	if !found {
-	if end == -1 {
 		return addr
 	}
 
-	// Find delimiter of credentials (end of username)
+	return addr[:start] + url.QueryEscape(addr[start:end]) + addr[end:]
-	start := strings.Index(addr, "://")
-	if start == -1 {
-		return addr
-	}
-	start += 3
-	delim := strings.Index(addr[:start], ":")
-	if delim == -1 {
-		return addr
-	}
-	delim += 1
-
-	if start+delim > end {
-		return addr // No password portion presented
-	}
-
-	return addr[:start+delim] + url.QueryEscape(addr[start+delim:end]) + addr[end:]
 }
 
 // SaveAddress writes new address to Git repository config.

models/mirror_test.go

@@ -10,6 +10,52 @@ import (
 	. "github.com/smartystreets/goconvey/convey"
 )
 
+func Test_findPasswordInMirrorAddress(t *testing.T) {
+	Convey("Find password portion in mirror address", t, func() {
+		testCases := []struct {
+			addr       string
+			start, end int
+			found      bool
+			password   string
+		}{
+			{"http://localhost:3000/user/repo.git", -1, -1, false, ""},
+			{"http://user@localhost:3000/user/repo.git", -1, -1, false, ""},
+			{"http://user:@localhost:3000/user/repo.git", -1, -1, false, ""},
+			{"http://user:password@localhost:3000/user/repo.git", 12, 20, true, "password"},
+			{"http://username:my%3Asecure%3Bpassword@localhost:3000/user/repo.git", 16, 38, true, "my%3Asecure%3Bpassword"},
+			{"http://username:my%40secure%23password@localhost:3000/user/repo.git", 16, 38, true, "my%40secure%23password"},
+			{"http://username:@@localhost:3000/user/repo.git", 16, 17, true, "@"},
+		}
+
+		for _, tc := range testCases {
+			start, end, found := findPasswordInMirrorAddress(tc.addr)
+			So(start, ShouldEqual, tc.start)
+			So(end, ShouldEqual, tc.end)
+			So(found, ShouldEqual, tc.found)
+			if found {
+				So(tc.addr[start:end], ShouldEqual, tc.password)
+			}
+		}
+	})
+}
+
+func Test_unescapeMirrorCredentials(t *testing.T) {
+	Convey("Escape credentials in mirror address", t, func() {
+		testCases := []string{
+			"http://localhost:3000/user/repo.git", "http://localhost:3000/user/repo.git",
+			"http://user@localhost:3000/user/repo.git", "http://user@localhost:3000/user/repo.git",
+			"http://user:@localhost:3000/user/repo.git", "http://user:@localhost:3000/user/repo.git",
+			"http://user:password@localhost:3000/user/repo.git", "http://user:password@localhost:3000/user/repo.git",
+			"http://user:my%3Asecure%3Bpassword@localhost:3000/user/repo.git", "http://user:my:secure;password@localhost:3000/user/repo.git",
+			"http://user:my%40secure%23password@localhost:3000/user/repo.git", "http://user:my@secure#password@localhost:3000/user/repo.git",
+		}
+
+		for i := 0; i < len(testCases); i += 2 {
+			So(unescapeMirrorCredentials(testCases[i]), ShouldEqual, testCases[i+1])
+		}
+	})
+}
+
 func Test_escapeMirrorCredentials(t *testing.T) {
 	Convey("Escape credentials in mirror address", t, func() {
 		testCases := []string{