diff --git a/pkg/cmd/grafana-cli/commands/install_command.go b/pkg/cmd/grafana-cli/commands/install_command.go index f2ba500a953..0301b29ede3 100644 --- a/pkg/cmd/grafana-cli/commands/install_command.go +++ b/pkg/cmd/grafana-cli/commands/install_command.go @@ -199,14 +199,14 @@ func extractFiles(body []byte, pluginName string, filePath string, allowSymlinks } for _, zf := range r.File { newFileName := RemoveGitBuildFromName(pluginName, zf.Name) - if !isPathSafe(newFileName, path.Join(filePath, pluginName)) { + if !isPathSafe(newFileName, filepath.Join(filePath, pluginName)) { return xerrors.Errorf("filepath: %v tries to write outside of plugin directory: %v. This can be a security risk.", zf.Name, path.Join(filePath, pluginName)) } newFile := path.Join(filePath, newFileName) if zf.FileInfo().IsDir() { err := os.Mkdir(newFile, 0755) - if permissionsError(err) { + if os.IsPermission(err) { return fmt.Errorf(permissionsDeniedMessage, newFile) } } else { @@ -234,10 +234,6 @@ func extractFiles(body []byte, pluginName string, filePath string, allowSymlinks return nil } -func permissionsError(err error) bool { - return err != nil && strings.Contains(err.Error(), "permission denied") -} - func isSymlink(file *zip.File) bool { return file.Mode()&os.ModeSymlink == os.ModeSymlink } @@ -269,7 +265,7 @@ func extractFile(file *zip.File, filePath string) (err error) { dst, err := os.OpenFile(filePath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fileMode) if err != nil { - if permissionsError(err) { + if os.IsPermission(err) { return xerrors.Errorf(permissionsDeniedMessage, filePath) } return errutil.Wrap("Failed to open file", err) diff --git a/pkg/cmd/grafana-cli/commands/install_command_test.go b/pkg/cmd/grafana-cli/commands/install_command_test.go index 5babecb8fe7..f9b45ed1d2d 100644 --- a/pkg/cmd/grafana-cli/commands/install_command_test.go +++ b/pkg/cmd/grafana-cli/commands/install_command_test.go @@ -48,6 +48,7 @@ func TestFoldernameReplacement(t *testing.T) { func TestExtractFiles(t *testing.T) { t.Run("Should preserve file permissions for plugin backend binaries for linux and darwin", func(t *testing.T) { + skipWindows(t) pluginDir, del := setupFakePluginsDir(t) defer del() @@ -95,6 +96,7 @@ func TestExtractFiles(t *testing.T) { }) t.Run("Should extract symlinks if allowed", func(t *testing.T) { + skipWindows(t) pluginDir, del := setupFakePluginsDir(t) defer del() @@ -119,16 +121,18 @@ func TestInstallPluginCommand(t *testing.T) { } func TestIsPathSafe(t *testing.T) { + dest := fmt.Sprintf("%stest%spath", string(os.PathSeparator), string(os.PathSeparator)) + t.Run("Should be true on nested destinations", func(t *testing.T) { - assert.True(t, isPathSafe("dest", "/test/path")) - assert.True(t, isPathSafe("dest/one", "/test/path")) - assert.True(t, isPathSafe("../path/dest/one", "/test/path")) + assert.True(t, isPathSafe("dest", dest)) + assert.True(t, isPathSafe("dest/one", dest)) + assert.True(t, isPathSafe("../path/dest/one", dest)) }) t.Run("Should be false on destinations outside of path", func(t *testing.T) { - assert.False(t, isPathSafe("../dest", "/test/path")) - assert.False(t, isPathSafe("../../", "/test/path")) - assert.False(t, isPathSafe("../../test", "/test/path")) + assert.False(t, isPathSafe("../dest", dest)) + assert.False(t, isPathSafe("../../", dest)) + assert.False(t, isPathSafe("../../test", dest)) }) } @@ -189,3 +193,9 @@ func setupFakePluginsDir(t *testing.T) (string, func()) { assert.Nil(t, err) } } + +func skipWindows(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows") + } +}