API: Fix redirect issues (#22285)

* Revert "API: Fix redirect issue when configured to use a subpath (#21652)" (#22671)

This reverts commit 0e2d874ecf.

* Fix redirect validation (#22675)

* Chore: Add test for parse of app url and app sub url

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

* Fix redirect: prepend subpath only if it's missing (#22676)

* Validate redirect in login oauth (#22677)

* Fix invalid redirect for authenticated user (#22678)

* Login: Use correct path for OAuth logos

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

pkg/api/login.go

@@ -35,9 +35,9 @@ func (hs *HTTPServer) validateRedirectTo(redirectTo string) error {
 	if to.IsAbs() {
 		return login.ErrAbsoluteRedirectTo
 	}
-	// when using a subUrl, the redirect_to should have a relative or absolute path that includes the subUrl, otherwise the redirect
+	// when using a subUrl, the redirect_to should start with the subUrl (which contains the leading slash), otherwise the redirect
 	// will send the user to the wrong location
-	if hs.Cfg.AppSubUrl != "" && !strings.HasPrefix(to.Path, hs.Cfg.AppSubUrl) && !strings.HasPrefix(to.Path, "/"+hs.Cfg.AppSubUrl) {
+	if hs.Cfg.AppSubUrl != "" && !strings.HasPrefix(to.Path, hs.Cfg.AppSubUrl+"/") {
 		return login.ErrInvalidRedirectTo
 	}
 	return nil
@@ -90,10 +90,10 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
 
 		if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
 			if err := hs.validateRedirectTo(redirectTo); err != nil {
-				viewData.Settings["loginError"] = err.Error()
-				c.HTML(200, getViewIndex(), viewData)
-				middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg)
-				return
+				// the user is already logged so instead of rendering the login page with error
+				// it should be redirected to the home page.
+				log.Debug("Ignored invalid redirect_to cookie value: %v", redirectTo)
+				redirectTo = hs.Cfg.AppSubUrl + "/"
 			}
 			middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg)
 			c.Redirect(redirectTo)
@@ -179,11 +179,6 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
 
 	if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
 		if err := hs.validateRedirectTo(redirectTo); err == nil {
-			// remove subpath if it exists at the beginning of the redirect_to
-			// LoginCtrl.tsx is already prepending the redirectUrl with the subpath
-			if setting.AppSubUrl != "" && strings.Index(redirectTo, setting.AppSubUrl) == 0 {
-				redirectTo = strings.Replace(redirectTo, setting.AppSubUrl, "", 1)
-			}
 			result["redirectUrl"] = redirectTo
 		} else {
 			log.Info("Ignored invalid redirect_to cookie value: %v", redirectTo)

pkg/api/login_oauth.go

@@ -223,9 +223,12 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
 	metrics.MApiLoginOAuth.Inc()
 
 	if redirectTo, _ := url.QueryUnescape(ctx.GetCookie("redirect_to")); len(redirectTo) > 0 {
-		middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg)
-		ctx.Redirect(redirectTo)
-		return
+		if err := hs.validateRedirectTo(redirectTo); err == nil {
+			middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg)
+			ctx.Redirect(redirectTo)
+			return
+		}
+		log.Debug("Ignored invalid redirect_to cookie value: %v", redirectTo)
 	}
 
 	ctx.Redirect(setting.AppSubUrl + "/")

pkg/api/login_test.go

@@ -4,13 +4,14 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"github.com/grafana/grafana/pkg/services/licensing"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
+	"github.com/grafana/grafana/pkg/services/licensing"
+
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/simplejson"
@@ -66,13 +67,13 @@ func (stub *FakeLogger) Info(testMessage string, ctx ...interface{}) {
 }
 
 type redirectCase struct {
-	desc      string
-	url       string
-	status    int
-	err       error
-	appURL    string
-	appSubURL string
-	path      string
+	desc        string
+	url         string
+	status      int
+	err         error
+	appURL      string
+	appSubURL   string
+	redirectURL string
 }
 
 func TestLoginErrorCookieApiEndpoint(t *testing.T) {
@@ -152,68 +153,56 @@ func TestLoginViewRedirect(t *testing.T) {
 
 	redirectCases := []redirectCase{
 		{
-			desc:   "grafana relative url without subpath",
-			url:    "/profile",
-			appURL: "http://localhost:3000",
-			path:   "/",
-			status: 302,
+			desc:        "grafana relative url without subpath",
+			url:         "/profile",
+			redirectURL: "/profile",
+			appURL:      "http://localhost:3000/",
+			status:      302,
 		},
 		{
-			desc:      "grafana relative url with subpath",
-			url:       "/grafana/profile",
-			appURL:    "http://localhost:3000",
-			appSubURL: "grafana",
-			path:      "grafana/",
-			status:    302,
+			desc:        "grafana invalid relative url starting with the subpath",
+			url:         "/grafanablah",
+			redirectURL: "/grafana/",
+			appURL:      "http://localhost:3000/",
+			appSubURL:   "/grafana",
+			status:      302,
 		},
 		{
-			desc:      "grafana slashed relative url with subpath",
-			url:       "/grafana/profile",
-			appURL:    "http://localhost:3000",
-			appSubURL: "grafana",
-			path:      "/grafana/",
-			status:    302,
+			desc:        "grafana relative url with subpath with leading slash",
+			url:         "/grafana/profile",
+			redirectURL: "/grafana/profile",
+			appURL:      "http://localhost:3000",
+			appSubURL:   "/grafana",
+			status:      302,
 		},
 		{
-			desc:      "relative url with missing subpath",
-			url:       "/profile",
-			appURL:    "http://localhost:3000",
-			appSubURL: "grafana",
-			path:      "grafana/",
-			status:    200,
-			err:       login.ErrInvalidRedirectTo,
+			desc:        "relative url with missing subpath",
+			url:         "/profile",
+			redirectURL: "/grafana/",
+			appURL:      "http://localhost:3000/",
+			appSubURL:   "/grafana",
+			status:      302,
 		},
 		{
-			desc:      "grafana subpath absolute url",
-			url:       "http://localhost:3000/grafana/profile",
-			appURL:    "http://localhost:3000",
-			appSubURL: "grafana",
-			path:      "/grafana/profile",
-			status:    200,
+			desc:        "grafana absolute url",
+			url:         "http://localhost:3000/profile",
+			redirectURL: "/",
+			appURL:      "http://localhost:3000/",
+			status:      302,
 		},
 		{
-			desc:   "grafana absolute url",
-			url:    "http://localhost:3000/profile",
-			appURL: "http://localhost:3000",
-			path:   "/",
-			status: 200,
-			err:    login.ErrAbsoluteRedirectTo,
+			desc:        "non grafana absolute url",
+			url:         "http://example.com",
+			redirectURL: "/",
+			appURL:      "http://localhost:3000/",
+			status:      302,
 		},
 		{
-			desc:   "non grafana absolute url",
-			url:    "http://example.com",
-			appURL: "http://localhost:3000",
-			path:   "/",
-			status: 200,
-			err:    login.ErrAbsoluteRedirectTo,
-		},
-		{
-			desc:   "invalid url",
-			url:    ":foo",
-			appURL: "http://localhost:3000",
-			path:   "/",
-			status: 200,
-			err:    login.ErrInvalidRedirectTo,
+			desc:        "invalid url",
+			url:         ":foo",
+			redirectURL: "/",
+			appURL:      "http://localhost:3000/",
+			status:      302,
 		},
 	}
 
@@ -226,7 +215,7 @@ func TestLoginViewRedirect(t *testing.T) {
 				MaxAge:   60,
 				Value:    c.url,
 				HttpOnly: true,
-				Path:     c.path,
+				Path:     hs.Cfg.AppSubUrl + "/",
 				Secure:   hs.Cfg.CookieSecure,
 				SameSite: hs.Cfg.CookieSameSiteMode,
 			}
@@ -236,15 +225,22 @@ func TestLoginViewRedirect(t *testing.T) {
 			if c.status == 302 {
 				location, ok := sc.resp.Header()["Location"]
 				assert.True(t, ok)
-				assert.Equal(t, location[0], c.url)
+				assert.Equal(t, location[0], c.redirectURL)
 
 				setCookie, ok := sc.resp.Header()["Set-Cookie"]
 				assert.True(t, ok, "Set-Cookie exists")
 				assert.Greater(t, len(setCookie), 0)
 				var redirectToCookieFound bool
-				expCookieValue := fmt.Sprintf("redirect_to=%v; Path=%v; Max-Age=60; HttpOnly; Secure", c.url, c.path)
+				redirectToCookieShouldBeDeleted := c.url != c.redirectURL
+				expCookieValue := c.redirectURL
+				expCookieMaxAge := 60
+				if redirectToCookieShouldBeDeleted {
+					expCookieValue = ""
+					expCookieMaxAge = 0
+				}
+				expCookie := fmt.Sprintf("redirect_to=%v; Path=%v; Max-Age=%v; HttpOnly; Secure", expCookieValue, hs.Cfg.AppSubUrl+"/", expCookieMaxAge)
 				for _, cookieValue := range setCookie {
-					if cookieValue == expCookieValue {
+					if cookieValue == expCookie {
 						redirectToCookieFound = true
 						break
 					}
@@ -296,37 +292,38 @@ func TestLoginPostRedirect(t *testing.T) {
 		{
 			desc:   "grafana relative url without subpath",
 			url:    "/profile",
-			appURL: "https://localhost:3000",
+			appURL: "https://localhost:3000/",
 		},
 		{
-			desc:      "grafana relative url with subpath",
+			desc:      "grafana relative url with subpath with leading slash",
 			url:       "/grafana/profile",
-			appURL:    "https://localhost:3000",
-			appSubURL: "grafana",
+			appURL:    "https://localhost:3000/",
+			appSubURL: "/grafana",
 		},
 		{
-			desc:      "grafana no slash relative url with subpath",
-			url:       "grafana/profile",
-			appURL:    "https://localhost:3000",
-			appSubURL: "grafana",
+			desc:      "grafana invalid relative url starting with subpath",
+			url:       "/grafanablah",
+			appURL:    "https://localhost:3000/",
+			appSubURL: "/grafana",
+			err:       login.ErrInvalidRedirectTo,
 		},
 		{
 			desc:      "relative url with missing subpath",
 			url:       "/profile",
-			appURL:    "https://localhost:3000",
-			appSubURL: "grafana",
+			appURL:    "https://localhost:3000/",
+			appSubURL: "/grafana",
 			err:       login.ErrInvalidRedirectTo,
 		},
 		{
 			desc:   "grafana absolute url",
 			url:    "http://localhost:3000/profile",
-			appURL: "http://localhost:3000",
+			appURL: "http://localhost:3000/",
 			err:    login.ErrAbsoluteRedirectTo,
 		},
 		{
 			desc:   "non grafana absolute url",
 			url:    "http://example.com",
-			appURL: "https://localhost:3000",
+			appURL: "https://localhost:3000/",
 			err:    login.ErrAbsoluteRedirectTo,
 		},
 	}

pkg/middleware/auth.go

@@ -47,7 +47,11 @@ func notAuthorized(c *models.ReqContext) {
 		return
 	}
 
-	WriteCookie(c.Resp, "redirect_to", url.QueryEscape(c.Req.RequestURI), 0, newCookieOptions)
+	redirectTo := c.Req.RequestURI
+	if setting.AppSubUrl != "" && !strings.HasPrefix(redirectTo, setting.AppSubUrl) {
+		redirectTo = setting.AppSubUrl + c.Req.RequestURI
+	}
+	WriteCookie(c.Resp, "redirect_to", url.QueryEscape(redirectTo), 0, newCookieOptions)
 
 	c.Redirect(setting.AppSubUrl + "/login")
 }

pkg/setting/setting_test.go

@@ -9,6 +9,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"gopkg.in/ini.v1"
 
 	. "github.com/smartystreets/goconvey/convey"
@@ -298,3 +300,28 @@ func TestLoadingSettings(t *testing.T) {
 
 	})
 }
+
+func TestParseAppUrlAndSubUrl(t *testing.T) {
+	testCases := []struct {
+		rootURL           string
+		expectedAppURL    string
+		expectedAppSubURL string
+	}{
+		{rootURL: "http://localhost:3000/", expectedAppURL: "http://localhost:3000/"},
+		{rootURL: "http://localhost:3000", expectedAppURL: "http://localhost:3000/"},
+		{rootURL: "http://localhost:3000/grafana", expectedAppURL: "http://localhost:3000/grafana/", expectedAppSubURL: "/grafana"},
+		{rootURL: "http://localhost:3000/grafana/", expectedAppURL: "http://localhost:3000/grafana/", expectedAppSubURL: "/grafana"},
+	}
+
+	for _, tc := range testCases {
+		f := ini.Empty()
+		s, err := f.NewSection("server")
+		require.NoError(t, err)
+		_, err = s.NewKey("root_url", tc.rootURL)
+		require.NoError(t, err)
+		appURL, appSubURL, err := parseAppUrlAndSubUrl(s)
+		require.NoError(t, err)
+		require.Equal(t, tc.expectedAppURL, appURL)
+		require.Equal(t, tc.expectedAppSubURL, appSubURL)
+	}
+}

public/app/core/components/Login/LoginCtrl.tsx

@@ -104,9 +104,17 @@ export class LoginCtrl extends PureComponent<Props, State> {
     const params = this.props.routeParams;
     // Use window.location.href to force page reload
     if (params.redirect && params.redirect[0] === '/') {
-      window.location.href = config.appSubUrl + params.redirect;
+      if (config.appSubUrl !== '' && !params.redirect.startsWith(config.appSubUrl)) {
+        window.location.href = config.appSubUrl + params.redirect;
+      } else {
+        window.location.href = params.redirect;
+      }
     } else if (this.result.redirectUrl) {
-      window.location.href = config.appSubUrl + this.result.redirectUrl;
+      if (config.appSubUrl !== '' && !this.result.redirectUrl.startsWith(config.appSubUrl)) {
+        window.location.href = config.appSubUrl + this.result.redirectUrl;
+      } else {
+        window.location.href = this.result.redirectUrl;
+      }
     } else {
       window.location.href = config.appSubUrl + '/';
     }

public/sass/components/_buttons.scss

@@ -209,7 +209,7 @@ $btn-service-icon-width: 35px;
 
 .btn-service--grafanacom {
   .btn-service-icon {
-    background-image: url(/public/img/grafana_mask_icon_white.svg);
+    background-image: url(../img/grafana_mask_icon_white.svg);
     background-repeat: no-repeat;
     background-position: 50%;
     background-size: 60%;
@@ -218,7 +218,7 @@ $btn-service-icon-width: 35px;
 
 .btn-service--azuread {
   .btn-service-icon {
-    background-image: url(/public/img/microsoft_auth_icon.svg);
+    background-image: url(../img/microsoft_auth_icon.svg);
     background-repeat: no-repeat;
     background-position: 50%;
     background-size: 60%;