track signature files + add warn log (#38938)

pkg/api/api.go

@@ -133,7 +133,7 @@ func (hs *HTTPServer) registerRoutes() {
 	r.Get("/api/login/ping", quota("session"), routing.Wrap(hs.LoginAPIPing))
 
 	// expose plugin file system assets
-	r.Get("/public/plugins/:pluginId/*", hs.GetPluginAssets)
+	r.Get("/public/plugins/:pluginId/*", hs.getPluginAssets)
 
 	// authed api
 	r.Group("/api", func(apiRoute routing.RouteRegister) {

pkg/api/plugins.go

@@ -3,12 +3,10 @@ package api
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
 	"sort"
-	"strings"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
 	"github.com/grafana/grafana/pkg/api/dtos"
@@ -21,14 +19,6 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 )
 
-var permittedFileExts = []string{
-	".html", ".xhtml", ".css", ".js", ".json", ".jsonld", ".map", ".mjs",
-	".jpeg", ".jpg", ".png", ".gif", ".svg", ".webp", ".ico",
-	".woff", ".woff2", ".eot", ".ttf", ".otf",
-	".wav", ".mp3",
-	".md", ".pdf", ".txt",
-}
-
 func (hs *HTTPServer) GetPluginList(c *models.ReqContext) response.Response {
 	typeFilter := c.Query("type")
 	enabledFilter := c.Query("enabled")
@@ -262,10 +252,10 @@ func (hs *HTTPServer) CollectPluginMetrics(c *models.ReqContext) response.Respon
 	return response.CreateNormalResponse(headers, resp.PrometheusMetrics, http.StatusOK)
 }
 
-// GetPluginAssets returns public plugin assets (images, JS, etc.)
+// getPluginAssets returns public plugin assets (images, JS, etc.)
 //
 // /public/plugins/:pluginId/*
-func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) {
+func (hs *HTTPServer) getPluginAssets(c *models.ReqContext) {
 	pluginID := c.Params("pluginId")
 	plugin := hs.PluginManager.GetPlugin(pluginID)
 	if plugin == nil {
@@ -276,6 +266,11 @@ func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) {
 	requestedFile := filepath.Clean(c.Params("*"))
 	pluginFilePath := filepath.Join(plugin.PluginDir, requestedFile)
 
+	if !plugin.IncludedInSignature(requestedFile) {
+		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)
+	}
+
 	// 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
@@ -300,12 +295,6 @@ func (hs *HTTPServer) GetPluginAssets(c *models.ReqContext) {
 		return
 	}
 
-	if accessForbidden(fi.Name()) {
-		c.JsonApiErr(403, "Plugin file access forbidden",
-			fmt.Errorf("access is forbidden to plugin file %s", pluginFilePath))
-		return
-	}
-
 	if hs.Cfg.Env == setting.Dev {
 		c.Resp.Header().Set("Cache-Control", "max-age=0, must-revalidate, no-cache")
 	} else {
@@ -448,14 +437,3 @@ func translatePluginRequestErrorToAPIError(err error) response.Response {
 
 	return response.Error(500, "Plugin request failed", err)
 }
-
-func accessForbidden(pluginFilename string) bool {
-	ext := filepath.Ext(pluginFilename)
-
-	for _, permittedExt := range permittedFileExts {
-		if strings.EqualFold(permittedExt, ext) {
-			return false
-		}
-	}
-	return true
-}

pkg/api/plugins_test.go

@@ -1,89 +1,201 @@
 package api
 
 import (
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"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/plugins/manager"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
-func Test_accessForbidden(t *testing.T) {
-	type testCase struct {
-		filename string
-	}
-	tests := []struct {
-		name            string
-		t               testCase
-		accessForbidden bool
-	}{
-		{
-			name: ".exe files are forbidden",
-			t: testCase{
-				filename: "test.exe",
-			},
-			accessForbidden: true,
-		},
-		{
-			name: ".sh files are forbidden",
-			t: testCase{
-				filename: "test.sh",
-			},
-			accessForbidden: true,
-		},
-		{
-			name: "js is not forbidden",
-			t: testCase{
+func Test_GetPluginAssets(t *testing.T) {
+	pluginID := "test-plugin"
+	pluginDir := "."
+	tmpFile, err := ioutil.TempFile(pluginDir, "")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		err := os.RemoveAll(tmpFile.Name())
+		assert.NoError(t, err)
+	})
+	expectedBody := "Plugin test"
+	_, err = tmpFile.WriteString(expectedBody)
+	assert.NoError(t, err)
 
-				filename: "module.js",
-			},
-			accessForbidden: false,
-		},
-		{
-			name: "logos are not forbidden",
-			t: testCase{
+	requestedFile := filepath.Clean(tmpFile.Name())
 
-				filename: "logo.svg",
+	t.Run("Given a request for an existing plugin file that is listed as a signature covered file", func(t *testing.T) {
+		p := &plugins.PluginBase{
+			Id:        pluginID,
+			PluginDir: pluginDir,
+			SignedFiles: map[string]struct{}{
+				requestedFile: {},
 			},
-			accessForbidden: false,
-		},
-		{
-			name: "JPGs are not forbidden",
-			t: testCase{
-				filename: "img/test.jpg",
+		}
+		service := &pluginManager{
+			plugins: map[string]*plugins.PluginBase{
+				pluginID: p,
 			},
-			accessForbidden: false,
-		},
-		{
-			name: "JPEGs are not forbidden",
-			t: testCase{
-				filename: "img/test.jpeg",
+		}
+		l := &logger{}
+
+		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.Empty(t, l.warnings)
+			})
+	})
+
+	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.PluginBase{
+			Id:        pluginID,
+			PluginDir: pluginDir,
+		}
+		service := &pluginManager{
+			plugins: map[string]*plugins.PluginBase{
+				pluginID: p,
 			},
-			accessForbidden: false,
-		},
-		{
-			name: "ext case is ignored",
-			t: testCase{
-				filename: "scripts/runThis.SH",
+		}
+		l := &logger{}
+
+		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.Empty(t, l.warnings)
+			})
+	})
+
+	t.Run("Given a request for an non-existing plugin file", func(t *testing.T) {
+		p := &plugins.PluginBase{
+			Id:        pluginID,
+			PluginDir: pluginDir,
+		}
+		service := &pluginManager{
+			plugins: map[string]*plugins.PluginBase{
+				pluginID: p,
 			},
-			accessForbidden: true,
-		},
-		{
-			name: "no file ext is forbidden",
-			t: testCase{
-				filename: "scripts/runThis",
+		}
+		l := &logger{}
+
+		requestedFile := "nonExistent"
+		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)
+
+				var respJson map[string]interface{}
+				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.Empty(t, l.warnings)
+			})
+	})
+
+	t.Run("Given a request for an non-existing plugin", func(t *testing.T) {
+		service := &pluginManager{
+			plugins: map[string]*plugins.PluginBase{},
+		}
+		l := &logger{}
+
+		requestedFile := "nonExistent"
+		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)
+
+				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.Empty(t, l.warnings)
+			})
+	})
+
+	t.Run("Given a request for a core plugin's file", func(t *testing.T) {
+		service := &pluginManager{
+			plugins: map[string]*plugins.PluginBase{
+				pluginID: {
+					IsCorePlugin: true,
+				},
 			},
-			accessForbidden: true,
-		},
-		{
-			name: "empty file ext is forbidden",
-			t: testCase{
-				filename: "scripts/runThis.",
-			},
-			accessForbidden: true,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := accessForbidden(tt.t.filename); got != tt.accessForbidden {
-				t.Errorf("accessForbidden() = %v, accessForbidden %v", got, tt.accessForbidden)
-			}
-		})
-	}
+		}
+		l := &logger{}
+
+		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.Empty(t, l.warnings)
+			})
+	})
+}
+
+func callGetPluginAsset(sc *scenarioContext) {
+	sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
+}
+
+func pluginAssetScenario(t *testing.T, desc string, url string, urlPattern string, pluginManager plugins.Manager,
+	logger log.Logger, fn scenarioFunc) {
+	t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) {
+		defer bus.ClearBusHandlers()
+
+		hs := HTTPServer{
+			Cfg:           setting.NewCfg(),
+			PluginManager: pluginManager,
+			log:           logger,
+		}
+
+		sc := setupScenarioContext(t, url)
+		sc.defaultHandler = func(c *models.ReqContext) {
+			sc.context = c
+			hs.getPluginAssets(c)
+		}
+
+		sc.m.Get(urlPattern, sc.defaultHandler)
+
+		fn(sc)
+	})
+}
+
+type pluginManager struct {
+	manager.PluginManager
+
+	plugins map[string]*plugins.PluginBase
+}
+
+func (pm *pluginManager) GetPlugin(id string) *plugins.PluginBase {
+	return pm.plugins[id]
+}
+
+type logger struct {
+	log.Logger
+
+	warnings []string
+}
+
+func (l *logger) Warn(msg string, ctx ...interface{}) {
+	l.warnings = append(l.warnings, msg)
 }

