From 967e9b39e8a74fb33ac6a7a5835c80c109fb8a7e Mon Sep 17 00:00:00 2001 From: Victor Cinaglia Date: Tue, 17 Nov 2020 04:56:42 -0500 Subject: [PATCH] Fix panic when using complex dynamic URLs in app plugin routes (#27977) * remove unused function to interpolate URLs * share function to add headers between ds/plugin proxies * stop performing unnecessary plugin setting lookup * fix bug causing runtime errors when using complex templated URLs * lower case util functions not used outside of pluginproxy package * change test URL to a (valid) dummy URL to make intent clearer Co-authored-by: Arve Knudsen --- pkg/api/app_routes.go | 7 +- pkg/api/pluginproxy/access_token_provider.go | 10 +- pkg/api/pluginproxy/ds_auth_provider.go | 34 +------ pkg/api/pluginproxy/ds_auth_provider_test.go | 2 +- pkg/api/pluginproxy/pluginproxy.go | 102 +++++-------------- pkg/api/pluginproxy/pluginproxy_test.go | 48 +++++++-- pkg/api/pluginproxy/utils.go | 51 ++++++---- pkg/api/pluginproxy/utils_test.go | 2 +- 8 files changed, 104 insertions(+), 152 deletions(-) diff --git a/pkg/api/app_routes.go b/pkg/api/app_routes.go index a4ac9d6d360..55687aa247f 100644 --- a/pkg/api/app_routes.go +++ b/pkg/api/app_routes.go @@ -57,12 +57,7 @@ func AppPluginRoute(route *plugins.AppPluginRoute, appID string, hs *HTTPServer) return func(c *models.ReqContext) { path := c.Params("*") - proxy, err := pluginproxy.NewApiPluginProxy(c, path, route, appID, hs.Cfg) - if err != nil { - c.JsonApiErr(500, "Failed to create API plugin proxy", err) - return - } - + proxy := pluginproxy.NewApiPluginProxy(c, path, route, appID, hs.Cfg) proxy.Transport = pluginProxyTransport proxy.ServeHTTP(c.Resp, c.Req.Request) } diff --git a/pkg/api/pluginproxy/access_token_provider.go b/pkg/api/pluginproxy/access_token_provider.go index d6a687dc8fd..27ef1d45088 100644 --- a/pkg/api/pluginproxy/access_token_provider.go +++ b/pkg/api/pluginproxy/access_token_provider.go @@ -99,14 +99,14 @@ func (provider *accessTokenProvider) getAccessToken(data templateData) (string, } } - urlInterpolated, err := InterpolateString(provider.route.TokenAuth.Url, data) + urlInterpolated, err := interpolateString(provider.route.TokenAuth.Url, data) if err != nil { return "", err } params := make(url.Values) for key, value := range provider.route.TokenAuth.Params { - interpolatedParam, err := InterpolateString(value, data) + interpolatedParam, err := interpolateString(value, data) if err != nil { return "", err } @@ -147,7 +147,7 @@ func (provider *accessTokenProvider) getJwtAccessToken(ctx context.Context, data conf := &jwt.Config{} if val, ok := provider.route.JwtTokenAuth.Params["client_email"]; ok { - interpolatedVal, err := InterpolateString(val, data) + interpolatedVal, err := interpolateString(val, data) if err != nil { return "", err } @@ -155,7 +155,7 @@ func (provider *accessTokenProvider) getJwtAccessToken(ctx context.Context, data } if val, ok := provider.route.JwtTokenAuth.Params["private_key"]; ok { - interpolatedVal, err := InterpolateString(val, data) + interpolatedVal, err := interpolateString(val, data) if err != nil { return "", err } @@ -163,7 +163,7 @@ func (provider *accessTokenProvider) getJwtAccessToken(ctx context.Context, data } if val, ok := provider.route.JwtTokenAuth.Params["token_uri"]; ok { - interpolatedVal, err := InterpolateString(val, data) + interpolatedVal, err := interpolateString(val, data) if err != nil { return "", err } diff --git a/pkg/api/pluginproxy/ds_auth_provider.go b/pkg/api/pluginproxy/ds_auth_provider.go index c9a612625c1..a92fcc4e738 100644 --- a/pkg/api/pluginproxy/ds_auth_provider.go +++ b/pkg/api/pluginproxy/ds_auth_provider.go @@ -23,7 +23,7 @@ func ApplyRoute(ctx context.Context, req *http.Request, proxyPath string, route } if len(route.URL) > 0 { - interpolatedURL, err := InterpolateString(route.URL, data) + interpolatedURL, err := interpolateString(route.URL, data) if err != nil { logger.Error("Error interpolating proxy url", "error", err) return @@ -84,35 +84,3 @@ func ApplyRoute(ctx context.Context, req *http.Request, proxyPath string, route logger.Info("Requesting", "url", req.URL.String()) } - -func addQueryString(req *http.Request, route *plugins.AppPluginRoute, data templateData) error { - q := req.URL.Query() - for _, param := range route.URLParams { - interpolatedName, err := InterpolateString(param.Name, data) - if err != nil { - return err - } - - interpolatedContent, err := InterpolateString(param.Content, data) - if err != nil { - return err - } - - q.Add(interpolatedName, interpolatedContent) - } - req.URL.RawQuery = q.Encode() - - return nil -} - -func addHeaders(reqHeaders *http.Header, route *plugins.AppPluginRoute, data templateData) error { - for _, header := range route.Headers { - interpolated, err := InterpolateString(header.Content, data) - if err != nil { - return err - } - reqHeaders.Add(header.Name, interpolated) - } - - return nil -} diff --git a/pkg/api/pluginproxy/ds_auth_provider_test.go b/pkg/api/pluginproxy/ds_auth_provider_test.go index fa22cc0a168..9bd98a339e5 100644 --- a/pkg/api/pluginproxy/ds_auth_provider_test.go +++ b/pkg/api/pluginproxy/ds_auth_provider_test.go @@ -14,7 +14,7 @@ func TestDsAuthProvider(t *testing.T) { }, } - interpolated, err := InterpolateString("{{.SecureJsonData.Test}}", data) + interpolated, err := interpolateString("{{.SecureJsonData.Test}}", data) So(err, ShouldBeNil) So(interpolated, ShouldEqual, "0asd+asd") }) diff --git a/pkg/api/pluginproxy/pluginproxy.go b/pkg/api/pluginproxy/pluginproxy.go index d9cda9a3eb8..34e88459ef3 100644 --- a/pkg/api/pluginproxy/pluginproxy.go +++ b/pkg/api/pluginproxy/pluginproxy.go @@ -7,7 +7,6 @@ import ( "net/url" "github.com/grafana/grafana/pkg/bus" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/setting" @@ -20,55 +19,35 @@ type templateData struct { SecureJsonData map[string]string } -func getHeaders(route *plugins.AppPluginRoute, orgId int64, appID string) (http.Header, error) { - result := http.Header{} - - query := models.GetPluginSettingByIdQuery{OrgId: orgId, PluginId: appID} - - if err := bus.Dispatch(&query); err != nil { - return nil, err - } - - data := templateData{ - JsonData: query.Result.JsonData, - SecureJsonData: query.Result.SecureJsonData.Decrypt(), - } - - err := addHeaders(&result, route, data) - return result, err -} - -func updateURL(route *plugins.AppPluginRoute, orgId int64, appID string) (string, error) { - query := models.GetPluginSettingByIdQuery{OrgId: orgId, PluginId: appID} - if err := bus.Dispatch(&query); err != nil { - return "", err - } - - data := templateData{ - JsonData: query.Result.JsonData, - SecureJsonData: query.Result.SecureJsonData.Decrypt(), - } - interpolated, err := InterpolateString(route.URL, data) - if err != nil { - return "", err - } - return interpolated, err -} - // NewApiPluginProxy create a plugin proxy -func NewApiPluginProxy(ctx *models.ReqContext, proxyPath string, route *plugins.AppPluginRoute, appID string, - cfg *setting.Cfg) (*httputil.ReverseProxy, error) { - targetURL, err := url.Parse(route.URL) - if err != nil { - return nil, err - } - +func NewApiPluginProxy(ctx *models.ReqContext, proxyPath string, route *plugins.AppPluginRoute, appID string, cfg *setting.Cfg) *httputil.ReverseProxy { director := func(req *http.Request) { + query := models.GetPluginSettingByIdQuery{OrgId: ctx.OrgId, PluginId: appID} + if err := bus.Dispatch(&query); err != nil { + ctx.JsonApiErr(500, "Failed to fetch plugin settings", err) + return + } + + data := templateData{ + JsonData: query.Result.JsonData, + SecureJsonData: query.Result.SecureJsonData.Decrypt(), + } + + interpolatedURL, err := interpolateString(route.URL, data) + if err != nil { + ctx.JsonApiErr(500, "Could not interpolate plugin route url", err) + return + } + targetURL, err := url.Parse(interpolatedURL) + if err != nil { + ctx.JsonApiErr(500, "Could not parse url", err) + return + } req.URL.Scheme = targetURL.Scheme req.URL.Host = targetURL.Host req.Host = targetURL.Host - req.URL.Path = util.JoinURLFragments(targetURL.Path, proxyPath) + // clear cookie headers req.Header.Del("Cookie") req.Header.Del("Set-Cookie") @@ -86,38 +65,11 @@ func NewApiPluginProxy(ctx *models.ReqContext, proxyPath string, route *plugins. applyUserHeader(cfg.SendUserHeader, req, ctx.SignedInUser) - if len(route.Headers) > 0 { - headers, err := getHeaders(route, ctx.OrgId, appID) - if err != nil { - ctx.JsonApiErr(500, "Could not generate plugin route header", err) - return - } - - for key, value := range headers { - log.Tracef("setting key %v value ", key) - req.Header.Set(key, value[0]) - } + if err := addHeaders(&req.Header, route, data); err != nil { + ctx.JsonApiErr(500, "Failed to render plugin headers", err) + return } - - if len(route.URL) > 0 { - interpolatedURL, err := updateURL(route, ctx.OrgId, appID) - if err != nil { - ctx.JsonApiErr(500, "Could not interpolate plugin route url", err) - } - targetURL, err := url.Parse(interpolatedURL) - if err != nil { - ctx.JsonApiErr(500, "Could not parse custom url: %v", err) - return - } - req.URL.Scheme = targetURL.Scheme - req.URL.Host = targetURL.Host - req.Host = targetURL.Host - req.URL.Path = util.JoinURLFragments(targetURL.Path, proxyPath) - } - - // reqBytes, _ := httputil.DumpRequestOut(req, true); - // log.Tracef("Proxying plugin request: %s", string(reqBytes)) } - return &httputil.ReverseProxy{Director: director}, nil + return &httputil.ReverseProxy{Director: director} } diff --git a/pkg/api/pluginproxy/pluginproxy_test.go b/pkg/api/pluginproxy/pluginproxy_test.go index 8a6604bc0bc..372dcfd58eb 100644 --- a/pkg/api/pluginproxy/pluginproxy_test.go +++ b/pkg/api/pluginproxy/pluginproxy_test.go @@ -36,11 +36,18 @@ func TestPluginProxy(t *testing.T) { return nil }) - header, err := getHeaders(route, 1, "my-app") - So(err, ShouldBeNil) + req := getPluginProxiedRequest( + &models.ReqContext{ + SignedInUser: &models.SignedInUser{ + Login: "test_user", + }, + }, + &setting.Cfg{SendUserHeader: true}, + route, + ) Convey("Should render header template", func() { - So(header.Get("x-header"), ShouldEqual, "my secret 123") + So(req.Header.Get("x-header"), ShouldEqual, "my secret 123") }) }) @@ -116,11 +123,6 @@ func TestPluginProxy(t *testing.T) { &setting.Cfg{SendUserHeader: true}, route, ) - Convey("Headers should be updated", func() { - header, err := getHeaders(route, 1, "my-app") - So(err, ShouldBeNil) - So(header.Get("X-Grafana-User"), ShouldEqual, "") - }) Convey("Should set req.URL to be interpolated value from jsonData", func() { So(req.URL.String(), ShouldEqual, "https://dynamic.grafana.com") }) @@ -128,6 +130,31 @@ func TestPluginProxy(t *testing.T) { So(route.URL, ShouldEqual, "{{.JsonData.dynamicUrl}}") }) }) + + Convey("When getting complex templated url", t, func() { + route := &plugins.AppPluginRoute{ + URL: "{{if .JsonData.apiHost}}{{.JsonData.apiHost}}{{else}}https://example.com{{end}}", + Method: "GET", + } + + bus.AddHandler("test", func(query *models.GetPluginSettingByIdQuery) error { + query.Result = &models.PluginSetting{} + return nil + }) + + req := getPluginProxiedRequest( + &models.ReqContext{ + SignedInUser: &models.SignedInUser{ + Login: "test_user", + }, + }, + &setting.Cfg{SendUserHeader: true}, + route, + ) + Convey("Should set req.URL to be interpolated default value", func() { + So(req.URL.String(), ShouldEqual, "https://example.com") + }) + }) } // getPluginProxiedRequest is a helper for easier setup of tests based on global config and ReqContext. @@ -140,10 +167,9 @@ func getPluginProxiedRequest(ctx *models.ReqContext, cfg *setting.Cfg, route *pl ReqRole: models.ROLE_EDITOR, } } - proxy, err := NewApiPluginProxy(ctx, "", route, "", cfg) - So(err, ShouldBeNil) + proxy := NewApiPluginProxy(ctx, "", route, "", cfg) - req, err := http.NewRequest(http.MethodGet, route.URL, nil) + req, err := http.NewRequest(http.MethodGet, "/api/plugin-proxy/grafana-simple-app/api/v4/alerts", nil) So(err, ShouldBeNil) proxy.Director(req) return req diff --git a/pkg/api/pluginproxy/utils.go b/pkg/api/pluginproxy/utils.go index 61dcc04b6b6..b1bc6d1c912 100644 --- a/pkg/api/pluginproxy/utils.go +++ b/pkg/api/pluginproxy/utils.go @@ -4,15 +4,14 @@ import ( "bytes" "fmt" "net/http" - "net/url" "text/template" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" ) -// InterpolateString accepts template data and return a string with substitutions -func InterpolateString(text string, data templateData) (string, error) { +// interpolateString accepts template data and return a string with substitutions +func interpolateString(text string, data templateData) (string, error) { t, err := template.New("content").Parse(text) if err != nil { return "", fmt.Errorf("could not parse template %s", text) @@ -27,26 +26,38 @@ func InterpolateString(text string, data templateData) (string, error) { return contentBuf.String(), nil } -// InterpolateURL accepts template data and return a string with substitutions -func InterpolateURL(anURL *url.URL, route *plugins.AppPluginRoute, orgID int64, appID string) (*url.URL, error) { - query := models.GetPluginSettingByIdQuery{OrgId: orgID, PluginId: appID} - result, err := url.Parse(anURL.String()) - if query.Result != nil { - if len(query.Result.JsonData) > 0 { - data := templateData{ - JsonData: query.Result.JsonData, - } - interpolatedResult, err := InterpolateString(anURL.String(), data) - if err == nil { - result, err = url.Parse(interpolatedResult) - if err != nil { - return nil, fmt.Errorf("error parsing plugin route URL: %w", err) - } - } +// addHeaders interpolates route headers and injects them into the request headers +func addHeaders(reqHeaders *http.Header, route *plugins.AppPluginRoute, data templateData) error { + for _, header := range route.Headers { + interpolated, err := interpolateString(header.Content, data) + if err != nil { + return err } + reqHeaders.Set(header.Name, interpolated) } - return result, err + return nil +} + +// addQueryString interpolates route params and injects them into the request object +func addQueryString(req *http.Request, route *plugins.AppPluginRoute, data templateData) error { + q := req.URL.Query() + for _, param := range route.URLParams { + interpolatedName, err := interpolateString(param.Name, data) + if err != nil { + return err + } + + interpolatedContent, err := interpolateString(param.Content, data) + if err != nil { + return err + } + + q.Add(interpolatedName, interpolatedContent) + } + req.URL.RawQuery = q.Encode() + + return nil } // Set the X-Grafana-User header if needed (and remove if not) diff --git a/pkg/api/pluginproxy/utils_test.go b/pkg/api/pluginproxy/utils_test.go index 430ce34115c..69423e6a85e 100644 --- a/pkg/api/pluginproxy/utils_test.go +++ b/pkg/api/pluginproxy/utils_test.go @@ -14,7 +14,7 @@ func TestInterpolateString(t *testing.T) { }, } - interpolated, err := InterpolateString("{{.SecureJsonData.Test}}", data) + interpolated, err := interpolateString("{{.SecureJsonData.Test}}", data) So(err, ShouldBeNil) So(interpolated, ShouldEqual, "0asd+asd") })