From 9f159c5e3d6ad17fdb6ffc2e172acb1226386a23 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Thu, 13 Aug 2020 09:38:05 +0200 Subject: [PATCH] grafana-cli: Fix file path processing, returning of errors (#26954) * grafana-cli: Fix file path processing, returning of errors --- .../grafana-cli/commands/install_command.go | 69 +++++++++-------- .../commands/install_command_test.go | 77 ++++++++----------- pkg/cmd/grafana-cli/services/api_client.go | 8 +- pkg/cmd/grafana-cli/services/services.go | 14 ++-- pkg/cmd/grafana-cli/utils/grafana_path.go | 3 +- 5 files changed, 78 insertions(+), 93 deletions(-) diff --git a/pkg/cmd/grafana-cli/commands/install_command.go b/pkg/cmd/grafana-cli/commands/install_command.go index 8d9ba342c37..cb37108b938 100644 --- a/pkg/cmd/grafana-cli/commands/install_command.go +++ b/pkg/cmd/grafana-cli/commands/install_command.go @@ -8,7 +8,6 @@ import ( "io" "io/ioutil" "os" - "path" "path/filepath" "regexp" "runtime" @@ -205,9 +204,10 @@ func SelectVersion(plugin *models.Plugin, version string) (*models.Version, erro return &ver, nil } -func RemoveGitBuildFromName(pluginName, filename string) string { - r := regexp.MustCompile("^[a-zA-Z0-9_.-]*/") - return r.ReplaceAllString(filename, pluginName+"/") +var reGitBuild = regexp.MustCompile("^[a-zA-Z0-9_.-]*/") + +func removeGitBuildFromName(pluginName, filename string) string { + return reGitBuild.ReplaceAllString(filename, pluginName+"/") } const permissionsDeniedMessage = "could not create %q, permission denied, make sure you have write access to plugin dir" @@ -220,41 +220,44 @@ func extractFiles(archiveFile string, pluginName string, filePath string, allowS return err } for _, zf := range r.File { - newFileName := RemoveGitBuildFromName(pluginName, zf.Name) + newFileName := removeGitBuildFromName(pluginName, zf.Name) if !isPathSafe(newFileName, filepath.Join(filePath, pluginName)) { return fmt.Errorf("filepath: %q tries to write outside of plugin directory: %q, this can be a security risk", zf.Name, filepath.Join(filePath, pluginName)) } - newFile := path.Join(filePath, newFileName) + newFile := filepath.Join(filePath, newFileName) if zf.FileInfo().IsDir() { - err := os.Mkdir(newFile, 0755) - if os.IsPermission(err) { - return fmt.Errorf(permissionsDeniedMessage, newFile) - } - } else { - // Create needed directories to extract file - err := os.MkdirAll(filepath.Dir(newFile), 0755) - if err != nil { - return errutil.Wrap("failed to create directory to extract plugin files", err) + if err := os.MkdirAll(newFile, 0755); err != nil { + if os.IsPermission(err) { + return fmt.Errorf(permissionsDeniedMessage, newFile) + } + + return err } - if isSymlink(zf) { - if !allowSymlinks { - logger.Errorf("%v: plugin archive contains symlink which is not allowed. Skipping \n", zf.Name) - continue - } - err = extractSymlink(zf, newFile) - if err != nil { - logger.Errorf("Failed to extract symlink: %v \n", err) - continue - } - } else { - err = extractFile(zf, newFile) - if err != nil { - return errutil.Wrap("failed to extract file", err) - } + continue + } + + // Create needed directories to extract file + if err := os.MkdirAll(filepath.Dir(newFile), 0755); err != nil { + return errutil.Wrap("failed to create directory to extract plugin files", err) + } + + if isSymlink(zf) { + if !allowSymlinks { + logger.Warnf("%v: plugin archive contains a symlink, which is not allowed. Skipping \n", zf.Name) + continue } + if err := extractSymlink(zf, newFile); err != nil { + logger.Errorf("Failed to extract symlink: %v \n", err) + continue + } + continue + } + + if err := extractFile(zf, newFile); err != nil { + return errutil.Wrap("failed to extract file", err) } } @@ -272,12 +275,10 @@ func extractSymlink(file *zip.File, filePath string) error { return errutil.Wrap("failed to extract file", err) } buf := new(bytes.Buffer) - _, err = io.Copy(buf, src) - if err != nil { + if _, err := io.Copy(buf, src); err != nil { return errutil.Wrap("failed to copy symlink contents", err) } - err = os.Symlink(strings.TrimSpace(buf.String()), filePath) - if err != nil { + if err := os.Symlink(strings.TrimSpace(buf.String()), filePath); err != nil { return errutil.Wrapf(err, "failed to make symbolic link for %v", filePath) } return nil diff --git a/pkg/cmd/grafana-cli/commands/install_command_test.go b/pkg/cmd/grafana-cli/commands/install_command_test.go index e65b7c48205..b4dc637c557 100644 --- a/pkg/cmd/grafana-cli/commands/install_command_test.go +++ b/pkg/cmd/grafana-cli/commands/install_command_test.go @@ -4,46 +4,30 @@ import ( "fmt" "io" "os" + "path/filepath" "runtime" "testing" "github.com/grafana/grafana/pkg/cmd/grafana-cli/commands/commandstest" "github.com/grafana/grafana/pkg/cmd/grafana-cli/models" - . "github.com/smartystreets/goconvey/convey" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestFolderNameReplacement(t *testing.T) { - Convey("path containing git commit path", t, func() { - pluginName := "datasource-plugin-kairosdb" +func TestRemoveGitBuildFromName(t *testing.T) { + pluginName := "datasource-kairosdb" - paths := map[string]string{ - "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/": "datasource-plugin-kairosdb/", - "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/README.md": "datasource-plugin-kairosdb/README.md", - "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/": "datasource-plugin-kairosdb/partials/", - "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/config.html": "datasource-plugin-kairosdb/partials/config.html", - } - - Convey("should be replaced with plugin name", func() { - for k, v := range paths { - So(RemoveGitBuildFromName(pluginName, k), ShouldEqual, v) - } - }) - }) - - Convey("path containing git commit path", t, func() { - pluginName := "app-example" - paths := map[string]string{ - "app-plugin-example-3c28f65ac6fb7f1e234b0364b97081d836495439/": "app-example/", - } - - Convey("should be replaced with plugin name", func() { - for k, v := range paths { - So(RemoveGitBuildFromName(pluginName, k), ShouldEqual, v) - } - }) - }) + // The root directory should get renamed to the plugin name + paths := map[string]string{ + "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/": "datasource-kairosdb/", + "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/README.md": "datasource-kairosdb/README.md", + "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/": "datasource-kairosdb/partials/", + "datasource-plugin-kairosdb-cc4a3965ef5d3eb1ae0ee4f93e9e78ec7db69e64/partials/config.html": "datasource-kairosdb/partials/config.html", + } + for pth, exp := range paths { + name := removeGitBuildFromName(pluginName, pth) + assert.Equal(t, exp, name) + } } func TestExtractFiles(t *testing.T) { @@ -52,28 +36,29 @@ func TestExtractFiles(t *testing.T) { pluginDir, del := setupFakePluginsDir(t) defer del() - archive := "testdata/grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip" + archive := filepath.Join("testdata", "grafana-simple-json-datasource-ec18fa4da8096a952608a7e4c7782b4260b41bcf.zip") err := extractFiles(archive, "grafana-simple-json-datasource", pluginDir, false) - assert.Nil(t, err) + require.NoError(t, err) //File in zip has permissions 755 - fileInfo, err := os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_darwin_amd64") - assert.Nil(t, err) + fileInfo, err := os.Stat(filepath.Join(pluginDir, "grafana-simple-json-datasource", + "simple-plugin_darwin_amd64")) + require.NoError(t, err) assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) //File in zip has permission 755 fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_linux_amd64") - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) //File in zip has permission 644 fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/simple-plugin_windows_amd64.exe") - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "-rw-r--r--", fileInfo.Mode().String()) //File in zip has permission 755 fileInfo, err = os.Stat(pluginDir + "/grafana-simple-json-datasource/non-plugin-binary") - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "-rwxr-xr-x", fileInfo.Mode().String()) }) @@ -82,10 +67,10 @@ func TestExtractFiles(t *testing.T) { defer del() err := extractFiles("testdata/plugin-with-symlink.zip", "plugin-with-symlink", pluginDir, false) - assert.Nil(t, err) + require.NoError(t, err) _, err = os.Stat(pluginDir + "/plugin-with-symlink/text.txt") - assert.Nil(t, err) + require.NoError(t, err) _, err = os.Stat(pluginDir + "/plugin-with-symlink/symlink_to_txt") assert.NotNil(t, err) }) @@ -96,10 +81,10 @@ func TestExtractFiles(t *testing.T) { defer del() err := extractFiles("testdata/plugin-with-symlink.zip", "plugin-with-symlink", pluginDir, true) - assert.Nil(t, err) + require.NoError(t, err) _, err = os.Stat(pluginDir + "/plugin-with-symlink/symlink_to_txt") - assert.Nil(t, err) + require.NoError(t, err) fmt.Println(err) }) } @@ -199,7 +184,7 @@ func TestSelectVersion(t *testing.T) { ), "", ) - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "1.0.0", ver.Version) }) @@ -208,7 +193,7 @@ func TestSelectVersion(t *testing.T) { makePluginWithVersions(versionArg{Version: "2.0.0"}, versionArg{Version: "1.0.0"}), "", ) - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "2.0.0", ver.Version) }) @@ -220,7 +205,7 @@ func TestSelectVersion(t *testing.T) { ), "1.0.0", ) - assert.Nil(t, err) + require.NoError(t, err) assert.Equal(t, "1.0.0", ver.Version) }) } @@ -234,8 +219,8 @@ func setupFakePluginsDir(t *testing.T) (string, func()) { require.Nil(t, err) return dirname, func() { - err = os.RemoveAll(dirname) - assert.Nil(t, err) + err := os.RemoveAll(dirname) + require.NoError(t, err) } } diff --git a/pkg/cmd/grafana-cli/services/api_client.go b/pkg/cmd/grafana-cli/services/api_client.go index 24fab854852..9a7eecbe25b 100644 --- a/pkg/cmd/grafana-cli/services/api_client.go +++ b/pkg/cmd/grafana-cli/services/api_client.go @@ -146,7 +146,11 @@ func sendRequest(client http.Client, repoUrl string, subPaths ...string) (io.Rea } func createRequest(repoUrl string, subPaths ...string) (*http.Request, error) { - u, _ := url.Parse(repoUrl) + u, err := url.Parse(repoUrl) + if err != nil { + return nil, err + } + for _, v := range subPaths { u.Path = path.Join(u.Path, v) } @@ -170,7 +174,7 @@ func handleResponse(res *http.Response) (io.ReadCloser, error) { } if res.StatusCode/100 != 2 && res.StatusCode/100 != 4 { - return nil, fmt.Errorf("Api returned invalid status: %s", res.Status) + return nil, fmt.Errorf("API returned invalid status: %s", res.Status) } if res.StatusCode/100 == 4 { diff --git a/pkg/cmd/grafana-cli/services/services.go b/pkg/cmd/grafana-cli/services/services.go index f6f804cd303..8b2fed2397b 100644 --- a/pkg/cmd/grafana-cli/services/services.go +++ b/pkg/cmd/grafana-cli/services/services.go @@ -7,7 +7,7 @@ import ( "fmt" "net" "net/http" - "path" + "path/filepath" "time" "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" @@ -64,16 +64,12 @@ func makeHttpClient(skipTLSVerify bool, timeout time.Duration) http.Client { } func ReadPlugin(pluginDir, pluginName string) (models.InstalledPlugin, error) { - distPluginDataPath := path.Join(pluginDir, pluginName, "dist", "plugin.json") - - var data []byte - var err error - data, err = IoHelper.ReadFile(distPluginDataPath) + distPluginDataPath := filepath.Join(pluginDir, pluginName, "dist", "plugin.json") + data, err := IoHelper.ReadFile(distPluginDataPath) if err != nil { - pluginDataPath := path.Join(pluginDir, pluginName, "plugin.json") + pluginDataPath := filepath.Join(pluginDir, pluginName, "plugin.json") data, err = IoHelper.ReadFile(pluginDataPath) - if err != nil { return models.InstalledPlugin{}, errors.New("Could not find dist/plugin.json or plugin.json on " + pluginName + " in " + pluginDir) } @@ -110,7 +106,7 @@ func GetLocalPlugins(pluginDir string) []models.InstalledPlugin { func RemoveInstalledPlugin(pluginPath, pluginName string) error { logger.Infof("Removing plugin: %v\n", pluginName) - pluginDir := path.Join(pluginPath, pluginName) + pluginDir := filepath.Join(pluginPath, pluginName) _, err := IoHelper.Stat(pluginDir) if err != nil { diff --git a/pkg/cmd/grafana-cli/utils/grafana_path.go b/pkg/cmd/grafana-cli/utils/grafana_path.go index 264405b2883..6786f1bf7fa 100644 --- a/pkg/cmd/grafana-cli/utils/grafana_path.go +++ b/pkg/cmd/grafana-cli/utils/grafana_path.go @@ -2,7 +2,6 @@ package utils import ( "os" - "path" "path/filepath" "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" @@ -26,7 +25,7 @@ func getGrafanaRoot() (string, error) { return "", errutil.Wrap("failed to get executable path", err) } exPath := filepath.Dir(ex) - _, last := path.Split(exPath) + _, last := filepath.Split(exPath) if last == "bin" { // In dev env the executable for current platform is created in 'bin/' dir return filepath.Join(exPath, ".."), nil