Skip to content

Commit b12dc89

Browse files
papagianpapagian
authored
API: Validate redirect_to cookie has valid (Grafana) url (grafana#21057)
* Restrict redirect_to to valid relative paths * Add tests
1 parent cd39c2b commit b12dc89

File tree

3 files changed

+240
-3
lines changed

3 files changed

+240
-3
lines changed

‎pkg/api/login.go

+27-2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"encoding/hex"
55
"net/http"
66
"net/url"
7+
"strings"
78

89
"github.com/grafana/grafana/pkg/api/dtos"
910
"github.com/grafana/grafana/pkg/bus"
@@ -27,6 +28,20 @@ var getViewIndex = func() string {
2728
return ViewIndex
2829
}
2930

31+
func validateRedirectTo(redirectTo string) error {
32+
to, err := url.Parse(redirectTo)
33+
if err != nil {
34+
return login.ErrInvalidRedirectTo
35+
}
36+
if to.IsAbs() {
37+
return login.ErrAbsoluteRedirectTo
38+
}
39+
if setting.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+setting.AppSubUrl) {
40+
return login.ErrInvalidRedirectTo
41+
}
42+
return nil
43+
}
44+
3045
func (hs *HTTPServer) LoginView(c *models.ReqContext) {
3146
viewData, err := setIndexViewData(hs, c)
3247
if err != nil {
@@ -64,6 +79,12 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
6479
}
6580

6681
if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
82+
if err := validateRedirectTo(redirectTo); err != nil {
83+
viewData.Settings["loginError"] = err.Error()
84+
c.HTML(200, getViewIndex(), viewData)
85+
c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
86+
return
87+
}
6788
c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
6889
c.Redirect(redirectTo)
6990
return
@@ -73,7 +94,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
7394
return
7495
}
7596

76-
c.HTML(200, ViewIndex, viewData)
97+
c.HTML(200, getViewIndex(), viewData)
7798
}
7899

79100
func (hs *HTTPServer) loginAuthProxyUser(c *models.ReqContext) {
@@ -147,7 +168,11 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
147168
}
148169

149170
if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
150-
result["redirectUrl"] = redirectTo
171+
if err := validateRedirectTo(redirectTo); err == nil {
172+
result["redirectUrl"] = redirectTo
173+
} else {
174+
log.Info("Ignored invalid redirect_to cookie value: %v", redirectTo)
175+
}
151176
c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
152177
}
153178

‎pkg/api/login_test.go

