PMM-12182 fix (#2201)

* PMM-12182 Fix the bug. * PMM-12182 Fix linter. * PMM-12182 Improve error message. * Apply suggestions from code review Co-authored-by: Artem Gavrilov <[email protected]> --------- Co-authored-by: Artem Gavrilov <[email protected]>
percona · May 31, 2023 · 5d6fdcc · 5d6fdcc
1 parent f85fa9a
commit 5d6fdcc
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 4 deletions.
diff --git a/managed/services/grafana/auth_server.go b/managed/services/grafana/auth_server.go
@@ -23,6 +23,8 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httputil"
+	"net/url"
+	"path"
 	"strings"
 	"sync"
 	"time"
@@ -459,8 +461,19 @@ func nextPrefix(path string) string {
 // Paths which require no Grafana role return zero value for
 // some user fields such as authUser.userID.
 func (s *AuthServer) authenticate(ctx context.Context, req *http.Request, l *logrus.Entry) (*authUser, *authError) {
+	// Unescape the URL-encoded parts of the path.
+	p := req.URL.Path
+	cleanedPath, err := cleanPath(p)
+	if err != nil {
+		l.Warnf("Error while unescaping path %s: %q", p, err)
+		return nil, &authError{
+			code:    codes.Internal,
+			message: "Internal server error.",
+		}
+	}
+
 	// find the longest prefix present in rules
-	prefix := req.URL.Path
+	prefix := cleanedPath
 	for prefix != "/" {
 		if _, ok := rules[prefix]; ok {
 			break
@@ -483,9 +496,9 @@ func (s *AuthServer) authenticate(ctx context.Context, req *http.Request, l *log
 	}
 
 	// Get authenticated user from Grafana
-	authUser, err := s.getAuthUser(ctx, req, l)
-	if err != nil {
-		return nil, err
+	authUser, authErr := s.getAuthUser(ctx, req, l)
+	if authErr != nil {
+		return nil, authErr
 	}
 
 	l = l.WithField("role", authUser.role.String())
@@ -504,6 +517,15 @@ func (s *AuthServer) authenticate(ctx context.Context, req *http.Request, l *log
 	return nil, &authError{code: codes.PermissionDenied, message: "Access denied."}
 }
 
+func cleanPath(p string) (string, error) {
+	unescaped, err := url.PathUnescape(p)
+	if err != nil {
+		return "", err
+	}
+
+	return path.Clean(unescaped), nil
+}
+
 func (s *AuthServer) getAuthUser(ctx context.Context, req *http.Request, l *logrus.Entry) (*authUser, *authError) {
 	// check Grafana with some headers from request
 	authHeaders := make(http.Header)

diff --git a/managed/services/grafana/auth_server_test.go b/managed/services/grafana/auth_server_test.go
@@ -49,6 +49,7 @@ func TestNextPrefix(t *testing.T) {
 		{"./", "/", "/"},
 		{"hax0r", "/", "/"},
 		{"", "/"},
+		{"/v1/AWSInstanceCheck/..%2finventory/Services/List'"},
 	} {
 		t.Run(paths[0], func(t *testing.T) {
 			for i, path := range paths[:len(paths)-1] {
@@ -220,6 +221,9 @@ func TestAuthServerAuthenticate(t *testing.T) {
 		"/v1/AWSInstanceCheck":                             none,
 		"/v1/Platform/Connect":                             admin,
 
+		"/v1/AWSInstanceCheck/..%2finventory/Services/List": admin,
+		"/v1/AWSInstanceCheck/..%2f..%2flogs.zip":           admin,
+
 		"/v1/readyz": none,
 		"/ping":      none,
 
@@ -378,3 +382,31 @@ func TestAuthServerAddVMGatewayToken(t *testing.T) {
 		require.Equal(t, parsed[1], "filter B")
 	})
 }
+
+func Test_cleanPath(t *testing.T) {
+    t.Parallel()
+	tests := []struct {
+		path     string
+		expected string
+	}{
+		{
+			"/v1/AWSInstanceCheck/..%2finventory/Services/List",
+			"/v1/inventory/Services/List",
+		}, {
+			"/v1/AWSInstanceCheck/..%2f..%2fmanaged/logs.zip",
+			"/managed/logs.zip",
+		}, {
+			"/v1/AWSInstanceCheck/..%2f..%2f/logs.zip",
+			"/logs.zip",
+		},
+	}
+	for _, tt := range tests {
+	tt := tt
+		t.Run(tt.path, func(t *testing.T) {
+		t.Parallel()
+			cleanedPath, err := cleanPath(tt.path)
+			require.NoError(t, err)
+			assert.Equalf(t, tt.expected, cleanedPath, "cleanPath(%v)", tt.path)
+		})
+	}
+}