From c0edf88f9f565eaa5549c221a2ded5fdb7d6ea25 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 17 Mar 2021 12:17:41 +0100 Subject: [PATCH] Data proxy: Fix encoded characters in URL path should be proxied encoded (#30597) Fix encoded characters in URL path should be proxied as encoded in the data proxy. Fixes #26870 Fixes #31438 Co-authored-by: Arve Knudsen --- devenv/docker/blocks/slow_proxy/Dockerfile | 4 +- .../docker/blocks/slow_proxy_mac/Dockerfile | 4 +- pkg/api/dataproxy.go | 21 ++++---- pkg/api/dataproxy_test.go | 48 +++++++++---------- pkg/api/pluginproxy/ds_proxy.go | 14 ++++-- pkg/api/pluginproxy/ds_proxy_test.go | 28 +++++++++-- 6 files changed, 73 insertions(+), 46 deletions(-) diff --git a/devenv/docker/blocks/slow_proxy/Dockerfile b/devenv/docker/blocks/slow_proxy/Dockerfile index e553cb6727c..d48c945f5ab 100644 --- a/devenv/docker/blocks/slow_proxy/Dockerfile +++ b/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"] diff --git a/devenv/docker/blocks/slow_proxy_mac/Dockerfile b/devenv/docker/blocks/slow_proxy_mac/Dockerfile index e553cb6727c..d48c945f5ab 100644 --- a/devenv/docker/blocks/slow_proxy_mac/Dockerfile +++ b/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"] diff --git a/pkg/api/dataproxy.go b/pkg/api/dataproxy.go index 88ad41b5174..4b9b90da781 100644 --- a/pkg/api/dataproxy.go +++ b/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]+\/?`) - return proxyPath +func extractProxyPath(originalRawPath string) string { + return proxyPathRegexp.ReplaceAllString(originalRawPath, "") +} + +func getProxyPath(c *models.ReqContext) string { + return extractProxyPath(c.Req.URL.EscapedPath()) } diff --git a/pkg/api/dataproxy_test.go b/pkg/api/dataproxy_test.go index 950cd0a4654..be0f98cd77d 100644 --- a/pkg/api/dataproxy_test.go +++ b/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)) + }) + } + }) } diff --git a/pkg/api/pluginproxy/ds_proxy.go b/pkg/api/pluginproxy/ds_proxy.go index 4ba6a511833..1e14d21377c 100644 --- a/pkg/api/pluginproxy/ds_proxy.go +++ b/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())) diff --git a/pkg/api/pluginproxy/ds_proxy_test.go b/pkg/api/pluginproxy/ds_proxy_test.go index a000dfef76c..07dbd1d7074 100644 --- a/pkg/api/pluginproxy/ds_proxy_test.go +++ b/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) {