[release-11.4.9] Forbid more redirect patterns (#110506)

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
 	}
 

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())
 				})

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)
 }

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.m.Get(url.QueryEscape(path), sc.defaultHandler)
+			sc.fakeReq("GET", fmt.Sprintf("%s?orgId=3", path)).exec()
-		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)
+		}
 	})
 }