+211-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ import (
1010
"testing"
1111

1212
"github.com/grafana/grafana/pkg/api/dtos"
13+
"github.com/grafana/grafana/pkg/bus"
14+
"github.com/grafana/grafana/pkg/components/simplejson"
1315
"github.com/grafana/grafana/pkg/infra/log"
16+
"github.com/grafana/grafana/pkg/login"
1417
"github.com/grafana/grafana/pkg/models"
1518
"github.com/grafana/grafana/pkg/services/auth"
1619
"github.com/grafana/grafana/pkg/setting"
@@ -53,6 +56,22 @@ func getBody(resp *httptest.ResponseRecorder) (string, error) {
5356
return string(responseData), nil
5457
}
5558

59+
type FakeLogger struct {
60+
log.Logger
61+
}
62+
63+
func (stub *FakeLogger) Info(testMessage string, ctx ...interface{}) {
64+
}
65+
66+
type redirectCase struct {
67+
desc string
68+
url string
69+
status int
70+
err error
71+
appURL string
72+
appSubURL string
73+
}
74+
5675
func TestLoginErrorCookieApiEndpoint(t *testing.T) {
5776
mockSetIndexViewData()
5877
defer resetSetIndexViewData()
@@ -100,10 +119,201 @@ func TestLoginErrorCookieApiEndpoint(t *testing.T) {
100119
assert.Equal(t, sc.resp.Code, 200)
101120

102121
responseString, err := getBody(sc.resp)
103-
assert.Nil(t, err)
122+
assert.NoError(t, err)
104123
assert.True(t, strings.Contains(responseString, oauthError.Error()))
105124
}
106125

126+
func TestLoginViewRedirect(t *testing.T) {
127+
mockSetIndexViewData()
128+
defer resetSetIndexViewData()
129+
130+
mockViewIndex()
131+
defer resetViewIndex()
132+
sc := setupScenarioContext("/login")
133+
hs := &HTTPServer{
134+
Cfg: setting.NewCfg(),
135+
License: models.OSSLicensingService{},
136+
}
137+
138+
sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) {
139+
c.IsSignedIn = true
140+
c.SignedInUser = &models.SignedInUser{
141+
UserId: 10,
142+
}
143+
hs.LoginView(c)
144+
})
145+
146+
setting.OAuthService = &setting.OAuther{}
147+
setting.OAuthService.OAuthInfos = make(map[string]*setting.OAuthInfo)
148+
149+
redirectCases := []redirectCase{
150+
{
151+
desc: "grafana relative url without subpath",
152+
url: "/profile",
153+
appURL: "http://localhost:3000",
154+
status: 302,
155+
},
156+
{
157+
desc: "grafana relative url with subpath",
158+
url: "/grafana/profile",
159+
appURL: "http://localhost:3000",
160+
appSubURL: "grafana",
161+
status: 302,
162+
},
163+
{
164+
desc: "relative url with missing subpath",
165+
url: "/profile",
166+
appURL: "http://localhost:3000",
167+
appSubURL: "grafana",
168+
status: 200,
169+
err: login.ErrInvalidRedirectTo,
170+
},
171+
{
172+
desc: "grafana absolute url",
173+
url: "http://localhost:3000/profile",
174+
appURL: "http://localhost:3000",
175+
status: 200,
176+
err: login.ErrAbsoluteRedirectTo,
177+
},
178+
{
179+
desc: "non grafana absolute url",
180+
url: "http://example.com",
181+
appURL: "http://localhost:3000",
182+
status: 200,
183+
err: login.ErrAbsoluteRedirectTo,
184+
},
185+
{
186+
desc: "invalid url",
187+
url: ":foo",
188+
appURL: "http://localhost:3000",
189+
status: 200,
190+
err: login.ErrInvalidRedirectTo,
191+
},
192+
}
193+
194+
for _, c := range redirectCases {
195+
setting.AppUrl = c.appURL
196+
setting.AppSubUrl = c.appSubURL
197+
t.Run(c.desc, func(t *testing.T) {
198+
cookie := http.Cookie{
199+
Name: "redirect_to",
200+
MaxAge: 60,
201+
Value: c.url,
202+
HttpOnly: true,
203+
Path: setting.AppSubUrl + "/",
204+
Secure: hs.Cfg.CookieSecure,
205+
SameSite: hs.Cfg.CookieSameSite,
206+
}
207+
sc.m.Get(sc.url, sc.defaultHandler)
208+
sc.fakeReqNoAssertionsWithCookie("GET", sc.url, cookie).exec()
209+
assert.Equal(t, c.status, sc.resp.Code)
210+
if c.status == 302 {
211+
location, ok := sc.resp.Header()["Location"]
212+
assert.True(t, ok)
213+
assert.Equal(t, location[0], c.url)
214+
}
215+
216+
responseString, err := getBody(sc.resp)
217+
assert.NoError(t, err)
218+
if c.err != nil {
219+
assert.True(t, strings.Contains(responseString, c.err.Error()))
220+
}
221+
})
222+
}
223+
}
224+
225+
func TestLoginPostRedirect(t *testing.T) {
226+
mockSetIndexViewData()
227+
defer resetSetIndexViewData()
228+
229+
mockViewIndex()
230+
defer resetViewIndex()
231+
sc := setupScenarioContext("/login")
232+
hs := &HTTPServer{
233+
log: &FakeLogger{},
234+
Cfg: setting.NewCfg(),
235+
License: models.OSSLicensingService{},
236+
AuthTokenService: auth.NewFakeUserAuthTokenService(),
237+
}
238+
239+
sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response {
240+
cmd := dtos.LoginCommand{
241+
User: "admin",
242+
Password: "admin",
243+
}
244+
return hs.LoginPost(c, cmd)
245+
})
246+
247+
bus.AddHandler("grafana-auth", func(query *models.LoginUserQuery) error {
248+
query.User = &models.User{
249+
Id: 42,
250+
Email: "",
251+
}
252+
return nil
253+
})
254+
255+
redirectCases := []redirectCase{
256+
{
257+
desc: "grafana relative url without subpath",
258+
url: "/profile",
259+
appURL: "https://localhost:3000",
260+
},
261+
{
262+
desc: "grafana relative url with subpath",
263+
url: "/grafana/profile",
264+
appURL: "https://localhost:3000",
265+
appSubURL: "grafana",
266+
},
267+
{
268+
desc: "relative url with missing subpath",
269+
url: "/profile",
270+
appURL: "https://localhost:3000",
271+
appSubURL: "grafana",
272+
err: login.ErrInvalidRedirectTo,
273+
},
274+
{
275+
desc: "grafana absolute url",
276+
url: "http://localhost:3000/profile",
277+
appURL: "http://localhost:3000",
278+
err: login.ErrAbsoluteRedirectTo,
279+
},
280+
{
281+
desc: "non grafana absolute url",
282+
url: "http://example.com",
283+
appURL: "https://localhost:3000",
284+
err: login.ErrAbsoluteRedirectTo,
285+
},
286+
}
287+
288+
for _, c := range redirectCases {
289+
setting.AppUrl = c.appURL
290+
setting.AppSubUrl = c.appSubURL
291+
t.Run(c.desc, func(t *testing.T) {
292+
cookie := http.Cookie{
293+
Name: "redirect_to",
294+
MaxAge: 60,
295+
Value: c.url,
296+
HttpOnly: true,
297+
Path: setting.AppSubUrl + "/",
298+
Secure: hs.Cfg.CookieSecure,
299+
SameSite: hs.Cfg.CookieSameSite,
300+
}
301+
sc.m.Post(sc.url, sc.defaultHandler)
302+
sc.fakeReqNoAssertionsWithCookie("POST", sc.url, cookie).exec()
303+
assert.Equal(t, sc.resp.Code, 200)
304+
305+
respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes())
306+
assert.NoError(t, err)
307+
redirectURL := respJSON.Get("redirectUrl").MustString()
308+
if c.err != nil {
309+
assert.Equal(t, "", redirectURL)
310+
} else {
311+
assert.Equal(t, c.url, redirectURL)
312+
}
313+
})
314+
}
315+
}
316+
107317
func TestLoginOAuthRedirect(t *testing.T) {
108318
mockSetIndexViewData()
109319
defer resetSetIndexViewData()

‎pkg/login/auth.go

+2
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ var (
1818
ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked")
1919
ErrPasswordEmpty = errors.New("No password provided")
2020
ErrUserDisabled = errors.New("User is disabled")
21+
ErrAbsoluteRedirectTo = errors.New("Absolute urls are not allowed for redirect_to cookie value")
22+
ErrInvalidRedirectTo = errors.New("Invalid redirect_to cookie value")
2123
)
2224

2325
var loginLogger = log.New("login")

0 commit comments

Comments
 (0)