Repository: cli/cli
Pull request #7720 (2023-07-18 10:51:53)

Use filepath.Clean to sanitize path for archive downloads
Fixes https://github.com/github/cli/issues/176


Patch:
diff --git a/api/sanitize_ascii.go b/api/sanitize_ascii.go
index 75cb3062b..d5d253452 100644
--- a/api/sanitize_ascii.go
+++ b/api/sanitize_ascii.go
@@ -1,12 +1,11 @@
 package api
 
 import (
-	"bytes"
 	"io"
 	"net/http"
 	"regexp"
-	"strings"
 
+	"github.com/cli/cli/v2/internal/asciisanitizer"
 	"golang.org/x/text/transform"
 )
 
@@ -14,8 +13,6 @@ var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`)
 
 // GitHub servers do not sanitize their API output for terminal display
 // and leave in unescaped ASCII control characters.
-// C0 control characters are represented in their unicode code point form ranging from \u0000 to \u001F.
-// C1 control characters are represented in two bytes, the first being 0xC2 and the second ranging from 0x80 to 0x9F.
 // These control characters will be interpreted by the terminal, this behaviour can be
 // used maliciously as an attack vector, especially the control characters \u001B and \u009B.
 // This function wraps JSON response bodies in a ReadCloser that transforms C0 and C1
@@ -37,187 +34,7 @@ func sanitizedReadCloser(rc io.ReadCloser) io.ReadCloser {
 		io.Reader
 		io.Closer
 	}{
-		Reader: transform.NewReader(rc, &sanitizer{}),
+		Reader: transform.NewReader(rc, &asciisanitizer.Sanitizer{}),
 		Closer: rc,
 	}
 }
-
-// Sanitizer implements transform.Transformer interface.
-type sanitizer struct {
-	addEscape bool
-}
-
-// Transform uses a sliding window algorithm to detect C0 and C1
-// ASCII control sequences as they are read and replaces them
-// with equivalent inert characters. Characters that are not part
-// of a control sequence are not modified.
-func (t *sanitizer) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) {
-	lSrc := len(src)
-	lDst := len(dst)
-
-	for nSrc < lSrc-6 && nDst < lDst {
-		window := src[nSrc : nSrc+6]
-
-		// Replace C1 Control Characters
-		if repl, found := mapC1ToCaret(window[:2]); found {
-			if len(repl)+nDst > lDst {
-				err = transform.ErrShortDst
-				return
-			}
-			for j := 0; j < len(repl); j++ {
-				dst[nDst] = repl[j]
-				nDst++
-			}
-			nSrc += 2
-			continue
-		}
-
-		// Replace C0 Control Characters
-		if repl, found := mapC0ToCaret(window); found {
-			if t.addEscape {
-				repl = append([]byte{'\\'}, repl...)
-			}
-			if len(repl)+nDst > lDst {
-				err = transform.ErrShortDst
-				return
-			}
-			for j := 0; j < len(repl); j++ {
-				dst[nDst] = repl[j]
-				nDst++
-			}
-			t.addEscape = false
-			nSrc += 6
-			continue
-		}
-
-		if window[0] == '\\' {
-			t.addEscape = !t.addEscape
-		} else {
-			t.addEscape = false
-		}
-
-		dst[nDst] = src[nSrc]
-		nDst++
-		nSrc++
-	}
-
-	if !atEOF {
-		err = transform.ErrShortSrc
-		return
-	}
-
-	remaining := lSrc - nSrc
-	if remaining+nDst > lDst {
-		err = transform.ErrShortDst
-		return
-	}
-
-	for j := 0; j < remaining; j++ {
-		dst[nDst] = src[nSrc]
-		nDst++
-		nSrc++
-	}
-
-	return
-}
-
-func (t *sanitizer) Reset() {
-	t.addEscape = false
-}
-
-// mapC0ToCaret maps C0 control sequences to caret notation.
-func mapC0ToCaret(b []byte) ([]byte, bool) {
-	if len(b) != 6 {
-		return b, false
-	}
-	if !bytes.HasPrefix(b, []byte(`\u00`)) {
-		return b, false
-	}
-	m := map[string]string{
-		`\u0000`: `^@`,
-		`\u0001`: `^A`,
-		`\u0002`: `^B`,
-		`\u0003`: `^C`,
-		`\u0004`: `^D`,
-		`\u0005`: `^E`,
-		`\u0006`: `^F`,
-		`\u0007`: `^G`,
-		`\u0008`: `^H`,
-		`\u0009`: `^I`,
-		`\u000a`: `^J`,
-		`\u000b`: `^K`,
-		`\u000c`: `^L`,
-		`\u000d`: `^M`,
-		`\u000e`: `^N`,
-		`\u000f`: `^O`,
-		`\u0010`: `^P`,
-		`\u0011`: `^Q`,
-		`\u0012`: `^R`,
-		`\u0013`: `^S`,
-		`\u0014`: `^T`,
-		`\u0015`: `^U`,
-		`\u0016`: `^V`,
-		`\u0017`: `^W`,
-		`\u0018`: `^X`,
-		`\u0019`: `^Y`,
-		`\u001a`: `^Z`,
-		`\u001b`: `^[`,
-		`\u001c`: `^\\`,
-		`\u001d`: `^]`,
-		`\u001e`: `^^`,
-		`\u001f`: `^_`,
-	}
-	if c, ok := m[strings.ToLower(string(b))]; ok {
-		return []byte(c), true
-	}
-	return b, false
-}
-
-// mapC1ToCaret maps C1 control sequences to caret notation.
-// C1 control sequences are two bytes long where the first byte is 0xC2.
-func mapC1ToCaret(b []byte) ([]byte, bool) {
-	if len(b) != 2 {
-		return b, false
-	}
-	if b[0] != 0xC2 {
-		return b, false
-	}
-	m := map[byte]string{
-		128: `^@`,
-		129: `^A`,
-		130: `^B`,
-		131: `^C`,
-		132: `^D`,
-		133: `^E`,
-		134: `^F`,
-		135: `^G`,
-		136: `^H`,
-		137: `^I`,
-		138: `^J`,
-		139: `^K`,
-		140: `^L`,
-		141: `^M`,
-		142: `^N`,
-		143: `^O`,
-		144: `^P`,
-		145: `^Q`,
-		146: `^R`,
-		147: `^S`,
-		148: `^T`,
-		149: `^U`,
-		150: `^V`,
-		151: `^W`,
-		152: `^X`,
-		153: `^Y`,
-		154: `^Z`,
-		155: `^[`,
-		156: `^\\`,
-		157: `^]`,
-		158: `^^`,
-		159: `^_`,
-	}
-	if c, ok := m[b[1]]; ok {
-		return []byte(c), true
-	}
-	return b, false
-}
diff --git a/internal/asciisanitizer/sanitizer.go b/internal/asciisanitizer/sanitizer.go
new file mode 100644
index 000000000..8c169f22b
--- /dev/null
+++ b/internal/asciisanitizer/sanitizer.go
@@ -0,0 +1,220 @@
+// Package asciisanitizer implements an ASCII control character sanitizer for GitHub API responses so they can be
+// safely displayed in the terminal. The GitHub API does not sanitize their responses for terminal display and will
+// leave in unescaped ASCII control characters. These ASCII control characters will be interpreted by the terminal,
+// this behaviour can be used maliciously as an attack vector, especially the ASCII control characters \u001B and \u009B.
+package asciisanitizer
+
+import (
+	"bytes"
+	"errors"
+	"strings"
+	"unicode"
+	"unicode/utf8"
+
+	"golang.org/x/text/transform"
+)
+
+// Sanitizer implements transform.Transformer interface.
+type Sanitizer struct {
+	addEscape bool
+}
+
+// Transform uses a sliding window algorithm to detect C0 and C1 control characters as they are read and replaces
+// them with equivalent inert characters. Bytes that are not part of a control character are not modified.
+// C0 control characters can be either encoded or unencoded. C1 control characters are not encoded.
+// Encoded C0 control characters are six byte strings, representing the unicode code point, ranging from \u0000 to \u001F.
+func (t *Sanitizer) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) {
+	transfer := func(write, read []byte) error {
+		readLength := len(read)
+		writeLength := len(write)
+		if writeLength > len(dst) {
+			return transform.ErrShortDst
+		}
+		copy(dst, write)
+		nDst += writeLength
+		dst = dst[writeLength:]
+		nSrc += readLength
+		src = src[readLength:]
+		return nil
+	}
+
+	for len(src) > 0 {
+		// Make sure we always have 6 bytes if there are 6 bytes available.
+		// This is necessary for encoded C0 control characters.
+		if len(src) < 6 && !atEOF {
+			err = transform.ErrShortSrc
+			return
+		}
+		// Only support UTF-8 encoded data.
+		r, size := utf8.DecodeRune(src)
+		if r == utf8.RuneError {
+			if !atEOF {
+				err = transform.ErrShortSrc
+				return
+			} else {
+				err = errors.New("invalid UTF-8 string")
+				return
+			}
+		}
+		// Replace C0 and C1 control characters.
+		if unicode.IsControl(r) {
+			if repl, found := mapControlToCaret(r); found {
+				err = transfer(repl, src[:size])
+				if err != nil {
+					return
+				}
+				continue
+			}
+		}
+		// Replace encoded C0 control characters.
+		if len(src) >= 6 {
+			if repl, found := mapEncodedControlToCaret(src[:6]); found {
+				if t.addEscape {
+					// Add an escape character when necessary to prevent creating
+					// invalid JSON with our replacements.
+					repl = append([]byte{'\\'}, repl...)
+				}
+				err = transfer(repl, src[:6])
+				if err != nil {
+					return
+				}
+				continue
+			}
+		}
+		err = transfer(src[:size], src[:size])
+		if err != nil {
+			return
+		}
+		if r == '\\' {
+			t.addEscape = !t.addEscape
+		} else {
+			t.addEscape = false
+		}
+	}
+	return
+}
+
+// Reset resets the state and allows the Sanitizer to be reused.
+func (t *Sanitizer) Reset() {
+	t.addEscape = false
+}
+
+// mapControlToCaret maps C0 and C1 control characters to their caret notation.
+func mapControlToCaret(r rune) ([]byte, bool) {
+	//\t (09), \n (10), \v (11), \r (13) are valid C0 characters and are not sanitized.
+	m := map[rune]string{
+		0:   `^@`,
+		1:   `^A`,
+		2:   `^B`,
+		3:   `^C`,
+		4:   `^D`,
+		5:   `^E`,
+		6:   `^F`,
+		7:   `^G`,
+		8:   `^H`,
+		12:  `^L`,
+		14:  `^N`,
+		15:  `^O`,
+		16:  `^P`,
+		17:  `^Q`,
+		18:  `^R`,
+		19:  `^S`,
+		20:  `^T`,
+		21:  `^U`,
+		22:  `^V`,
+		23:  `^W`,
+		24:  `^X`,
+		25:  `^Y`,
+		26:  `^Z`,
+		27:  `^[`,
+		28:  `^\\`,
+		29:  `^]`,
+		30:  `^^`,
+		31:  `^_`,
+		128: `^@`,
+		129: `^A`,
+		130: `^B`,
+		131: `^C`,
+		132: `^D`,
+		133: `^E`,
+		134: `^F`,
+		135: `^G`,
+		136: `^H`,
+		137: `^I`,
+		138: `^J`,
+		139: `^K`,
+		140: `^L`,
+		141: `^M`,
+		142: `^N`,
+		143: `^O`,
+		144: `^P`,
+		145: `^Q`,
+		146: `^R`,
+		147: `^S`,
+		148: `^T`,
+		149: `^U`,
+		150: `^V`,
+		151: `^W`,
+		152: `^X`,
+		153: `^Y`,
+		154: `^Z`,
+		155: `^[`,
+		156: `^\\`,
+		157: `^]`,
+		158: `^^`,
+		159: `^_`,
+	}
+	if c, ok := m[r]; ok {
+		return []byte(c), true
+	}
+	return nil, false
+}
+
+// mapEncodedControlToCaret maps encoded C0 control characters to their caret notation.
+// Encoded C0 control characters are six byte strings, representing the unicode code point, ranging from \u0000 to \u001F.
+func mapEncodedControlToCaret(b []byte) ([]byte, bool) {
+	if len(b) != 6 {
+		return nil, false
+	}
+	if !bytes.HasPrefix(b, []byte(`\u00`)) {
+		return nil, false
+	}
+	m := map[string]string{
+		`\u0000`: `^@`,
+		`\u0001`: `^A`,
+		`\u0002`: `^B`,
+		`\u0003`: `^C`,
+		`\u0004`: `^D`,
+		`\u0005`: `^E`,
+		`\u0006`: `^F`,
+		`\u0007`: `^G`,
+		`\u0008`: `^H`,
+		`\u0009`: `^I`,
+		`\u000a`: `^J`,
+		`\u000b`: `^K`,
+		`\u000c`: `^L`,
+		`\u000d`: `^M`,
+		`\u000e`: `^N`,
+		`\u000f`: `^O`,
+		`\u0010`: `^P`,
+		`\u0011`: `^Q`,
+		`\u0012`: `^R`,
+		`\u0013`: `^S`,
+		`\u0014`: `^T`,
+		`\u0015`: `^U`,
+		`\u0016`: `^V`,
+		`\u0017`: `^W`,
+		`\u0018`: `^X`,
+		`\u0019`: `^Y`,
+		`\u001a`: `^Z`,
+		`\u001b`: `^[`,
+		`\u001c`: `^\\`,
+		`\u001d`: `^]`,
+		`\u001e`: `^^`,
+		`\u001f`: `^_`,
+	}
+	if c, ok := m[strings.ToLower(string(b))]; ok {
+		return []byte(c), true
+	}
+	return nil, false
+}
diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go
index 2c7f4a18a..16e95db29 100644
--- a/pkg/cmd/release/download/download.go
+++ b/pkg/cmd/release/download/download.go
@@ -290,7 +290,7 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL, f
 			return fmt.Errorf("unable to parse file name of archive: %w", err)
 		}
 		if serverFileName, ok := params["filename"]; ok {
-			fileName = serverFileName
+			fileName = filepath.Clean(serverFileName)
 		} else {
 			return errors.New("unable to determine file name of archive")
 		}
diff --git a/pkg/cmd/repo/view/http.go b/pkg/cmd/repo/view/http.go
index fe2584a90..da7196fef 100644
--- a/pkg/cmd/repo/view/http.go
+++ b/pkg/cmd/repo/view/http.go
@@ -1,13 +1,17 @@
 package view
 
 import (
+	"bytes"
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 
 	"github.com/cli/cli/v2/api"
+	"github.com/cli/cli/v2/internal/asciisanitizer"
 	"github.com/cli/cli/v2/internal/ghrepo"
+	"golang.org/x/text/transform"
 )
 
 var NotFoundError = errors.New("not found")
@@ -40,9 +44,14 @@ func RepositoryReadme(client *http.Client, repo ghrepo.Interface, branch string)
 		return nil, fmt.Errorf("failed to decode readme: %w", err)
 	}
 
+	sanitized, err := io.ReadAll(transform.NewReader(bytes.NewReader(decoded), &asciisanitizer.Sanitizer{}))
+	if err != nil {
+		return nil, err
+	}
+
 	return &RepoReadme{
 		Filename: response.Name,
-		Content:  string(decoded),
+		Content:  string(sanitized),
 		BaseURL:  response.HTMLURL,
 	}, nil
 }
diff --git a/pkg/cmd/workflow/shared/shared.go b/pkg/cmd/workflow/shared/shared.go
index efd21529e..11b0bd03e 100644
--- a/pkg/cmd/workflow/shared/shared.go
+++ b/pkg/cmd/workflow/shared/shared.go
@@ -1,9 +1,11 @@
 package shared
 
 import (
+	"bytes"
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"io"
 	"net/url"
 	"path"
 	"strconv"
@@ -11,9 +13,11 @@ import (
 
 	"github.com/AlecAivazis/survey/v2"
 	"github.com/cli/cli/v2/api"
+	"github.com/cli/cli/v2/internal/asciisanitizer"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/iostreams"
 	"github.com/cli/cli/v2/pkg/prompt"
+	"golang.org/x/text/transform"
 )
 
 const (
@@ -243,5 +247,10 @@ func GetWorkflowContent(client *api.Client, repo ghrepo.Interface, workflow Work
 		return nil, fmt.Errorf("failed to decode workflow file: %w", err)
 	}
 
-	return decoded, nil
+	sanitized, err := io.ReadAll(transform.NewReader(bytes.NewReader(decoded), &asciisanitizer.Sanitizer{}))
+	if err != nil {
+		return nil, err
+	}
+
+	return sanitized, nil
 }


Test patch:
diff --git a/internal/asciisanitizer/sanitizer_test.go b/internal/asciisanitizer/sanitizer_test.go
new file mode 100644
index 000000000..80b251c5f
--- /dev/null
+++ b/internal/asciisanitizer/sanitizer_test.go
@@ -0,0 +1,87 @@
+package asciisanitizer
+
+import (
+	"bytes"
+	"testing"
+	"testing/iotest"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/text/transform"
+)
+
+func TestSanitizerTransform(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{
+			name:  "No control characters",
+			input: "The quick brown fox jumped over the lazy dog",
+			want:  "The quick brown fox jumped over the lazy dog",
+		},
+		{
+			name:  "Encoded C0 control character",
+			input: `1\u0001`,
+			want:  "1^A",
+		},
+		{
+			name: "Encoded C0 control characters",
+			input: `1\u0001 2\u0002 3\u0003 4\u0004 5\u0005 6\u0006 7\u0007 8\u0008 9\t ` +
+				`A\r\n B\u000b C\u000c D\r\n E\u000e F\u000f ` +
+				`10\u0010 11\u0011 12\u0012 13\u0013 14\u0014 15\u0015 16\u0016 17\u0017 18\u0018 19\u0019 ` +
+				`1A\u001a 1B\u001b 1C\u001c 1D\u001d 1E\u001e 1F\u001f ` +
+				`\\u00\u001b ` +
+				`\u001B \\u001B \\\u001B \\\\u001B `,
+			want: `1^A 2^B 3^C 4^D 5^E 6^F 7^G 8^H 9\t ` +
+				`A\r\n B^K C^L D\r\n E^N F^O ` +
+				`10^P 11^Q 12^R 13^S 14^T 15^U 16^V 17^W 18^X 19^Y ` +
+				`1A^Z 1B^[ 1C^\\ 1D^] 1E^^ 1F^_ ` +
+				`\\u00^[ ` +
+				`^[ \\^[ \\^[ \\\\^[ `,
+		},
+		{
+			name:  "C0 control character",
+			input: "1\x01",
+			want:  "1^A",
+		},
+		{
+			name: "C0 control characters",
+			input: "0\x00 1\x01 2\x02 3\x03 4\x04 5\x05 6\x06 7\x07 8\x08 9\x09 " +
+				"A\x0A B\x0B C\x0C D\x0D E\x0E F\x0F " +
+				"10\x10 11\x11 12\x12 13\x13 14\x14 15\x15 16\x16 17\x17 18\x18 19\x19 " +
+				"1A\x1A 1B\x1B 1C\x1C 1D\x1D 1E\x1E 1F\x1F ",
+			want: "0^@ 1^A 2^B 3^C 4^D 5^E 6^F 7^G 8^H 9\t " +
+				"A\n B\v C^L D\r E^N F^O " +
+				"10^P 11^Q 12^R 13^S 14^T 15^U 16^V 17^W 18^X 19^Y " +
+				"1A^Z 1B^[ 1C^\\\\ 1D^] 1E^^ 1F^_ ",
+		},
+		{
+			name:  "C1 control character",
+			input: "80\xC2\x80",
+			want:  "80^@",
+		},
+		{
+			name: "C1 control characters",
+			input: "80\xC2\x80 81\xC2\x81 82\xC2\x82 83\xC2\x83 84\xC2\x84 85\xC2\x85 86\xC2\x86 87\xC2\x87 88\xC2\x88 89\xC2\x89 " +
+				"8A\xC2\x8A 8B\xC2\x8B 8C\xC2\x8C 8D\xC2\x8D 8E\xC2\x8E 8F\xC2\x8F " +
+				"90\xC2\x90 91\xC2\x91 92\xC2\x92 93\xC2\x93 94\xC2\x94 95\xC2\x95 96\xC2\x96 97\xC2\x97 98\xC2\x98 99\xC2\x99 " +
+				"9A\xC2\x9A 9B\xC2\x9B 9C\xC2\x9C 9D\xC2\x9D 9E\xC2\x9E 9F\xC2\x9F " +
+				"\xC2\xA1 ",
+			want: "80^@ 81^A 82^B 83^C 84^D 85^E 86^F 87^G 88^H 89^I " +
+				"8A^J 8B^K 8C^L 8D^M 8E^N 8F^O " +
+				"90^P 91^Q 92^R 93^S 94^T 95^U 96^V 97^W 98^X 99^Y " +
+				"9A^Z 9B^[ 9C^\\\\ 9D^] 9E^^ 9F^_ " +
+				"¡ ",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			sanitizer := &Sanitizer{}
+			reader := bytes.NewReader([]byte(tt.input))
+			transformReader := transform.NewReader(reader, sanitizer)
+			err := iotest.TestReader(transformReader, []byte(tt.want))
+			require.NoError(t, err)
+		})
+	}
+}