Upload: Delete invalid files, improve type checks and import logs photoprism#4895

Signed-off-by: Michael Mayer <michael@photoprism.app>

Author: lastzero

internal/api/users_upload.go

@@ -86,36 +86,55 @@ func UploadUserFiles(router *gin.RouterGroup) {
 			return
 		}
 
-		// List of allowed file extensions (all types allowed if empty).
-		allowExt := conf.UploadAllow()
+		// If the file extension list is empty, all file types may
+		// be uploaded except raw files if raw support is disabled.
+		allowedExt := conf.UploadAllow()
+		rejectRaw := conf.DisableRaw()
 
-		// Save uploaded files if their extension is allowed.
+		// Save uploaded files and append their names
+		// to "uploads" if they pass all checks.
 		for _, file := range files {
-			fileName := filepath.Base(file.Filename)
-			filePath := path.Join(uploadDir, fileName)
-			fileType := fs.FileType(fileName)
+			baseName := filepath.Base(file.Filename)
+			destName := path.Join(uploadDir, baseName)
+			fileType := fs.FileType(baseName)
 
+			// Reject unsupported files and files with extensions that aren't allowed.
 			if fileType == fs.TypeUnknown {
-				log.Warnf("upload: rejected %s due to unknown or unsupported extension", clean.Log(fileName))
+				log.Warnf("upload: rejected %s because it has an unsupported file extension", clean.Log(baseName))
 				continue
-			} else if allowExt.Excludes(fileType.DefaultExt()) {
-				log.Warnf("upload: rejected %s because the file type is not allowed", clean.Log(fileName))
+			} else if allowedExt.Excludes(fileType.DefaultExt()) {
+				log.Warnf("upload: rejected %s because its extension is not allowed", clean.Log(baseName))
 				continue
 			}
 
-			if err = c.SaveUploadedFile(file, filePath); err != nil {
-				log.Errorf("upload: failed saving file %s", clean.Log(fileName))
+			// Save uploaded file in the user upload path.
+			if err = c.SaveUploadedFile(file, destName); err != nil {
+				log.Errorf("upload: failed to save %s", clean.Log(baseName))
+				log.Debugf("upload: %s in %s", clean.Error(err), clean.Log(baseName))
 				Abort(c, http.StatusBadRequest, i18n.ErrUploadFailed)
 				return
 			} else {
-				log.Debugf("upload: saved file %s", clean.Log(fileName))
-				event.Publish("upload.saved", event.Data{"uid": s.UserUID, "file": fileName})
+				log.Debugf("upload: saved %s in user upload path", clean.Log(baseName))
+				event.Publish("upload.saved", event.Data{"uid": s.UserUID, "file": baseName})
 			}
 
-			uploads = append(uploads, filePath)
+			// Make sure the file is supported and has the correct extension before importing it.
+			if mediaFile, mediaErr := photoprism.NewMediaFile(destName); mediaErr != nil {
+				log.Errorf("upload: rejected %s, %s", clean.Error(err), clean.Log(baseName))
+				logErr("upload", os.Remove(destName))
+			} else if typeErr := mediaFile.CheckType(); typeErr != nil {
+				log.Warnf("upload: rejected %s %s", clean.Log(baseName), typeErr)
+				logErr("upload", os.Remove(destName))
+			} else if rejectRaw && mediaFile.IsRaw() {
+				log.Warnf("upload: rejected %s because raw support is disabled", clean.Log(baseName))
+				logErr("upload", os.Remove(destName))
+			} else {
+				// Successfully validated upload.
+				uploads = append(uploads, destName)
+			}
 		}
 
-		// Check if uploaded file is safe.
+		// Check if the uploaded file may contain inappropriate content.
 		if len(uploads) > 0 && !conf.UploadNSFW() {
 			nd := get.NsfwDetector()
 
@@ -152,6 +171,7 @@ func UploadUserFiles(router *gin.RouterGroup) {
 
 		elapsed := int(time.Since(start).Seconds())
 
+		// Log number of successfully uploaded files.
 		msg := i18n.Msg(i18n.MsgFilesUploadedIn, len(uploads), elapsed)
 
 		log.Info(msg)
@@ -240,7 +260,7 @@ func ProcessUserUpload(router *gin.RouterGroup) {
 		}
 
 		// Update moments if files have been imported.
-		if n := len(imported); n == 0 {
+		if n := imported.Processed(); n == 0 {
 			log.Infof("upload: found no new files to import from %s", clean.Log(uploadPath))
 		} else {
 			log.Infof("upload: imported %s", english.Plural(n, "file", "files"))

internal/photoprism/import.go

@@ -183,7 +183,15 @@ func (imp *Import) Start(opt ImportOptions) fs.Done {
 			// Report files that have a missing or invalid filename extension,
 			// see https://github.com/photoprism/photoprism/issues/3518.
 			if typeErr := mf.CheckType(); typeErr != nil {
-				log.Warnf("import: %s has %s", clean.Log(mf.RootRelName()), typeErr)
+				if !opt.RemoveInvalidFiles {
+					log.Warnf("import: %s %s and will not be indexed", clean.Log(mf.RootRelName()), typeErr)
+				} else if removeErr := mf.Remove(); removeErr != nil {
+					log.Errorf("import: %s %s and %s", clean.Log(mf.RootRelName()), typeErr, removeErr)
+					return nil
+				} else {
+					log.Warnf("import: %s %s and was deleted", clean.Log(mf.RootRelName()), typeErr)
+					return nil
+				}
 			}
 
 			// Create JSON sidecar file, if needed.

internal/photoprism/import_options.go

@@ -12,6 +12,7 @@ type ImportOptions struct {
 	NonBlocking            bool
 	DestFolder             string
 	RemoveDotFiles         bool
+	RemoveInvalidFiles     bool
 	RemoveExistingFiles    bool
 	RemoveEmptyDirectories bool
 }
@@ -35,6 +36,7 @@ func ImportOptionsCopy(importPath, destFolder string) ImportOptions {
 		NonBlocking:            false,
 		DestFolder:             destFolder,
 		RemoveDotFiles:         false,
+		RemoveInvalidFiles:     false,
 		RemoveExistingFiles:    false,
 		RemoveEmptyDirectories: false,
 	}
@@ -52,6 +54,7 @@ func ImportOptionsMove(importPath, destFolder string) ImportOptions {
 		NonBlocking:            false,
 		DestFolder:             destFolder,
 		RemoveDotFiles:         true,
+		RemoveInvalidFiles:     false,
 		RemoveExistingFiles:    true,
 		RemoveEmptyDirectories: true,
 	}
@@ -69,6 +72,7 @@ func ImportOptionsUpload(uploadPath, destFolder string) ImportOptions {
 		NonBlocking:            true,
 		DestFolder:             destFolder,
 		RemoveDotFiles:         true,
+		RemoveInvalidFiles:     true,
 		RemoveExistingFiles:    true,
 		RemoveEmptyDirectories: true,
 	}

internal/photoprism/index.go

@@ -222,7 +222,7 @@ func (ind *Index) Start(o IndexOptions) (found fs.Done, updated int) {
 			// Skip files if the filename extension does not match their mime type,
 			// see https://github.com/photoprism/photoprism/issues/3518 for details.
 			if typeErr := mf.CheckType(); typeErr != nil {
-				log.Warnf("index: skipped %s due to %s", clean.Log(mf.RootRelName()), typeErr)
+				log.Warnf("index: skipped %s because it %s", clean.Log(mf.RootRelName()), typeErr)
 				return nil
 			}
 

internal/photoprism/index_main.go

@@ -22,7 +22,7 @@ func IndexMain(related *RelatedFiles, ind *Index, o IndexOptions) (result IndexR
 	if typeErr := f.CheckType(); typeErr != nil {
 		// Skip files if the filename extension does not match their mime type,
 		// see https://github.com/photoprism/photoprism/issues/3518 for details.
-		result.Err = fmt.Errorf("index: skipped %s due to %w", clean.Log(f.RootRelName()), typeErr)
+		result.Err = fmt.Errorf("index: skipped %s because it %w", clean.Log(f.RootRelName()), typeErr)
 		result.Status = IndexFailed
 		return result
 	} else if limitErr, _ := f.ExceedsBytes(o.ByteLimit); limitErr != nil {

internal/photoprism/index_related.go

@@ -52,7 +52,7 @@ func IndexRelated(related RelatedFiles, ind *Index, o IndexOptions) (result Inde
 		// Skip files if the filename extension does not match their mime type,
 		// see https://github.com/photoprism/photoprism/issues/3518 for details.
 		if typeErr := f.CheckType(); typeErr != nil {
-			result.Err = fmt.Errorf("index: skipped %s due to %w", clean.Log(f.RootRelName()), typeErr)
+			result.Err = fmt.Errorf("index: skipped %s because it %w", clean.Log(f.RootRelName()), typeErr)
 			result.Status = IndexFailed
 			continue
 		}

internal/photoprism/mediafile.go

@@ -875,14 +875,14 @@ func (m *MediaFile) CheckType() error {
 	extension := m.Extension()
 
 	if extension == "" {
-		return fmt.Errorf("missing extension")
+		return fmt.Errorf("has no file extension")
 	}
 
 	// Detect file type and return error if unknown.
 	fileType := fs.FileType(m.fileName)
 
 	if fileType == fs.TypeUnknown {
-		return fmt.Errorf("unknown file type")
+		return fmt.Errorf("is an unknown file type")
 	}
 
 	// Detect media type (formerly known as a MIME type),
@@ -915,10 +915,10 @@ func (m *MediaFile) CheckType() error {
 
 	// Exclude mime type from the error message if it could not be detected.
 	if mimeType == fs.MimeTypeUnknown {
-		return fmt.Errorf("invalid extension (unknown media type)")
+		return fmt.Errorf("has an invalid extension (unknown media type)")
 	}
 
-	return fmt.Errorf("invalid extension for media type %s", clean.LogQuote(mimeType))
+	return fmt.Errorf("has an invalid extension for media type %s", clean.LogQuote(mimeType))
 }
 
 // Media returns the media content type (video, image, raw, sidecar,...).

pkg/fs/done.go

@@ -9,6 +9,19 @@ const (
 
 type Done map[string]Status
 
+// Processed counts the number of processed files.
+func (d Done) Processed() int {
+	count := 0
+
+	for _, s := range d {
+		if s.Processed() {
+			count++
+		}
+	}
+
+	return count
+}
+
 func (s Status) Exists() bool {
 	return s > 0
 }

pkg/fs/ext_list.go

@@ -111,10 +111,9 @@ func (b ExtList) Accept() string {
 
 	list := make([]string, 0, len(b))
 
-	for typeExt := range b {
-		allExt := FileTypesLower[FileType("."+typeExt)]
-		for _, s := range allExt {
-			list = append(list, s)
+	for s := range b {
+		if s = TrimExt(s); s != "" {
+			list = append(list, "."+s)
 		}
 	}
 

pkg/fs/ext_list_test.go

@@ -130,11 +130,15 @@ func TestExtList_String(t *testing.T) {
 func TestExtList_Accept(t *testing.T) {
 	t.Run("One", func(t *testing.T) {
 		list := NewExtList("jpg")
-		assert.Equal(t, ".jfi,.jfif,.jif,.jpe,.jpeg,.jpg", list.Accept())
+		assert.Equal(t, ".jpg", list.Accept())
 	})
 	t.Run("Two", func(t *testing.T) {
 		list := NewExtList("mp4, avi")
-		assert.Equal(t, ".avi,.mp,.mp4", list.Accept())
+		assert.Equal(t, ".avi,.mp4", list.Accept())
+	})
+	t.Run("Jpeg", func(t *testing.T) {
+		list := NewExtList("jpg,jpeg,jxl")
+		assert.Equal(t, ".jpeg,.jpg,.jxl", list.Accept())
 	})
 	t.Run("Empty", func(t *testing.T) {
 		list := NewExtList("")