Commit 4add9b04 authored by memoclaw's avatar memoclaw

fix: prevent local attachment uploads from overwriting files

parent a24d4209
...@@ -360,7 +360,7 @@ func SaveAttachmentBlob(ctx context.Context, profile *profile.Profile, stores *s ...@@ -360,7 +360,7 @@ func SaveAttachmentBlob(ctx context.Context, profile *profile.Profile, stores *s
} }
if instanceStorageSetting.StorageType == storepb.InstanceStorageSetting_LOCAL { if instanceStorageSetting.StorageType == storepb.InstanceStorageSetting_LOCAL {
filepathTemplate := "assets/{timestamp}_{filename}" filepathTemplate := "assets/{timestamp}_{uuid}_{filename}"
if instanceStorageSetting.FilepathTemplate != "" { if instanceStorageSetting.FilepathTemplate != "" {
filepathTemplate = instanceStorageSetting.FilepathTemplate filepathTemplate = instanceStorageSetting.FilepathTemplate
} }
...@@ -377,6 +377,15 @@ func SaveAttachmentBlob(ctx context.Context, profile *profile.Profile, stores *s ...@@ -377,6 +377,15 @@ func SaveAttachmentBlob(ctx context.Context, profile *profile.Profile, stores *s
if !filepath.IsAbs(osPath) { if !filepath.IsAbs(osPath) {
osPath = filepath.Join(profile.Data, osPath) osPath = filepath.Join(profile.Data, osPath)
} }
osPath = ensureUniqueLocalAttachmentPath(osPath, create.UID)
internalPath = filepath.ToSlash(osPath)
if !filepath.IsAbs(filepath.FromSlash(internalPath)) {
internalPath, err = filepath.Rel(profile.Data, osPath)
if err != nil {
return errors.Wrap(err, "Failed to get relative path")
}
internalPath = filepath.ToSlash(internalPath)
}
dir := filepath.Dir(osPath) dir := filepath.Dir(osPath)
if err = os.MkdirAll(dir, os.ModePerm); err != nil { if err = os.MkdirAll(dir, os.ModePerm); err != nil {
return errors.Wrap(err, "Failed to create directory") return errors.Wrap(err, "Failed to create directory")
...@@ -514,6 +523,16 @@ func replaceFilenameWithPathTemplate(path, filename string) string { ...@@ -514,6 +523,16 @@ func replaceFilenameWithPathTemplate(path, filename string) string {
return path return path
} }
func ensureUniqueLocalAttachmentPath(path, uid string) string {
if _, err := os.Stat(path); err != nil {
return path
}
ext := filepath.Ext(path)
base := strings.TrimSuffix(path, ext)
return base + "_" + uid + ext
}
func validateFilename(filename string) bool { func validateFilename(filename string) bool {
// Reject path traversal attempts and make sure no additional directories are created // Reject path traversal attempts and make sure no additional directories are created
if !filepath.IsLocal(filename) || strings.ContainsAny(filename, "/\\") { if !filepath.IsLocal(filename) || strings.ContainsAny(filename, "/\\") {
......
...@@ -7,6 +7,9 @@ import ( ...@@ -7,6 +7,9 @@ import (
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
v1pb "github.com/usememos/memos/proto/gen/api/v1" v1pb "github.com/usememos/memos/proto/gen/api/v1"
storepb "github.com/usememos/memos/proto/gen/store"
apiv1 "github.com/usememos/memos/server/router/api/v1"
"github.com/usememos/memos/store"
) )
func TestCreateAttachment(t *testing.T) { func TestCreateAttachment(t *testing.T) {
...@@ -56,4 +59,56 @@ func TestCreateAttachment(t *testing.T) { ...@@ -56,4 +59,56 @@ func TestCreateAttachment(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, "application/octet-stream", attachment.Type) require.Equal(t, "application/octet-stream", attachment.Type)
}) })
t.Run("LocalStorage_PathCollisionUsesUniqueReference", func(t *testing.T) {
_, err := ts.Store.UpsertInstanceSetting(ctx, &storepb.InstanceSetting{
Key: storepb.InstanceSettingKey_STORAGE,
Value: &storepb.InstanceSetting_StorageSetting{
StorageSetting: &storepb.InstanceStorageSetting{
StorageType: storepb.InstanceStorageSetting_LOCAL,
FilepathTemplate: "assets/{filename}",
},
},
})
require.NoError(t, err)
first, err := ts.Service.CreateAttachment(userCtx, &v1pb.CreateAttachmentRequest{
Attachment: &v1pb.Attachment{
Filename: "screenshot.png",
Type: "image/png",
Content: []byte("first-image"),
},
})
require.NoError(t, err)
second, err := ts.Service.CreateAttachment(userCtx, &v1pb.CreateAttachmentRequest{
Attachment: &v1pb.Attachment{
Filename: "screenshot.png",
Type: "image/png",
Content: []byte("second-image"),
},
})
require.NoError(t, err)
firstUID, err := apiv1.ExtractAttachmentUIDFromName(first.Name)
require.NoError(t, err)
secondUID, err := apiv1.ExtractAttachmentUIDFromName(second.Name)
require.NoError(t, err)
firstStoreAttachment, err := ts.Store.GetAttachment(ctx, &store.FindAttachment{UID: &firstUID})
require.NoError(t, err)
secondStoreAttachment, err := ts.Store.GetAttachment(ctx, &store.FindAttachment{UID: &secondUID})
require.NoError(t, err)
require.NotNil(t, firstStoreAttachment)
require.NotNil(t, secondStoreAttachment)
require.NotEqual(t, firstStoreAttachment.Reference, secondStoreAttachment.Reference)
firstBlob, err := ts.Service.GetAttachmentBlob(firstStoreAttachment)
require.NoError(t, err)
secondBlob, err := ts.Service.GetAttachmentBlob(secondStoreAttachment)
require.NoError(t, err)
require.Equal(t, []byte("first-image"), firstBlob)
require.Equal(t, []byte("second-image"), secondBlob)
})
} }
...@@ -219,7 +219,7 @@ func (s *Store) GetInstanceNotificationSetting(ctx context.Context) (*storepb.In ...@@ -219,7 +219,7 @@ func (s *Store) GetInstanceNotificationSetting(ctx context.Context) (*storepb.In
const ( const (
defaultInstanceStorageType = storepb.InstanceStorageSetting_LOCAL defaultInstanceStorageType = storepb.InstanceStorageSetting_LOCAL
defaultInstanceUploadSizeLimitMb = 30 defaultInstanceUploadSizeLimitMb = 30
defaultInstanceFilepathTemplate = "assets/{timestamp}_{filename}" defaultInstanceFilepathTemplate = "assets/{timestamp}_{uuid}_{filename}"
) )
func (s *Store) GetInstanceStorageSetting(ctx context.Context) (*storepb.InstanceStorageSetting, error) { func (s *Store) GetInstanceStorageSetting(ctx context.Context) (*storepb.InstanceStorageSetting, error) {
......
...@@ -196,7 +196,7 @@ func TestInstanceSettingStorageSetting(t *testing.T) { ...@@ -196,7 +196,7 @@ func TestInstanceSettingStorageSetting(t *testing.T) {
require.NotNil(t, storageSetting) require.NotNil(t, storageSetting)
require.Equal(t, storepb.InstanceStorageSetting_LOCAL, storageSetting.StorageType) require.Equal(t, storepb.InstanceStorageSetting_LOCAL, storageSetting.StorageType)
require.Equal(t, int64(30), storageSetting.UploadSizeLimitMb) require.Equal(t, int64(30), storageSetting.UploadSizeLimitMb)
require.Equal(t, "assets/{timestamp}_{filename}", storageSetting.FilepathTemplate) require.Equal(t, "assets/{timestamp}_{uuid}_{filename}", storageSetting.FilepathTemplate)
// Set custom storage setting // Set custom storage setting
_, err = ts.UpsertInstanceSetting(ctx, &storepb.InstanceSetting{ _, err = ts.UpsertInstanceSetting(ctx, &storepb.InstanceSetting{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment