Plugins: Unexport PluginDir field from PluginDTO (#59190)

* unexport pluginDir from dto * more err checks * tidy * fix tests * fix dboard file tests * fix import * fix tests * apply PR feedback * combine interfaces * fix logs and clean up test * filepath clean * use fs.File * rm explicit type
2026-08-11 13:44:59 -05:00 · 2022-12-02 13:46:55 +01:00
parent 6d1bcd9f40
commit 76233f9997
15 changed files with 223 additions and 286 deletions
@@ -1,12 +1,13 @@
 package api

 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
-	"os"
 	"path"
 	"path/filepath"
 	"runtime"
@@ -16,7 +17,6 @@ import (
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
-	"github.com/grafana/grafana/pkg/infra/fs"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/backendplugin"
@@ -28,6 +28,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/pluginsettings"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/web"
 )

@@ -309,42 +310,25 @@ func (hs *HTTPServer) getPluginAssets(c *models.ReqContext) {
 	}

 	// prepend slash for cleaning relative paths
-	requestedFile := filepath.Clean(filepath.Join("/", web.Params(c.Req)["*"]))
-	rel, err := filepath.Rel("/", requestedFile)
+	requestedFile, err := util.CleanRelativePath(web.Params(c.Req)["*"])
 	if err != nil {
 		// slash is prepended above therefore this is not expected to fail
-		c.JsonApiErr(500, "Failed to get the relative path", err)
+		c.JsonApiErr(500, "Failed to clean relative file path", err)
 		return
 	}

-	if !plugin.IncludedInSignature(rel) {
-		hs.log.Warn("Access to requested plugin file will be forbidden in upcoming Grafana versions as the file "+
-			"is not included in the plugin signature", "file", requestedFile)
-	}
-
-	absPluginDir, err := filepath.Abs(plugin.PluginDir)
+	f, err := plugin.File(requestedFile)
 	if err != nil {
-		c.JsonApiErr(500, "Failed to get plugin absolute path", nil)
-		return
-	}
-
-	pluginFilePath := filepath.Join(absPluginDir, rel)
-
-	// It's safe to ignore gosec warning G304 since we already clean the requested file path and subsequently
-	// use this with a prefix of the plugin's directory, which is set during plugin loading
-	// nolint:gosec
-	f, err := os.Open(pluginFilePath)
-	if err != nil {
-		if os.IsNotExist(err) {
-			c.JsonApiErr(404, "Plugin file not found", err)
+		if errors.Is(err, plugins.ErrFileNotExist) {
+			c.JsonApiErr(404, "Plugin file not found", nil)
 			return
 		}
 		c.JsonApiErr(500, "Could not open plugin file", err)
 		return
 	}
 	defer func() {
-		if err := f.Close(); err != nil {
-			hs.log.Error("Failed to close file", "err", err)
+		if err = f.Close(); err != nil {
+			hs.log.Error("Failed to close plugin file", "err", err)
 		}
 	}()

@@ -360,7 +344,16 @@ func (hs *HTTPServer) getPluginAssets(c *models.ReqContext) {
 		c.Resp.Header().Set("Cache-Control", "public, max-age=3600")
 	}

-	http.ServeContent(c.Resp, c.Req, pluginFilePath, fi.ModTime(), f)
+	if rs, ok := f.(io.ReadSeeker); ok {
+		http.ServeContent(c.Resp, c.Req, requestedFile, fi.ModTime(), rs)
+	} else {
+		b, err := io.ReadAll(f)
+		if err != nil {
+			c.JsonApiErr(500, "Plugin file exists but could not read", err)
+			return
+		}
+		http.ServeContent(c.Resp, c.Req, requestedFile, fi.ModTime(), bytes.NewReader(b))
+	}
 }

 // CheckHealth returns the health of a plugin.
@@ -496,34 +489,24 @@ func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginId string, name
 		return nil, plugins.NotFoundError{PluginID: pluginId}
 	}

-	// nolint:gosec
-	// We can ignore the gosec G304 warning since we have cleaned the requested file path and subsequently
-	// use this with a prefix of the plugin's directory, which is set during plugin loading
-	path := filepath.Join(plugin.PluginDir, mdFilepath(strings.ToUpper(name)))
-	exists, err := fs.Exists(path)
+	md, err := plugin.File(mdFilepath(strings.ToUpper(name)))
 	if err != nil {
-		return nil, err
-	}
-	if !exists {
-		path = filepath.Join(plugin.PluginDir, mdFilepath(strings.ToLower(name)))
+		md, err = plugin.File(mdFilepath(strings.ToUpper(name)))
+		if err != nil {
+			return make([]byte, 0), nil
+		}
 	}
+	defer func() {
+		if err = md.Close(); err != nil {
+			hs.log.Error("Failed to close plugin markdown file", "err", err)
+		}
+	}()

-	exists, err = fs.Exists(path)
+	d, err := io.ReadAll(md)
 	if err != nil {
-		return nil, err
-	}
-	if !exists {
 		return make([]byte, 0), nil
 	}
-
-	// nolint:gosec
-	// We can ignore the gosec G304 warning since we have cleaned the requested file path and subsequently
-	// use this with a prefix of the plugin's directory, which is set during plugin loading
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return nil, err
-	}
-	return data, nil
+	return d, nil
 }

 func mdFilepath(mdFilename string) string {
@@ -12,7 +12,6 @@ import (
 	"strings"
 	"testing"

-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"

 	"github.com/grafana/grafana-plugin-sdk-go/backend"
@@ -135,13 +134,13 @@ func Test_PluginsInstallAndUninstall_AccessControl(t *testing.T) {
 		t.Run(testName("Install", tc), func(t *testing.T) {
 			input := strings.NewReader("{ \"version\": \"1.0.2\" }")
 			response := callAPI(sc.server, http.MethodPost, "/api/plugins/test/install", input, t)
-			assert.Equal(t, tc.expectedCode, response.Code)
+			require.Equal(t, tc.expectedCode, response.Code)
 		})

 		t.Run(testName("Uninstall", tc), func(t *testing.T) {
 			input := strings.NewReader("{ }")
 			response := callAPI(sc.server, http.MethodPost, "/api/plugins/test/uninstall", input, t)
-			assert.Equal(t, tc.expectedCode, response.Code)
+			require.Equal(t, tc.expectedCode, response.Code)
 		})
 	}
 }
@@ -155,56 +154,45 @@ func Test_GetPluginAssets(t *testing.T) {
 	require.NoError(t, err)
 	t.Cleanup(func() {
 		err := os.RemoveAll(tmpFile.Name())
-		assert.NoError(t, err)
+		require.NoError(t, err)
 		err = os.RemoveAll(tmpFileInParentDir.Name())
-		assert.NoError(t, err)
+		require.NoError(t, err)
 	})
 	expectedBody := "Plugin test"
 	_, err = tmpFile.WriteString(expectedBody)
-	assert.NoError(t, err)
+	require.NoError(t, err)

 	requestedFile := filepath.Clean(tmpFile.Name())

-	t.Run("Given a request for an existing plugin file that is listed as a signature covered file", func(t *testing.T) {
-		p := plugins.PluginDTO{
+	t.Run("Given a request for an existing plugin file", func(t *testing.T) {
+		p := &plugins.Plugin{
 			JSONData: plugins.JSONData{
 				ID: pluginID,
 			},
 			PluginDir: pluginDir,
-			SignedFiles: map[string]struct{}{
-				requestedFile: {},
-			},
 		}
 		service := &plugins.FakePluginStore{
-			PluginList: []plugins.PluginDTO{p},
+			PluginList: []plugins.PluginDTO{p.ToDTO()},
 		}
-		l := &logtest.Fake{}

 		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile)
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
+		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service,
 			func(sc *scenarioContext) {
 				callGetPluginAsset(sc)

 				require.Equal(t, 200, sc.resp.Code)
-				assert.Equal(t, expectedBody, sc.resp.Body.String())
-				assert.Zero(t, l.WarnLogs.Calls)
+				require.Equal(t, expectedBody, sc.resp.Body.String())
 			})
 	})

 	t.Run("Given a request for a relative path", func(t *testing.T) {
-		p := plugins.PluginDTO{
-			JSONData: plugins.JSONData{
-				ID: pluginID,
-			},
-			PluginDir: pluginDir,
-		}
+		p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.External, pluginDir)
 		service := &plugins.FakePluginStore{
 			PluginList: []plugins.PluginDTO{p},
 		}
-		l := &logtest.Fake{}

 		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, tmpFileInParentDir.Name())
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
+		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service,
 			func(sc *scenarioContext) {
 				callGetPluginAsset(sc)

@@ -212,44 +200,15 @@ func Test_GetPluginAssets(t *testing.T) {
 			})
 	})

-	t.Run("Given a request for an existing plugin file that is not listed as a signature covered file", func(t *testing.T) {
-		p := plugins.PluginDTO{
-			JSONData: plugins.JSONData{
-				ID: pluginID,
-			},
-			PluginDir: pluginDir,
-		}
-		service := &plugins.FakePluginStore{
-			PluginList: []plugins.PluginDTO{p},
-		}
-		l := &logtest.Fake{}
-
-		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile)
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
-			func(sc *scenarioContext) {
-				callGetPluginAsset(sc)
-
-				require.Equal(t, 200, sc.resp.Code)
-				assert.Equal(t, expectedBody, sc.resp.Body.String())
-				assert.Zero(t, l.WarnLogs.Calls)
-			})
-	})
-
 	t.Run("Given a request for an non-existing plugin file", func(t *testing.T) {
-		p := plugins.PluginDTO{
-			JSONData: plugins.JSONData{
-				ID: pluginID,
-			},
-			PluginDir: pluginDir,
-		}
+		p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.External, pluginDir)
 		service := &plugins.FakePluginStore{
 			PluginList: []plugins.PluginDTO{p},
 		}
-		l := &logtest.Fake{}

 		requestedFile := "nonExistent"
 		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile)
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
+		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service,
 			func(sc *scenarioContext) {
 				callGetPluginAsset(sc)

@@ -257,8 +216,7 @@ func Test_GetPluginAssets(t *testing.T) {
 				err := json.NewDecoder(sc.resp.Body).Decode(&respJson)
 				require.NoError(t, err)
 				require.Equal(t, 404, sc.resp.Code)
-				assert.Equal(t, "Plugin file not found", respJson["message"])
-				assert.Zero(t, l.WarnLogs.Calls)
+				require.Equal(t, "Plugin file not found", respJson["message"])
 			})
 	})

