From b12dc890b890ae6320f4f1b02caf27f40941207a Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki Date: Thu, 12 Dec 2019 17:08:34 +0200 Subject: [PATCH] API: Validate redirect_to cookie has valid (Grafana) url (#21057) * Restrict redirect_to to valid relative paths * Add tests --- pkg/api/login.go | 29 +++++- pkg/api/login_test.go | 212 +++++++++++++++++++++++++++++++++++++++++- pkg/login/auth.go | 2 + 3 files changed, 240 insertions(+), 3 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index fd11d9c428a..e9248da247c 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -4,6 +4,7 @@ import ( "encoding/hex" "net/http" "net/url" + "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" @@ -27,6 +28,20 @@ var getViewIndex = func() string { return ViewIndex } +func validateRedirectTo(redirectTo string) error { + to, err := url.Parse(redirectTo) + if err != nil { + return login.ErrInvalidRedirectTo + } + if to.IsAbs() { + return login.ErrAbsoluteRedirectTo + } + if setting.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+setting.AppSubUrl) { + return login.ErrInvalidRedirectTo + } + return nil +} + func (hs *HTTPServer) LoginView(c *models.ReqContext) { viewData, err := setIndexViewData(hs, c) if err != nil { @@ -64,6 +79,12 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { } if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 { + if err := validateRedirectTo(redirectTo); err != nil { + viewData.Settings["loginError"] = err.Error() + c.HTML(200, getViewIndex(), viewData) + c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") + return + } c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") c.Redirect(redirectTo) return @@ -73,7 +94,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { return } - c.HTML(200, ViewIndex, viewData) + c.HTML(200, getViewIndex(), viewData) } func (hs *HTTPServer) loginAuthProxyUser(c *models.ReqContext) { @@ -147,7 +168,11 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res } if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 { - result["redirectUrl"] = redirectTo + if err := validateRedirectTo(redirectTo); err == nil { + result["redirectUrl"] = redirectTo + } else { + log.Info("Ignored invalid redirect_to cookie value: %v", redirectTo) + } c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") } diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index 48a5d1a0f74..428e146f3a5 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -10,7 +10,10 @@ import ( "testing" "github.com/grafana/grafana/pkg/api/dtos" + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/setting" @@ -53,6 +56,22 @@ func getBody(resp *httptest.ResponseRecorder) (string, error) { return string(responseData), nil } +type FakeLogger struct { + log.Logger +} + +func (stub *FakeLogger) Info(testMessage string, ctx ...interface{}) { +} + +type redirectCase struct { + desc string + url string + status int + err error + appURL string + appSubURL string +} + func TestLoginErrorCookieApiEndpoint(t *testing.T) { mockSetIndexViewData() defer resetSetIndexViewData() @@ -100,10 +119,201 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) { assert.Equal(t, sc.resp.Code, 200) responseString, err := getBody(sc.resp) - assert.Nil(t, err) + assert.NoError(t, err) assert.True(t, strings.Contains(responseString, oauthError.Error())) } +func TestLoginViewRedirect(t *testing.T) { + mockSetIndexViewData() + defer resetSetIndexViewData() + + mockViewIndex() + defer resetViewIndex() + sc := setupScenarioContext("/login") + hs := &HTTPServer{ + Cfg: setting.NewCfg(), + License: models.OSSLicensingService{}, + } + + sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) { + c.IsSignedIn = true + c.SignedInUser = &models.SignedInUser{ + UserId: 10, + } + hs.LoginView(c) + }) + + setting.OAuthService = &setting.OAuther{} + setting.OAuthService.OAuthInfos = make(map[string]*setting.OAuthInfo) + + redirectCases := []redirectCase{ + { + desc: "grafana relative url without subpath", + url: "/profile", + appURL: "http://localhost:3000", + status: 302, + }, + { + desc: "grafana relative url with subpath", + url: "/grafana/profile", + appURL: "http://localhost:3000", + appSubURL: "grafana", + status: 302, + }, + { + desc: "relative url with missing subpath", + url: "/profile", + appURL: "http://localhost:3000", + appSubURL: "grafana", + status: 200, + err: login.ErrInvalidRedirectTo, + }, + { + desc: "grafana absolute url", + url: "http://localhost:3000/profile", + appURL: "http://localhost:3000", + status: 200, + err: login.ErrAbsoluteRedirectTo, + }, + { + desc: "non grafana absolute url", + url: "http://example.com", + appURL: "http://localhost:3000", + status: 200, + err: login.ErrAbsoluteRedirectTo, + }, + { + desc: "invalid url", + url: ":foo", + appURL: "http://localhost:3000", + status: 200, + err: login.ErrInvalidRedirectTo, + }, + } + + for _, c := range redirectCases { + setting.AppUrl = c.appURL + setting.AppSubUrl = c.appSubURL + t.Run(c.desc, func(t *testing.T) { + cookie := http.Cookie{ + Name: "redirect_to", + MaxAge: 60, + Value: c.url, + HttpOnly: true, + Path: setting.AppSubUrl + "/", + Secure: hs.Cfg.CookieSecure, + SameSite: hs.Cfg.CookieSameSite, + } + sc.m.Get(sc.url, sc.defaultHandler) + sc.fakeReqNoAssertionsWithCookie("GET", sc.url, cookie).exec() + assert.Equal(t, c.status, sc.resp.Code) + if c.status == 302 { + location, ok := sc.resp.Header()["Location"] + assert.True(t, ok) + assert.Equal(t, location[0], c.url) + } + + responseString, err := getBody(sc.resp) + assert.NoError(t, err) + if c.err != nil { + assert.True(t, strings.Contains(responseString, c.err.Error())) + } + }) + } +} + +func TestLoginPostRedirect(t *testing.T) { + mockSetIndexViewData() + defer resetSetIndexViewData() + + mockViewIndex() + defer resetViewIndex() + sc := setupScenarioContext("/login") + hs := &HTTPServer{ + log: &FakeLogger{}, + Cfg: setting.NewCfg(), + License: models.OSSLicensingService{}, + AuthTokenService: auth.NewFakeUserAuthTokenService(), + } + + sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response { + cmd := dtos.LoginCommand{ + User: "admin", + Password: "admin", + } + return hs.LoginPost(c, cmd) + }) + + bus.AddHandler("grafana-auth", func(query *models.LoginUserQuery) error { + query.User = &models.User{ + Id: 42, + Email: "", + } + return nil + }) + + redirectCases := []redirectCase{ + { + desc: "grafana relative url without subpath", + url: "/profile", + appURL: "https://localhost:3000", + }, + { + desc: "grafana relative url with subpath", + url: "/grafana/profile", + appURL: "https://localhost:3000", + appSubURL: "grafana", + }, + { + desc: "relative url with missing subpath", + url: "/profile", + appURL: "https://localhost:3000", + appSubURL: "grafana", + err: login.ErrInvalidRedirectTo, + }, + { + desc: "grafana absolute url", + url: "http://localhost:3000/profile", + appURL: "http://localhost:3000", + err: login.ErrAbsoluteRedirectTo, + }, + { + desc: "non grafana absolute url", + url: "http://example.com", + appURL: "https://localhost:3000", + err: login.ErrAbsoluteRedirectTo, + }, + } + + for _, c := range redirectCases { + setting.AppUrl = c.appURL + setting.AppSubUrl = c.appSubURL + t.Run(c.desc, func(t *testing.T) { + cookie := http.Cookie{ + Name: "redirect_to", + MaxAge: 60, + Value: c.url, + HttpOnly: true, + Path: setting.AppSubUrl + "/", + Secure: hs.Cfg.CookieSecure, + SameSite: hs.Cfg.CookieSameSite, + } + sc.m.Post(sc.url, sc.defaultHandler) + sc.fakeReqNoAssertionsWithCookie("POST", sc.url, cookie).exec() + assert.Equal(t, sc.resp.Code, 200) + + respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes()) + assert.NoError(t, err) + redirectURL := respJSON.Get("redirectUrl").MustString() + if c.err != nil { + assert.Equal(t, "", redirectURL) + } else { + assert.Equal(t, c.url, redirectURL) + } + }) + } +} + func TestLoginOAuthRedirect(t *testing.T) { mockSetIndexViewData() defer resetSetIndexViewData() diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 7b384336d35..98ad360a1f2 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -18,6 +18,8 @@ var ( ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked") ErrPasswordEmpty = errors.New("No password provided") ErrUserDisabled = errors.New("User is disabled") + ErrAbsoluteRedirectTo = errors.New("Absolute urls are not allowed for redirect_to cookie value") + ErrInvalidRedirectTo = errors.New("Invalid redirect_to cookie value") ) var loginLogger = log.New("login")