diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index 1a0870306..4631cdb7d 100644 --- a/proxy/http_integration_test.go +++ b/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"}, } diff --git a/proxy/http_proxy.go b/proxy/http_proxy.go index 0898d7afe..95758bbe1 100644 --- a/proxy/http_proxy.go +++ b/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) } diff --git a/route/table.go b/route/table.go index 8e74bded1..c417e2c34 100644 --- a/route/table.go +++ b/route/table.go @@ -312,12 +312,11 @@ 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 @@ -325,7 +324,16 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche 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 } } diff --git a/route/table_test.go b/route/table_test.go index eb0fdea71..ce8a0dca2 100644 --- a/route/table_test.go +++ b/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 diff --git a/route/target.go b/route/target.go index b5b63452a..db9e5244b 100644 --- a/route/target.go +++ b/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 = "/" + } } diff --git a/route/target_test.go b/route/target_test.go index 7d9c7794b..21ad2bc42 100644 --- a/route/target_test.go +++ b/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) } }