MM-68439 Centralize filename handling for FileInfo (#36223) (#36255)

Automatic Merge

server/channels/app/upload.go

@@ -23,6 +23,11 @@ import (
 const minFirstPartSize = 5 * 1024 * 1024 // 5MB
 
 func (a *App) genFileInfoFromReader(name string, file io.ReadSeeker, size int64) (*model.FileInfo, error) {
+	name = model.SanitizeFilename(name)
+	if name == "" {
+		return nil, model.NewAppError("genFileInfoFromReader", "app.upload.gen_file_info.invalid_filename.app_error", nil, "", http.StatusBadRequest)
+	}
+
 	ext := strings.ToLower(filepath.Ext(name))
 
 	info := &model.FileInfo{
@@ -276,8 +281,14 @@ func (a *App) UploadData(c request.CTX, us *model.UploadSession, rd io.Reader) (
 	info, genErr := a.genFileInfoFromReader(us.Filename, file, us.FileSize)
 	file.Close()
 	if genErr != nil {
+		var appErr *model.AppError
+		switch {
+		case errors.As(genErr, &appErr):
+			return nil, appErr
+		default:
 			return nil, model.NewAppError("UploadData", "app.upload.upload_data.gen_info.app_error", nil, "", http.StatusInternalServerError).Wrap(genErr)
 		}
+	}
 
 	info.CreatorId = us.UserId
 	info.ChannelId = us.ChannelId

server/channels/store/searchtest/file_info_layer.go

@@ -1621,7 +1621,7 @@ func testFileInfoSlashShouldNotBeCharSeparator(t *testing.T, th *SearchTestHelpe
 	require.NoError(t, err)
 	defer th.deleteUserPosts(th.User.Id)
 
-	p1, err := th.createFileInfo(th.User.Id, post.Id, post.ChannelId, "alpha/beta gamma, theta", "alpha/beta gamma, theta", "jpg", "image/jpeg", 0, 0)
+	p1, err := th.createFileInfo(th.User.Id, post.Id, post.ChannelId, "testfile.jpg", "alpha/beta gamma, theta", "jpg", "image/jpeg", 0, 0)
 	require.NoError(t, err)
 	defer th.deleteUserFileInfos(th.User.Id)
 

server/i18n/en.json

@@ -7428,6 +7428,10 @@
     "id": "app.upload.create.save.app_error",
     "translation": "Failed to save upload."
   },
+  {
+    "id": "app.upload.gen_file_info.invalid_filename.app_error",
+    "translation": "Invalid filename."
+  },
   {
     "id": "app.upload.get.app_error",
     "translation": "Failed to get upload."
@@ -9868,6 +9872,10 @@
     "id": "model.file_info.is_valid.id.app_error",
     "translation": "Invalid value for id."
   },
+  {
+    "id": "model.file_info.is_valid.name.app_error",
+    "translation": "Invalid value for name."
+  },
   {
     "id": "model.file_info.is_valid.path.app_error",
     "translation": "Invalid value for path."

server/public/model/file_info.go

@@ -8,11 +8,19 @@ import (
 	"net/http"
 	"path/filepath"
 	"strings"
+	"unicode/utf8"
+
+	"golang.org/x/text/unicode/norm"
 )
 
 const (
 	FileinfoSortByCreated = "CreateAt"
 	FileinfoSortBySize    = "Size"
+
+	// MaxFilenameLength is the maximum length, in Unicode codepoints, of a
+	// sanitized FileInfo.Name. It matches the VARCHAR(256) width of the
+	// fileinfo.name column.
+	MaxFilenameLength = 256
 )
 
 // GetFileInfosOptions contains options for getting FileInfos
@@ -117,9 +125,67 @@ func (fi *FileInfo) IsValid() *AppError {
 		return NewAppError("FileInfo.IsValid", "model.file_info.is_valid.path.app_error", nil, "id="+fi.Id, http.StatusBadRequest)
 	}
 
+	if fi.Name != "" && !IsValidFilename(fi.Name) {
+		return NewAppError("FileInfo.IsValid", "model.file_info.is_valid.name.app_error", nil, "id="+fi.Id, http.StatusBadRequest)
+	}
+
 	return nil
 }
 
+// IsValidFilename reports whether name is acceptable as FileInfo.Name.
+// It rejects empty strings, bare "." and "..", names exceeding
+// MaxFilenameLength, path separators, and ASCII control characters.
+// The input is not mutated; see SanitizeFilename for the mutating form.
+func IsValidFilename(name string) bool {
+	if name == "" || name == "." || name == ".." {
+		return false
+	}
+	if utf8.RuneCountInString(name) > MaxFilenameLength {
+		return false
+	}
+	if strings.ContainsAny(name, `/\`) {
+		return false
+	}
+	return !strings.ContainsFunc(name, func(r rune) bool {
+		return r < 0x20 || r == 0x7f
+	})
+}
+
+// SanitizeFilename returns a canonical form of name suitable for
+// FileInfo.Name. It NFC-normalizes Unicode, removes ASCII control
+// characters, collapses backslashes to forward slashes, reduces the
+// value to its final path element via filepath.Base, and truncates
+// to MaxFilenameLength codepoints to match the DB column width.
+//
+// Returns an empty string when nothing usable remains (for example
+// when the input was "", ".", "..", "/", or entirely control
+// characters); callers should treat an empty result as a failure.
+func SanitizeFilename(name string) string {
+	if name == "" {
+		return ""
+	}
+
+	name = norm.NFC.String(name)
+	name = strings.Map(func(r rune) rune {
+		if r < 0x20 || r == 0x7f {
+			return -1
+		}
+		return r
+	}, name)
+	name = strings.ReplaceAll(name, `\`, "/")
+	name = filepath.Base(name)
+
+	if name == "." || name == ".." || name == string(filepath.Separator) {
+		return ""
+	}
+
+	if runes := []rune(name); len(runes) > MaxFilenameLength {
+		name = string(runes[:MaxFilenameLength])
+	}
+
+	return name
+}
+
 func (fi *FileInfo) IsImage() bool {
 	return strings.HasPrefix(fi.MimeType, "image")
 }

server/public/model/file_info_test.go

@@ -6,6 +6,7 @@ package model
 import (
 	_ "image/gif"
 	_ "image/png"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -60,6 +61,91 @@ func TestFileInfoIsValid(t *testing.T) {
 		assert.Nil(t, info.IsValid(), "creatorId isn't valid")
 		info.CreatorId = creatorId
 	})
+
+	t.Run("Empty Name is valid", func(t *testing.T) {
+		info.Name = ""
+		assert.Nil(t, info.IsValid())
+	})
+
+	t.Run("Non-empty Name must be a plain filename", func(t *testing.T) {
+		originalName := info.Name
+		defer func() { info.Name = originalName }()
+
+		badNames := []string{
+			".",
+			"..",
+			"../a.png",
+			`..\..\a.png`,
+			"foo/bar.png",
+			`foo\bar.png`,
+			"foo\x00.png",
+		}
+		for _, bad := range badNames {
+			info.Name = bad
+			assert.NotNilf(t, info.IsValid(), "expected %q to be rejected", bad)
+		}
+	})
+}
+
+func TestIsValidFilename(t *testing.T) {
+	cases := []struct {
+		name  string
+		valid bool
+	}{
+		{"hello.png", true},
+		{"hello world (1).png", true},
+		{"日本語.txt", true},
+		{"", false},
+		{".", false},
+		{"..", false},
+		{"../a.png", false},
+		{`..\..\a`, false},
+		{"a/b", false},
+		{`foo\bar.png`, false},
+		{"a\x00b", false},
+		{"foo\tbar.png", false},
+		{"foo\rbar.png", false},
+		// MaxFilenameLength matches the VARCHAR(256) column; longer inputs
+		// that bypass SanitizeFilename's truncation must still fail here.
+		{strings.Repeat("a", MaxFilenameLength+1), false},
+		{strings.Repeat("a", MaxFilenameLength), true},
+	}
+	for _, tc := range cases {
+		assert.Equalf(t, tc.valid, IsValidFilename(tc.name), "input %q", tc.name)
+	}
+}
+
+func TestSanitizeFilename(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{"plain name unchanged", "hello.png", "hello.png"},
+		{"preserves spaces and parens", "hello world (1).png", "hello world (1).png"},
+		{"reduces leading dotdot path to basename", "../../a.png", "a.png"},
+		{"handles backslash separators", `..\..\a.exe`, "a.exe"},
+		{"reduces nested path to basename", "a/b/c.png", "c.png"},
+		{"strips null bytes", "foo\x00bar.png", "foobar.png"},
+		{"strips control chars", "foo\tbar\x1f.png", "foobar.png"},
+		{"rejects bare dotdot", "..", ""},
+		{"rejects bare dot", ".", ""},
+		{"rejects empty", "", ""},
+		{"rejects root", "/", ""},
+		{"rejects path ending in separator", "../", ""},
+		{"truncates to max length by runes", strings.Repeat("a", MaxFilenameLength+50), strings.Repeat("a", MaxFilenameLength)},
+		{"NFC-normalizes NFD input", "ガ.txt", "ガ.txt"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := SanitizeFilename(tc.in)
+			assert.Equal(t, tc.want, got)
+			if got != "" {
+				// SanitizeFilename output must always satisfy IsValidFilename.
+				assert.True(t, IsValidFilename(got), "sanitized output %q must be valid", got)
+			}
+		})
+	}
 }
 
 func TestFileInfoIsImage(t *testing.T) {