Drop NORMAL-type quote-replies without any content

signalapp · Jan 9, 2025 · 03f9f1a · 03f9f1a
1 parent 1d5cfc0
commit 03f9f1a
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 15 deletions.
diff --git a/...lServiceKit/MessageBackup/Archivers/ChatItem/MessageBackupTSMessageContentsArchiver.swift b/...lServiceKit/MessageBackup/Archivers/ChatItem/MessageBackupTSMessageContentsArchiver.swift
@@ -353,21 +353,18 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
         }
 
         if let quotedMessage = message.quotedMessage {
-            let quote: BackupProto_Quote
             let quoteResult = archiveQuote(
                 quotedMessage,
                 interactionUniqueId: message.uniqueInteractionId,
                 messageRowId: messageRowId,
                 context: context
             )
             switch quoteResult.bubbleUp(ChatItemType.self, partialErrors: &partialErrors) {
-            case .continue(let _quote):
-                quote = _quote
+            case .continue(let quote):
+                quote.map { standardMessage.quote = $0 }
             case .bubbleUpError(let errorResult):
                 return errorResult
             }
-
-            standardMessage.quote = quote
         }
 
         if let linkPreview = message.linkPreview {
@@ -448,7 +445,7 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
         interactionUniqueId: MessageBackup.InteractionUniqueId,
         messageRowId: Int64,
         context: MessageBackup.RecipientArchivingContext
-    ) -> ArchiveInteractionResult<BackupProto_Quote> {
+    ) -> ArchiveInteractionResult<BackupProto_Quote?> {
         var partialErrors = [ArchiveFrameError]()
 
         guard let authorAddress = quotedMessage.authorAddress.asSingleServiceIdBackupAddress() else {
@@ -465,13 +462,6 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
 
         var quote = BackupProto_Quote()
         quote.authorID = authorId.value
-        if quotedMessage.isGiftBadge {
-            quote.type = .giftBadge
-        } else if quotedMessage.isTargetMessageViewOnce {
-            quote.type = .viewOnce
-        } else {
-            quote.type = .normal
-        }
 
         let targetSentTimestamp: UInt64? = {
             switch quotedMessage.bodySource {
@@ -490,13 +480,16 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
             quote.targetSentTimestamp = targetSentTimestamp
         }
 
+        var didArchiveText = false
+        var didArchiveAttachments = false
+
         if let body = quotedMessage.body {
             let textResult = archiveText(
                 MessageBody(text: body, ranges: quotedMessage.bodyRanges ?? .empty),
                 interactionUniqueId: interactionUniqueId
             )
             let text: BackupProto_Text
-            switch textResult.bubbleUp(BackupProto_Quote.self, partialErrors: &partialErrors) {
+            switch textResult.bubbleUp(Optional<BackupProto_Quote>.self, partialErrors: &partialErrors) {
             case .continue(let value):
                 text = value
             case .bubbleUpError(let errorResult):
@@ -509,6 +502,8 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
                 quoteText.bodyRanges = text.bodyRanges
                 return quoteText
             }()
+
+            didArchiveText = true
         }
 
         if let attachmentInfo = quotedMessage.attachmentInfo() {
@@ -518,12 +513,33 @@ class MessageBackupTSMessageContentsArchiver: MessageBackupProtoArchiver {
                 messageRowId: messageRowId,
                 context: context
             )
-            switch quoteAttachmentResult.bubbleUp(BackupProto_Quote.self, partialErrors: &partialErrors) {
+            switch quoteAttachmentResult.bubbleUp(Optional<BackupProto_Quote>.self, partialErrors: &partialErrors) {
             case .continue(let quoteAttachmentProto):
                 quote.attachments = [quoteAttachmentProto]
             case .bubbleUpError(let errorResult):
                 return errorResult
             }
+
+            didArchiveAttachments = true
+        }
+
+        if quotedMessage.isGiftBadge {
+            quote.type = .giftBadge
+        } else if quotedMessage.isTargetMessageViewOnce {
+            quote.type = .viewOnce
+        } else {
+            guard didArchiveText || didArchiveAttachments else {
+                // NORMAL-type quotes must have either text or attachments, lest
+                // they be rejected by the validator.
+                partialErrors.append(.archiveFrameError(
+                    .quoteTypeNormalMissingTextAndAttachments,
+                    interactionUniqueId
+                ))
+
+                return .partialFailure(nil, partialErrors)
+            }
+
+            quote.type = .normal
         }
 
         if partialErrors.isEmpty {

diff --git a/SignalServiceKit/MessageBackup/Archivers/MessageBackupErrors.swift b/SignalServiceKit/MessageBackup/Archivers/MessageBackupErrors.swift
@@ -69,6 +69,9 @@ extension MessageBackup {
             /// causing the containing message to be skipped.
             case invalidQuoteAuthor
 
+            /// A quote of type `NORMAL` was missing both text and attachments.
+            case quoteTypeNormalMissingTextAndAttachments
+
             /// A link preview is missing its url
             case linkPreviewMissingUrl
             /// A link preview's URL isn't in the message body
@@ -262,6 +265,7 @@ extension MessageBackup {
                     .incomingMessageFromSelf,
                     .invalidOutgoingMessageRecipient,
                     .invalidQuoteAuthor,
+                    .quoteTypeNormalMissingTextAndAttachments,
                     .linkPreviewMissingUrl,
                     .linkPreviewUrlNotInBody,
                     .stickerMessageMissingStickerAttachment,
@@ -373,6 +377,10 @@ extension MessageBackup {
                 // (see `hasRenderableChanges`), so drop them from the backup
                 // with a warning but not an error.
                 return .warning
+            case .quoteTypeNormalMissingTextAndAttachments:
+                // We've seen real-world databases with "empty" quotes; we will
+                // drop the quote on export and issue a warning.
+                return .warning
             case .linkPreviewUrlNotInBody:
                 // We've seen real world databases with invalid link previews; we
                 // just drop these on export and just issue a warning.