fix: get valid UTF-8 for diff_hunk and composer readme (#39932)

* Fixes #39929
* Fixes #39994

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/charset/charset.go

@@ -52,7 +52,7 @@ func Lookup(label string) (e encoding.Encoding, name string) {
 
 type ConvertOpts struct {
 	KeepBOM           bool
-	ErrorReplacement  []byte
+	ErrorReplacement  []byte // leave it empty to remove the invalid bytes
 	ErrorReturnOrigin bool
 }
 
@@ -99,7 +99,12 @@ func ToUTF8DropErrors(content []byte) []byte {
 func ToUTF8(content []byte, opts ConvertOpts) []byte {
 	charsetLabel, _ := DetectEncoding(content)
 	if charsetLabel == "UTF-8" {
-		return maybeRemoveBOM(content, opts)
+		// even if the detected charset is UTF-8, the content may contain invalid UTF-8 sequences, so need to convert it to valid UTF-8
+		content = maybeRemoveBOM(content, opts)
+		if opts.ErrorReturnOrigin {
+			return content
+		}
+		return bytes.ToValidUTF8(content, opts.ErrorReplacement)
 	}
 
 	encoding, _ := Lookup(charsetLabel)
@@ -123,11 +128,7 @@ func ToUTF8(content []byte, opts ConvertOpts) []byte {
 		if opts.ErrorReturnOrigin {
 			return content
 		}
-		if opts.ErrorReplacement == nil {
-			decoded = append(decoded, content[idx+n])
-		} else {
-			decoded = append(decoded, opts.ErrorReplacement...)
-		}
+		decoded = append(decoded, opts.ErrorReplacement...)
 		idx += n + 1
 	}
 	return maybeRemoveBOM(decoded, opts)

modules/charset/charset_test.go

@@ -171,6 +171,8 @@ func TestToUTF8DropErrors(t *testing.T) {
 
 	res = ToUTF8DropErrors([]byte{0x00, 0x00, 0x00, 0x00})
 	assert.Equal(t, []byte{0x00, 0x00, 0x00, 0x00}, res)
+
+	assert.Equal(t, "caf ", string(ToUTF8DropErrors([]byte("caf\xe9"))))
 }
 
 func TestDetectEncoding(t *testing.T) {

modules/json/json.go

@@ -23,7 +23,7 @@ type Decoder interface {
 // Interface represents an interface to handle json data
 type Interface interface {
 	Marshal(v any) ([]byte, error)
-	MarshalWrite(w io.Writer, v any) error
+	MarshalWriteRelaxed(w io.Writer, v any) error
 	Unmarshal(data []byte, v any) error
 	NewEncoder(writer io.Writer) Encoder
 	NewDecoder(reader io.Reader) Decoder
@@ -37,9 +37,10 @@ func Marshal(v any) ([]byte, error) {
 	return DefaultJSONHandler.Marshal(v)
 }
 
-// MarshalWrite writes the JSON encoding of v to the given writer
-func MarshalWrite(w io.Writer, v any) error {
-	return DefaultJSONHandler.MarshalWrite(w, v)
+// MarshalWriteRelaxed writes the JSON encoding of v to the given writer
+// It tolerates invalid UTF-8 in strings and replaces them with the Unicode replacement character U+FFFD.
+func MarshalWriteRelaxed(w io.Writer, v any) error {
+	return DefaultJSONHandler.MarshalWriteRelaxed(w, v)
 }
 
 // Unmarshal decodes object from bytes

modules/json/jsonv2.go

@@ -13,6 +13,7 @@ import (
 
 type JSONv2 struct {
 	marshalOptions                  jsonv2.Options
+	marshalRelaxedOptions           jsonv2.Options
 	marshalKeepOptionalEmptyOptions jsonv2.Options
 	unmarshalOptions                jsonv2.Options
 	unmarshalCaseInsensitiveOptions jsonv2.Options
@@ -26,6 +27,8 @@ func init() {
 		jsonv2.FormatNilMapAsNull(true),
 	}
 	jsonV2.marshalOptions = jsonv2.JoinOptions(commonMarshalOptions...)
+	// when handling user's data, we want to allow invalid UTF-8 to be replaced with the replacement character, rather than returning an error.
+	jsonV2.marshalRelaxedOptions = jsonv2.JoinOptions(append(commonMarshalOptions, jsontext.AllowInvalidUTF8(true))...)
 	jsonV2.unmarshalOptions = jsonv2.DefaultOptionsV2()
 
 	// By default, "json/v2" omitempty removes all `""` empty strings, no matter where it comes from.
@@ -53,8 +56,8 @@ func (j *JSONv2) Marshal(v any) ([]byte, error) {
 	return jsonv2.Marshal(v, j.marshalOptions)
 }
 
-func (j *JSONv2) MarshalWrite(w io.Writer, v any) error {
-	return jsonv2.MarshalWrite(w, v, j.marshalOptions)
+func (j *JSONv2) MarshalWriteRelaxed(w io.Writer, v any) error {
+	return jsonv2.MarshalWrite(w, v, j.marshalRelaxedOptions)
 }
 
 func (j *JSONv2) Unmarshal(data []byte, v any) error {

modules/packages/composer/metadata.go

@@ -15,6 +15,7 @@ import (
 	"regexp"
 	"strings"
 
+	"gitea.dev/modules/charset"
 	"gitea.dev/modules/json"
 	"gitea.dev/modules/util"
 	"gitea.dev/modules/validation"
@@ -261,6 +262,7 @@ func ParsePackage(r ReadSeekAt, optVersion ...string) (*PackageInfo, error) {
 		cj.Readme = "README.md"
 	}
 	dataReadmeMd, _ := readPackageFile(pkgExt, r, cj.Readme, 10*1024)
+	dataReadmeMd = charset.ToUTF8DropErrors(dataReadmeMd)
 
 	// FIXME: legacy problem, the "Readme" field is abused, it should always be the path to the readme file
 	if len(dataReadmeMd) == 0 {

modules/packages/composer/metadata_test.go

@@ -4,8 +4,6 @@
 package composer
 
 import (
-	"archive/tar"
-	"archive/zip"
 	"bytes"
 	"compress/gzip"
 	"io"
@@ -13,6 +11,7 @@ import (
 	"testing"
 
 	"gitea.dev/modules/json"
+	"gitea.dev/modules/test"
 
 	"github.com/dsnet/compress/bzip2"
 	"github.com/stretchr/testify/assert"
@@ -76,37 +75,8 @@ func TestCommentsUnmarshal(t *testing.T) {
 }
 
 func TestParsePackage(t *testing.T) {
-	createArchive := func(files map[string]string) []byte {
-		var buf bytes.Buffer
-		archive := zip.NewWriter(&buf)
-		for name, content := range files {
-			w, _ := archive.Create(name)
-			_, _ = w.Write([]byte(content))
-		}
-		_ = archive.Close()
-		return buf.Bytes()
-	}
-
-	createArchiveTar := func(comp func(io.Writer) io.WriteCloser, files map[string]string) []byte {
-		var buf bytes.Buffer
-		w := comp(&buf)
-		archive := tar.NewWriter(w)
-		for name, content := range files {
-			hdr := &tar.Header{
-				Name: name,
-				Mode: 0o600,
-				Size: int64(len(content)),
-			}
-			_ = archive.WriteHeader(hdr)
-			_, _ = archive.Write([]byte(content))
-		}
-		_ = w.Close()
-		_ = archive.Close()
-		return buf.Bytes()
-	}
-
 	t.Run("MissingComposerFile", func(t *testing.T) {
-		data := createArchive(map[string]string{"dummy.txt": ""})
+		data := test.WriteZipArchive(map[string]string{"dummy.txt": ""}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.Nil(t, cp)
@@ -114,7 +84,7 @@ func TestParsePackage(t *testing.T) {
 	})
 
 	t.Run("MissingComposerFileInRoot", func(t *testing.T) {
-		data := createArchive(map[string]string{"sub/sub/composer.json": ""})
+		data := test.WriteZipArchive(map[string]string{"sub/sub/composer.json": ""}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.Nil(t, cp)
@@ -122,7 +92,7 @@ func TestParsePackage(t *testing.T) {
 	})
 
 	t.Run("InvalidComposerFile", func(t *testing.T) {
-		data := createArchive(map[string]string{"composer.json": ""})
+		data := test.WriteZipArchive(map[string]string{"composer.json": ""}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.Nil(t, cp)
@@ -130,7 +100,7 @@ func TestParsePackage(t *testing.T) {
 	})
 
 	t.Run("InvalidPackageName", func(t *testing.T) {
-		data := createArchive(map[string]string{"composer.json": "{}"})
+		data := test.WriteZipArchive(map[string]string{"composer.json": "{}"}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.Nil(t, cp)
@@ -138,7 +108,7 @@ func TestParsePackage(t *testing.T) {
 	})
 
 	t.Run("InvalidPackageVersion", func(t *testing.T) {
-		data := createArchive(map[string]string{"composer.json": `{"name": "gitea/composer-package", "version": "1.a.3"}`})
+		data := test.WriteZipArchive(map[string]string{"composer.json": `{"name": "gitea/composer-package", "version": "1.a.3"}`}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.Nil(t, cp)
@@ -146,7 +116,7 @@ func TestParsePackage(t *testing.T) {
 	})
 
 	t.Run("InvalidReadmePath", func(t *testing.T) {
-		data := createArchive(map[string]string{"composer.json": `{"name": "gitea/composer-package", "readme": "sub/README.md"}`})
+		data := test.WriteZipArchive(map[string]string{"composer.json": `{"name": "gitea/composer-package", "readme": "sub/README.md"}`}).Bytes()
 
 		cp, err := ParsePackage(bytes.NewReader(data))
 		assert.NoError(t, err)
@@ -177,22 +147,29 @@ func TestParsePackage(t *testing.T) {
 	}
 
 	t.Run("ValidZip", func(t *testing.T) {
-		data := createArchive(map[string]string{"composer.json": buildComposerContent(""), "README.md": readme})
+		data := test.WriteZipArchive(map[string]string{"composer.json": buildComposerContent(""), "README.md": readme}).Bytes()
 		assertValidPackage(t, data, "", "gitea-composer-package.zip")
 	})
 
 	t.Run("ValidTarBz2", func(t *testing.T) {
-		data := createArchiveTar(func(w io.Writer) io.WriteCloser {
+		data := test.WriteTarCompression(func(w io.Writer) io.WriteCloser {
 			bz2Writer, _ := bzip2.NewWriter(w, nil)
 			return bz2Writer
-		}, map[string]string{"composer.json": buildComposerContent("1.0"), "README.md": readme})
+		}, map[string]string{"composer.json": buildComposerContent("1.0"), "README.md": readme}).Bytes()
 		assertValidPackage(t, data, "1.0", "gitea-composer-package.1.0.tar.bz2")
 	})
 
 	t.Run("ValidTarGz", func(t *testing.T) {
-		data := createArchiveTar(func(w io.Writer) io.WriteCloser {
+		data := test.WriteTarCompression(func(w io.Writer) io.WriteCloser {
 			return gzip.NewWriter(w)
-		}, map[string]string{"composer.json": buildComposerContent(""), "README.md": readme})
+		}, map[string]string{"composer.json": buildComposerContent(""), "README.md": readme}).Bytes()
 		assertValidPackage(t, data, "", "gitea-composer-package.tar.gz")
 	})
+
+	t.Run("ReadmeEncoding", func(t *testing.T) {
+		data := test.WriteZipArchive(map[string]string{"composer.json": buildComposerContent(""), "README.md": "🌞🌞"[:6]}).Bytes()
+		cp, err := ParsePackage(bytes.NewReader(data))
+		require.NoError(t, err)
+		assert.Equal(t, "🌞 ", cp.Metadata.Readme)
+	})
 }

services/context/base.go

@@ -114,7 +114,7 @@ func (b *Base) HTTPError(status int, contents ...string) {
 func (b *Base) JSON(status int, content any) {
 	b.Resp.Header().Set("Content-Type", "application/json;charset=utf-8")
 	b.Resp.WriteHeader(status)
-	if err := json.MarshalWrite(b.Resp, content); err != nil {
+	if err := json.MarshalWriteRelaxed(b.Resp, content); err != nil {
 		log.Error("Render JSON failed: %v", err)
 	}
 }

services/convert/git_commit.go

@@ -189,17 +189,17 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep
 
 	// Retrieve files affected by the commit
 	if opts.Files {
-		fileStatus, err := git.GetCommitFileStatus(ctx, repo, commit.ID.String())
+		fileStatusSummary, err := git.GetCommitFileStatus(ctx, repo, commit.ID.String())
 		if err != nil {
 			return nil, err
 		}
 
-		affectedFileList := make([]*api.CommitAffectedFiles, 0, len(fileStatus.Added)+len(fileStatus.Removed)+len(fileStatus.Modified))
-		for filestatus, files := range map[string][]string{"added": fileStatus.Added, "removed": fileStatus.Removed, "modified": fileStatus.Modified} {
+		affectedFileList := make([]*api.CommitAffectedFiles, 0, len(fileStatusSummary.Added)+len(fileStatusSummary.Removed)+len(fileStatusSummary.Modified))
+		for fileStatus, files := range map[string][]string{"added": fileStatusSummary.Added, "removed": fileStatusSummary.Removed, "modified": fileStatusSummary.Modified} {
 			for _, filename := range files {
 				affectedFileList = append(affectedFileList, &api.CommitAffectedFiles{
 					Filename: filename,
-					Status:   filestatus,
+					Status:   fileStatus,
 				})
 			}
 		}

services/convert/pull_review.go

@@ -6,10 +6,13 @@ package convert
 import (
 	"context"
 	"strings"
+	"unicode/utf8"
 
 	issues_model "gitea.dev/models/issues"
 	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/charset"
 	api "gitea.dev/modules/structs"
+	"gitea.dev/modules/util"
 )
 
 // ToPullReview convert a review to api format
@@ -127,9 +130,13 @@ func ToPullReviewComment(ctx context.Context, comment *issues_model.Comment, doe
 }
 
 func patch2diff(patch string) string {
-	split := strings.Split(patch, "\n@@")
-	if len(split) == 2 {
-		return "@@" + split[1]
+	idx := strings.Index(patch, "\n@@")
+	if idx == -1 {
+		return ""
 	}
-	return ""
+	diffContent := patch[idx+1:]
+	// Although there might be an edge case that deleted lines and added lines are in different encodings,
+	// it's not worth to make things more complicated at the moment, and most code in modern codebases should be written in UTF-8.
+	hunk := charset.ToUTF8(util.UnsafeStringToBytes(diffContent), charset.ConvertOpts{ErrorReplacement: []byte(string(utf8.RuneError))})
+	return util.UnsafeBytesToString(hunk)
 }

services/convert/pull_review_test.go

@@ -49,3 +49,7 @@ func Test_ToPullReview(t *testing.T) {
 		assert.Len(t, prList, 1)
 	})
 }
+
+func TestPatch2DiffInvalidUTF8(t *testing.T) {
+	assert.Equal(t, "@@ -1 +1 @@\n+caf�", patch2diff("diff --git a/a b/a\n@@ -1 +1 @@\n+caf\xe9"))
+}

services/websocket/events.go

@@ -38,7 +38,7 @@ func MakeUserEventMessage(eventType string, eventData any) []byte {
 	buf := &bytes.Buffer{}
 	buf.WriteString(eventType)
 	buf.WriteByte('\n')
-	err := json.MarshalWrite(buf, &UserEventMessage[any]{EventType: eventType, EventData: eventData})
+	err := json.MarshalWriteRelaxed(buf, &UserEventMessage[any]{EventType: eventType, EventData: eventData})
 	payloadBytes := bytes.TrimSuffix(buf.Bytes(), []byte("\n")) // json v1 adds extra "\n" but we don't want it
 	if err != nil {
 		setting.PanicInDevOrTesting("websocket: marshal event: %v", err)