diff --git a/pkg/api/login.go b/pkg/api/login.go index e9248da247c..48af35c2ef4 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -2,7 +2,6 @@ package api import ( "encoding/hex" - "net/http" "net/url" "strings" @@ -28,7 +27,7 @@ var getViewIndex = func() string { return ViewIndex } -func validateRedirectTo(redirectTo string) error { +func (hs *HTTPServer) validateRedirectTo(redirectTo string) error { to, err := url.Parse(redirectTo) if err != nil { return login.ErrInvalidRedirectTo @@ -36,12 +35,20 @@ func validateRedirectTo(redirectTo string) error { if to.IsAbs() { return login.ErrAbsoluteRedirectTo } - if setting.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+setting.AppSubUrl) { + if hs.Cfg.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+hs.Cfg.AppSubUrl) { return login.ErrInvalidRedirectTo } return nil } +func (hs *HTTPServer) cookieOptionsFromCfg() middleware.CookieOptions { + return middleware.CookieOptions{ + Path: hs.Cfg.AppSubUrl + "/", + Secure: hs.Cfg.CookieSecure, + SameSite: hs.Cfg.CookieSameSite, + } +} + func (hs *HTTPServer) LoginView(c *models.ReqContext) { viewData, err := setIndexViewData(hs, c) if err != nil { @@ -62,7 +69,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { //therefore the loginError should be passed to the view data //and the view should return immediately before attempting //to login again via OAuth and enter to a redirect loop - deleteCookie(c, LoginErrorCookieName) + middleware.DeleteCookie(c.Resp, LoginErrorCookieName, hs.cookieOptionsFromCfg) viewData.Settings["loginError"] = loginError c.HTML(200, getViewIndex(), viewData) return @@ -79,13 +86,13 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { } if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 { - if err := validateRedirectTo(redirectTo); err != nil { + if err := hs.validateRedirectTo(redirectTo); err != nil { viewData.Settings["loginError"] = err.Error() c.HTML(200, getViewIndex(), viewData) - c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") + middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg) return } - c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") + middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg) c.Redirect(redirectTo) return } @@ -168,12 +175,12 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res } if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 { - if err := validateRedirectTo(redirectTo); err == nil { + if err := hs.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+"/") + middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg) } metrics.MApiLoginPost.Inc() @@ -223,28 +230,13 @@ func tryGetEncryptedCookie(ctx *models.ReqContext, cookieName string) (string, b return string(decryptedError), err == nil } -func deleteCookie(ctx *models.ReqContext, cookieName string) { - ctx.SetCookie(cookieName, "", -1, setting.AppSubUrl+"/") -} - func (hs *HTTPServer) trySetEncryptedCookie(ctx *models.ReqContext, cookieName string, value string, maxAge int) error { encryptedError, err := util.Encrypt([]byte(value), setting.SecretKey) if err != nil { return err } - cookie := http.Cookie{ - Name: cookieName, - MaxAge: 60, - Value: hex.EncodeToString(encryptedError), - HttpOnly: true, - Path: setting.AppSubUrl + "/", - Secure: hs.Cfg.CookieSecure, - } - if hs.Cfg.CookieSameSite != http.SameSiteDefaultMode { - cookie.SameSite = hs.Cfg.CookieSameSite - } - http.SetCookie(ctx.Resp, &cookie) + middleware.WriteCookie(ctx.Resp, cookieName, hex.EncodeToString(encryptedError), 60, hs.cookieOptionsFromCfg) return nil } diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 53349906939..c8c9f05b334 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -20,6 +20,7 @@ import ( "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" ) @@ -69,7 +70,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) { } hashedState := hashStatecode(state, setting.OAuthService.OAuthInfos[name].ClientSecret) - hs.writeCookie(ctx.Resp, OauthStateCookieName, hashedState, 60, hs.Cfg.CookieSameSite) + middleware.WriteCookie(ctx.Resp, OauthStateCookieName, hashedState, 60, hs.cookieOptionsFromCfg) if setting.OAuthService.OAuthInfos[name].HostedDomain == "" { ctx.Redirect(connect.AuthCodeURL(state, oauth2.AccessTypeOnline)) } else { @@ -81,8 +82,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) { cookieState := ctx.GetCookie(OauthStateCookieName) // delete cookie - ctx.Resp.Header().Del("Set-Cookie") - hs.deleteCookie(ctx.Resp, OauthStateCookieName, hs.Cfg.CookieSameSite) + middleware.DeleteCookie(ctx.Resp, OauthStateCookieName, hs.cookieOptionsFromCfg) if cookieState == "" { ctx.Handle(500, "login.OAuthLogin(missing saved state)", nil) @@ -217,7 +217,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) { metrics.MApiLoginOAuth.Inc() if redirectTo, _ := url.QueryUnescape(ctx.GetCookie("redirect_to")); len(redirectTo) > 0 { - ctx.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/") + middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg) ctx.Redirect(redirectTo) return } @@ -225,25 +225,6 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) { ctx.Redirect(setting.AppSubUrl + "/") } -func (hs *HTTPServer) deleteCookie(w http.ResponseWriter, name string, sameSite http.SameSite) { - hs.writeCookie(w, name, "", -1, sameSite) -} - -func (hs *HTTPServer) writeCookie(w http.ResponseWriter, name string, value string, maxAge int, sameSite http.SameSite) { - cookie := http.Cookie{ - Name: name, - MaxAge: maxAge, - Value: value, - HttpOnly: true, - Path: setting.AppSubUrl + "/", - Secure: hs.Cfg.CookieSecure, - } - if sameSite != http.SameSiteDefaultMode { - cookie.SameSite = sameSite - } - http.SetCookie(w, &cookie) -} - func hashStatecode(code, seed string) string { hashBytes := sha256.Sum256([]byte(code + setting.SecretKey + seed)) return hex.EncodeToString(hashBytes[:]) diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index 428e146f3a5..684f3ddccce 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -3,6 +3,7 @@ package api import ( "encoding/hex" "errors" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -134,6 +135,7 @@ func TestLoginViewRedirect(t *testing.T) { Cfg: setting.NewCfg(), License: models.OSSLicensingService{}, } + hs.Cfg.CookieSecure = true sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) { c.IsSignedIn = true @@ -192,15 +194,15 @@ func TestLoginViewRedirect(t *testing.T) { } for _, c := range redirectCases { - setting.AppUrl = c.appURL - setting.AppSubUrl = c.appSubURL + hs.Cfg.AppUrl = c.appURL + hs.Cfg.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 + "/", + Path: hs.Cfg.AppSubUrl + "/", Secure: hs.Cfg.CookieSecure, SameSite: hs.Cfg.CookieSameSite, } @@ -211,6 +213,19 @@ func TestLoginViewRedirect(t *testing.T) { location, ok := sc.resp.Header()["Location"] assert.True(t, ok) assert.Equal(t, location[0], c.url) + + 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, hs.Cfg.AppSubUrl+"/") + for _, cookieValue := range setCookie { + if cookieValue == expCookieValue { + redirectToCookieFound = true + break + } + } + assert.True(t, redirectToCookieFound) } responseString, err := getBody(sc.resp) @@ -235,6 +250,7 @@ func TestLoginPostRedirect(t *testing.T) { License: models.OSSLicensingService{}, AuthTokenService: auth.NewFakeUserAuthTokenService(), } + hs.Cfg.CookieSecure = true sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response { cmd := dtos.LoginCommand{ @@ -286,15 +302,15 @@ func TestLoginPostRedirect(t *testing.T) { } for _, c := range redirectCases { - setting.AppUrl = c.appURL - setting.AppSubUrl = c.appSubURL + hs.Cfg.AppUrl = c.appURL + hs.Cfg.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 + "/", + Path: hs.Cfg.AppSubUrl + "/", Secure: hs.Cfg.CookieSecure, SameSite: hs.Cfg.CookieSameSite, } @@ -310,6 +326,19 @@ func TestLoginPostRedirect(t *testing.T) { } else { assert.Equal(t, c.url, redirectURL) } + // assert redirect_to cookie is deleted + 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=; Path=%v; Max-Age=0; HttpOnly; Secure", hs.Cfg.AppSubUrl+"/") + for _, cookieValue := range setCookie { + if cookieValue == expCookieValue { + redirectToCookieFound = true + break + } + } + assert.True(t, redirectToCookieFound) }) } } diff --git a/pkg/middleware/auth.go b/pkg/middleware/auth.go index f5007be9a3c..aef8830f983 100644 --- a/pkg/middleware/auth.go +++ b/pkg/middleware/auth.go @@ -47,7 +47,7 @@ func notAuthorized(c *m.ReqContext) { return } - c.SetCookie("redirect_to", url.QueryEscape(setting.AppSubUrl+c.Req.RequestURI), 0, setting.AppSubUrl+"/", nil, false, true) + WriteCookie(c.Resp, "redirect_to", url.QueryEscape(setting.AppSubUrl+c.Req.RequestURI), 0, newCookieOptions) c.Redirect(setting.AppSubUrl + "/login") } diff --git a/pkg/middleware/cookie.go b/pkg/middleware/cookie.go new file mode 100644 index 00000000000..175aca60ce1 --- /dev/null +++ b/pkg/middleware/cookie.go @@ -0,0 +1,43 @@ +package middleware + +import ( + "net/http" + + "github.com/grafana/grafana/pkg/setting" +) + +type CookieOptions struct { + Path string + Secure bool + SameSite http.SameSite +} + +func newCookieOptions() CookieOptions { + return CookieOptions{ + Path: setting.AppSubUrl + "/", + Secure: setting.CookieSecure, + SameSite: setting.CookieSameSite, + } +} + +type GetCookieOptionsFunc func() CookieOptions + +func DeleteCookie(w http.ResponseWriter, name string, getCookieOptionsFunc GetCookieOptionsFunc) { + WriteCookie(w, name, "", -1, getCookieOptionsFunc) +} + +func WriteCookie(w http.ResponseWriter, name string, value string, maxAge int, getCookieOptionsFunc GetCookieOptionsFunc) { + options := getCookieOptionsFunc() + cookie := http.Cookie{ + Name: name, + MaxAge: maxAge, + Value: value, + HttpOnly: true, + Path: options.Path, + Secure: options.Secure, + } + if options.SameSite != http.SameSiteDefaultMode { + cookie.SameSite = options.SameSite + } + http.SetCookie(w, &cookie) +} diff --git a/pkg/middleware/middleware.go b/pkg/middleware/middleware.go index 1febc3f9fc5..d0e62da4098 100644 --- a/pkg/middleware/middleware.go +++ b/pkg/middleware/middleware.go @@ -2,7 +2,6 @@ package middleware import ( "fmt" - "net/http" "net/url" "strconv" "strings" @@ -253,20 +252,7 @@ func WriteSessionCookie(ctx *models.ReqContext, value string, maxLifetimeDays in maxAge = int(maxAgeHours.Seconds()) } - ctx.Resp.Header().Del("Set-Cookie") - cookie := http.Cookie{ - Name: setting.LoginCookieName, - Value: url.QueryEscape(value), - HttpOnly: true, - Path: setting.AppSubUrl + "/", - Secure: setting.CookieSecure, - MaxAge: maxAge, - } - if setting.CookieSameSite != http.SameSiteDefaultMode { - cookie.SameSite = setting.CookieSameSite - } - - http.SetCookie(ctx.Resp, &cookie) + WriteCookie(ctx.Resp, setting.LoginCookieName, url.QueryEscape(value), maxAge, newCookieOptions) } func AddDefaultResponseHeaders() macaron.Handler {