Include file extension checks in attachment API (#32151)

From testing, I found that issue posters and users with repository write
access are able to edit attachment names in a way that circumvents the
instance-level file extension restrictions using the edit attachment
APIs. This snapshot adds checks for these endpoints.

2 files changed, 26 insertions, 6 deletions

diff --git a/services/attachment/attachment.go b/services/attachment/attachment.go
index 0fd51e4fa5..ccb97c66c8 100644
--- a/services/attachment/attachment.go
+++ b/services/attachment/attachment.go
@@ -50,3 +50,12 @@ func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string,
 
 	return NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), file), fileSize)
 }
+
+// UpdateAttachment updates an attachment, verifying that its name is among the allowed types.
+func UpdateAttachment(ctx context.Context, allowedTypes string, attach *repo_model.Attachment) error {
+	if err := upload.Verify(nil, attach.Name, allowedTypes); err != nil {
+		return err
+	}
+
+	return repo_model.UpdateAttachment(ctx, attach)
+}
diff --git a/services/context/upload/upload.go b/services/context/upload/upload.go
index 7123420e99..cefd13ebb6 100644
--- a/services/context/upload/upload.go
+++ b/services/context/upload/upload.go
@@ -28,12 +28,13 @@ func IsErrFileTypeForbidden(err error) bool {
 }
 
 func (err ErrFileTypeForbidden) Error() string {
-	return "This file extension or type is not allowed to be uploaded."
+	return "This file cannot be uploaded or modified due to a forbidden file extension or type."
 }
 
 var wildcardTypeRe = regexp.MustCompile(`^[a-z]+/\*$`)
 
-// Verify validates whether a file is allowed to be uploaded.
+// Verify validates whether a file is allowed to be uploaded. If buf is empty, it will just check if the file
+// has an allowed file extension.
 func Verify(buf []byte, fileName, allowedTypesStr string) error {
 	allowedTypesStr = strings.ReplaceAll(allowedTypesStr, "|", ",") // compat for old config format
 
@@ -56,21 +57,31 @@ func Verify(buf []byte, fileName, allowedTypesStr string) error {
 		return ErrFileTypeForbidden{Type: fullMimeType}
 	}
 	extension := strings.ToLower(path.Ext(fileName))
+	isBufEmpty := len(buf) <= 1
 
 	// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#Unique_file_type_specifiers
 	for _, allowEntry := range allowedTypes {
 		if allowEntry == "*/*" {
 			return nil // everything allowed
-		} else if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
+		}
+		if strings.HasPrefix(allowEntry, ".") && allowEntry == extension {
 			return nil // extension is allowed
-		} else if mimeType == allowEntry {
+		}
+		if isBufEmpty {
+			continue // skip mime type checks if buffer is empty
+		}
+		if mimeType == allowEntry {
 			return nil // mime type is allowed
-		} else if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
+		}
+		if wildcardTypeRe.MatchString(allowEntry) && strings.HasPrefix(mimeType, allowEntry[:len(allowEntry)-1]) {
 			return nil // wildcard match, e.g. image/*
 		}
 	}
 
-	log.Info("Attachment with type %s blocked from upload", fullMimeType)
+	if !isBufEmpty {
+		log.Info("Attachment with type %s blocked from upload", fullMimeType)
+	}
+
 	return ErrFileTypeForbidden{Type: fullMimeType}
 }