diff --git a/app/file.go b/app/file.go index 5815e6451f..85a248f866 100644 --- a/app/file.go +++ b/app/file.go @@ -18,6 +18,7 @@ import ( "net/http" "net/url" "path/filepath" + "regexp" "strings" "sync" "time" @@ -135,33 +136,8 @@ func (a *App) ListDirectory(path string) ([]string, *model.AppError) { return *paths, nil } -func (a *App) GetInfoForFilename(post *model.Post, teamId string, filename string) *model.FileInfo { - // Find the path from the Filename of the form /{channelId}/{userId}/{uid}/{nameWithExtension} - split := strings.SplitN(filename, "/", 5) - if len(split) < 5 { - mlog.Error( - "Unable to decipher filename when migrating post to use FileInfos", - mlog.String("post_id", post.Id), - mlog.String("filename", filename), - ) - return nil - } - - channelId := split[1] - userId := split[2] - oldId := split[3] - name, _ := url.QueryUnescape(split[4]) - - if split[0] != "" || split[1] != post.ChannelId || split[2] != post.UserId || strings.Contains(split[4], "/") { - mlog.Warn( - "Found an unusual filename when migrating post to use FileInfos", - mlog.String("post_id", post.Id), - mlog.String("channel_id", post.ChannelId), - mlog.String("user_id", post.UserId), - mlog.String("filename", filename), - ) - } - +func (a *App) getInfoForFilename(post *model.Post, teamId, channelId, userId, oldId, filename string) *model.FileInfo { + name, _ := url.QueryUnescape(filename) pathPrefix := fmt.Sprintf("teams/%s/channels/%s/users/%s/%s/", teamId, channelId, userId, oldId) path := pathPrefix + name @@ -204,10 +180,8 @@ func (a *App) GetInfoForFilename(post *model.Post, teamId string, filename strin return info } -func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string { - split := strings.SplitN(filename, "/", 5) - id := split[3] - name, _ := url.QueryUnescape(split[4]) +func (a *App) findTeamIdForFilename(post *model.Post, id, filename string) string { + name, _ := url.QueryUnescape(filename) // This post is in a direct channel so we need to figure out what team the files are stored under. teams, err := a.Srv.Store.Team().GetTeamsByUserId(post.UserId) @@ -223,7 +197,7 @@ func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string { for _, team := range teams { path := fmt.Sprintf("teams/%s/channels/%s/users/%s/%s/%s", team.Id, post.ChannelId, post.UserId, id, name) - if _, err := a.ReadFile(path); err == nil { + if ok, err := a.FileExists(path); ok && err == nil { // Found the team that this file was posted from return team.Id } @@ -233,6 +207,27 @@ func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string { } var fileMigrationLock sync.Mutex +var oldFilenameMatchExp *regexp.Regexp = regexp.MustCompile(`^\/([a-z\d]{26})\/([a-z\d]{26})\/([a-z\d]{26})\/([^\/]+)$`) + +// Parse the path from the Filename of the form /{channelId}/{userId}/{uid}/{nameWithExtension} +func parseOldFilenames(filenames []string, channelId, userId string) [][]string { + parsed := [][]string{} + for _, filename := range filenames { + matches := oldFilenameMatchExp.FindStringSubmatch(filename) + if len(matches) != 5 { + mlog.Error("Failed to parse old Filename", mlog.String("filename", filename)) + continue + } + if matches[1] != channelId { + mlog.Error("ChannelId in Filename does not match", mlog.String("channel_id", channelId), mlog.String("matched", matches[1])) + } else if matches[2] != userId { + mlog.Error("UserId in Filename does not match", mlog.String("user_id", userId), mlog.String("matched", matches[2])) + } else { + parsed = append(parsed, matches[1:]) + } + } + return parsed +} // Creates and stores FileInfos for a post created before the FileInfos table existed. func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { @@ -254,11 +249,19 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { return []*model.FileInfo{} } + // Parse and validate filenames before further processing + parsedFilenames := parseOldFilenames(filenames, post.ChannelId, post.UserId) + + if len(parsedFilenames) == 0 { + mlog.Error("Unable to parse filenames") + return []*model.FileInfo{} + } + // Find the team that was used to make this post since its part of the file path that isn't saved in the Filename var teamId string if channel.TeamId == "" { // This post was made in a cross-team DM channel, so we need to find where its files were saved - teamId = a.FindTeamIdForFilename(post, filenames[0]) + teamId = a.findTeamIdForFilename(post, parsedFilenames[0][2], parsedFilenames[0][3]) } else { teamId = channel.TeamId } @@ -272,8 +275,8 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { mlog.String("post_id", post.Id), ) } else { - for _, filename := range filenames { - info := a.GetInfoForFilename(post, teamId, filename) + for _, parsed := range parsedFilenames { + info := a.getInfoForFilename(post, teamId, parsed[0], parsed[1], parsed[2], parsed[3]) if info == nil { continue } diff --git a/app/file_test.go b/app/file_test.go index e6263121ea..898551f833 100644 --- a/app/file_test.go +++ b/app/file_test.go @@ -53,7 +53,7 @@ func TestDoUploadFile(t *testing.T) { }() value := fmt.Sprintf("20070204/teams/%v/channels/%v/users/%v/%v/%v", teamId, channelId, userId, info1.Id, filename) - assert.Equal(t, value, info1.Path, "stored file at incorrect path" ) + assert.Equal(t, value, info1.Path, "stored file at incorrect path") info2, err := th.App.DoUploadFile(time.Date(2007, 2, 4, 1, 2, 3, 4, time.Local), teamId, channelId, userId, filename, data) require.Nil(t, err, "DoUploadFile should succeed with valid data") @@ -106,6 +106,103 @@ func TestUploadFile(t *testing.T) { assert.Equal(t, value, info1.Path, "Stored file at incorrect path") } +func TestParseOldFilenames(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + fileId := model.NewId() + + tests := []struct { + description string + filenames []string + channelId string + userId string + expected [][]string + }{ + { + description: "Empty input should result in empty output", + filenames: []string{}, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{}, + }, + { + description: "Filename with invalid format should not parse", + filenames: []string{"/path/to/some/file.png"}, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{}, + }, + { + description: "ChannelId in Filename should not match", + filenames: []string{ + fmt.Sprintf("/%v/%v/%v/file.png", model.NewId(), th.BasicUser.Id, fileId), + }, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{}, + }, + { + description: "UserId in Filename should not match", + filenames: []string{ + fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, model.NewId(), fileId), + }, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{}, + }, + { + description: "../ in filename should not parse", + filenames: []string{ + fmt.Sprintf("/%v/%v/%v/../../../file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId), + }, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{}, + }, + { + description: "Should only parse valid filenames", + filenames: []string{ + fmt.Sprintf("/%v/%v/%v/../otherfile.png", th.BasicChannel.Id, th.BasicUser.Id, fileId), + fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId), + }, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{ + { + th.BasicChannel.Id, + th.BasicUser.Id, + fileId, + "file.png", + }, + }, + }, + { + description: "Valid Filename should parse", + filenames: []string{ + fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId), + }, + channelId: th.BasicChannel.Id, + userId: th.BasicUser.Id, + expected: [][]string{ + { + th.BasicChannel.Id, + th.BasicUser.Id, + fileId, + "file.png", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.description, func(tt *testing.T) { + result := parseOldFilenames(test.filenames, test.channelId, test.userId) + require.Equal(tt, result, test.expected) + }) + } +} + func TestGetInfoForFilename(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -113,10 +210,7 @@ func TestGetInfoForFilename(t *testing.T) { post := th.BasicPost teamId := th.BasicTeam.Id - info := th.App.GetInfoForFilename(post, teamId, "sometestfile") - assert.Nil(t, info, "Test bad filename") - - info = th.App.GetInfoForFilename(post, teamId, "/somechannel/someuser/someid/somefile.png") + info := th.App.getInfoForFilename(post, teamId, post.ChannelId, post.UserId, "someid", "somefile.png") assert.Nil(t, info, "Test non-existent file") } @@ -124,13 +218,13 @@ func TestFindTeamIdForFilename(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - teamId := th.App.FindTeamIdForFilename(th.BasicPost, fmt.Sprintf("/%v/%v/%v/blargh.png", th.BasicChannel.Id, th.BasicUser.Id, "someid")) + teamId := th.App.findTeamIdForFilename(th.BasicPost, "someid", "somefile.png") assert.Equal(t, th.BasicTeam.Id, teamId) _, err := th.App.CreateTeamWithUser(&model.Team{Email: th.BasicUser.Email, Name: "zz" + model.NewId(), DisplayName: "Joram's Test Team", Type: model.TEAM_OPEN}, th.BasicUser.Id) require.Nil(t, err) - teamId = th.App.FindTeamIdForFilename(th.BasicPost, fmt.Sprintf("/%v/%v/%v/blargh.png", th.BasicChannel.Id, th.BasicUser.Id, "someid")) + teamId = th.App.findTeamIdForFilename(th.BasicPost, "someid", "somefile.png") assert.Equal(t, "", teamId) } @@ -151,14 +245,21 @@ func TestMigrateFilenamesToFileInfos(t *testing.T) { require.Nil(t, fileErr) defer file.Close() - fpath := fmt.Sprintf("/teams/%v/channels/%v/users/%v/%v/test.png", th.BasicTeam.Id, th.BasicChannel.Id, th.BasicUser.Id, "someid") + fileId := model.NewId() + fpath := fmt.Sprintf("/teams/%v/channels/%v/users/%v/%v/test.png", th.BasicTeam.Id, th.BasicChannel.Id, th.BasicUser.Id, fileId) _, err := th.App.WriteFile(file, fpath) require.Nil(t, err) - rpost, err := th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/test.png", th.BasicChannel.Id, th.BasicUser.Id, "someid")}}, th.BasicChannel, false) + rpost, err := th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/test.png", th.BasicChannel.Id, th.BasicUser.Id, fileId)}}, th.BasicChannel, false) require.Nil(t, err) infos = th.App.MigrateFilenamesToFileInfos(rpost) assert.Equal(t, 1, len(infos)) + + rpost, err = th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/../../test.png", th.BasicChannel.Id, th.BasicUser.Id, fileId)}}, th.BasicChannel, false) + require.Nil(t, err) + + infos = th.App.MigrateFilenamesToFileInfos(rpost) + assert.Equal(t, 0, len(infos)) } func TestCopyFileInfos(t *testing.T) {