@@ -270,16 +228,16 @@ func Test_GetPluginAssets(t *testing.T) {

 		requestedFile := "nonExistent"
 		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile)
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
+		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service,
 			func(sc *scenarioContext) {
 				callGetPluginAsset(sc)

 				var respJson map[string]interface{}
 				err := json.NewDecoder(sc.resp.Body).Decode(&respJson)
 				require.NoError(t, err)
-				assert.Equal(t, 404, sc.resp.Code)
-				assert.Equal(t, "Plugin not found", respJson["message"])
-				assert.Zero(t, l.WarnLogs.Calls)
+				require.Equal(t, 404, sc.resp.Code)
+				require.Equal(t, "Plugin not found", respJson["message"])
+				require.Zero(t, l.WarnLogs.Calls)
 			})
 	})

@@ -295,13 +253,13 @@ func Test_GetPluginAssets(t *testing.T) {
 		l := &logtest.Fake{}

 		url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile)
-		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service, l,
+		pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", service,
 			func(sc *scenarioContext) {
 				callGetPluginAsset(sc)

 				require.Equal(t, 200, sc.resp.Code)
-				assert.Equal(t, expectedBody, sc.resp.Body.String())
-				assert.Zero(t, l.WarnLogs.Calls)
+				require.Equal(t, expectedBody, sc.resp.Body.String())
+				require.Zero(t, l.WarnLogs.Calls)
 			})
 	})
 }
