From ef82f3e034844005bfdb4139d0bcce44dc594657 Mon Sep 17 00:00:00 2001 From: orangain Date: Sat, 3 Feb 2018 00:31:07 +0900 Subject: [PATCH] Avoid redirect loop in HTTPSRedirect middleware (#1058) In HTTPSRedirect and similar middlewares, determining if redirection is needed using `c.IsTLS()` causes redirect loop when an application is running behind a TLS termination proxy, e.g. AWS ELB. Instead, I believe, redirection should be determined by `c.Scheme() != "https"`. This works well even behind a TLS termination proxy. --- middleware/redirect.go | 22 ++++++++++----------- middleware/redirect_test.go | 39 +++++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/middleware/redirect.go b/middleware/redirect.go index a1b068f8..422263de 100644 --- a/middleware/redirect.go +++ b/middleware/redirect.go @@ -16,10 +16,10 @@ type RedirectConfig struct { Code int `yaml:"code"` } -// redirectLogic represents a function that given a tls flag, host and uri +// redirectLogic represents a function that given a scheme, host and uri // can both: 1) determine if redirect is needed (will set ok accordingly) and // 2) return the appropriate redirect url. -type redirectLogic func(tls bool, scheme, host, uri string) (ok bool, url string) +type redirectLogic func(scheme, host, uri string) (ok bool, url string) const www = "www" @@ -40,8 +40,8 @@ func HTTPSRedirect() echo.MiddlewareFunc { // HTTPSRedirectWithConfig returns an HTTPSRedirect middleware with config. // See `HTTPSRedirect()`. func HTTPSRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc { - return redirect(config, func(tls bool, _, host, uri string) (ok bool, url string) { - if ok = !tls; ok { + return redirect(config, func(scheme, host, uri string) (ok bool, url string) { + if ok = scheme != "https"; ok { url = "https://" + host + uri } return @@ -59,8 +59,8 @@ func HTTPSWWWRedirect() echo.MiddlewareFunc { // HTTPSWWWRedirectWithConfig returns an HTTPSRedirect middleware with config. // See `HTTPSWWWRedirect()`. func HTTPSWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc { - return redirect(config, func(tls bool, _, host, uri string) (ok bool, url string) { - if ok = !tls && host[:3] != www; ok { + return redirect(config, func(scheme, host, uri string) (ok bool, url string) { + if ok = scheme != "https" && host[:3] != www; ok { url = "https://www." + host + uri } return @@ -78,8 +78,8 @@ func HTTPSNonWWWRedirect() echo.MiddlewareFunc { // HTTPSNonWWWRedirectWithConfig returns an HTTPSRedirect middleware with config. // See `HTTPSNonWWWRedirect()`. func HTTPSNonWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc { - return redirect(config, func(tls bool, _, host, uri string) (ok bool, url string) { - if ok = !tls; ok { + return redirect(config, func(scheme, host, uri string) (ok bool, url string) { + if ok = scheme != "https"; ok { if host[:3] == www { host = host[4:] } @@ -100,7 +100,7 @@ func WWWRedirect() echo.MiddlewareFunc { // WWWRedirectWithConfig returns an HTTPSRedirect middleware with config. // See `WWWRedirect()`. func WWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc { - return redirect(config, func(_ bool, scheme, host, uri string) (ok bool, url string) { + return redirect(config, func(scheme, host, uri string) (ok bool, url string) { if ok = host[:3] != www; ok { url = scheme + "://www." + host + uri } @@ -119,7 +119,7 @@ func NonWWWRedirect() echo.MiddlewareFunc { // NonWWWRedirectWithConfig returns an HTTPSRedirect middleware with config. // See `NonWWWRedirect()`. func NonWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc { - return redirect(config, func(tls bool, scheme, host, uri string) (ok bool, url string) { + return redirect(config, func(scheme, host, uri string) (ok bool, url string) { if ok = host[:3] == www; ok { url = scheme + "://" + host[4:] + uri } @@ -143,7 +143,7 @@ func redirect(config RedirectConfig, cb redirectLogic) echo.MiddlewareFunc { req, scheme := c.Request(), c.Scheme() host := req.Host - if ok, url := cb(c.IsTLS(), scheme, host, req.RequestURI); ok { + if ok, url := cb(scheme, host, req.RequestURI); ok { return c.Redirect(config.Code, url) } diff --git a/middleware/redirect_test.go b/middleware/redirect_test.go index 1b8b2223..8eddfcaf 100644 --- a/middleware/redirect_test.go +++ b/middleware/redirect_test.go @@ -12,47 +12,74 @@ import ( type middlewareGenerator func() echo.MiddlewareFunc func TestRedirectHTTPSRedirect(t *testing.T) { - res := redirectTest(HTTPSRedirect, "labstack.com") + res := redirectTest(HTTPSRedirect, "labstack.com", nil) assert.Equal(t, http.StatusMovedPermanently, res.Code) assert.Equal(t, "https://labstack.com/", res.Header().Get(echo.HeaderLocation)) } +func TestHTTPSRedirectBehindTLSTerminationProxy(t *testing.T) { + header := http.Header{} + header.Set(echo.HeaderXForwardedProto, "https") + res := redirectTest(HTTPSRedirect, "labstack.com", header) + + assert.Equal(t, http.StatusOK, res.Code) +} + func TestRedirectHTTPSWWWRedirect(t *testing.T) { - res := redirectTest(HTTPSWWWRedirect, "labstack.com") + res := redirectTest(HTTPSWWWRedirect, "labstack.com", nil) assert.Equal(t, http.StatusMovedPermanently, res.Code) assert.Equal(t, "https://www.labstack.com/", res.Header().Get(echo.HeaderLocation)) } +func TestRedirectHTTPSWWWRedirectBehindTLSTerminationProxy(t *testing.T) { + header := http.Header{} + header.Set(echo.HeaderXForwardedProto, "https") + res := redirectTest(HTTPSWWWRedirect, "labstack.com", header) + + assert.Equal(t, http.StatusOK, res.Code) +} + func TestRedirectHTTPSNonWWWRedirect(t *testing.T) { - res := redirectTest(HTTPSNonWWWRedirect, "www.labstack.com") + res := redirectTest(HTTPSNonWWWRedirect, "www.labstack.com", nil) assert.Equal(t, http.StatusMovedPermanently, res.Code) assert.Equal(t, "https://labstack.com/", res.Header().Get(echo.HeaderLocation)) } +func TestRedirectHTTPSNonWWWRedirectBehindTLSTerminationProxy(t *testing.T) { + header := http.Header{} + header.Set(echo.HeaderXForwardedProto, "https") + res := redirectTest(HTTPSNonWWWRedirect, "www.labstack.com", header) + + assert.Equal(t, http.StatusOK, res.Code) +} + func TestRedirectWWWRedirect(t *testing.T) { - res := redirectTest(WWWRedirect, "labstack.com") + res := redirectTest(WWWRedirect, "labstack.com", nil) assert.Equal(t, http.StatusMovedPermanently, res.Code) assert.Equal(t, "http://www.labstack.com/", res.Header().Get(echo.HeaderLocation)) } func TestRedirectNonWWWRedirect(t *testing.T) { - res := redirectTest(NonWWWRedirect, "www.labstack.com") + res := redirectTest(NonWWWRedirect, "www.labstack.com", nil) assert.Equal(t, http.StatusMovedPermanently, res.Code) assert.Equal(t, "http://labstack.com/", res.Header().Get(echo.HeaderLocation)) } -func redirectTest(fn middlewareGenerator, host string) *httptest.ResponseRecorder { +func redirectTest(fn middlewareGenerator, host string, header http.Header) *httptest.ResponseRecorder { e := echo.New() next := func(c echo.Context) (err error) { return c.NoContent(http.StatusOK) } req := httptest.NewRequest(echo.GET, "/", nil) req.Host = host + if header != nil { + req.Header = header + } res := httptest.NewRecorder() c := e.NewContext(req, res)