Repository: cli/cli
Pull request #3042 (2021-02-26 04:51:13)

Remove functionality to add, view and edit binary files in gists
This commit adds file validation to check if file contents are binary in nature, and if so halts creation, edition and rendering of contents directly to stdout.

Closes https://github.com/cli/cli/issues/2998

g14a: @mislav I've updated the PR and standardized some options. Please check at your leisure. Hope the changes I've made are for the better and not the worse.
g14a: Anything else to be done here? @mislav 
g14a: Thank you for the feedback @mislav it helped a lot. I've updated the PR.

Patch:
diff --git a/go.mod b/go.mod
index 2ddd3697b..c2cc17338 100644
--- a/go.mod
+++ b/go.mod
@@ -11,6 +11,7 @@ require (
 	github.com/cli/safeexec v1.0.0
 	github.com/cpuguy83/go-md2man/v2 v2.0.0
 	github.com/enescakir/emoji v1.0.0
+	github.com/gabriel-vasile/mimetype v1.1.2
 	github.com/google/go-cmp v0.5.2
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
 	github.com/hashicorp/go-version v1.2.1
diff --git a/go.sum b/go.sum
index dfbd5af86..ad31293d6 100644
--- a/go.sum
+++ b/go.sum
@@ -74,6 +74,8 @@ github.com/enescakir/emoji v1.0.0/go.mod h1:Bt1EKuLnKDTYpLALApstIkAjdDrS/8IAgTkK
 github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys=
 github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
 github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
+github.com/gabriel-vasile/mimetype v1.1.2 h1:gaPnPcNor5aZSVCJVSGipcpbgMWiAAj9z182ocSGbHU=
+github.com/gabriel-vasile/mimetype v1.1.2/go.mod h1:6CDPel/o/3/s4+bp6kIbsWATq8pmgOisOPG40CJa6To=
 github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
 github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU=
 github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go
index fe6fac748..5ba3d56c7 100644
--- a/pkg/cmd/gist/create/create.go
+++ b/pkg/cmd/gist/create/create.go
@@ -30,8 +30,7 @@ type CreateOptions struct {
 	Public           bool
 	Filenames        []string
 	FilenameOverride string
-
-	WebMode bool
+	WebMode          bool
 
 	HttpClient func() (*http.Client, error)
 }
@@ -164,6 +163,7 @@ func processFiles(stdin io.ReadCloser, filenameOverride string, filenames []stri
 		var filename string
 		var content []byte
 		var err error
+
 		if f == "-" {
 			if filenameOverride != "" {
 				filename = filenameOverride
@@ -175,11 +175,24 @@ func processFiles(stdin io.ReadCloser, filenameOverride string, filenames []stri
 				return fs, fmt.Errorf("failed to read from stdin: %w", err)
 			}
 			stdin.Close()
+
+			if shared.IsBinaryContents(content) {
+				return nil, fmt.Errorf("binary file contents not supported")
+			}
 		} else {
+			isBinary, err := shared.IsBinaryFile(f)
+			if err != nil {
+				return fs, fmt.Errorf("failed to read file %s: %w", f, err)
+			}
+			if isBinary {
+				return nil, fmt.Errorf("failed to upload %s: binary file not supported", f)
+			}
+
 			content, err = ioutil.ReadFile(f)
 			if err != nil {
 				return fs, fmt.Errorf("failed to read file %s: %w", f, err)
 			}
+
 			filename = path.Base(f)
 		}
 
diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go
index 27df4a104..1310f1482 100644
--- a/pkg/cmd/gist/edit/edit.go
+++ b/pkg/cmd/gist/edit/edit.go
@@ -144,22 +144,25 @@ func editRun(opts *EditOptions) error {
 			}
 		}
 
-		if _, ok := gist.Files[filename]; !ok {
+		gistFile, found := gist.Files[filename]
+		if !found {
 			return fmt.Errorf("gist has no file %q", filename)
 		}
+		if shared.IsBinaryContents([]byte(gistFile.Content)) {
+			return fmt.Errorf("editing binary files not supported")
+		}
 
 		editorCommand, err := cmdutil.DetermineEditor(opts.Config)
 		if err != nil {
 			return err
 		}
-		text, err := opts.Edit(editorCommand, filename, gist.Files[filename].Content, opts.IO)
+		text, err := opts.Edit(editorCommand, filename, gistFile.Content, opts.IO)
 
 		if err != nil {
 			return err
 		}
 
-		if text != gist.Files[filename].Content {
-			gistFile := gist.Files[filename]
+		if text != gistFile.Content {
 			gistFile.Content = text // so it appears if they re-edit
 			filesToUpdate[filename] = text
 		}
@@ -242,6 +245,14 @@ func updateGist(apiClient *api.Client, hostname string, gist *shared.Gist) error
 }
 
 func getFilesToAdd(file string) (map[string]*shared.GistFile, error) {
+	isBinary, err := shared.IsBinaryFile(file)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read file %s: %w", file, err)
+	}
+	if isBinary {
+		return nil, fmt.Errorf("failed to upload %s: binary file not supported", file)
+	}
+
 	content, err := ioutil.ReadFile(file)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read file %s: %w", file, err)
diff --git a/pkg/cmd/gist/shared/shared.go b/pkg/cmd/gist/shared/shared.go
index 56bb3bb20..98d66a9f8 100644
--- a/pkg/cmd/gist/shared/shared.go
+++ b/pkg/cmd/gist/shared/shared.go
@@ -9,11 +9,11 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cli/cli/api"
 	"github.com/cli/cli/internal/ghinstance"
+	"github.com/gabriel-vasile/mimetype"
 	"github.com/shurcooL/githubv4"
 	"github.com/shurcooL/graphql"
-
-	"github.com/cli/cli/api"
 )
 
 type GistFile struct {
@@ -147,3 +147,30 @@ pagination:
 
 	return gists, nil
 }
+
+func IsBinaryFile(file string) (bool, error) {
+	detectedMime, err := mimetype.DetectFile(file)
+	if err != nil {
+		return false, err
+	}
+
+	isBinary := true
+	for mime := detectedMime; mime != nil; mime = mime.Parent() {
+		if mime.Is("text/plain") {
+			isBinary = false
+			break
+		}
+	}
+	return isBinary, nil
+}
+
+func IsBinaryContents(contents []byte) bool {
+	isBinary := true
+	for mime := mimetype.Detect(contents); mime != nil; mime = mime.Parent() {
+		if mime.Is("text/plain") {
+			isBinary = false
+			break
+		}
+	}
+	return isBinary
+}
diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go
index 176d565ef..d33e60747 100644
--- a/pkg/cmd/gist/view/view.go
+++ b/pkg/cmd/gist/view/view.go
@@ -118,6 +118,14 @@ func viewRun(opts *ViewOptions) error {
 	defer opts.IO.StopPager()
 
 	render := func(gf *shared.GistFile) error {
+		if shared.IsBinaryContents([]byte(gf.Content)) {
+			if len(gist.Files) == 1 || opts.Filename != "" {
+				return fmt.Errorf("error: file is binary")
+			}
+			_, err = fmt.Fprintln(opts.IO.Out, cs.Gray("(skipping rendering binary content)"))
+			return nil
+		}
+
 		if strings.Contains(gf.Type, "markdown") && !opts.Raw {
 			rendered, err := markdown.Render(gf.Content, markdownStyle, "")
 			if err != nil {
@@ -134,6 +142,7 @@ func viewRun(opts *ViewOptions) error {
 			_, err := fmt.Fprint(opts.IO.Out, "\n")
 			return err
 		}
+
 		return nil
 	}
 
@@ -154,7 +163,10 @@ func viewRun(opts *ViewOptions) error {
 	for fn := range gist.Files {
 		filenames = append(filenames, fn)
 	}
-	sort.Strings(filenames)
+
+	sort.Slice(filenames, func(i, j int) bool {
+		return strings.ToLower(filenames[i]) < strings.ToLower(filenames[j])
+	})
 
 	if opts.ListFiles {
 		for _, fn := range filenames {


Test patch:
diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go
index 73a53e523..71250d5e5 100644
--- a/pkg/cmd/gist/edit/edit_test.go
+++ b/pkg/cmd/gist/edit/edit_test.go
@@ -301,7 +301,6 @@ func Test_editRun(t *testing.T) {
 		io.SetStdoutTTY(!tt.nontty)
 		io.SetStdinTTY(!tt.nontty)
 		tt.opts.IO = io
-
 		tt.opts.Selector = "1234"
 
 		tt.opts.Config = func() (config.Config, error) {
diff --git a/pkg/cmd/gist/shared/shared_test.go b/pkg/cmd/gist/shared/shared_test.go
index 0f80db690..c55cbea67 100644
--- a/pkg/cmd/gist/shared/shared_test.go
+++ b/pkg/cmd/gist/shared/shared_test.go
@@ -50,3 +50,38 @@ func Test_GetGistIDFromURL(t *testing.T) {
 		})
 	}
 }
+
+func TestIsBinaryContents(t *testing.T) {
+	tests := []struct {
+		fileContent []byte
+		want        bool
+	}{
+		{
+			want:        false,
+			fileContent: []byte("package main"),
+		},
+		{
+			want:        false,
+			fileContent: []byte(""),
+		},
+		{
+			want:        false,
+			fileContent: []byte(nil),
+		},
+		{
+			want: true,
+			fileContent: []byte{239, 191, 189, 239, 191, 189, 239, 191, 189, 239,
+				191, 189, 239, 191, 189, 16, 74, 70, 73, 70, 239, 191, 189, 1, 1, 1,
+				1, 44, 1, 44, 239, 191, 189, 239, 191, 189, 239, 191, 189, 239, 191,
+				189, 239, 191, 189, 67, 239, 191, 189, 8, 6, 6, 7, 6, 5, 8, 7, 7, 7,
+				9, 9, 8, 10, 12, 20, 10, 12, 11, 11, 12, 25, 18, 19, 15, 20, 29, 26,
+				31, 30, 29, 26, 28, 28, 32, 36, 46, 39, 32, 34, 44, 35, 28, 28, 40,
+				55, 41, 44, 48, 49, 52, 52, 52, 31, 39, 57, 61, 56, 50, 60, 46, 51,
+				52, 50, 239, 191, 189, 239, 191, 189, 239, 191, 189, 67, 1, 9, 9, 9, 12},
+		},
+	}
+
+	for _, tt := range tests {
+		assert.Equal(t, tt.want, IsBinaryContents(tt.fileContent))
+	}
+}