pkg/plugins/manager/manager.go

@@ -512,6 +512,7 @@ func (pm *PluginManager) loadPlugin(jsonParser *json.Decoder, pluginBase *plugin
 	pb.Signature = pluginBase.Signature
 	pb.SignatureType = pluginBase.SignatureType
 	pb.SignatureOrg = pluginBase.SignatureOrg
+	pb.SignedFiles = pluginBase.SignedFiles
 
 	pm.plugins[pb.Id] = pb
 	pm.log.Debug("Successfully added plugin", "id", pb.Id)
@@ -581,6 +582,7 @@ func (s *PluginScanner) loadPlugin(pluginJSONFilePath string) error {
 	pluginCommon.Signature = signatureState.Status
 	pluginCommon.SignatureType = signatureState.Type
 	pluginCommon.SignatureOrg = signatureState.SigningOrg
+	pluginCommon.SignedFiles = signatureState.Files
 
 	s.plugins[currentDir] = &pluginCommon
 

pkg/plugins/manager/manager_test.go

@@ -232,6 +232,7 @@ func TestPluginManager_Init(t *testing.T) {
 				Signature:     plugins.PluginSignatureValid,
 				SignatureType: plugins.GrafanaType,
 				SignatureOrg:  "Grafana Labs",
+				SignedFiles:   plugins.PluginFiles{"plugin.json": {}},
 				Dependencies: plugins.PluginDependencies{
 					GrafanaVersion: "*",
 					Plugins:        []plugins.PluginDependencyItem{},
@@ -497,6 +498,7 @@ func TestPluginManager_Installer(t *testing.T) {
 			Signature:     plugins.PluginSignatureValid,
 			SignatureType: plugins.GrafanaType,
 			SignatureOrg:  "Grafana Labs",
+			SignedFiles:   plugins.PluginFiles{"plugin.json": {}},
 			Dependencies: plugins.PluginDependencies{
 				GrafanaVersion: "*",
 				Plugins:        []plugins.PluginDependencyItem{},

pkg/plugins/manager/manifest.go

@@ -169,7 +169,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin
 		}
 	}
 
-	manifestFiles := make(map[string]bool, len(manifest.Files))
+	manifestFiles := make(map[string]struct{}, len(manifest.Files))
 
 	// Verify the manifest contents
 	log.Debug("Verifying contents of plugin manifest", "plugin", plugin.Id)
@@ -207,7 +207,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin
 				Status: plugins.PluginSignatureModified,
 			}, nil
 		}
-		manifestFiles[p] = true
+		manifestFiles[p] = struct{}{}
 	}
 
 	if manifest.isV2() {
@@ -241,6 +241,7 @@ func getPluginSignatureState(log log.Logger, plugin *plugins.PluginBase) (plugin
 		Status:     plugins.PluginSignatureValid,
 		Type:       manifest.SignatureType,
 		SigningOrg: manifest.SignedByOrgName,
+		Files:      manifestFiles,
 	}, nil
 }
 

pkg/plugins/models.go

@@ -73,6 +73,7 @@ type PluginBase struct {
 	IsCorePlugin    bool                `json:"-"`
 	SignatureType   PluginSignatureType `json:"-"`
 	SignatureOrg    string              `json:"-"`
+	SignedFiles     PluginFiles         `json:"-"`
 
 	GrafanaNetVersion   string `json:"-"`
 	GrafanaNetHasUpdate bool   `json:"-"`
@@ -80,6 +81,23 @@ type PluginBase struct {
 	Root *PluginBase
 }
 
+func (p *PluginBase) IncludedInSignature(file string) bool {
+	// permit Core plugin files
+	if p.IsCorePlugin {
+		return true
+	}
+
+	// permit when no signed files (no MANIFEST)
+	if p.SignedFiles == nil {
+		return true
+	}
+
+	if _, exists := p.SignedFiles[file]; !exists {
+		return false
+	}
+	return true
+}
+
 type PluginDependencies struct {
 	GrafanaVersion string                 `json:"grafanaVersion"`
 	Plugins        []PluginDependencyItem `json:"plugins"`

pkg/plugins/state.go

@@ -31,8 +31,11 @@ const (
 	PrivateType PluginSignatureType = "private"
 )
 
+type PluginFiles map[string]struct{}
+
 type PluginSignatureState struct {
 	Status     PluginSignatureStatus
 	Type       PluginSignatureType
 	SigningOrg string
+	Files      PluginFiles
 }