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.

Author: orangain

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)
 			}
 

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)