From f801b9a7bd5566be12ca83ba6ba77f6380c92268 Mon Sep 17 00:00:00 2001 From: Josh Hawn Date: Sat, 31 Jan 2015 11:43:06 -0800 Subject: [PATCH] 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 (github: jlhawn) --- api/v2/urls.go | 50 +++++++++++++--- api/v2/urls_test.go | 141 ++++++++++++++++++++++++++++++-------------- 2 files changed, 141 insertions(+), 50 deletions(-) diff --git a/api/v2/urls.go b/api/v2/urls.go index 47c7beab5..6f2fd6e8e 100644 --- a/api/v2/urls.go +++ b/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, - Host: r.Host, + Scheme: scheme, + 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. - Schemes(ub.root.Scheme). - Host(ub.root.Host) + *route = *ub.router.GetRoute(name) // clone the route + *root = *ub.root + + 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. diff --git a/api/v2/urls_test.go b/api/v2/urls_test.go index a9590dba9..d8001c2a4 100644 --- a/api/v2/urls_test.go +++ b/api/v2/urls_test.go @@ -1,67 +1,55 @@ package v2 import ( + "net/http" "net/url" "testing" ) type urlBuilderTestCase struct { - description string - expected string - build func() (string, error) + description string + expectedPath string + build func() (string, error) } -// TestURLBuilder tests the various url building functions, ensuring they are -// returning the expected values. -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) - }{ +func makeURLBuilderTestCases(urlBuilder *URLBuilder) []urlBuilderTestCase { + return []urlBuilderTestCase{ { - description: "test base url", - expected: "http://localhost:5000/v2/", - build: urlBuilder.BuildBaseURL, + description: "test base url", + expectedPath: "/v2/", + build: urlBuilder.BuildBaseURL, }, { - description: "test tags url", - expected: "http://localhost:5000/v2/foo/bar/tags/list", + description: "test tags url", + expectedPath: "/v2/foo/bar/tags/list", build: func() (string, error) { return urlBuilder.BuildTagsURL("foo/bar") }, }, { - description: "test manifest url", - expected: "http://localhost:5000/v2/foo/bar/manifests/tag", + description: "test manifest url", + expectedPath: "/v2/foo/bar/manifests/tag", build: func() (string, error) { return urlBuilder.BuildManifestURL("foo/bar", "tag") }, }, { - description: "build blob url", - expected: "http://localhost:5000/v2/foo/bar/blobs/tarsum.v1+sha256:abcdef0123456789", + description: "build blob url", + 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", - expected: "http://localhost:5000/v2/foo/bar/blobs/uploads/", + description: "build blob upload url", + expectedPath: "/v2/foo/bar/blobs/uploads/", build: func() (string, error) { return urlBuilder.BuildBlobUploadURL("foo/bar") }, }, { - 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", + description: "build blob upload url with digest and size", + 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", - expected: "http://localhost:5000/v2/foo/bar/blobs/uploads/uuid-part", + description: "build blob upload chunk url", + 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", - expected: "http://localhost:5000/v2/foo/bar/blobs/uploads/uuid-part?digest=tarsum.v1%2Bsha256%3Aabcdef0123456789&size=10000", + description: "build blob upload chunk url with digest and size", + 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 { - t.Fatalf("%s: %q != %q", testcase.description, u, testcase.expected) - } +// TestURLBuilder tests the various url building functions, ensuring they are +// 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) + } + } + } }