Data proxy: Fix encoded characters in URL path should be proxied enco…

…ded (#30597) (#32060)

Fix encoded characters in URL path should be proxied as encoded in the data proxy.

Fixes #26870
Fixes #31438

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
(cherry picked from commit c0edf88)

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

devenv/docker/blocks/slow_proxy/Dockerfile

@@ -1,7 +1,7 @@
 
-FROM golang:latest 
+FROM golang:latest
 ADD main.go /
 WORKDIR /
-RUN go build -o main . 
+RUN GO111MODULE=off go build -o main .
 EXPOSE 3011
 ENTRYPOINT ["/main"]

devenv/docker/blocks/slow_proxy_mac/Dockerfile

@@ -1,7 +1,7 @@
 
-FROM golang:latest 
+FROM golang:latest
 ADD main.go /
 WORKDIR /
-RUN go build -o main . 
+RUN GO111MODULE=off go build -o main .
 EXPOSE 3011
 ENTRYPOINT ["/main"]

pkg/api/dataproxy.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"regexp"
 
 	"github.com/grafana/grafana/pkg/api/datasource"
 	"github.com/grafana/grafana/pkg/api/pluginproxy"
@@ -40,9 +41,7 @@ func (hs *HTTPServer) ProxyDataSourceRequest(c *models.ReqContext) {
 		return
 	}
 
-	// macaron does not include trailing slashes when resolving a wildcard path
-	proxyPath := ensureProxyPathTrailingSlash(c.Req.URL.Path, c.Params("*"))
-
+	proxyPath := getProxyPath(c)
 	proxy, err := pluginproxy.NewDataSourceProxy(ds, plugin, c, proxyPath, hs.Cfg)
 	if err != nil {
 		if errors.Is(err, datasource.URLValidationError{}) {
@@ -55,14 +54,12 @@ func (hs *HTTPServer) ProxyDataSourceRequest(c *models.ReqContext) {
 	proxy.HandleRequest()
 }
 
-// ensureProxyPathTrailingSlash Check for a trailing slash in original path and makes
-// sure that a trailing slash is added to proxy path, if not already exists.
-func ensureProxyPathTrailingSlash(originalPath, proxyPath string) string {
-	if len(proxyPath) > 1 {
-		if originalPath[len(originalPath)-1] == '/' && proxyPath[len(proxyPath)-1] != '/' {
-			return proxyPath + "/"
-		}
-	}
+var proxyPathRegexp = regexp.MustCompile(`^\/api\/datasources\/proxy\/[\d]+\/?`)
+
+func extractProxyPath(originalRawPath string) string {
+	return proxyPathRegexp.ReplaceAllString(originalRawPath, "")
+}
 
-	return proxyPath
+func getProxyPath(c *models.ReqContext) string {
+	return extractProxyPath(c.Req.URL.EscapedPath())
 }

pkg/api/dataproxy_test.go

@@ -7,28 +7,28 @@ import (
 )
 
 func TestDataProxy(t *testing.T) {
-	testCases := []struct {
-		desc      string
-		origPath  string
-		proxyPath string
-		exp       string
-	}{
-		{
-			"Should append trailing slash to proxy path if original path has a trailing slash",
-			"/api/datasources/proxy/6/api/v1/query_range/",
-			"api/v1/query_range/",
-			"api/v1/query_range/",
-		},
-		{
-			"Should not append trailing slash to proxy path if original path doesn't have a trailing slash",
-			"/api/datasources/proxy/6/api/v1/query_range",
-			"api/v1/query_range",
-			"api/v1/query_range",
-		},
-	}
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			assert.Equal(t, tc.exp, ensureProxyPathTrailingSlash(tc.origPath, tc.proxyPath))
-		})
-	}
+	t.Run("extractProxyPath", func(t *testing.T) {
+		testCases := []struct {
+			originalRawPath string
+			exp             string
+		}{
+			{
+				"/api/datasources/proxy/1",
+				"",
+			},
+			{
+				"/api/datasources/proxy/1/some/thing",
+				"some/thing",
+			},
+			{
+				"/api/datasources/proxy/54/api/services/afsd%2Fafsd/operations",
+				"api/services/afsd%2Fafsd/operations",
+			},
+		}
+		for _, tc := range testCases {
+			t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) {
+				assert.Equal(t, tc.exp, extractProxyPath(tc.originalRawPath))
+			})
+		}
+	})
 }

pkg/api/pluginproxy/ds_proxy.go

