Skip to content

N
nexpie-grafana-theme
Commit d94796a0 by Erik Sundell Committed by GitHub
Options

Auth: Remove trailing / in cookies' path (#22265) (#22265) 

According to the stackoverflow answer below, it is recommended to not
include a trailing / in cookies' path. By removing the trailing / for
our cookies path value, people's browsers visiting grafana will pass the
cookie not only to /grafana/ sub paths but also to /grafana sub paths.

This commit avoids the situation where a user would visit
http://localhost/grafana, get redirected to
http://localhost/grafana/login, and following login get redirected back
to http://localhost/grafana, but since the grafana_session cookie isn't
passed along get redirected back once more to
http://localhost/grafana/login.

ref: https://stackoverflow.com/questions/36131023/setting-a-slash-on-cookie-path/53784228#53784228
ref: https://tools.ietf.org/html/rfc6265#section-5.1.4
parent 2299e6bf
Showing with 38 additions and 11 deletions
pkg/api/login.go
......@@ -46,8 +46,12 @@ func (hs *HTTPServer) validateRedirectTo(redirectTo string) error {
}
func (hs *HTTPServer) cookieOptionsFromCfg() middleware.CookieOptions {
path := "/"
if len(hs.Cfg.AppSubUrl) > 0 {
path = hs.Cfg.AppSubUrl
}
return middleware.CookieOptions{
Path: hs.Cfg.AppSubUrl + "/",
Path: path,
Secure: hs.Cfg.CookieSecure,
SameSiteDisabled: hs.Cfg.CookieSameSiteDisabled,
SameSiteMode: hs.Cfg.CookieSameSiteMode,
......
pkg/api/login_test.go
......@@ -10,8 +10,6 @@ import (
"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"
......@@ -19,6 +17,7 @@ import (
"github.com/grafana/grafana/pkg/login"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/services/licensing"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util"
"github.com/stretchr/testify/assert"
......@@ -109,12 +108,16 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) {
oauthError := errors.New("User not a member of one of the required organizations")
encryptedError, _ := util.Encrypt([]byte(oauthError.Error()), setting.SecretKey)
expCookiePath := "/"
if len(setting.AppSubUrl) > 0 {
expCookiePath = setting.AppSubUrl
}
cookie := http.Cookie{
Name: LoginErrorCookieName,
MaxAge: 60,
Value: hex.EncodeToString(encryptedError),
HttpOnly: true,
Path: setting.AppSubUrl + "/",
Path: expCookiePath,
Secure: hs.Cfg.CookieSecure,
SameSite: hs.Cfg.CookieSameSiteMode,
}
......@@ -210,12 +213,16 @@ func TestLoginViewRedirect(t *testing.T) {
hs.Cfg.AppUrl = c.appURL
hs.Cfg.AppSubUrl = c.appSubURL
t.Run(c.desc, func(t *testing.T) {
expCookiePath := "/"
if len(hs.Cfg.AppSubUrl) > 0 {
expCookiePath = hs.Cfg.AppSubUrl
}
cookie := http.Cookie{
Name: "redirect_to",
MaxAge: 60,
Value: c.url,
HttpOnly: true,
Path: hs.Cfg.AppSubUrl + "/",
Path: expCookiePath,
Secure: hs.Cfg.CookieSecure,
SameSite: hs.Cfg.CookieSameSiteMode,
}
......@@ -238,7 +245,7 @@ func TestLoginViewRedirect(t *testing.T) {
expCookieValue = ""
expCookieMaxAge = 0
}
expCookie := fmt.Sprintf("redirect_to=%v; Path=%v; Max-Age=%v; HttpOnly; Secure", expCookieValue, hs.Cfg.AppSubUrl+"/", expCookieMaxAge)
expCookie := fmt.Sprintf("redirect_to=%v; Path=%v; Max-Age=%v; HttpOnly; Secure", expCookieValue, expCookiePath, expCookieMaxAge)
for _, cookieValue := range setCookie {
if cookieValue == expCookie {
redirectToCookieFound = true
......@@ -332,12 +339,16 @@ func TestLoginPostRedirect(t *testing.T) {
hs.Cfg.AppUrl = c.appURL
hs.Cfg.AppSubUrl = c.appSubURL
t.Run(c.desc, func(t *testing.T) {
expCookiePath := "/"
if len(hs.Cfg.AppSubUrl) > 0 {
expCookiePath = hs.Cfg.AppSubUrl
}
cookie := http.Cookie{
Name: "redirect_to",
MaxAge: 60,
Value: c.url,
HttpOnly: true,
Path: hs.Cfg.AppSubUrl + "/",
Path: expCookiePath,
Secure: hs.Cfg.CookieSecure,
SameSite: hs.Cfg.CookieSameSiteMode,
}
......@@ -358,7 +369,7 @@ func TestLoginPostRedirect(t *testing.T) {
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+"/")
expCookieValue := fmt.Sprintf("redirect_to=; Path=%v; Max-Age=0; HttpOnly; Secure", expCookiePath)
for _, cookieValue := range setCookie {
if cookieValue == expCookieValue {
redirectToCookieFound = true
......
pkg/middleware/cookie.go
......@@ -14,8 +14,12 @@ type CookieOptions struct {
}
func newCookieOptions() CookieOptions {
path := "/"
if len(setting.AppSubUrl) > 0 {
path = setting.AppSubUrl
}
return CookieOptions{
Path: setting.AppSubUrl + "/",
Path: path,
Secure: setting.CookieSecure,
SameSiteDisabled: setting.CookieSameSiteDisabled,
SameSiteMode: setting.CookieSameSiteMode,
......
pkg/middleware/middleware_test.go
......@@ -264,10 +264,14 @@ func TestMiddlewareContext(t *testing.T) {
}
for _, sameSitePolicy := range sameSitePolicies {
setting.CookieSameSiteMode = sameSitePolicy
expectedCookiePath := "/"
if len(setting.AppSubUrl) > 0 {
expectedCookiePath = setting.AppSubUrl
}
expectedCookie := &http.Cookie{
Name: setting.LoginCookieName,
Value: "rotated",
Path: setting.AppSubUrl + "/",
Path: expectedCookiePath,
HttpOnly: true,
MaxAge: int(maxAge),
Secure: setting.CookieSecure,
......@@ -291,10 +295,14 @@ func TestMiddlewareContext(t *testing.T) {
Convey("Should not set cookie with SameSite attribute when setting.CookieSameSiteDisabled is true", func() {
setting.CookieSameSiteDisabled = true
setting.CookieSameSiteMode = http.SameSiteLaxMode
expectedCookiePath := "/"
if len(setting.AppSubUrl) > 0 {
expectedCookiePath = setting.AppSubUrl
}
expectedCookie := &http.Cookie{
Name: setting.LoginCookieName,
Value: "rotated",
Path: setting.AppSubUrl + "/",
Path: expectedCookiePath,
HttpOnly: true,
MaxAge: int(maxAge),
Secure: setting.CookieSecure,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment