diff --git a/pkg/api/plugin_resource_test.go b/pkg/api/plugin_resource_test.go index a2d3c93111f..deb9373d4fa 100644 --- a/pkg/api/plugin_resource_test.go +++ b/pkg/api/plugin_resource_test.go @@ -66,7 +66,7 @@ func TestCallResource(t *testing.T) { require.NoError(t, err) reg := registry.ProvideService() l := loader.ProvideService(pCfg, fakes.NewFakeLicensingService(), signature.NewUnsignedAuthorizer(pCfg), - reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(), fakes.NewFakeRoleRegistry(), + reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg), fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(pCfg)), signature.ProvideService(pCfg, keystore.ProvideService(kvstore.NewFakeKVStore()))) srcs := sources.ProvideService(cfg, pCfg) ps, err := store.ProvideService(reg, srcs, l) diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index 9dce4ff7ee9..9cf95c431d2 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -277,7 +277,7 @@ func Test_GetPluginAssets(t *testing.T) { requestedFile := filepath.Clean(tmpFile.Name()) t.Run("Given a request for an existing plugin file", func(t *testing.T) { - p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{requestedFile: {}}, filepath.Dir(requestedFile))) + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(filepath.Dir(requestedFile))) pluginRegistry := &fakes.FakePluginRegistry{ Store: map[string]*plugins.Plugin{ p.ID: p, @@ -295,7 +295,7 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for a relative path", func(t *testing.T) { - p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewFakeFS()) pluginRegistry := &fakes.FakePluginRegistry{ Store: map[string]*plugins.Plugin{ p.ID: p, @@ -312,9 +312,7 @@ 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 := createPlugin(plugins.JSONData{ID: pluginID}, plugins.Core, plugins.NewLocalFS(map[string]struct{}{ - requestedFile: {}, - }, "")) + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.Core, plugins.NewLocalFS(filepath.Dir(requestedFile))) pluginRegistry := &fakes.FakePluginRegistry{ Store: map[string]*plugins.Plugin{ p.ID: p, @@ -332,7 +330,7 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for an non-existing plugin file", func(t *testing.T) { - p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewFakeFS()) service := &fakes.FakePluginRegistry{ Store: map[string]*plugins.Plugin{ p.ID: p, @@ -625,13 +623,13 @@ func Test_PluginsList_AccessControl(t *testing.T) { ID: "test-app", Type: "app", Name: "test-app", Info: plugins.Info{ Version: "1.0.0", - }}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) + }}, plugins.External, plugins.NewFakeFS()) p2 := createPlugin( 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", - }}, plugins.Core, plugins.NewLocalFS(map[string]struct{}{}, "")) + }}, plugins.Core, plugins.NewFakeFS()) pluginRegistry := &fakes.FakePluginRegistry{ Store: map[string]*plugins.Plugin{ diff --git a/pkg/plugins/ifaces.go b/pkg/plugins/ifaces.go index a4ce3c6f736..b146f32bcd5 100644 --- a/pkg/plugins/ifaces.go +++ b/pkg/plugins/ifaces.go @@ -55,7 +55,7 @@ type FS interface { fs.FS Base() string - Files() []string + Files() ([]string, error) } type FSRemover interface { diff --git a/pkg/plugins/localfiles.go b/pkg/plugins/localfiles.go index cb0b442f8d1..8def6dd7503 100644 --- a/pkg/plugins/localfiles.go +++ b/pkg/plugins/localfiles.go @@ -2,6 +2,7 @@ package plugins import ( "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -10,78 +11,152 @@ import ( "github.com/grafana/grafana/pkg/util" ) -var _ fs.FS = (*LocalFS)(nil) +var ( + _ fs.File = &LocalFile{} + + _ FS = &LocalFS{} + _ FS = &StaticFS{} +) // LocalFS is a plugins.FS that allows accessing files on the local file system. type LocalFS struct { - // m is a map of relative file paths that can be accessed on the local filesystem. - // The path separator must be os-specific. - m map[string]*LocalFile - - // basePath is the basePath that will be prepended to all the files (in m map) before accessing them. + // basePath is the basePath that will be prepended to all the files to get their absolute path. basePath string } -// NewLocalFS returns a new LocalFS that can access the specified files in the specified base path. -// Both the map keys and basePath should use the os-specific path separator for Open() to work properly. -func NewLocalFS(m map[string]struct{}, basePath string) LocalFS { - pfs := make(map[string]*LocalFile, len(m)) - for k := range m { - pfs[k] = &LocalFile{ - path: k, +// NewLocalFS returns a new LocalFS that can access any file in the specified base path on the filesystem. +// basePath must use os-specific path separator for Open() to work properly. +func NewLocalFS(basePath string) LocalFS { + return LocalFS{basePath: basePath} +} + +// fileIsAllowed takes an absolute path to a file and an os.FileInfo for that file, and it checks if access to that +// file is allowed or not. Access to a file is allowed if the file is in the FS's Base() directory, and if it's a +// symbolic link it should not end up outside the plugin's directory. +func (f LocalFS) fileIsAllowed(basePath string, absolutePath string, info os.FileInfo) (bool, error) { + if info.Mode()&os.ModeSymlink == os.ModeSymlink { + symlinkPath, err := filepath.EvalSymlinks(absolutePath) + if err != nil { + return false, err + } + + symlink, err := os.Stat(symlinkPath) + if err != nil { + return false, err + } + + // verify that symlinked file is within plugin directory + p, err := filepath.Rel(basePath, symlinkPath) + if err != nil { + return false, err + } + if p == ".." || strings.HasPrefix(p, ".."+string(filepath.Separator)) { + return false, fmt.Errorf("file '%s' not inside of plugin directory", p) + } + + // skip adding symlinked directories + if symlink.IsDir() { + return false, nil } } - return LocalFS{ - m: pfs, - basePath: basePath, + // skip directories + if info.IsDir() { + return false, nil } -} -// Open opens the specified file on the local filesystem, and returns the corresponding fs.File. -// If a nil error is returned, the caller should take care of closing the returned file. -func (f LocalFS) Open(name string) (fs.File, error) { - cleanPath, err := util.CleanRelativePath(name) + // verify that file is within plugin directory + file, err := filepath.Rel(f.Base(), absolutePath) if err != nil { - return nil, err + return false, err } + if strings.HasPrefix(file, ".."+string(filepath.Separator)) { + return false, fmt.Errorf("file '%s' not inside of plugin directory", file) + } + return true, nil +} - if kv, exists := f.m[filepath.Join(f.basePath, cleanPath)]; exists { - if kv.f != nil { - return kv.f, nil +// walkFunc returns a filepath.WalkFunc that accumulates absolute file paths into acc by walking over f.Base(). +// f.fileIsAllowed is used as WalkFunc, see its documentation for more information on which files are collected. +func (f LocalFS) walkFunc(basePath string, acc map[string]struct{}) filepath.WalkFunc { + return func(path string, info os.FileInfo, err error) error { + if err != nil { + return err } - file, err := os.Open(kv.path) + ok, err := f.fileIsAllowed(basePath, path, info) if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil, ErrFileNotExist - } - return nil, ErrPluginFileRead + return err + } + if !ok { + return nil } - return file, nil + acc[path] = struct{}{} + return nil } - return nil, ErrFileNotExist +} + +// Open opens the specified file on the local filesystem. +// The provided name must be a relative file name that uses os-specific path separators. +// The function returns the corresponding fs.File for accessing the file on the local filesystem. +// If a nil error is returned, the caller should take care of calling Close() the returned fs.File. +// If the file does not exist, ErrFileNotExist is returned. +func (f LocalFS) Open(name string) (fs.File, error) { + cleanPath, err := util.CleanRelativePath(name) + if err != nil { + return nil, err + } + basePath := f.Base() + absFn := filepath.Join(basePath, cleanPath) + finfo, err := os.Stat(absFn) + if err != nil { + return nil, ErrFileNotExist + } + // Make sure access to the file is allowed (symlink check, etc) + ok, err := f.fileIsAllowed(basePath, absFn, finfo) + if err != nil { + return nil, err + } + if !ok { + return nil, ErrFileNotExist + } + return &LocalFile{path: absFn}, nil } // Base returns the base path for the LocalFS. +// The returned string uses os-specific path separator. func (f LocalFS) Base() string { return f.basePath } -// Files returns a slice of all the file paths in the LocalFS relative to the base path. -// The returned strings use the same path separator as the -func (f LocalFS) Files() []string { - var files []string - for p := range f.m { - r, err := filepath.Rel(f.basePath, p) - if strings.Contains(r, "..") || err != nil { +// Files returns a slice of all the relative file paths on the LocalFS. +// The returned strings can be passed to Open() to open those files. +// The returned strings use os-specific path separator. +func (f LocalFS) Files() ([]string, error) { + // Accumulate all files into filesMap by calling f.collectFilesFunc, which will write into the accumulator. + // Those are absolute because filepath.Walk uses absolute file paths. + absFilePaths := make(map[string]struct{}) + if err := filepath.Walk(f.basePath, f.walkFunc(f.Base(), absFilePaths)); err != nil { + return nil, fmt.Errorf("walk: %w", err) + } + // Convert the accumulator into a slice of relative path strings + relFiles := make([]string, 0, len(absFilePaths)) + base := f.Base() + for fn := range absFilePaths { + relPath, err := filepath.Rel(base, fn) + if err != nil { + return nil, err + } + clenRelPath, err := util.CleanRelativePath(relPath) + if strings.Contains(clenRelPath, "..") || err != nil { continue } - files = append(files, r) + relFiles = append(relFiles, clenRelPath) } - - return files + return relFiles, nil } +// Remove removes a plugin from the local filesystem by deleting all files in the folder. +// It returns ErrUninstallInvalidPluginDir is the plugin does not contain plugin.json nor dist/plugin.json. func (f LocalFS) Remove() error { // extra security check to ensure we only remove a directory that looks like a plugin if _, err := os.Stat(filepath.Join(f.basePath, "plugin.json")); os.IsNotExist(err) { @@ -89,11 +164,72 @@ func (f LocalFS) Remove() error { return ErrUninstallInvalidPluginDir } } - return os.RemoveAll(f.basePath) } -var _ fs.File = (*LocalFile)(nil) +// staticFilesMap is a set-like map that contains files that can be accessed from a plugins.FS. +type staticFilesMap map[string]struct{} + +// isAllowed returns true if the provided path is allowed. +// path is a string accepted by an FS Open() method. +func (a staticFilesMap) isAllowed(path string) bool { + _, ok := a[path] + return ok +} + +// newStaticFilesMap creates a new staticFilesMap from a list of allowed file paths. +func newStaticFilesMap(files ...string) staticFilesMap { + m := staticFilesMap(make(map[string]struct{}, len(files))) + for _, k := range files { + m[k] = struct{}{} + } + return m +} + +// StaticFS wraps an FS and allows accessing only the files in the allowList. +// This is a more secure implementation of a FS suitable for production environments. +// The keys of the allow list must be in the same format used by the underlying FS' Open() method. +type StaticFS struct { + FS + + // staticFilesMap is a map of allowed paths (accepted by FS.Open()) + staticFilesMap staticFilesMap +} + +// NewStaticFS returns a new StaticFS that can access the files on an underlying FS, +// but only if they are also specified in a static list, which is constructed when creating the object +// by calling Files() on the underlying FS. +func NewStaticFS(fs FS) (StaticFS, error) { + files, err := fs.Files() + if err != nil { + return StaticFS{}, err + } + return StaticFS{ + FS: fs, + staticFilesMap: newStaticFilesMap(files...), + }, nil +} + +// Open checks that name is an allowed file and, if so, it returns a fs.File to access it, by calling the +// underlying FS' Open() method. +// If access is denied, the function returns ErrFileNotExist. +func (f StaticFS) Open(name string) (fs.File, error) { + // Ensure access to the file is allowed + if !f.staticFilesMap.isAllowed(name) { + return nil, ErrFileNotExist + } + // Use the wrapped FS to access the file + return f.FS.Open(name) +} + +// Files returns a slice of all static file paths relative to the base path. +func (f StaticFS) Files() ([]string, error) { + files := make([]string, 0, len(f.staticFilesMap)) + for fn := range f.staticFilesMap { + files = append(files, fn) + } + return files, nil +} // LocalFile implements a fs.File for accessing the local filesystem. type LocalFile struct { diff --git a/pkg/plugins/localfiles_test.go b/pkg/plugins/localfiles_test.go index 86ddc6d9ec8..af523cde9b7 100644 --- a/pkg/plugins/localfiles_test.go +++ b/pkg/plugins/localfiles_test.go @@ -1,9 +1,11 @@ package plugins import ( + "errors" "io" "os" "path/filepath" + "sort" "testing" "github.com/stretchr/testify/require" @@ -17,14 +19,7 @@ func TestLocalFS_Remove(t *testing.T) { require.NoError(t, err) err = f.Close() require.NoError(t, err) - - fs := NewLocalFS( - map[string]struct{}{ - "plugin.json": {}, - }, - pluginDir, - ) - + fs := NewLocalFS(pluginDir) err = fs.Remove() require.NoError(t, err) @@ -49,12 +44,7 @@ func TestLocalFS_Remove(t *testing.T) { pluginDir = filepath.Dir(pluginDistDir) - fs = NewLocalFS( - map[string]struct{}{ - "dist/plugin.json": {}, - }, - pluginDir, - ) + fs = NewLocalFS(pluginDir) err = fs.Remove() require.NoError(t, err) @@ -78,12 +68,7 @@ func TestLocalFS_Remove(t *testing.T) { err = f.Close() require.NoError(t, err) - fs = NewLocalFS( - map[string]struct{}{ - "system32/important.exe": {}, - }, - pluginDir, - ) + fs = NewLocalFS(pluginDir) err = fs.Remove() require.ErrorIs(t, err, ErrUninstallInvalidPluginDir) @@ -222,3 +207,73 @@ func newTempFileScenarioForTest(t *testing.T) tempFileScenario { require.NoError(t, err) return s } + +func createDummyTempFile(dir, fn string) (err error) { + f, err := os.Create(filepath.Join(dir, fn)) // nolint: gosec + if err != nil { + return err + } + defer func() { + if closeErr := f.Close(); closeErr != nil && err == nil { + err = closeErr + } + }() + _, err = f.WriteString(fn) + return +} + +func TestStaticFS(t *testing.T) { + tmp := t.TempDir() + const allowedFn, deniedFn = "allowed.txt", "denied.txt" + require.NoError(t, createDummyTempFile(tmp, allowedFn)) + + localFS := NewLocalFS(tmp) + staticFS, err := NewStaticFS(localFS) + require.NoError(t, err) + + t.Run("open allowed", func(t *testing.T) { + f, err := staticFS.Open(allowedFn) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, f.Close()) }) + b, err := io.ReadAll(f) + require.NoError(t, err) + require.Equal(t, []byte(allowedFn), b) + }) + + t.Run("open denied", func(t *testing.T) { + // Add file after initialization + require.NoError(t, createDummyTempFile(tmp, deniedFn)) + + // StaticFS should fail + _, err := staticFS.Open(deniedFn) + require.True(t, errors.Is(err, ErrFileNotExist)) + + // Underlying FS should succeed + f, err := localFS.Open(deniedFn) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, f.Close()) }) + b, err := io.ReadAll(f) + require.NoError(t, err) + require.Equal(t, []byte("denied.txt"), b) + }) + + t.Run("open not existing", func(t *testing.T) { + _, err := staticFS.Open("unknown.txt") + require.True(t, errors.Is(err, ErrFileNotExist)) + }) + + t.Run("list files", func(t *testing.T) { + t.Run("underlying fs has extra files", func(t *testing.T) { + files, err := localFS.Files() + require.NoError(t, err) + sort.Strings(files) + require.Equal(t, []string{allowedFn, deniedFn}, files) + }) + + t.Run("staticfs filters underelying fs's files", func(t *testing.T) { + files, err := staticFS.Files() + require.NoError(t, err) + require.Equal(t, []string{allowedFn}, files) + }) + }) +} diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index 51cea698f6a..73585979579 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -364,8 +364,8 @@ func (f *FakePluginFiles) Base() string { return f.base } -func (f *FakePluginFiles) Files() []string { - return []string{} +func (f *FakePluginFiles) Files() ([]string, error) { + return []string{}, nil } func (f *FakePluginFiles) Remove() error { diff --git a/pkg/plugins/manager/loader/finder/local.go b/pkg/plugins/manager/loader/finder/local.go index 30752497e09..a3217697a60 100644 --- a/pkg/plugins/manager/loader/finder/local.go +++ b/pkg/plugins/manager/loader/finder/local.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/infra/fs" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/log" "github.com/grafana/grafana/pkg/util" ) @@ -24,15 +25,21 @@ var ( ) type Local struct { - log log.Logger + log log.Logger + production bool } -func NewLocalFinder() *Local { +func NewLocalFinder(cfg *config.Cfg) *Local { return &Local{ - log: log.New("local.finder"), + production: !cfg.DevMode, + log: log.New("local.finder"), } } +func ProvideLocalFinder(cfg *config.Cfg) *Local { + return NewLocalFinder(cfg) +} + func (l *Local) Find(ctx context.Context, src plugins.PluginSource) ([]*plugins.FoundBundle, error) { if len(src.PluginURIs(ctx)) == 0 { return []*plugins.FoundBundle{}, nil @@ -81,15 +88,21 @@ func (l *Local) Find(ctx context.Context, src plugins.PluginSource) ([]*plugins. var res = make(map[string]*plugins.FoundBundle) for pluginDir, data := range foundPlugins { - files, err := collectFilesWithin(pluginDir) - if err != nil { - return nil, err + var pluginFs plugins.FS + pluginFs = plugins.NewLocalFS(pluginDir) + if l.production { + // In prod, tighten up security by allowing access only to the files present up to this point. + // Any new file "sneaked in" won't be allowed and will acts as if the file did not exist. + var err error + pluginFs, err = plugins.NewStaticFS(pluginFs) + if err != nil { + return nil, err + } } - res[pluginDir] = &plugins.FoundBundle{ Primary: plugins.FoundPlugin{ JSONData: data, - FS: plugins.NewLocalFS(files, pluginDir), + FS: pluginFs, }, } } @@ -190,61 +203,6 @@ func (l *Local) getAbsPluginJSONPaths(path string) ([]string, error) { return pluginJSONPaths, nil } -func collectFilesWithin(dir string) (map[string]struct{}, error) { - files := map[string]struct{}{} - err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - if info.Mode()&os.ModeSymlink == os.ModeSymlink { - symlinkPath, err := filepath.EvalSymlinks(path) - if err != nil { - return err - } - - symlink, err := os.Stat(symlinkPath) - if err != nil { - return err - } - - // verify that symlinked file is within plugin directory - p, err := filepath.Rel(dir, symlinkPath) - if err != nil { - return err - } - if p == ".." || strings.HasPrefix(p, ".."+string(filepath.Separator)) { - return fmt.Errorf("file '%s' not inside of plugin directory", p) - } - - // skip adding symlinked directories - if symlink.IsDir() { - return nil - } - } - - // skip directories - if info.IsDir() { - return nil - } - - // verify that file is within plugin directory - file, err := filepath.Rel(dir, path) - if err != nil { - return err - } - if strings.HasPrefix(file, ".."+string(filepath.Separator)) { - return fmt.Errorf("file '%s' not inside of plugin directory", file) - } - - files[path] = struct{}{} - - return nil - }) - - return files, err -} - func (l *Local) readFile(pluginJSONPath string) (io.ReadCloser, error) { l.log.Debug("Loading plugin", "path", pluginJSONPath) diff --git a/pkg/plugins/manager/loader/finder/local_test.go b/pkg/plugins/manager/loader/finder/local_test.go index f1d240da749..e6b4f031e46 100644 --- a/pkg/plugins/manager/loader/finder/local_test.go +++ b/pkg/plugins/manager/loader/finder/local_test.go @@ -14,7 +14,10 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/manager/fakes" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/pluginsintegration/config" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -23,6 +26,11 @@ func TestFinder_Find(t *testing.T) { if err != nil { require.NoError(t, err) } + + cfg := setting.NewCfg() + pCfg, err := config.ProvideConfig(setting.ProvideProvider(cfg), cfg, featuremgmt.WithFeatures()) + require.NoError(t, err) + testCases := []struct { name string pluginDirs []string @@ -55,10 +63,7 @@ func TestFinder_Find(t *testing.T) { Backend: true, Executable: "test", }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "valid-v2-signature/plugin/plugin.json"): {}, - filepath.Join(testData, "valid-v2-signature/plugin/MANIFEST.txt"): {}, - }, filepath.Join(testData, "valid-v2-signature/plugin")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "valid-v2-signature/plugin")), }, }, }, @@ -87,12 +92,7 @@ func TestFinder_Find(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "duplicate-plugins/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/MANIFEST.txt"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/MANIFEST.txt"): {}, - }, filepath.Join(testData, "duplicate-plugins/nested")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "duplicate-plugins/nested")), }, Children: []*plugins.FoundPlugin{ { @@ -114,10 +114,7 @@ func TestFinder_Find(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "duplicate-plugins/nested/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/MANIFEST.txt"): {}, - }, filepath.Join(testData, "duplicate-plugins/nested/nested")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "duplicate-plugins/nested/nested")), }, }, }, @@ -178,14 +175,7 @@ func TestFinder_Find(t *testing.T) { {Name: "Nginx Datasource", Type: "datasource", Role: "Viewer"}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "includes-symlinks/MANIFEST.txt"): {}, - filepath.Join(testData, "includes-symlinks/dashboards/connections.json"): {}, - filepath.Join(testData, "includes-symlinks/dashboards/extra/memory.json"): {}, - filepath.Join(testData, "includes-symlinks/plugin.json"): {}, - filepath.Join(testData, "includes-symlinks/symlink_to_txt"): {}, - filepath.Join(testData, "includes-symlinks/text.txt"): {}, - }, filepath.Join(testData, "includes-symlinks")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "includes-symlinks")), }, }, }, @@ -213,12 +203,7 @@ func TestFinder_Find(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "duplicate-plugins/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/MANIFEST.txt"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/MANIFEST.txt"): {}, - }, filepath.Join(testData, "duplicate-plugins/nested")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "duplicate-plugins/nested")), }, Children: []*plugins.FoundPlugin{ { @@ -240,10 +225,7 @@ func TestFinder_Find(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "duplicate-plugins/nested/nested/plugin.json"): {}, - filepath.Join(testData, "duplicate-plugins/nested/nested/MANIFEST.txt"): {}, - }, filepath.Join(testData, "duplicate-plugins/nested/nested")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "duplicate-plugins/nested/nested")), }, }, }, @@ -267,10 +249,7 @@ func TestFinder_Find(t *testing.T) { State: plugins.AlphaRelease, Backend: true, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(testData, "invalid-v1-signature/plugin/plugin.json"): {}, - filepath.Join(testData, "invalid-v1-signature/plugin/MANIFEST.txt"): {}, - }, filepath.Join(testData, "invalid-v1-signature/plugin")), + FS: mustNewStaticFSForTests(t, filepath.Join(testData, "invalid-v1-signature/plugin")), }, }, }, @@ -278,7 +257,7 @@ func TestFinder_Find(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - f := NewLocalFinder() + f := NewLocalFinder(pCfg) pluginBundles, err := f.Find(context.Background(), &fakes.FakePluginSource{ PluginURIsFunc: func(ctx context.Context) []string { return tc.pluginDirs @@ -294,14 +273,18 @@ func TestFinder_Find(t *testing.T) { return pluginBundles[i].Primary.JSONData.ID < pluginBundles[j].Primary.JSONData.ID }) - if !cmp.Equal(pluginBundles, tc.expectedBundles, localFSComparer) { - t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(pluginBundles, tc.expectedBundles, localFSComparer)) + if !cmp.Equal(pluginBundles, tc.expectedBundles, fsComparer) { + t.Fatalf("Result mismatch (-want +got):\n%s", cmp.Diff(pluginBundles, tc.expectedBundles, fsComparer)) } }) } } func TestFinder_getAbsPluginJSONPaths(t *testing.T) { + cfg := setting.NewCfg() + pCfg, err := config.ProvideConfig(setting.ProvideProvider(cfg), cfg, featuremgmt.WithFeatures()) + require.NoError(t, err) + t.Run("When scanning a folder that doesn't exists shouldn't return an error", func(t *testing.T) { origWalk := walk walk = func(path string, followSymlinks, detectSymlinkInfiniteLoop bool, walkFn util.WalkFunc) error { @@ -311,7 +294,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder() + finder := NewLocalFinder(pCfg) paths, err := finder.getAbsPluginJSONPaths("test") require.NoError(t, err) require.Empty(t, paths) @@ -326,7 +309,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder() + finder := NewLocalFinder(pCfg) paths, err := finder.getAbsPluginJSONPaths("test") require.NoError(t, err) require.Empty(t, paths) @@ -341,7 +324,7 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) { walk = origWalk }) - finder := NewLocalFinder() + finder := NewLocalFinder(pCfg) paths, err := finder.getAbsPluginJSONPaths("test") require.Error(t, err) require.Empty(t, paths) @@ -469,9 +452,15 @@ func TestFinder_readPluginJSON(t *testing.T) { } } -var localFSComparer = cmp.Comparer(func(fs1 plugins.LocalFS, fs2 plugins.LocalFS) bool { - fs1Files := fs1.Files() - fs2Files := fs2.Files() +var fsComparer = cmp.Comparer(func(fs1 plugins.FS, fs2 plugins.FS) bool { + fs1Files, err := fs1.Files() + if err != nil { + panic(err) + } + fs2Files, err := fs2.Files() + if err != nil { + panic(err) + } sort.SliceStable(fs1Files, func(i, j int) bool { return fs1Files[i] < fs1Files[j] @@ -483,3 +472,9 @@ var localFSComparer = cmp.Comparer(func(fs1 plugins.LocalFS, fs2 plugins.LocalFS return cmp.Equal(fs1Files, fs2Files) && fs1.Base() == fs2.Base() }) + +func mustNewStaticFSForTests(t *testing.T, dir string) plugins.FS { + sfs, err := plugins.NewStaticFS(plugins.NewLocalFS(dir)) + require.NoError(t, err) + return sfs +} diff --git a/pkg/plugins/manager/loader/loader_test.go b/pkg/plugins/manager/loader/loader_test.go index 0fa139bd86b..e96b6291313 100644 --- a/pkg/plugins/manager/loader/loader_test.go +++ b/pkg/plugins/manager/loader/loader_test.go @@ -3,7 +3,6 @@ package loader import ( "context" "fmt" - "os" "path/filepath" "sort" "testing" @@ -28,13 +27,18 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -var compareOpts = []cmp.Option{cmpopts.IgnoreFields(plugins.Plugin{}, "client", "log"), localFSComparer} +var compareOpts = []cmp.Option{cmpopts.IgnoreFields(plugins.Plugin{}, "client", "log"), fsComparer} -var localFSComparer = cmp.Comparer(func(fs1 plugins.LocalFS, fs2 plugins.LocalFS) bool { - fs1Files := fs1.Files() - fs2Files := fs2.Files() +var fsComparer = cmp.Comparer(func(fs1 plugins.FS, fs2 plugins.FS) bool { + fs1Files, err := fs1.Files() + if err != nil { + panic(err) + } + fs2Files, err := fs2.Files() + if err != nil { + panic(err) + } - finder.NewLocalFinder() sort.SliceStable(fs1Files, func(i, j int) bool { return fs1Files[i] < fs1Files[j] }) @@ -108,9 +112,8 @@ func TestLoader_Load(t *testing.T) { }, Module: "app/plugins/datasource/cloudwatch/module", BaseURL: "public/app/plugins/datasource/cloudwatch", - FS: plugins.NewLocalFS( - filesInDir(t, filepath.Join(corePluginDir, "app/plugins/datasource/cloudwatch")), - filepath.Join(corePluginDir, "app/plugins/datasource/cloudwatch")), + + FS: mustNewStaticFSForTests(t, filepath.Join(corePluginDir, "app/plugins/datasource/cloudwatch")), Signature: plugins.SignatureInternal, Class: plugins.Core, }, @@ -147,12 +150,9 @@ func TestLoader_Load(t *testing.T) { Backend: true, State: "alpha", }, - Module: "plugins/test-datasource/module", - BaseURL: "public/plugins/test-datasource", - FS: plugins.NewLocalFS( - filesInDir(t, filepath.Join(parentDir, "testdata/valid-v2-signature/plugin/")), - filepath.Join(parentDir, "testdata/valid-v2-signature/plugin/"), - ), + Module: "plugins/test-datasource/module", + BaseURL: "public/plugins/test-datasource", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/valid-v2-signature/plugin/")), Signature: "valid", SignatureType: plugins.GrafanaSignature, SignatureOrg: "Grafana Labs", @@ -226,20 +226,10 @@ func TestLoader_Load(t *testing.T) { }, }, }, - Class: plugins.External, - Module: "plugins/test-app/module", - BaseURL: "public/plugins/test-app", - FS: plugins.NewLocalFS( - map[string]struct{}{ - filepath.Join(parentDir, "testdata/includes-symlinks", "/MANIFEST.txt"): {}, - filepath.Join(parentDir, "testdata/includes-symlinks", "dashboards/connections.json"): {}, - filepath.Join(parentDir, "testdata/includes-symlinks", "dashboards/extra/memory.json"): {}, - filepath.Join(parentDir, "testdata/includes-symlinks", "plugin.json"): {}, - filepath.Join(parentDir, "testdata/includes-symlinks", "symlink_to_txt"): {}, - filepath.Join(parentDir, "testdata/includes-symlinks", "text.txt"): {}, - }, - filepath.Join(parentDir, "testdata/includes-symlinks"), - ), + Class: plugins.External, + Module: "plugins/test-app/module", + BaseURL: "public/plugins/test-app", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/includes-symlinks")), Signature: "valid", SignatureType: plugins.GrafanaSignature, SignatureOrg: "Grafana Labs", @@ -276,13 +266,10 @@ func TestLoader_Load(t *testing.T) { Backend: true, State: plugins.AlphaRelease, }, - Class: plugins.External, - Module: "plugins/test-datasource/module", - BaseURL: "public/plugins/test-datasource", - FS: plugins.NewLocalFS( - filesInDir(t, filepath.Join(parentDir, "testdata/unsigned-datasource/plugin")), - filepath.Join(parentDir, "testdata/unsigned-datasource/plugin"), - ), + Class: plugins.External, + Module: "plugins/test-datasource/module", + BaseURL: "public/plugins/test-datasource", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/unsigned-datasource/plugin")), Signature: "unsigned", }, }, @@ -330,13 +317,10 @@ func TestLoader_Load(t *testing.T) { Backend: true, State: plugins.AlphaRelease, }, - Class: plugins.External, - Module: "plugins/test-datasource/module", - BaseURL: "public/plugins/test-datasource", - FS: plugins.NewLocalFS( - filesInDir(t, filepath.Join(parentDir, "testdata/unsigned-datasource/plugin")), - filepath.Join(parentDir, "testdata/unsigned-datasource/plugin"), - ), + Class: plugins.External, + Module: "plugins/test-datasource/module", + BaseURL: "public/plugins/test-datasource", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/unsigned-datasource/plugin")), Signature: plugins.SignatureUnsigned, }, }, @@ -440,14 +424,11 @@ func TestLoader_Load(t *testing.T) { Backend: false, }, DefaultNavURL: "/plugins/test-app/page/root-page-react", - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(parentDir, "testdata/test-app-with-includes", "dashboards/memory.json"): {}, - filepath.Join(parentDir, "testdata/test-app-with-includes", "plugin.json"): {}, - }, filepath.Join(parentDir, "testdata/test-app-with-includes")), - Class: plugins.External, - Signature: plugins.SignatureUnsigned, - Module: "plugins/test-app/module", - BaseURL: "public/plugins/test-app", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/test-app-with-includes")), + Class: plugins.External, + Signature: plugins.SignatureUnsigned, + Module: "plugins/test-app/module", + BaseURL: "public/plugins/test-app", }, }, }, @@ -532,9 +513,7 @@ func TestLoader_Load_CustomSource(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(parentDir, "testdata/cdn/plugin", "plugin.json"): {}, - }, filepath.Join(parentDir, "testdata/cdn/plugin")), + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/cdn/plugin")), Class: plugins.Bundled, Signature: plugins.SignatureValid, BaseURL: "plugin-cdn/grafana-worldmap-panel/0.3.3/public/plugins/grafana-worldmap-panel", @@ -670,13 +649,10 @@ func TestLoader_Load_MultiplePlugins(t *testing.T) { Executable: "test", State: plugins.AlphaRelease, }, - Class: plugins.External, - Module: "plugins/test-datasource/module", - BaseURL: "public/plugins/test-datasource", - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(parentDir, "testdata/valid-v2-pvt-signature/plugin/plugin.json"): {}, - filepath.Join(parentDir, "testdata/valid-v2-pvt-signature/plugin/MANIFEST.txt"): {}, - }, filepath.Join(parentDir, "testdata/valid-v2-pvt-signature/plugin")), + Class: plugins.External, + Module: "plugins/test-datasource/module", + BaseURL: "public/plugins/test-datasource", + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "testdata/valid-v2-pvt-signature/plugin")), Signature: "valid", SignatureType: plugins.PrivateSignature, SignatureOrg: "Will Browne", @@ -795,10 +771,7 @@ func TestLoader_Load_RBACReady(t *testing.T) { }, Backend: false, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(pluginDir, "plugin.json"): {}, - filepath.Join(pluginDir, "MANIFEST.txt"): {}, - }, pluginDir), + FS: mustNewStaticFSForTests(t, pluginDir), Class: plugins.External, Signature: plugins.SignatureValid, SignatureType: plugins.PrivateSignature, @@ -885,10 +858,7 @@ func TestLoader_Load_Signature_RootURL(t *testing.T) { Backend: true, Executable: "test", }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(filepath.Join(parentDir, "/testdata/valid-v2-pvt-signature-root-url-uri/plugin"), "plugin.json"): {}, - filepath.Join(filepath.Join(parentDir, "/testdata/valid-v2-pvt-signature-root-url-uri/plugin"), "MANIFEST.txt"): {}, - }, filepath.Join(parentDir, "/testdata/valid-v2-pvt-signature-root-url-uri/plugin")), + FS: mustNewStaticFSForTests(t, filepath.Join(parentDir, "/testdata/valid-v2-pvt-signature-root-url-uri/plugin")), Class: plugins.External, Signature: plugins.SignatureValid, SignatureType: plugins.PrivateSignature, @@ -972,7 +942,7 @@ func TestLoader_Load_DuplicatePlugins(t *testing.T) { }, Backend: false, }, - FS: plugins.NewLocalFS(filesInDir(t, pluginDir), pluginDir), + FS: mustNewStaticFSForTests(t, pluginDir), Class: plugins.External, Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, @@ -1062,7 +1032,7 @@ func TestLoader_Load_SkipUninitializedPlugins(t *testing.T) { }, Backend: false, }, - FS: plugins.NewLocalFS(filesInDir(t, pluginDir1), pluginDir1), + FS: mustNewStaticFSForTests(t, pluginDir1), Class: plugins.External, Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, @@ -1137,10 +1107,9 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { }, Backend: true, }, - Module: "plugins/test-datasource/module", - BaseURL: "public/plugins/test-datasource", - FS: plugins.NewLocalFS(filesInDir(t, filepath.Join(rootDir, "testdata/nested-plugins/parent")), - filepath.Join(rootDir, "testdata/nested-plugins/parent")), + Module: "plugins/test-datasource/module", + BaseURL: "public/plugins/test-datasource", + FS: mustNewStaticFSForTests(t, filepath.Join(rootDir, "testdata/nested-plugins/parent")), Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, SignatureOrg: "Grafana Labs", @@ -1170,10 +1139,9 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - Module: "plugins/test-panel/module", - BaseURL: "public/plugins/test-panel", - FS: plugins.NewLocalFS(filesInDir(t, filepath.Join(rootDir, "testdata/nested-plugins/parent/nested")), - filepath.Join(rootDir, "testdata/nested-plugins/parent/nested")), + Module: "plugins/test-panel/module", + BaseURL: "public/plugins/test-panel", + FS: mustNewStaticFSForTests(t, filepath.Join(rootDir, "testdata/nested-plugins/parent/nested")), Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, SignatureOrg: "Grafana Labs", @@ -1310,10 +1278,9 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { }, Backend: false, }, - Module: "plugins/myorgid-simple-app/module", - BaseURL: "public/plugins/myorgid-simple-app", - FS: plugins.NewLocalFS(filesInDir(t, filepath.Join(rootDir, "testdata/app-with-child/dist")), - filepath.Join(rootDir, "testdata/app-with-child/dist")), + Module: "plugins/myorgid-simple-app/module", + BaseURL: "public/plugins/myorgid-simple-app", + FS: mustNewStaticFSForTests(t, filepath.Join(rootDir, "testdata/app-with-child/dist")), DefaultNavURL: "/plugins/myorgid-simple-app/page/root-page-react", Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, @@ -1349,10 +1316,9 @@ func TestLoader_Load_NestedPlugins(t *testing.T) { Plugins: []plugins.Dependency{}, }, }, - Module: "plugins/myorgid-simple-app/child/module", - BaseURL: "public/plugins/myorgid-simple-app", - FS: plugins.NewLocalFS(filesInDir(t, filepath.Join(rootDir, "testdata/app-with-child/dist/child")), - filepath.Join(rootDir, "testdata/app-with-child/dist/child")), + Module: "plugins/myorgid-simple-app/child/module", + BaseURL: "public/plugins/myorgid-simple-app", + FS: mustNewStaticFSForTests(t, filepath.Join(rootDir, "testdata/app-with-child/dist/child")), IncludedInAppID: parent.ID, Signature: plugins.SignatureValid, SignatureType: plugins.GrafanaSignature, @@ -1421,7 +1387,7 @@ func Test_setPathsBasedOnApp(t *testing.T) { func newLoader(cfg *config.Cfg, cbs ...func(loader *Loader)) *Loader { l := New(cfg, &fakes.FakeLicensingService{}, signature.NewUnsignedAuthorizer(cfg), fakes.NewFakePluginRegistry(), fakes.NewFakeBackendProcessProvider(), fakes.NewFakeProcessManager(), fakes.NewFakeRoleRegistry(), - assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(), + assetpath.ProvideService(pluginscdn.ProvideService(cfg)), finder.NewLocalFinder(cfg), signature.ProvideService(cfg, keystore.ProvideService(kvstore.NewFakeKVStore()))) for _, cb := range cbs { @@ -1453,39 +1419,8 @@ func verifyState(t *testing.T, ps []*plugins.Plugin, reg *fakes.FakePluginRegist } } -func filesInDir(t *testing.T, dir string) map[string]struct{} { - files, err := collectFilesWithin(dir) - if err != nil { - t.Logf("Could not collect plugin file info. Err: %v", err) - return map[string]struct{}{} - } - return files -} - -func collectFilesWithin(dir string) (map[string]struct{}, error) { - files := map[string]struct{}{} - err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - // skip directories - if info.IsDir() { - return nil - } - - // verify that file is within plugin directory - //file, err := filepath.Rel(dir, path) - //if err != nil { - // return err - //} - //if strings.HasPrefix(file, ".."+string(filepath.Separator)) { - // return fmt.Errorf("file '%s' not inside of plugin directory", file) - //} - - files[path] = struct{}{} - return nil - }) - - return files, err +func mustNewStaticFSForTests(t *testing.T, dir string) plugins.FS { + sfs, err := plugins.NewStaticFS(plugins.NewLocalFS(dir)) + require.NoError(t, err) + return sfs } diff --git a/pkg/plugins/manager/manager_integration_test.go b/pkg/plugins/manager/manager_integration_test.go index 1e20c169614..074cbdaa0db 100644 --- a/pkg/plugins/manager/manager_integration_test.go +++ b/pkg/plugins/manager/manager_integration_test.go @@ -118,7 +118,7 @@ func TestIntegrationPluginManager(t *testing.T) { reg := registry.ProvideService() lic := plicensing.ProvideLicensing(cfg, &licensing.OSSLicensingService{Cfg: cfg}) l := loader.ProvideService(pCfg, lic, signature.NewUnsignedAuthorizer(pCfg), - reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(), fakes.NewFakeRoleRegistry(), + reg, provider.ProvideService(coreRegistry), finder.NewLocalFinder(pCfg), fakes.NewFakeRoleRegistry(), assetpath.ProvideService(pluginscdn.ProvideService(pCfg)), signature.ProvideService(pCfg, keystore.ProvideService(kvstore.NewFakeKVStore()))) srcs := sources.ProvideService(cfg, pCfg) ps, err := store.ProvideService(reg, srcs, l) diff --git a/pkg/plugins/manager/signature/manifest.go b/pkg/plugins/manager/signature/manifest.go index 2f4bbcf853f..992a9080566 100644 --- a/pkg/plugins/manager/signature/manifest.go +++ b/pkg/plugins/manager/signature/manifest.go @@ -17,6 +17,7 @@ import ( "github.com/ProtonMail/go-crypto/openpgp/clearsign" "github.com/gobwas/glob" + "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/log" @@ -29,6 +30,9 @@ var ( // toSlash is filepath.ToSlash, but can be overwritten in tests path separators cross-platform toSlash = filepath.ToSlash + + // fromSlash is filepath.FromSlash, but can be overwritten in tests path separators cross-platform + fromSlash = filepath.FromSlash ) // PluginManifest holds details for the file manifest @@ -100,8 +104,11 @@ func (s *Signature) Calculate(ctx context.Context, src plugins.PluginSource, plu if defaultSignature, exists := src.DefaultSignature(ctx); exists { return defaultSignature, nil } - - if len(plugin.FS.Files()) == 0 { + fsFiles, err := plugin.FS.Files() + if err != nil { + return plugins.Signature{}, fmt.Errorf("files: %w", err) + } + if len(fsFiles) == 0 { s.mlog.Warn("No plugin file information in directory", "pluginID", plugin.JSONData.ID) return plugins.Signature{ Status: plugins.SignatureInvalid, @@ -190,7 +197,7 @@ func (s *Signature) Calculate(ctx context.Context, src plugins.PluginSource, plu // Track files missing from the manifest var unsignedFiles []string - for _, f := range plugin.FS.Files() { + for _, f := range fsFiles { // Ensure slashes are used, because MANIFEST.txt always uses slashes regardless of the filesystem f = toSlash(f) @@ -223,6 +230,8 @@ func (s *Signature) Calculate(ctx context.Context, src plugins.PluginSource, plu } func verifyHash(mlog log.Logger, plugin plugins.FoundPlugin, path, hash string) error { + path = fromSlash(path) + // nolint:gosec // We can ignore the gosec G304 warning on this one because `path` is based // on the path provided in a manifest file for a plugin and not user input. diff --git a/pkg/plugins/manager/signature/manifest_test.go b/pkg/plugins/manager/signature/manifest_test.go index 5c9d78f7d37..069f7495136 100644 --- a/pkg/plugins/manager/signature/manifest_test.go +++ b/pkg/plugins/manager/signature/manifest_test.go @@ -2,6 +2,7 @@ package signature import ( "context" + "io/fs" "path/filepath" "sort" "strings" @@ -172,10 +173,7 @@ func TestCalculate(t *testing.T) { Version: "1.0.0", }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(basePath, "MANIFEST.txt"): {}, - filepath.Join(basePath, "plugin.json"): {}, - }, basePath), + FS: mustNewStaticFSForTests(t, basePath), }) require.NoError(t, err) require.Equal(t, tc.expectedSignature, sig) @@ -204,11 +202,7 @@ func TestCalculate(t *testing.T) { Version: "1.0.0", }, }, - FS: plugins.NewLocalFS(map[string]struct{}{ - filepath.Join(basePath, "MANIFEST.txt"): {}, - filepath.Join(basePath, "plugin.json"): {}, - filepath.Join(basePath, "chrome-win/debug.log"): {}, - }, basePath), + FS: mustNewStaticFSForTests(t, basePath), }) require.NoError(t, err) require.Equal(t, plugins.Signature{ @@ -219,28 +213,51 @@ func TestCalculate(t *testing.T) { }) t.Run("Signature verification should work with any path separator", func(t *testing.T) { - var toSlashUnix = newToSlash('/') - var toSlashWindows = newToSlash('\\') + const basePath = "../testdata/app-with-child/dist" - for _, tc := range []struct { - name string - sep string - toSlash func(string) string + platformWindows := fsPlatform{separator: '\\'} + platformUnix := fsPlatform{separator: '/'} + + type testCase struct { + name string + platform fsPlatform + fsFactory func() (plugins.FS, error) + } + var testCases []testCase + for _, fsFactory := range []struct { + name string + f func() (plugins.FS, error) }{ - {"unix", "/", toSlashUnix}, - {"windows", "\\", toSlashWindows}, + {"local fs", func() (plugins.FS, error) { + return plugins.NewLocalFS(basePath), nil + }}, + {"static fs", func() (plugins.FS, error) { + return plugins.NewStaticFS(plugins.NewLocalFS(basePath)) + }}, } { + testCases = append(testCases, []testCase{ + {"unix " + fsFactory.name, platformUnix, fsFactory.f}, + {"windows " + fsFactory.name, platformWindows, fsFactory.f}, + }...) + } + + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Replace toSlash for cross-platform testing oldToSlash := toSlash + oldFromSlash := fromSlash t.Cleanup(func() { toSlash = oldToSlash + fromSlash = oldFromSlash }) - toSlash = tc.toSlash - - basePath := "../testdata/app-with-child/dist" + toSlash = tc.platform.toSlashFunc() + fromSlash = tc.platform.fromSlashFunc() s := ProvideService(&config.Cfg{}, keystore.ProvideService(kvstore.NewFakeKVStore())) + pfs, err := tc.fsFactory() + require.NoError(t, err) + pfs, err = newPathSeparatorOverrideFS(string(tc.platform.separator), pfs) + require.NoError(t, err) sig, err := s.Calculate(context.Background(), &fakes.FakePluginSource{ PluginClassFunc: func(ctx context.Context) plugins.Class { return plugins.External @@ -253,11 +270,7 @@ func TestCalculate(t *testing.T) { Version: "%VERSION%", }, }, - FS: newPathSeparatorOverrideFS(tc.sep, map[string]struct{}{ - filepath.Join(basePath, "MANIFEST.txt"): {}, - filepath.Join(basePath, "plugin.json"): {}, - filepath.Join(basePath, "child/plugin.json"): {}, - }, basePath), + FS: pfs, }) require.NoError(t, err) require.Equal(t, plugins.Signature{ @@ -270,20 +283,35 @@ func TestCalculate(t *testing.T) { }) } -// newToSlash returns a new function that acts as filepath.ToSlash but for the specified os-separator. +type fsPlatform struct { + separator rune +} + +// toSlashFunc returns a new function that acts as filepath.ToSlash but for the specified os-separator. // This can be used to test filepath.ToSlash-dependant code cross-platform. -func newToSlash(sep rune) func(string) string { +func (p fsPlatform) toSlashFunc() func(string) string { return func(path string) string { - if sep == '/' { + if p.separator == '/' { return path } - return strings.ReplaceAll(path, string(sep), "/") + return strings.ReplaceAll(path, string(p.separator), "/") } } -func TestNewToSlash(t *testing.T) { +// fromSlashFunc returns a new function that acts as filepath.FromSlash but for the specified os-separator. +// This can be used to test filepath.FromSlash-dependant code cross-platform. +func (p fsPlatform) fromSlashFunc() func(string) string { + return func(path string) string { + if p.separator == '/' { + return path + } + return strings.ReplaceAll(path, "/", string(p.separator)) + } +} + +func TestFsPlatform(t *testing.T) { t.Run("unix", func(t *testing.T) { - toSlashUnix := newToSlash('/') + toSlashUnix := fsPlatform{'/'}.toSlashFunc() require.Equal(t, "folder", toSlashUnix("folder")) require.Equal(t, "/folder", toSlashUnix("/folder")) require.Equal(t, "/folder/file", toSlashUnix("/folder/file")) @@ -291,39 +319,46 @@ func TestNewToSlash(t *testing.T) { }) t.Run("windows", func(t *testing.T) { - toSlashWindows := newToSlash('\\') + toSlashWindows := fsPlatform{'\\'}.toSlashFunc() require.Equal(t, "folder", toSlashWindows("folder")) require.Equal(t, "C:/folder", toSlashWindows("C:\\folder")) require.Equal(t, "folder/file.exe", toSlashWindows("folder\\file.exe")) }) } -// fsPathSeparatorFiles embeds plugins.LocalFS and overrides the Files() behaviour so all the returned elements +// fsPathSeparatorFiles embeds a plugins.FS and overrides the Files() behaviour so all the returned elements // have the specified path separator. This can be used to test Files() behaviour cross-platform. type fsPathSeparatorFiles struct { - plugins.LocalFS + plugins.FS separator string } // newPathSeparatorOverrideFS returns a new fsPathSeparatorFiles. Sep is the separator that will be used ONLY for -// the elements returned by Files(). Files and basePath MUST use the os-specific path separator (filepath.Separator) -// if Open() is required to work for the test case. -func newPathSeparatorOverrideFS(sep string, files map[string]struct{}, basePath string) fsPathSeparatorFiles { +// the elements returned by Files(). +func newPathSeparatorOverrideFS(sep string, ufs plugins.FS) (fsPathSeparatorFiles, error) { return fsPathSeparatorFiles{ - LocalFS: plugins.NewLocalFS(files, basePath), + FS: ufs, separator: sep, - } + }, nil } -// Files returns LocalFS.Files(), but all path separators (filepath.Separator) are replaced with f.separator. -func (f fsPathSeparatorFiles) Files() []string { - files := f.LocalFS.Files() +// Files returns LocalFS.Files(), but all path separators for the current platform (filepath.Separator) +// are replaced with f.separator. +func (f fsPathSeparatorFiles) Files() ([]string, error) { + files, err := f.FS.Files() + if err != nil { + return nil, err + } const osSepStr = string(filepath.Separator) for i := 0; i < len(files); i++ { files[i] = strings.ReplaceAll(files[i], osSepStr, f.separator) } - return files + return files, nil +} + +func (f fsPathSeparatorFiles) Open(name string) (fs.File, error) { + return f.FS.Open(strings.ReplaceAll(name, f.separator, string(filepath.Separator))) } func TestFSPathSeparatorFiles(t *testing.T) { @@ -335,17 +370,18 @@ func TestFSPathSeparatorFiles(t *testing.T) { {"windows", "\\"}, } { t.Run(tc.name, func(t *testing.T) { - fs := newPathSeparatorOverrideFS("/", map[string]struct{}{ - "a": {}, - strings.Join([]string{"a", "b", "c"}, tc.sep): {}, - }, ".") - files := fs.Files() - filesMap := make(map[string]struct{}, len(files)) - // Re-convert to map as the key order is not stable - for _, f := range files { - filesMap[f] = struct{}{} - } - require.Equal(t, filesMap, map[string]struct{}{"a": {}, strings.Join([]string{"a", "b", "c"}, tc.sep): {}}) + pfs, err := newPathSeparatorOverrideFS( + "/", plugins.NewInMemoryFS( + map[string][]byte{"a": nil, strings.Join([]string{"a", "b", "c"}, tc.sep): nil}, + ), + ) + require.NoError(t, err) + files, err := pfs.Files() + require.NoError(t, err) + exp := []string{"a", strings.Join([]string{"a", "b", "c"}, tc.sep)} + sort.Strings(files) + sort.Strings(exp) + require.Equal(t, exp, files) }) } } @@ -715,3 +751,9 @@ func createV2Manifest(t *testing.T, cbs ...func(*PluginManifest)) *PluginManifes return m } + +func mustNewStaticFSForTests(t *testing.T, dir string) plugins.FS { + sfs, err := plugins.NewStaticFS(plugins.NewLocalFS(dir)) + require.NoError(t, err) + return sfs +} diff --git a/pkg/plugins/test_utils.go b/pkg/plugins/test_utils.go new file mode 100644 index 00000000000..5a5487cb19e --- /dev/null +++ b/pkg/plugins/test_utils.go @@ -0,0 +1,82 @@ +package plugins + +import ( + "bytes" + "io/fs" + "os" + "time" +) + +var ( + _ FS = &inMemoryFS{} + _ fs.File = &inMemoryFile{} + _ fs.FileInfo = &inMemoryFileInfo{} +) + +// inMemoryFS is an FS that stores files in-memory. +type inMemoryFS struct { + files map[string][]byte +} + +// NewInMemoryFS returns a new FS with the specified files and content. +// The provided value is a map from file name (keys) to file content (values). +func NewInMemoryFS(files map[string][]byte) FS { + return &inMemoryFS{files: files} +} + +func (f inMemoryFS) Base() string { + return "" +} + +func (f inMemoryFS) Files() ([]string, error) { + fps := make([]string, 0, len(f.files)) + for fn := range f.files { + fps = append(fps, fn) + } + return fps, nil +} + +func (f inMemoryFS) Open(fn string) (fs.File, error) { + if _, ok := f.files[fn]; !ok { + return nil, ErrFileNotExist + } + return &inMemoryFile{path: fn, reader: bytes.NewReader(f.files[fn])}, nil +} + +// NewFakeFS returns a new FS that always returns ErrFileNotExist when trying to Open() and empty Files(). +func NewFakeFS() FS { + return NewInMemoryFS(nil) +} + +// inMemoryFile is a fs.File whose content is stored in memory. +type inMemoryFile struct { + path string + reader *bytes.Reader +} + +func (f *inMemoryFile) Stat() (fs.FileInfo, error) { + return inMemoryFileInfo{ + name: f.path, + size: f.reader.Size(), + }, nil +} + +func (f *inMemoryFile) Read(b []byte) (int, error) { + return f.reader.Read(b) +} + +func (f *inMemoryFile) Close() error { + return nil +} + +type inMemoryFileInfo struct { + name string + size int64 +} + +func (f inMemoryFileInfo) Name() string { return f.name } +func (f inMemoryFileInfo) Size() int64 { return f.size } +func (f inMemoryFileInfo) Mode() os.FileMode { return 0444 } // Read for all +func (f inMemoryFileInfo) ModTime() time.Time { return time.Time{} } +func (f inMemoryFileInfo) IsDir() bool { return false } +func (f inMemoryFileInfo) Sys() interface{} { return nil } diff --git a/pkg/services/pluginsintegration/pluginsintegration.go b/pkg/services/pluginsintegration/pluginsintegration.go index c057567102f..64c5daf6478 100644 --- a/pkg/services/pluginsintegration/pluginsintegration.go +++ b/pkg/services/pluginsintegration/pluginsintegration.go @@ -81,7 +81,7 @@ var WireExtensionSet = wire.NewSet( signature.ProvideOSSAuthorizer, wire.Bind(new(plugins.PluginLoaderAuthorizer), new(*signature.UnsignedPluginAuthorizer)), wire.Bind(new(finder.Finder), new(*finder.Local)), - finder.NewLocalFinder, + finder.ProvideLocalFinder, ) func ProvideClientDecorator(