Allow URLs with paths in NormalizeURL (#474)

This change also makes it an error to specify a URL scheme that is not
"https" or "http" instead of silently changing an invalid scheme to
"http".

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>

Author: bestbeforetoday

lib/client.go

@@ -19,7 +19,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"strconv"
 	"strings"
 
 	idemix "github.com/IBM/idemix/bccsp/schemes/dlog/crypto"
@@ -971,38 +970,44 @@ func newCfsslKeyRequest(bkr *api.KeyRequest) *csr.KeyRequest {
 	return &csr.KeyRequest{A: bkr.Algo, S: bkr.Size}
 }
 
-// NormalizeURL normalizes a URL (from cfssl)
+// NormalizeURL normalizes a full or partial URL to a full URL, using
+// configuration for missing elements.
 func NormalizeURL(addr string) (*url.URL, error) {
 	addr = strings.TrimSpace(addr)
+	if addr == "" {
+		addr = util.GetServerURL()
+	}
+
 	u, err := url.Parse(addr)
 	if err != nil {
 		return nil, err
 	}
+
 	if u.Opaque != "" {
-		u.Host = net.JoinHostPort(u.Scheme, u.Opaque)
+		port, path, found := strings.Cut(u.Opaque, "/")
+		if found {
+			u = u.JoinPath(path)
+		}
+		u.Host = net.JoinHostPort(u.Scheme, port)
+		u.Scheme = ""
 		u.Opaque = ""
-	} else if u.Path != "" && !strings.Contains(u.Path, ":") {
+	} else if u.Host == "" {
 		u.Host = net.JoinHostPort(u.Path, util.GetServerPort())
 		u.Path = ""
-	} else if u.Scheme == "" {
-		u.Host = u.Path
-		u.Path = ""
+	} else if u.Port() == "" {
+		u.Host = net.JoinHostPort(u.Host, util.GetServerPort())
 	}
-	if u.Scheme != "https" {
+
+	if u.Scheme == "" {
 		u.Scheme = "http"
+	} else if !isValidScheme(u.Scheme) {
+		return nil, errors.Errorf("invalid scheme: %s", u.Scheme)
 	}
-	_, port, err := net.SplitHostPort(u.Host)
-	if err != nil {
-		_, port, err = net.SplitHostPort(u.Host + ":" + util.GetServerPort())
-		if err != nil {
-			return nil, err
-		}
-	}
-	if port != "" {
-		_, err = strconv.Atoi(port)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return u, nil
+
+	// Parse again to ensure URL contains no invalid characters
+	return url.Parse(u.String())
+}
+
+func isValidScheme(scheme string) bool {
+	return scheme == "http" || scheme == "https"
 }

lib/client_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/hyperledger/fabric-lib-go/bccsp"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -1320,39 +1321,75 @@ func setupGenCRLTest(t *testing.T, serverHome, clientHome string) (*Server, *Ide
 	return server, adminID
 }
 
-func TestNormalizeUrl(t *testing.T) {
-	u, err := NormalizeURL("")
-	if err != nil {
-		t.Errorf("normalizeURL empty: %s", err)
-	} else {
-		t.Logf("URL %s, %s, %s", u.Scheme, u.Host, u.Path)
-	}
-	u, err = NormalizeURL("http://host:7054/path")
-	if err != nil {
-		t.Errorf("normalizeURL failed: %s", err)
-	} else {
-		t.Logf("URL %s, %s, %s", u.Scheme, u.Host, u.Path)
-	}
-	u, err = NormalizeURL("https://localhost:80/a%2Fb%2Fc")
-	if err != nil {
-		t.Errorf("NormalizeURL failed: %s", err)
-	} else {
-		t.Logf("URL %s, %s, %s", u.Scheme, u.Host, u.Path)
-	}
-	_, err = NormalizeURL("[")
-	t.Logf("NormalizeURL() error %v", err)
-	if err == nil {
-		t.Errorf("NormalizeURL '[' should have failed")
-	}
-	_, err = NormalizeURL("http://[/path")
-	t.Logf("NormalizeURL() error %v", err)
-	if err == nil {
-		t.Errorf("NormalizeURL 'http://[/path]' should have failed")
+func TestNormalizeURL(t *testing.T) {
+	for testName, testData := range map[string]struct {
+		input    string
+		expected string
+	}{
+		"HTTP server URL is accepted": {
+			input:    "http://host:9876",
+			expected: "http://host:9876",
+		},
+		"HTTPS server URL is accepted": {
+			input:    "https://host:9876",
+			expected: "https://host:9876",
+		},
+		"HTTP scheme if only host and port specified": {
+			input:    "host:9876",
+			expected: "http://host:9876",
+		},
+		"HTTP scheme and default port if only host specified": {
+			input:    "host",
+			expected: "http://host:7054",
+		},
+		"default port if only scheme and host specified": {
+			input:    "http://host",
+			expected: "http://host:7054",
+		},
+		"URL with path is accepted": {
+			input:    "http://host:9876/path",
+			expected: "http://host:9876/path",
+		},
+		"HTTP scheme if host, port and path specified": {
+			input:    "host:9876/path",
+			expected: "http://host:9876/path",
+		},
+		"URL encoded characters in path are unmodified": {
+			input:    "http://host:9876/a%2Fb%2Fc",
+			expected: "http://host:9876/a%2Fb%2Fc",
+		},
+		"URL encoded characters in path without scheme are unmodified": {
+			input:    "host:9876/a%2Fb%2Fc",
+			expected: "http://host:9876/a%2Fb%2Fc",
+		},
+		"empty returns default": {
+			input:    "",
+			expected: "http://localhost:7054",
+		},
+	} {
+		t.Run(testName, func(t *testing.T) {
+			u, err := NormalizeURL(testData.input)
+			require.NoError(t, err)
+
+			assert.Equal(t, testData.expected, u.String())
+		})
 	}
-	_, err = NormalizeURL("https:rootless/path")
-	t.Logf("NormalizeURL() error %v", err)
-	if err == nil {
-		t.Errorf("NormalizeURL 'https:rootless/path' should have failed")
+
+	t.Run("invalid scheme fails", func(t *testing.T) {
+		u, err := NormalizeURL("ftp://host:9876")
+		require.ErrorContains(t, err, "ftp", "URL: %s", u)
+	})
+
+	for testName, input := range map[string]string{
+		"invalid URL input fails":              "[",
+		"invalid characters in hostname fails": "http://[/path",
+		"rootless path fails":                  "https:rootless/path",
+		"non-numeric port fails":               "host:port",
+	} {
+		t.Run(testName, func(t *testing.T) {
+			u, err := NormalizeURL(input)
+			require.Error(t, err, "expected error, got: %s", u)
+		})
 	}
 }