From 61d68d2d6ee81a5919597d91c736c502d7156859 Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Fri, 24 Apr 2026 09:17:57 +0200 Subject: [PATCH] MM-68439 Centralize filename handling for FileInfo (#36223) (#36255) Automatic Merge --- server/channels/app/upload.go | 13 ++- .../store/searchtest/file_info_layer.go | 2 +- server/i18n/en.json | 8 ++ server/public/model/file_info.go | 66 ++++++++++++++ server/public/model/file_info_test.go | 86 +++++++++++++++++++ 5 files changed, 173 insertions(+), 2 deletions(-) diff --git a/server/channels/app/upload.go b/server/channels/app/upload.go index ac202bf1bd..cc16ede7d7 100644 --- a/server/channels/app/upload.go +++ b/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,7 +281,13 @@ 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 { - return nil, model.NewAppError("UploadData", "app.upload.upload_data.gen_info.app_error", nil, "", http.StatusInternalServerError).Wrap(genErr) + 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 diff --git a/server/channels/store/searchtest/file_info_layer.go b/server/channels/store/searchtest/file_info_layer.go index a83974e20b..57c9ee0dc8 100644 --- a/server/channels/store/searchtest/file_info_layer.go +++ b/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) diff --git a/server/i18n/en.json b/server/i18n/en.json index fe648db0d8..391a1d60a8 100644 --- a/server/i18n/en.json +++ b/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." diff --git a/server/public/model/file_info.go b/server/public/model/file_info.go index 7be518c994..ff6e6d30e7 100644 --- a/server/public/model/file_info.go +++ b/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") } diff --git a/server/public/model/file_info_test.go b/server/public/model/file_info_test.go index 9fcb53f4d2..e447b8423c 100644 --- a/server/public/model/file_info_test.go +++ b/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) {