Merge pull request #466 from myENA/issue/448

Check upstream X-Forwarded-Proto prior to redirect

proxy/http_integration_test.go

@@ -284,7 +284,7 @@ func TestRedirect(t *testing.T) {
 		{req: "/", wantCode: 301, wantLoc: "http://a.com/"},
 		{req: "/aaa/bbb", wantCode: 301, wantLoc: "http://a.com/aaa/bbb"},
 		{req: "/foo", wantCode: 301, wantLoc: "http://a.com/abc"},
-		{req: "/bar", wantCode: 302, wantLoc: "http://b.com"},
+		{req: "/bar", wantCode: 302, wantLoc: "http://b.com/"},
 		{req: "/bar/aaa", wantCode: 302, wantLoc: "http://b.com/aaa"},
 	}
 

proxy/http_proxy.go

@@ -99,8 +99,7 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if t.RedirectCode != 0 {
-		redirectURL := t.GetRedirectURL(requestURL)
-		http.Redirect(w, r, redirectURL.String(), t.RedirectCode)
+		http.Redirect(w, r, t.RedirectURL.String(), t.RedirectCode)
 		if t.Timer != nil {
 			t.Timer.Update(0)
 		}

route/table.go

@@ -312,20 +312,28 @@ func (t Table) matchingHosts(req *http.Request) (hosts []string) {
 // and if none matches then it falls back to generic routes without
 // a host. This is useful for a catch-all '/' rule.
 func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher) (target *Target) {
-	path := req.URL.Path
 	if trace != "" {
 		if len(trace) > 16 {
 			trace = trace[:15]
 		}
-		log.Printf("[TRACE] %s Tracing %s%s", trace, req.Host, path)
+		log.Printf("[TRACE] %s Tracing %s%s", trace, req.Host, req.URL.Path)
 	}
 
 	// find matching hosts for the request
 	// and add "no host" as the fallback option
 	hosts := t.matchingHosts(req)
 	hosts = append(hosts, "")
 	for _, h := range hosts {
-		if target = t.lookup(h, path, trace, pick, match); target != nil {
+		if target = t.lookup(h, req.URL.Path, trace, pick, match); target != nil {
+			if target.RedirectCode != 0 {
+				target.BuildRedirectURL(req.URL) // build redirect url and cache in target
+				if target.RedirectURL.Scheme == req.Header.Get("X-Forwarded-Proto") &&
+					target.RedirectURL.Host == req.Host &&
+					target.RedirectURL.Path == req.URL.Path {
+					log.Print("[INFO] Skipping redirect with same scheme, host and path")
+					continue
+				}
+			}
 			break
 		}
 	}

route/table_test.go

@@ -483,6 +483,80 @@ func TestNormalizeHost(t *testing.T) {
 	}
 }
 
+// see https://github.com/fabiolb/fabio/issues/448
+// for more information on the issue and purpose of this test
+func TestTableLookupIssue448(t *testing.T) {
+	s := `
+	route add mock foo.com:80/ https://foo.com/ opts "redirect=301"
+	route add mock aaa.com:80/ http://bbb.com/ opts "redirect=301"
+	route add mock ccc.com:443/bar https://ccc.com/baz opts "redirect=301"
+	route add mock / http://foo.com/
+	`
+
+	tbl, err := NewTable(s)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var tests = []struct {
+		req *http.Request
+		dst string
+	}{
+		{
+			req: &http.Request{
+				Host: "foo.com",
+				URL:  mustParse("/"),
+			},
+			dst: "https://foo.com/",
+			// empty upstream header should follow redirect - standard behavior
+		},
+		{
+			req: &http.Request{
+				Host:   "foo.com",
+				URL:    mustParse("/"),
+				Header: http.Header{"X-Forwarded-Proto": {"http"}},
+			},
+			dst: "https://foo.com/",
+			// upstream http request to same host and path should follow redirect
+		},
+		{
+			req: &http.Request{
+				Host:   "foo.com",
+				URL:    mustParse("/"),
+				Header: http.Header{"X-Forwarded-Proto": {"https"}},
+				TLS:    &tls.ConnectionState{},
+			},
+			dst: "http://foo.com/",
+			// upstream https request to same host and path should NOT follow redirect"
+		},
+		{
+			req: &http.Request{
+				Host:   "aaa.com",
+				URL:    mustParse("/"),
+				Header: http.Header{"X-Forwarded-Proto": {"http"}},
+			},
+			dst: "http://bbb.com/",
+			// upstream http request to different http host should follow redirect
+		},
+		{
+			req: &http.Request{
+				Host:   "ccc.com",
+				URL:    mustParse("/bar"),
+				Header: http.Header{"X-Forwarded-Proto": {"https"}},
+				TLS:    &tls.ConnectionState{},
+			},
+			dst: "https://ccc.com/baz",
+			// upstream https request to same https host with different path should follow redirect"
+		},
+	}
+
+	for i, tt := range tests {
+		if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher).URL.String(), tt.dst; got != want {
+			t.Errorf("%d: got %v want %v", i, got, want)
+		}
+	}
+}
+
 func TestTableLookup(t *testing.T) {
 	s := `
 	route add svc / http://foo.com:800

route/target.go

@@ -38,6 +38,10 @@ type Target struct {
 	// When set to a value > 0 the client is redirected to the target url.
 	RedirectCode int
 
+	// RedirectURL is the redirect target based on the request.
+	// This is cached here to prevent multiple generations per request.
+	RedirectURL *url.URL
+
 	// FixedWeight is the weight assigned to this target.
 	// If the value is 0 the targets weight is dynamic.
 	FixedWeight float64
@@ -55,28 +59,30 @@ type Target struct {
 	accessRules map[string][]interface{}
 }
 
-func (t *Target) GetRedirectURL(requestURL *url.URL) *url.URL {
-	redirectURL := &url.URL{
+func (t *Target) BuildRedirectURL(requestURL *url.URL) {
+	t.RedirectURL = &url.URL{
 		Scheme:   t.URL.Scheme,
 		Host:     t.URL.Host,
 		Path:     t.URL.Path,
 		RawQuery: t.URL.RawQuery,
 	}
-	if strings.HasSuffix(redirectURL.Host, "$path") {
-		redirectURL.Host = redirectURL.Host[:len(redirectURL.Host)-len("$path")]
-		redirectURL.Path = "$path"
+	if strings.HasSuffix(t.RedirectURL.Host, "$path") {
+		t.RedirectURL.Host = t.RedirectURL.Host[:len(t.RedirectURL.Host)-len("$path")]
+		t.RedirectURL.Path = "$path"
 	}
-	if strings.Contains(redirectURL.Path, "/$path") {
-		redirectURL.Path = strings.Replace(redirectURL.Path, "/$path", "$path", 1)
+	if strings.Contains(t.RedirectURL.Path, "/$path") {
+		t.RedirectURL.Path = strings.Replace(t.RedirectURL.Path, "/$path", "$path", 1)
 	}
-	if strings.Contains(redirectURL.Path, "$path") {
-		redirectURL.Path = strings.Replace(redirectURL.Path, "$path", requestURL.Path, 1)
-		if t.StripPath != "" && strings.HasPrefix(redirectURL.Path, t.StripPath) {
-			redirectURL.Path = redirectURL.Path[len(t.StripPath):]
+	if strings.Contains(t.RedirectURL.Path, "$path") {
+		t.RedirectURL.Path = strings.Replace(t.RedirectURL.Path, "$path", requestURL.Path, 1)
+		if t.StripPath != "" && strings.HasPrefix(t.RedirectURL.Path, t.StripPath) {
+			t.RedirectURL.Path = t.RedirectURL.Path[len(t.StripPath):]
 		}
-		if redirectURL.RawQuery == "" && requestURL.RawQuery != "" {
-			redirectURL.RawQuery = requestURL.RawQuery
+		if t.RedirectURL.RawQuery == "" && requestURL.RawQuery != "" {
+			t.RedirectURL.RawQuery = requestURL.RawQuery
 		}
 	}
-	return redirectURL
+	if t.RedirectURL.Path == "" {
+		t.RedirectURL.Path = "/"
+	}
 }

route/target_test.go

@@ -5,7 +5,7 @@ import (
 	"testing"
 )
 
-func TestTarget_GetRedirectURL(t *testing.T) {
+func TestTarget_BuildRedirectURL(t *testing.T) {
 	type routeTest struct {
 		req  string
 		want string
@@ -95,8 +95,8 @@ func TestTarget_GetRedirectURL(t *testing.T) {
 		target := route.Targets[0]
 		for _, rt := range tt.tests {
 			reqURL, _ := url.Parse("http://foo.com" + rt.req)
-			got := target.GetRedirectURL(reqURL)
-			if got.String() != rt.want {
+			target.BuildRedirectURL(reqURL)
+			if got := target.RedirectURL.String(); got != rt.want {
 				t.Errorf("Got %s, wanted %s", got, rt.want)
 			}
 		}