Skip to content

N
nexpie-grafana-theme
Commit b12dc890 by Sofia Papagiannaki Committed by GitHub
Options

API: Validate redirect_to cookie has valid (Grafana) url (#21057) 

* Restrict redirect_to to valid relative paths

* Add tests
parent cd39c2bd
Showing with 240 additions and 3 deletions
pkg/api/login.go
...@@ -4,6 +4,7 @@ import ( ...@@ -4,6 +4,7 @@ import (
"encoding/hex" "encoding/hex"
"net/http" "net/http"
"net/url" "net/url"
"strings"
"github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/dtos"
"github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/bus"
...@@ -27,6 +28,20 @@ var getViewIndex = func() string { ...@@ -27,6 +28,20 @@ var getViewIndex = func() string {
return ViewIndex 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) { func (hs *HTTPServer) LoginView(c *models.ReqContext) {
viewData, err := setIndexViewData(hs, c) viewData, err := setIndexViewData(hs, c)
if err != nil { if err != nil {
...@@ -64,6 +79,12 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { ...@@ -64,6 +79,12 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
} }
if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 { 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.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
c.Redirect(redirectTo) c.Redirect(redirectTo)
return return
...@@ -73,7 +94,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) { ...@@ -73,7 +94,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
return return
} }
c.HTML(200, ViewIndex, viewData) c.HTML(200, getViewIndex(), viewData)
} }
func (hs *HTTPServer) loginAuthProxyUser(c *models.ReqContext) { func (hs *HTTPServer) loginAuthProxyUser(c *models.ReqContext) {
...@@ -147,7 +168,11 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res ...@@ -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 { 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+"/") c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
} }
......
pkg/api/login_test.go
...@@ -10,7 +10,10 @@ import ( ...@@ -10,7 +10,10 @@ import (
"testing" "testing"
"github.com/grafana/grafana/pkg/api/dtos" "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/infra/log"
"github.com/grafana/grafana/pkg/login"
"github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/setting"
...@@ -53,6 +56,22 @@ func getBody(resp *httptest.ResponseRecorder) (string, error) { ...@@ -53,6 +56,22 @@ func getBody(resp *httptest.ResponseRecorder) (string, error) {
return string(responseData), nil 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) { func TestLoginErrorCookieApiEndpoint(t *testing.T) {
mockSetIndexViewData() mockSetIndexViewData()
defer resetSetIndexViewData() defer resetSetIndexViewData()
...@@ -100,10 +119,201 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) { ...@@ -100,10 +119,201 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) {
assert.Equal(t, sc.resp.Code, 200) assert.Equal(t, sc.resp.Code, 200)
responseString, err := getBody(sc.resp) responseString, err := getBody(sc.resp)
assert.Nil(t, err) assert.NoError(t, err)
assert.True(t, strings.Contains(responseString, oauthError.Error())) 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) { func TestLoginOAuthRedirect(t *testing.T) {
mockSetIndexViewData() mockSetIndexViewData()
defer resetSetIndexViewData() defer resetSetIndexViewData()
......
pkg/login/auth.go
...@@ -18,6 +18,8 @@ var ( ...@@ -18,6 +18,8 @@ var (
ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked") ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked")
ErrPasswordEmpty = errors.New("No password provided") ErrPasswordEmpty = errors.New("No password provided")
ErrUserDisabled = errors.New("User is disabled") 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") var loginLogger = log.New("login")
......
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