Browse Source

Backend: Do not set SameSite cookie attribute if cookie_samesite is none (#18462)

* Do not set SameSite login_error cookie attribute if cookie_samesite is none

* Do not set SameSite grafana_session cookie attribute if cookie_samesite is none

* Update middleware tests
Sofia Papagiannaki 6 years ago
parent
commit
4e29357d15
3 changed files with 37 additions and 22 deletions
  1. 6 3
      pkg/api/login.go
  2. 3 1
      pkg/middleware/middleware.go
  3. 28 18
      pkg/middleware/middleware_test.go

+ 6 - 3
pkg/api/login.go

@@ -199,15 +199,18 @@ func (hs *HTTPServer) trySetEncryptedCookie(ctx *models.ReqContext, cookieName s
 		return err
 	}
 
-	http.SetCookie(ctx.Resp, &http.Cookie{
+	cookie := http.Cookie{
 		Name:     cookieName,
 		MaxAge:   60,
 		Value:    hex.EncodeToString(encryptedError),
 		HttpOnly: true,
 		Path:     setting.AppSubUrl + "/",
 		Secure:   hs.Cfg.CookieSecure,
-		SameSite: hs.Cfg.CookieSameSite,
-	})
+	}
+	if hs.Cfg.CookieSameSite != http.SameSiteDefaultMode {
+		cookie.SameSite = hs.Cfg.CookieSameSite
+	}
+	http.SetCookie(ctx.Resp, &cookie)
 
 	return nil
 }

+ 3 - 1
pkg/middleware/middleware.go

@@ -256,7 +256,9 @@ func WriteSessionCookie(ctx *models.ReqContext, value string, maxLifetimeDays in
 		Path:     setting.AppSubUrl + "/",
 		Secure:   setting.CookieSecure,
 		MaxAge:   maxAge,
-		SameSite: setting.CookieSameSite,
+	}
+	if setting.CookieSameSite != http.SameSiteDefaultMode {
+		cookie.SameSite = setting.CookieSameSite
 	}
 
 	http.SetCookie(ctx.Resp, &cookie)

+ 28 - 18
pkg/middleware/middleware_test.go

@@ -252,28 +252,38 @@ func TestMiddlewareContext(t *testing.T) {
 			maxAgeHours := (time.Duration(setting.LoginMaxLifetimeDays) * 24 * time.Hour)
 			maxAge := (maxAgeHours + time.Hour).Seconds()
 
-			expectedCookie := &http.Cookie{
-				Name:     setting.LoginCookieName,
-				Value:    "rotated",
-				Path:     setting.AppSubUrl + "/",
-				HttpOnly: true,
-				MaxAge:   int(maxAge),
-				Secure:   setting.CookieSecure,
-				SameSite: setting.CookieSameSite,
+			sameSitePolicies := []http.SameSite{
+				http.SameSiteDefaultMode,
+				http.SameSiteLaxMode,
+				http.SameSiteStrictMode,
 			}
+			for _, sameSitePolicy := range sameSitePolicies {
+				setting.CookieSameSite = sameSitePolicy
+				expectedCookie := &http.Cookie{
+					Name:     setting.LoginCookieName,
+					Value:    "rotated",
+					Path:     setting.AppSubUrl + "/",
+					HttpOnly: true,
+					MaxAge:   int(maxAge),
+					Secure:   setting.CookieSecure,
+				}
+				if sameSitePolicy != http.SameSiteDefaultMode {
+					expectedCookie.SameSite = sameSitePolicy
+				}
 
-			sc.fakeReq("GET", "/").exec()
+				sc.fakeReq("GET", "/").exec()
 
-			Convey("Should init context with user info", func() {
-				So(sc.context.IsSignedIn, ShouldBeTrue)
-				So(sc.context.UserId, ShouldEqual, 12)
-				So(sc.context.UserToken.UserId, ShouldEqual, 12)
-				So(sc.context.UserToken.UnhashedToken, ShouldEqual, "rotated")
-			})
+				Convey(fmt.Sprintf("Should init context with user info and setting.SameSite=%v", sameSitePolicy), func() {
+					So(sc.context.IsSignedIn, ShouldBeTrue)
+					So(sc.context.UserId, ShouldEqual, 12)
+					So(sc.context.UserToken.UserId, ShouldEqual, 12)
+					So(sc.context.UserToken.UnhashedToken, ShouldEqual, "rotated")
+				})
 
-			Convey("Should set cookie", func() {
-				So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, expectedCookie.String())
-			})
+				Convey(fmt.Sprintf("Should set cookie with setting.SameSite=%v", sameSitePolicy), func() {
+					So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, expectedCookie.String())
+				})
+			}
 		})
 
 		middlewareScenario(t, "Invalid/expired auth token in cookie", func(sc *scenarioContext) {