diff --git a/pkg/api/login.go b/pkg/api/login.go index 88a4d8dc29b..7e17b94eabe 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -41,7 +41,7 @@ var getViewIndex = func() string { return viewIndex } -// Only allow redirects that start with an alphanumerical character, a dash or an underscore. +// Only allow redirects that start with a slash followed by an alphanumerical character, a dash or an underscore. var redirectRe = regexp.MustCompile(`^/[a-zA-Z0-9-_].*`) var ( @@ -73,12 +73,17 @@ func (hs *HTTPServer) ValidateRedirectTo(redirectTo string) error { return errForbiddenRedirectTo } + if to.Path != "/" && !redirectRe.MatchString(to.Path) { + return errForbiddenRedirectTo + } + cleanPath := path.Clean(to.Path) // "." is what path.Clean returns for empty paths if cleanPath == "." { return errForbiddenRedirectTo } - if to.Path != "/" && !redirectRe.MatchString(cleanPath) { + + if cleanPath != "/" && !redirectRe.MatchString(cleanPath) { return errForbiddenRedirectTo } diff --git a/pkg/api/user_token_test.go b/pkg/api/user_token_test.go index 204ab6af6e6..76378da9af5 100644 --- a/pkg/api/user_token_test.go +++ b/pkg/api/user_token_test.go @@ -166,6 +166,7 @@ func TestHTTPServer_RotateUserAuthTokenRedirect(t *testing.T) { // Invalid redirects should be converted to root {"backslash domain", `/\grafana.com`, "/"}, + {"backslash domain at the start of the path", `/\grafana.com/../a`, "/"}, {"traversal backslash domain", `/a/../\grafana.com`, "/"}, {"double slash", "//grafana", "/"}, {"missing initial slash", "missingInitialSlash", "/"}, @@ -232,7 +233,7 @@ func TestHTTPServer_RotateUserAuthTokenRedirect(t *testing.T) { res, err := server.Send(req) require.NoError(t, err) assert.Equal(t, 302, redirectStatusCode) - assert.Equal(t, redirectCase.expectedUrl, redirectLocation) + assert.Equal(t, redirectCase.expectedUrl, redirectLocation, "redirectTo=%s", redirectCase.redirectUrl) require.NoError(t, res.Body.Close()) }) diff --git a/pkg/middleware/org_redirect.go b/pkg/middleware/org_redirect.go index c59dd26aaab..e5ed22a9154 100644 --- a/pkg/middleware/org_redirect.go +++ b/pkg/middleware/org_redirect.go @@ -14,7 +14,7 @@ import ( "github.com/grafana/grafana/pkg/web" ) -// Only allow redirects that start with an alphanumerical character, a dash or an underscore. +// Only allow redirects that start with a slash followed by an alphanumerical character, a dash or an underscore. var redirectRe = regexp.MustCompile(`^/?[a-zA-Z0-9-_].*`) // OrgRedirect changes org and redirects users if the @@ -67,6 +67,9 @@ func OrgRedirect(cfg *setting.Cfg, userSvc user.Service) web.Handler { } func validRedirectPath(p string) bool { + if p != "" && p != "/" && !redirectRe.MatchString(p) { + return false + } cleanPath := path.Clean(p) return cleanPath == "." || cleanPath == "/" || redirectRe.MatchString(cleanPath) } diff --git a/pkg/middleware/org_redirect_test.go b/pkg/middleware/org_redirect_test.go index 29e9e80519a..b67135dda2b 100644 --- a/pkg/middleware/org_redirect_test.go +++ b/pkg/middleware/org_redirect_test.go @@ -71,12 +71,17 @@ func TestOrgRedirectMiddleware(t *testing.T) { }) middlewareScenario(t, "when redirecting to an invalid path", func(t *testing.T, sc *scenarioContext) { - sc.withIdentity(&authn.Identity{}) + testPaths := []string{ + url.QueryEscape(`/\example.com`), + `/%2fexample.com`, + } + for _, path := range testPaths { + sc.withIdentity(&authn.Identity{}) - path := url.QueryEscape(`/\example.com`) - sc.m.Get(url.QueryEscape(path), sc.defaultHandler) - sc.fakeReq("GET", fmt.Sprintf("%s?orgId=3", path)).exec() + sc.m.Get(url.QueryEscape(path), sc.defaultHandler) + sc.fakeReq("GET", fmt.Sprintf("%s?orgId=3", path)).exec() - require.Equal(t, 404, sc.resp.Code) + require.Equal(t, 404, sc.resp.Code, "path: %s", path) + } }) }