@@ -179,20 +179,28 @@ func (proxy *DataSourceProxy) director(req *http.Request) {
 
 	switch proxy.ds.Type {
 	case models.DS_INFLUXDB_08:
-		req.URL.Path = util.JoinURLFragments(proxy.targetUrl.Path, "db/"+proxy.ds.Database+"/"+proxy.proxyPath)
+		req.URL.RawPath = util.JoinURLFragments(proxy.targetUrl.Path, "db/"+proxy.ds.Database+"/"+proxy.proxyPath)
 		reqQueryVals.Add("u", proxy.ds.User)
 		reqQueryVals.Add("p", proxy.ds.DecryptedPassword())
 		req.URL.RawQuery = reqQueryVals.Encode()
 	case models.DS_INFLUXDB:
-		req.URL.Path = util.JoinURLFragments(proxy.targetUrl.Path, proxy.proxyPath)
+		req.URL.RawPath = util.JoinURLFragments(proxy.targetUrl.Path, proxy.proxyPath)
 		req.URL.RawQuery = reqQueryVals.Encode()
 		if !proxy.ds.BasicAuth {
 			req.Header.Set("Authorization", util.GetBasicAuthHeader(proxy.ds.User, proxy.ds.DecryptedPassword()))
 		}
 	default:
-		req.URL.Path = util.JoinURLFragments(proxy.targetUrl.Path, proxy.proxyPath)
+		req.URL.RawPath = util.JoinURLFragments(proxy.targetUrl.Path, proxy.proxyPath)
 	}
 
+	unescapedPath, err := url.PathUnescape(req.URL.RawPath)
+	if err != nil {
+		logger.Error("Failed to unescape raw path", "rawPath", req.URL.RawPath, "error", err)
+		return
+	}
+
+	req.URL.Path = unescapedPath
+
 	if proxy.ds.BasicAuth {
 		req.Header.Set("Authorization", util.GetBasicAuthHeader(proxy.ds.BasicAuthUser,
 			proxy.ds.DecryptedBasicAuthPassword()))

pkg/api/pluginproxy/ds_proxy_test.go

@@ -527,7 +527,7 @@ func TestDataSourceProxy_requestHandling(t *testing.T) {
 
 	type setUpCfg struct {
 		headers map[string]string
-		writeCb func(w http.ResponseWriter)
+		writeCb func(w http.ResponseWriter, r *http.Request)
 	}
 
 	setUp := func(t *testing.T, cfgs ...setUpCfg) (*models.ReqContext, *models.DataSource) {
@@ -539,7 +539,7 @@ func TestDataSourceProxy_requestHandling(t *testing.T) {
 			for _, cfg := range cfgs {
 				if cfg.writeCb != nil {
 					t.Log("Writing response via callback")
-					cfg.writeCb(w)
+					cfg.writeCb(w, r)
 					written = true
 				}
 			}
@@ -607,7 +607,7 @@ func TestDataSourceProxy_requestHandling(t *testing.T) {
 
 	t.Run("Data source returns status code 401", func(t *testing.T) {
 		ctx, ds := setUp(t, setUpCfg{
-			writeCb: func(w http.ResponseWriter) {
+			writeCb: func(w http.ResponseWriter, r *http.Request) {
 				w.WriteHeader(401)
 				w.Header().Set("www-authenticate", `Basic realm="Access to the server"`)
 				_, err := w.Write([]byte("Not authenticated"))
@@ -624,6 +624,28 @@ func TestDataSourceProxy_requestHandling(t *testing.T) {
 		assert.Equal(t, 400, proxy.ctx.Resp.Status(), "Status code 401 should be converted to 400")
 		assert.Empty(t, proxy.ctx.Resp.Header().Get("www-authenticate"))
 	})
+
+	t.Run("Data source should handle proxy path url encoding correctly", func(t *testing.T) {
+		var req *http.Request
+		ctx, ds := setUp(t, setUpCfg{
+			writeCb: func(w http.ResponseWriter, r *http.Request) {
+				req = r
+				w.WriteHeader(200)
+				_, err := w.Write([]byte("OK"))
+				require.NoError(t, err)
+			},
+		})
+
+		ctx.Req.Request = httptest.NewRequest("GET", "/api/datasources/proxy/1/path/%2Ftest%2Ftest%2F?query=%2Ftest%2Ftest%2F", nil)
+		proxy, err := NewDataSourceProxy(ds, plugin, ctx, "/path/%2Ftest%2Ftest%2F", &setting.Cfg{})
+		require.NoError(t, err)
+
+		proxy.HandleRequest()
+
+		require.NoError(t, writeErr)
+		require.NotNil(t, req)
+		require.Equal(t, "/path/%2Ftest%2Ftest%2F?query=%2Ftest%2Ftest%2F", req.RequestURI)
+	})
 }
 
 func TestNewDataSourceProxy_InvalidURL(t *testing.T) {