From be022d4239c982a4d99687a03413375584d69e1a Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki Date: Wed, 11 Mar 2020 11:04:48 +0200 Subject: [PATCH] API: Fix redirect issues (#22285) * Revert "API: Fix redirect issue when configured to use a subpath (#21652)" (#22671) This reverts commit 0e2d874ecf9277dcc17d562e05271917efc8b595. * Fix redirect validation (#22675) * Chore: Add test for parse of app url and app sub url Co-authored-by: Marcus Efraimsson * 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 --- pkg/api/login.go | 17 +- pkg/api/login_oauth.go | 9 +- pkg/api/login_test.go | 145 +++++++++--------- pkg/middleware/auth.go | 6 +- pkg/setting/setting_test.go | 27 ++++ .../app/core/components/Login/LoginCtrl.tsx | 12 +- public/sass/components/_buttons.scss | 4 +- 7 files changed, 127 insertions(+), 93 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index 5667f0741ed..b4bb82a1766 100644 --- a/pkg/api/login.go +++ b/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) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 5aae3a78220..018b9356c07 100644 --- a/pkg/api/login_oauth.go +++ b/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 + "/") diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index 76f2b53d32d..4ff624ac21a 100644 --- a/pkg/api/login_test.go +++ b/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, }, } diff --git a/pkg/middleware/auth.go b/pkg/middleware/auth.go index 4f491cfca2b..d61914a2eb7 100644 --- a/pkg/middleware/auth.go +++ b/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") } diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index 95f6e2b7d91..2a339ee9766 100644 --- a/pkg/setting/setting_test.go +++ b/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) + } +} diff --git a/public/app/core/components/Login/LoginCtrl.tsx b/public/app/core/components/Login/LoginCtrl.tsx index cf5c385dca0..5442e0ed05a 100644 --- a/public/app/core/components/Login/LoginCtrl.tsx +++ b/public/app/core/components/Login/LoginCtrl.tsx @@ -104,9 +104,17 @@ export class LoginCtrl extends PureComponent { 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 + '/'; } diff --git a/public/sass/components/_buttons.scss b/public/sass/components/_buttons.scss index f248d031f7c..f2649faedbb 100644 --- a/public/sass/components/_buttons.scss +++ b/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%;