Improve URL Builders

Handles an issue where mux.Route does not set the desired scheme
when building a url and always uses `http`.

Now uses X-Forwarded-Proto when creating a URLBuilder from a request.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)

api/v2/urls.go

@@ -43,9 +43,30 @@ func NewURLBuilderFromString(root string) (*URLBuilder, error) {
 // NewURLBuilderFromRequest uses information from an *http.Request to
 // construct the root url.
 func NewURLBuilderFromRequest(r *http.Request) *URLBuilder {
+	var scheme string
+
+	forwardedProto := r.Header.Get("X-Forwarded-Proto")
+
+	switch {
+	case len(forwardedProto) > 0:
+		scheme = forwardedProto
+	case r.TLS != nil:
+		scheme = "https"
+	case len(r.URL.Scheme) > 0:
+		scheme = r.URL.Scheme
+	default:
+		scheme = "http"
+	}
+
+	host := r.Host
+	forwardedHost := r.Header.Get("X-Forwarded-Host")
+	if len(forwardedHost) > 0 {
+		host = forwardedHost
+	}
+
 	u := &url.URL{
-		Scheme: r.URL.Scheme,
+		Scheme: scheme,
-		Host:   r.Host,
+		Host:   host,
 	}
 
 	return NewURLBuilder(u)
@@ -129,13 +150,28 @@ func (ub *URLBuilder) BuildBlobUploadChunkURL(name, uuid string, values ...url.V
 
 // clondedRoute returns a clone of the named route from the router. Routes
 // must be cloned to avoid modifying them during url generation.
-func (ub *URLBuilder) cloneRoute(name string) *mux.Route {
+func (ub *URLBuilder) cloneRoute(name string) clonedRoute {
 	route := new(mux.Route)
-	*route = *ub.router.GetRoute(name) // clone the route
+	root := new(url.URL)
 
-	return route.
+	*route = *ub.router.GetRoute(name) // clone the route
-		Schemes(ub.root.Scheme).
+	*root = *ub.root
-		Host(ub.root.Host)
+
+	return clonedRoute{Route: route, root: root}
+}
+
+type clonedRoute struct {
+	*mux.Route
+	root *url.URL
+}
+
+func (cr clonedRoute) URL(pairs ...string) (*url.URL, error) {
+	routeURL, err := cr.Route.URL(pairs...)
+	if err != nil {
+		return nil, err
+	}
+
+	return cr.root.ResolveReference(routeURL), nil
 }
 
 // appendValuesURL appends the parameters to the url.

api/v2/urls_test.go

@@ -1,67 +1,55 @@
 package v2
 
 import (
+	"net/http"
 	"net/url"
 	"testing"
 )
 
 type urlBuilderTestCase struct {
-	description string
+	description  string
-	expected    string
+	expectedPath string
-	build       func() (string, error)
+	build        func() (string, error)
 }
 
-// TestURLBuilder tests the various url building functions, ensuring they are
+func makeURLBuilderTestCases(urlBuilder *URLBuilder) []urlBuilderTestCase {
-// returning the expected values.
+	return []urlBuilderTestCase{
-func TestURLBuilder(t *testing.T) {
-
-	root := "http://localhost:5000/"
-	urlBuilder, err := NewURLBuilderFromString(root)
-	if err != nil {
-		t.Fatalf("unexpected error creating urlbuilder: %v", err)
-	}
-
-	for _, testcase := range []struct {
-		description string
-		expected    string
-		build       func() (string, error)
-	}{
 		{
-			description: "test base url",
+			description:  "test base url",
-			expected:    "http://localhost:5000/v2/",
+			expectedPath: "/v2/",
-			build:       urlBuilder.BuildBaseURL,
+			build:        urlBuilder.BuildBaseURL,
 		},
 		{
-			description: "test tags url",
+			description:  "test tags url",
-			expected:    "http://localhost:5000/v2/foo/bar/tags/list",
+			expectedPath: "/v2/foo/bar/tags/list",
 			build: func() (string, error) {
 				return urlBuilder.BuildTagsURL("foo/bar")
 			},
 		},
 		{
-			description: "test manifest url",
+			description:  "test manifest url",
-			expected:    "http://localhost:5000/v2/foo/bar/manifests/tag",
+			expectedPath: "/v2/foo/bar/manifests/tag",
 			build: func() (string, error) {
 				return urlBuilder.BuildManifestURL("foo/bar", "tag")
 			},
 		},
 		{
-			description: "build blob url",
+			description:  "build blob url",
-			expected:    "http://localhost:5000/v2/foo/bar/blobs/tarsum.v1+sha256:abcdef0123456789",
+			expectedPath: "/v2/foo/bar/blobs/tarsum.v1+sha256:abcdef0123456789",
 			build: func() (string, error) {
 				return urlBuilder.BuildBlobURL("foo/bar", "tarsum.v1+sha256:abcdef0123456789")
 			},
 		},
 		{
-			description: "build blob upload url",
+			description:  "build blob upload url",
-			expected:    "http://localhost:5000/v2/foo/bar/blobs/uploads/",
+			expectedPath: "/v2/foo/bar/blobs/uploads/",
 			build: func() (string, error) {
 				return urlBuilder.BuildBlobUploadURL("foo/bar")
 			},
 		},
 		{
-			description: "build blob upload url with digest and size",
+			description:  "build blob upload url with digest and size",
-			expected:    "http://localhost:5000/v2/foo/bar/blobs/uploads/?digest=tarsum.v1%2Bsha256%3Aabcdef0123456789&size=10000",
+			expectedPath: "/v2/foo/bar/blobs/uploads/?digest=tarsum.v1%2Bsha256%3Aabcdef0123456789&size=10000",
 			build: func() (string, error) {
 				return urlBuilder.BuildBlobUploadURL("foo/bar", url.Values{
 					"size":   []string{"10000"},
@@ -70,15 +58,15 @@ func TestURLBuilder(t *testing.T) {
 			},
 		},
 		{
-			description: "build blob upload chunk url",
+			description:  "build blob upload chunk url",
-			expected:    "http://localhost:5000/v2/foo/bar/blobs/uploads/uuid-part",
+			expectedPath: "/v2/foo/bar/blobs/uploads/uuid-part",
 			build: func() (string, error) {
 				return urlBuilder.BuildBlobUploadChunkURL("foo/bar", "uuid-part")
 			},
 		},
 		{
-			description: "build blob upload chunk url with digest and size",
+			description:  "build blob upload chunk url with digest and size",
-			expected:    "http://localhost:5000/v2/foo/bar/blobs/uploads/uuid-part?digest=tarsum.v1%2Bsha256%3Aabcdef0123456789&size=10000",
+			expectedPath: "/v2/foo/bar/blobs/uploads/uuid-part?digest=tarsum.v1%2Bsha256%3Aabcdef0123456789&size=10000",
 			build: func() (string, error) {
 				return urlBuilder.BuildBlobUploadChunkURL("foo/bar", "uuid-part", url.Values{
 					"size":   []string{"10000"},
@@ -86,15 +74,82 @@ func TestURLBuilder(t *testing.T) {
 				})
 			},
 		},
-	} {
+	}
-		u, err := testcase.build()
+}
-		if err != nil {
-			t.Fatalf("%s: error building url: %v", testcase.description, err)
-		}
 
-		if u != testcase.expected {
+// TestURLBuilder tests the various url building functions, ensuring they are
-			t.Fatalf("%s: %q != %q", testcase.description, u, testcase.expected)
+// returning the expected values.
-		}
+func TestURLBuilder(t *testing.T) {
+	roots := []string{
+		"http://example.com",
+		"https://example.com",
+		"http://localhost:5000",
+		"https://localhost:5443",
 	}
 
+	for _, root := range roots {
+		urlBuilder, err := NewURLBuilderFromString(root)
+		if err != nil {
+			t.Fatalf("unexpected error creating urlbuilder: %v", err)
+		}
+
+		for _, testCase := range makeURLBuilderTestCases(urlBuilder) {
+			url, err := testCase.build()
+			if err != nil {
+				t.Fatalf("%s: error building url: %v", testCase.description, err)
+			}
+
+			expectedURL := root + testCase.expectedPath
+
+			if url != expectedURL {
+				t.Fatalf("%s: %q != %q", testCase.description, url, expectedURL)
+			}
+		}
+	}
+}
+
+type builderFromRequestTestCase struct {
+	request *http.Request
+	base    string
+}
+
+func TestBuilderFromRequest(t *testing.T) {
+	u, err := url.Parse("http://example.com")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	forwardedProtoHeader := make(http.Header, 1)
+	forwardedProtoHeader.Set("X-Forwarded-Proto", "https")
+
+	testRequests := []struct {
+		request *http.Request
+		base    string
+	}{
+		{
+			request: &http.Request{URL: u, Host: u.Host},
+			base:    "http://example.com",
+		},
+		{
+			request: &http.Request{URL: u, Host: u.Host, Header: forwardedProtoHeader},
+			base:    "https://example.com",
+		},
+	}
+
+	for _, tr := range testRequests {
+		builder := NewURLBuilderFromRequest(tr.request)
+
+		for _, testCase := range makeURLBuilderTestCases(builder) {
+			url, err := testCase.build()
+			if err != nil {
+				t.Fatalf("%s: error building url: %v", testCase.description, err)
+			}
+
+			expectedURL := tr.base + testCase.expectedPath
+
+			if url != expectedURL {
+				t.Fatalf("%s: %q != %q", testCase.description, url, expectedURL)
+			}
+		}
+	}
 }