From f9aabb263ee7179403fd8b611ef9823a0ef0c6e8 Mon Sep 17 00:00:00 2001
From: Manuel Alejandro de Brito Fontes <aledbf@gmail.com>
Date: Thu, 19 Mar 2020 15:49:18 -0300
Subject: [PATCH] Redirect for app-root should preserve current scheme (#5266)

---
 internal/ingress/annotations/rewrite/main.go  | 27 +++++++++-
 .../ingress/annotations/rewrite/main_test.go  | 50 ++++++++++++++-----
 rootfs/etc/nginx/template/nginx.tmpl          |  6 +--
 test/e2e/annotations/approot.go               |  2 +-
 4 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/internal/ingress/annotations/rewrite/main.go b/internal/ingress/annotations/rewrite/main.go
index 82d1ff282..9ac5c0b7e 100644
--- a/internal/ingress/annotations/rewrite/main.go
+++ b/internal/ingress/annotations/rewrite/main.go
@@ -17,9 +17,13 @@ limitations under the License.
 package rewrite
 
 import (
+	"net/url"
+
 	networking "k8s.io/api/networking/v1beta1"
+	"k8s.io/klog"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
+	"k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
@@ -90,8 +94,29 @@ func (a rewrite) Parse(ing *networking.Ingress) (interface{}, error) {
 		config.ForceSSLRedirect = a.r.GetDefaultBackend().ForceSSLRedirect
 	}
 
-	config.AppRoot, _ = parser.GetStringAnnotation("app-root", ing)
 	config.UseRegex, _ = parser.GetBoolAnnotation("use-regex", ing)
 
+	config.AppRoot, err = parser.GetStringAnnotation("app-root", ing)
+	if err != nil {
+		if !errors.IsMissingAnnotations(err) && !errors.IsInvalidContent(err) {
+			klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		}
+
+		return config, nil
+	}
+
+	u, err := url.ParseRequestURI(config.AppRoot)
+	if err != nil {
+		klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		config.AppRoot = ""
+		return config, nil
+	}
+
+	if u.IsAbs() {
+		klog.Warningf("Annotation app-root only allows absolute paths (%v)", config.AppRoot)
+		config.AppRoot = ""
+		return config, nil
+	}
+
 	return config, nil
 }
diff --git a/internal/ingress/annotations/rewrite/main_test.go b/internal/ingress/annotations/rewrite/main_test.go
index 03afaa3c0..beece494a 100644
--- a/internal/ingress/annotations/rewrite/main_test.go
+++ b/internal/ingress/annotations/rewrite/main_test.go
@@ -41,8 +41,9 @@ func buildIngress() *networking.Ingress {
 
 	return &networking.Ingress{
 		ObjectMeta: meta_v1.ObjectMeta{
-			Name:      "foo",
-			Namespace: api.NamespaceDefault,
+			Name:        "foo",
+			Namespace:   api.NamespaceDefault,
+			Annotations: map[string]string{},
 		},
 		Spec: networking.IngressSpec{
 			Backend: &networking.IngressBackend{
@@ -163,19 +164,42 @@ func TestForceSSLRedirect(t *testing.T) {
 	}
 }
 func TestAppRoot(t *testing.T) {
-	ing := buildIngress()
+	ap := NewParser(mockBackend{redirect: true})
 
-	data := map[string]string{}
-	data[parser.GetAnnotationWithPrefix("app-root")] = "/app1"
-	ing.SetAnnotations(data)
-
-	i, _ := NewParser(mockBackend{redirect: true}).Parse(ing)
-	redirect, ok := i.(*Config)
-	if !ok {
-		t.Errorf("expected a App Context")
+	testCases := []struct {
+		title       string
+		path        string
+		expected    string
+		errExpected bool
+	}{
+		{"Empty path should return an error", "", "", true},
+		{"Relative paths are not allowed", "demo", "", true},
+		{"Path / should pass", "/", "/", false},
+		{"Path /demo should pass", "/demo", "/demo", false},
 	}
-	if redirect.AppRoot != "/app1" {
-		t.Errorf("Unexpected value got in AppRoot")
+
+	for _, testCase := range testCases {
+		t.Run(testCase.title, func(t *testing.T) {
+			ing := buildIngress()
+			ing.Annotations[parser.GetAnnotationWithPrefix("app-root")] = testCase.path
+			i, err := ap.Parse(ing)
+			if err != nil {
+				if testCase.errExpected {
+					return
+				}
+
+				t.Fatalf("%v: unexpected error obtaining running address/es: %v", testCase.title, err)
+			}
+
+			rewrite, ok := i.(*Config)
+			if !ok {
+				t.Fatalf("expected a rewrite Config")
+			}
+
+			if testCase.expected != rewrite.AppRoot {
+				t.Fatalf("%v: expected AppRoot with value %v but was returned: %v", testCase.title, testCase.expected, rewrite.AppRoot)
+			}
+		})
 	}
 }
 
diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
index 76856ee3f..52277075e 100755
--- a/rootfs/etc/nginx/template/nginx.tmpl
+++ b/rootfs/etc/nginx/template/nginx.tmpl
@@ -862,7 +862,7 @@ stream {
         proxy_ssl_verify_depth                  {{ $server.ProxySSL.VerifyDepth }};
         {{ if not (empty $server.ProxySSL.ProxySSLName) }}
         proxy_ssl_name                           {{ $server.ProxySSL.ProxySSLName }};
-        {{ end }}        
+        {{ end }}
         {{ end }}
 
         {{ if not (empty $server.ProxySSL.PemFileName) }}
@@ -896,9 +896,9 @@ stream {
         {{ $externalAuth = $all.Cfg.GlobalExternalAuth }}
         {{ end }}
 
-        {{ if not (empty $location.Rewrite.AppRoot)}}
+        {{ if not (empty $location.Rewrite.AppRoot) }}
         if ($uri = /) {
-            return 302 {{ $location.Rewrite.AppRoot }};
+            return 302 $scheme://$http_host{{ $location.Rewrite.AppRoot }};
         }
         {{ end }}
 
diff --git a/test/e2e/annotations/approot.go b/test/e2e/annotations/approot.go
index a43d2e3ba..a73c60253 100644
--- a/test/e2e/annotations/approot.go
+++ b/test/e2e/annotations/approot.go
@@ -45,7 +45,7 @@ var _ = framework.DescribeAnnotation("app-root", func() {
 		f.WaitForNginxServer(host,
 			func(server string) bool {
 				return strings.Contains(server, `if ($uri = /) {`) &&
-					strings.Contains(server, `return 302 /foo;`)
+					strings.Contains(server, `return 302 $scheme://$http_host/foo;`)
 			})
 
 		f.HTTPTestClient().