@@ -348,7 +306,7 @@ func TestMakePluginResourceRequestSetCookieNotPresent(t *testing.T) {
 			break
 		}
 	}
-	assert.Empty(t, resp.Header().Values("Set-Cookie"), "Set-Cookie header should not be present")
+	require.Empty(t, resp.Header().Values("Set-Cookie"), "Set-Cookie header should not be present")
 }

 func TestMakePluginResourceRequestContentTypeUnique(t *testing.T) {
@@ -382,8 +340,8 @@ func TestMakePluginResourceRequestContentTypeUnique(t *testing.T) {
 					break
 				}
 			}
-			assert.Len(t, resp.Header().Values("Content-Type"), 1, "should have 1 Content-Type header")
-			assert.Len(t, resp.Header().Values("x-another"), 1, "should have 1 X-Another header")
+			require.Len(t, resp.Header().Values("Content-Type"), 1, "should have 1 Content-Type header")
+			require.Len(t, resp.Header().Values("x-another"), 1, "should have 1 X-Another header")
 		})
 	}
 }
@@ -417,12 +375,11 @@ func callGetPluginAsset(sc *scenarioContext) {
 }

 func pluginAssetScenario(t *testing.T, desc string, url string, urlPattern string, pluginStore plugins.Store,
-	logger log.Logger, fn scenarioFunc) {
+	fn scenarioFunc) {
 	t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) {
 		hs := HTTPServer{
 			Cfg:         setting.NewCfg(),
 			pluginStore: pluginStore,
-			log:         logger,
 		}

 		sc := setupScenarioContext(t, url)
@@ -478,42 +435,40 @@ func (c *fakePluginClient) QueryData(ctx context.Context, req *backend.QueryData
 }

 func Test_PluginsList_AccessControl(t *testing.T) {
-	pluginStore := plugins.FakePluginStore{PluginList: []plugins.PluginDTO{
-		{
-			PluginDir:     "/grafana/plugins/test-app/dist",
-			Class:         "external",
-			DefaultNavURL: "/plugins/test-app/page/test",
-			Pinned:        false,
-			Signature:     "unsigned",
-			Module:        "plugins/test-app/module",
-			BaseURL:       "public/plugins/test-app",
-			JSONData: plugins.JSONData{
-				ID:   "test-app",
-				Type: "app",
-				Name: "test-app",
-				Info: plugins.Info{
-					Version: "1.0.0",
-				},
+	p1 := &plugins.Plugin{
+		PluginDir:     "/grafana/plugins/test-app/dist",
+		Class:         plugins.External,
+		DefaultNavURL: "/plugins/test-app/page/test",
+		Signature:     plugins.SignatureUnsigned,
+		Module:        "plugins/test-app/module",
+		BaseURL:       "public/plugins/test-app",
+		JSONData: plugins.JSONData{
+			ID:   "test-app",
+			Type: plugins.App,
+			Name: "test-app",
+			Info: plugins.Info{
+				Version: "1.0.0",
 			},
 		},
-		{
-			PluginDir: "/grafana/public/app/plugins/datasource/mysql",
-			Class:     "core",
-			Pinned:    false,
-			Signature: "internal",
-			Module:    "app/plugins/datasource/mysql/module",
-			BaseURL:   "public/app/plugins/datasource/mysql",
-			JSONData: plugins.JSONData{
-				ID:   "mysql",
-				Type: "datasource",
-				Name: "MySQL",
-				Info: plugins.Info{
-					Author:      plugins.InfoLink{Name: "Grafana Labs", URL: "https://grafana.com"},
-					Description: "Data source for MySQL databases",
-				},
+	}
+	p2 := &plugins.Plugin{
+		PluginDir: "/grafana/public/app/plugins/datasource/mysql",
+		Class:     plugins.Core,
+		Pinned:    false,
+		Signature: plugins.SignatureInternal,
+		Module:    "app/plugins/datasource/mysql/module",
+		BaseURL:   "public/app/plugins/datasource/mysql",
+		JSONData: plugins.JSONData{
+			ID:   "mysql",
+			Type: plugins.DataSource,
+			Name: "MySQL",
+			Info: plugins.Info{
+				Author:      plugins.InfoLink{Name: "Grafana Labs", URL: "https://grafana.com"},
+				Description: "Data source for MySQL databases",
 			},
 		},
-	}}
+	}
+	pluginStore := plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p1.ToDTO(), p2.ToDTO()}}

 	pluginSettings := pluginsettings.FakePluginSettings{Plugins: map[string]*pluginsettings.DTO{
 		"test-app": {ID: 0, OrgID: 1, PluginID: "test-app", PluginVersion: "1.0.0", Enabled: true},
@@ -574,3 +529,12 @@ func Test_PluginsList_AccessControl(t *testing.T) {
 		})
 	}
 }
+
+func createPluginDTO(jd plugins.JSONData, class plugins.Class, pluginDir string) plugins.PluginDTO {
+	p := &plugins.Plugin{
+		JSONData:  jd,
+		Class:     class,
+		PluginDir: pluginDir,
+	}
+	return p.ToDTO()
+}