Improve attachment filtering at receive and render time

Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com>
Co-authored-by: Backport Bot <backport-bot@signal.org>

ts/components/Lightbox.dom.tsx

@@ -161,7 +161,7 @@ export function Lightbox({
   const incrementalUrl = attachment?.incrementalUrl;
   const contentType = attachment?.contentType;
 
-  const isAttachmentGIF = isGIF(attachment ? [attachment] : undefined);
+  const isAttachmentGIF = isGIF(attachment);
   const isDownloading =
     attachment &&
     isIncremental(attachment) &&

ts/components/StoryImage.dom.tsx

@@ -128,7 +128,7 @@ export function StoryImage({
       />
     );
   } else if (!isThumbnail && isSupportedVideo) {
-    const shouldLoop = isGIF(attachment ? [attachment] : undefined);
+    const shouldLoop = isGIF(attachment);
 
     storyElement = (
       <video

ts/components/conversation/Message.dom.tsx

@@ -1204,7 +1204,7 @@ export class Message extends PureComponent<Props, State> {
           : null
       );
 
-      if (isGIF(attachments)) {
+      if (isGIF(firstAttachment)) {
         return (
           <div className={containerClassName}>
             {/* oxlint-disable-next-line react/jsx-pascal-case */}
@@ -2623,7 +2623,7 @@ export class Message extends PureComponent<Props, State> {
     }
 
     if (attachments && attachments.length) {
-      if (isGIF(attachments)) {
+      if (isGIF(attachments[0])) {
         // Message container border
         return GIF_SIZE + 2;
       }
@@ -2782,7 +2782,7 @@ export class Message extends PureComponent<Props, State> {
       detail = formatFileSize(firstAttachment.size);
     }
 
-    if (isVideo(attachments) || isGIF(attachments)) {
+    if (isVideo(attachments) || isGIF(firstAttachment)) {
       return {
         title: i18n('icu:Message--tap-to-view--video'),
         detail,
@@ -3339,7 +3339,7 @@ export class Message extends PureComponent<Props, State> {
 
     const containerClassnames = classNames(
       'module-message__container',
-      isGIF(attachments) && !isTapToView
+      isGIF(attachments?.[0]) && !isTapToView
         ? 'module-message__container--gif'
         : null,
       isTargeted ? 'module-message__container--targeted' : null,

ts/components/conversation/media-gallery/MediaGridItem.dom.tsx

@@ -164,7 +164,7 @@ function MetadataOverlay(props: MetadataOverlayProps): JSX.Element | undefined {
 
   if (
     status.state === 'ReadyToShow' &&
-    !isGIF([attachment]) &&
+    !isGIF(attachment) &&
     !isVideoAttachment(attachment) &&
     !showSize
   ) {
@@ -172,7 +172,7 @@ function MetadataOverlay(props: MetadataOverlayProps): JSX.Element | undefined {
   }
 
   let text: string;
-  if (!showSize && isGIF([attachment]) && status.state === 'ReadyToShow') {
+  if (!showSize && isGIF(attachment) && status.state === 'ReadyToShow') {
     text = i18n('icu:message--getNotificationText--gif');
   } else if (
     !showSize &&

ts/services/backups/export.preload.ts

@@ -3380,7 +3380,7 @@ export class BackupExportStream extends Readable {
 
       return Backups.MessageAttachment.Flag.VOICE_MESSAGE;
     }
-    if (isGIF([attachment])) {
+    if (isGIF(attachment)) {
       return Backups.MessageAttachment.Flag.GIF;
     }
     if (

ts/state/ducks/lightbox.preload.ts

@@ -19,6 +19,7 @@ import { createLogger } from '../../logging/log.std.ts';
 import { getMessageById } from '../../messages/getMessageById.preload.ts';
 import type { ReadonlyMessageAttributesType } from '../../model-types.d.ts';
 import {
+  getValidMessageAttachments,
   getUndownloadedAttachmentSignature,
   isIncremental,
 } from '../../util/Attachment.std.ts';
@@ -257,7 +258,7 @@ function showLightboxForViewOnceMedia(
 function filterValidAttachments(
   attributes: ReadonlyMessageAttributesType
 ): Array<AttachmentType> {
-  return (attributes.attachments ?? []).filter(
+  return getValidMessageAttachments(attributes.attachments ?? []).filter(
     item => (!item.pending || isIncremental(item)) && !item.error
   );
 }

ts/state/selectors/message.preload.ts

@@ -74,6 +74,7 @@ import {
   defaultBlurHash,
   isDownloadable,
   isDownloaded,
+  getValidMessageAttachments,
 } from '../../util/Attachment.std.ts';
 import type { MessageAttachmentType } from '../../types/AttachmentDownload.std.ts';
 import type {
@@ -341,18 +342,21 @@ export const getAttachmentsForMessage = (
       },
     ];
   }
-  return (
-    attachments
-      // Long message attachments are removed from message.attachments quickly,
-      // but in case they are still around, let's make sure not to show them
-      .filter(attachment => attachment.contentType !== LONG_MESSAGE)
-      .map(attachment =>
-        getPropsForAttachment(attachment, 'attachment', message, {
-          hasMediaBackups,
-        })
-      )
-      .filter(isNotNil)
-  );
+  return [
+    ...getValidMessageAttachments(
+      attachments
+        // Long message attachments are removed from message.attachments
+        // quickly, but in case they are still around, let's make sure not to
+        // show them
+        .filter(attachment => attachment.contentType !== LONG_MESSAGE)
+        .map(attachment =>
+          getPropsForAttachment(attachment, 'attachment', message, {
+            hasMediaBackups,
+          })
+        )
+        .filter(isNotNil)
+    ),
+  ];
 };
 
 export const processBodyRanges = (

ts/state/smart/PinnedMessagesBar.preload.tsx

@@ -45,7 +45,7 @@ function getPinMessageAttachment(
     return null;
   }
   const { contentType } = attachment;
-  if (contentType === MIME.IMAGE_GIF || Attachment.isGIF([attachment])) {
+  if (contentType === MIME.IMAGE_GIF || Attachment.isGIF(attachment)) {
     return { type: 'gif' };
   }
   if (Attachment.isImage([attachment])) {

ts/test-node/processDataMessage_test.preload.ts

@@ -5,12 +5,19 @@ import { assert } from 'chai';
 import { v4 as generateUuid } from 'uuid';
 
 import {
-  processDataMessage,
   ATTACHMENT_MAX,
+  processDataMessage,
 } from '../textsecure/processDataMessage.preload.ts';
 import type { ProcessedAttachment } from '../textsecure/Types.d.ts';
 import { SignalService as Proto } from '../protobuf/index.std.ts';
-import { IMAGE_GIF, IMAGE_JPEG, LONG_MESSAGE } from '../types/MIME.std.ts';
+import {
+  APPLICATION_OCTET_STREAM,
+  AUDIO_MP3,
+  IMAGE_GIF,
+  IMAGE_JPEG,
+  LONG_MESSAGE,
+  VIDEO_MP4,
+} from '../types/MIME.std.ts';
 import { toAciObject } from '../util/ServiceId.node.ts';
 import { uuidToBytes } from '../util/uuidToBytes.std.ts';
 import { generateAci } from '../test-helpers/serviceIdUtils.std.ts';
@@ -70,7 +77,7 @@ const UNPROCESSED_ATTACHMENT: Proto.AttachmentPointer.Params = {
   size: 34,
   height: 64,
   width: 128,
-  flags: 1,
+  flags: 0,
   fileName: 'fileName',
   thumbnail: null,
 };
@@ -91,10 +98,24 @@ const PROCESSED_ATTACHMENT: ProcessedAttachment = {
   uploadTimestamp: 456,
   height: 64,
   width: 128,
-  flags: 1,
+  flags: 0,
   fileName: 'fileName',
 };
 
+const IMAGE = { contentType: IMAGE_JPEG };
+const VIDEO = { contentType: VIDEO_MP4 };
+const FILE = { contentType: APPLICATION_OCTET_STREAM };
+const AUDIO = { contentType: AUDIO_MP3 };
+const LONG_TEXT = { contentType: LONG_MESSAGE };
+const VOICE = {
+  contentType: AUDIO_MP3,
+  flags: Proto.AttachmentPointer.Flags.VOICE_MESSAGE,
+};
+const GIF = {
+  contentType: VIDEO_MP4,
+  flags: Proto.AttachmentPointer.Flags.GIF,
+};
+
 describe('processDataMessage', () => {
   const check = (
     message: Partial<Omit<Proto.DataMessage.Params, 'timestamp'>>
@@ -113,116 +134,206 @@ describe('processDataMessage', () => {
       }
     );
 
+  const unprocessed = (
+    overrides?: Partial<Proto.AttachmentPointer.Params>
+  ): Proto.AttachmentPointer.Params => ({
+    ...UNPROCESSED_ATTACHMENT,
+    flags: 0,
+    ...overrides,
+  });
+
+  const processed = (
+    overrides?: Partial<ProcessedAttachment>
+  ): ProcessedAttachment => ({
+    ...PROCESSED_ATTACHMENT,
+    flags: 0,
+    downloadPath: 'random-path',
+    ...overrides,
+  });
+
   it('should process attachments', () => {
     const out = check({
-      attachments: [UNPROCESSED_ATTACHMENT],
+      attachments: [unprocessed()],
     });
 
-    assert.deepStrictEqual(out.attachments, [
-      {
-        ...PROCESSED_ATTACHMENT,
-        downloadPath: 'random-path',
-      },
-    ]);
+    assert.deepStrictEqual(out.attachments, [processed()]);
   });
 
   it('should process attachments with null fileName', () => {
     const out = check({
-      attachments: [
-        {
-          ...UNPROCESSED_ATTACHMENT,
-          fileName: null,
-        },
-      ],
+      attachments: [unprocessed({ fileName: null })],
     });
 
     assert.deepStrictEqual(out.attachments, [
-      {
-        ...PROCESSED_ATTACHMENT,
-        fileName: undefined,
-        downloadPath: 'random-path',
-      },
+      processed({ fileName: undefined }),
     ]);
   });
 
   it('should process attachments with 0 cdnId', () => {
     const out = check({
       attachments: [
-        {
-          ...UNPROCESSED_ATTACHMENT,
+        unprocessed({
           attachmentIdentifier: {
             cdnId: 0n,
           },
-        },
+        }),
       ],
     });
 
     assert.deepStrictEqual(out.attachments, [
-      {
-        ...PROCESSED_ATTACHMENT,
+      processed({
         cdnId: undefined,
         cdnKey: undefined,
-        downloadPath: 'random-path',
-      },
+      }),
     ]);
   });
 
   it('should move long text attachments to bodyAttachment', () => {
     const out = check({
-      attachments: [
-        UNPROCESSED_ATTACHMENT,
-        {
-          ...UNPROCESSED_ATTACHMENT,
-          contentType: LONG_MESSAGE,
-        },
-      ],
+      attachments: [unprocessed(), unprocessed(LONG_TEXT)],
     });
 
-    assert.deepStrictEqual(out.attachments, [
-      {
-        ...PROCESSED_ATTACHMENT,
-        downloadPath: 'random-path',
-      },
-    ]);
-    assert.deepStrictEqual(out.bodyAttachment, {
-      ...PROCESSED_ATTACHMENT,
-      downloadPath: 'random-path',
-      contentType: LONG_MESSAGE,
-    });
+    assert.deepStrictEqual(out.attachments, [processed()]);
+    assert.deepStrictEqual(out.bodyAttachment, processed(LONG_TEXT));
+  });
+
+  it('caps the number of attachments at ATTACHMENT_MAX', () => {
+    const attachments: Array<Proto.AttachmentPointer.Params> = [];
+    for (let i = 0; i < ATTACHMENT_MAX + 5; i += 1) {
+      attachments.push(unprocessed(IMAGE));
+    }
+
+    const out = check({ attachments });
+
+    assert.equal(out.attachments.length, ATTACHMENT_MAX);
+    assert.deepStrictEqual(
+      out.attachments,
+      Array.from({ length: ATTACHMENT_MAX }, () => processed(IMAGE))
+    );
+  });
+
+  it('allows long text + ATTACHMENT_MAX attachments', () => {
+    const attachments: Array<Proto.AttachmentPointer.Params> = [
+      unprocessed(LONG_TEXT),
+    ];
+    for (let i = 0; i < ATTACHMENT_MAX; i += 1) {
+      attachments.push(unprocessed(IMAGE));
+    }
+
+    const out = check({ attachments });
+
+    assert.deepStrictEqual(out.bodyAttachment, processed(LONG_TEXT));
+    assert.deepStrictEqual(
+      out.attachments,
+      Array.from({ length: ATTACHMENT_MAX }, () => processed(IMAGE))
+    );
   });
 
   it('should process attachments with incrementalMac/chunkSize', () => {
     const out = check({
       attachments: [
-        {
-          ...UNPROCESSED_ATTACHMENT,
+        unprocessed({
           incrementalMac: new Uint8Array([0, 0, 0]),
           chunkSize: 2,
-        },
+        }),
       ],
     });
 
     assert.deepStrictEqual(out.attachments, [
-      {
-        ...PROCESSED_ATTACHMENT,
-        downloadPath: 'random-path',
+      processed({
         incrementalMac: 'AAAA',
         chunkSize: 2,
-      },
+      }),
     ]);
   });
 
-  it('should throw on too many attachments', () => {
-    const attachments: Array<Proto.AttachmentPointer.Params> = [];
-    for (let i = 0; i < ATTACHMENT_MAX + 1; i += 1) {
-      attachments.push(UNPROCESSED_ATTACHMENT);
-    }
+  describe('drops attachments the UI would never render', () => {
+    it('keeps only the voice message', () => {
+      const out = check({
+        attachments: [unprocessed(VOICE), unprocessed(FILE)],
+      });
 
-    assert.throws(
-      () => check({ attachments }),
-      `Too many attachments: ${ATTACHMENT_MAX + 1} included in one message` +
-        `, max is ${ATTACHMENT_MAX}`
-    );
+      assert.deepStrictEqual(out.attachments, [processed(VOICE)]);
+    });
+
+    it('keeps only the first audio attachment', () => {
+      const out = check({
+        attachments: [
+          unprocessed(AUDIO),
+          unprocessed(AUDIO),
+          unprocessed(IMAGE),
+        ],
+      });
+
+      assert.deepStrictEqual(out.attachments, [processed(AUDIO)]);
+    });
+
+    it('keeps only GIF (rendered alone)', () => {
+      const out = check({
+        attachments: [unprocessed(GIF), unprocessed(IMAGE)],
+      });
+
+      assert.deepStrictEqual(out.attachments, [processed(GIF)]);
+    });
+
+    it('keeps only the first file attachment', () => {
+      const out = check({
+        attachments: [unprocessed(FILE), unprocessed(FILE)],
+      });
+
+      assert.deepStrictEqual(out.attachments, [processed(FILE)]);
+    });
+
+    it('keeps only the file when it leads visual media', () => {
+      const out = check({
+        attachments: [unprocessed(FILE), unprocessed(IMAGE)],
+      });
+
+      assert.deepStrictEqual(out.attachments, [processed(FILE)]);
+    });
+
+    it('keeps the leading run of visual media', () => {
+      const out = check({
+        attachments: [
+          unprocessed(IMAGE),
+          unprocessed(VIDEO),
+          unprocessed(FILE),
+        ],
+      });
+
+      assert.deepStrictEqual(out.attachments, [
+        processed(IMAGE),
+        processed(VIDEO),
+      ]);
+    });
+
+    it('stops at the first non-visual attachment', () => {
+      const out = check({
+        attachments: [
+          unprocessed(IMAGE),
+          unprocessed(FILE),
+          unprocessed(IMAGE),
+        ],
+      });
+
+      assert.deepStrictEqual(out.attachments, [processed(IMAGE)]);
+    });
+
+    it('keeps every attachment when they are all visual', () => {
+      const out = check({
+        attachments: [
+          unprocessed(IMAGE),
+          unprocessed(VIDEO),
+          unprocessed(IMAGE),
+        ],
+      });
+
+      assert.deepStrictEqual(out.attachments, [
+        processed(IMAGE),
+        processed(VIDEO),
+        processed(IMAGE),
+      ]);
+    });
   });
 
   it('should process groupv2 context', () => {

ts/textsecure/MessageReceiver.preload.ts

@@ -2205,6 +2205,14 @@ export default class MessageReceiver
       });
     }
 
+    if (attachments.length > 1) {
+      log.warn(
+        `${logId}: story has ${attachments.length} attachments; ` +
+          'dropping all but the first'
+      );
+      attachments.splice(1);
+    }
+
     const groupV2 = msg.group ? processGroupV2Context(msg.group) : undefined;
     if (groupV2 && this.#isGroupBlocked(groupV2.id)) {
       log.warn(`${logId}: ignored; destined for blocked group`);

ts/textsecure/processDataMessage.preload.ts

@@ -51,7 +51,11 @@ import { PaymentEventKind } from '../types/Payment.std.ts';
 import { filterAndClean } from '../util/BodyRange.node.ts';
 import { bytesToUuid } from '../util/uuidToBytes.std.ts';
 import { createName } from '../util/attachmentPath.node.ts';
-import { partitionBodyAndNormalAttachments } from '../util/Attachment.std.ts';
+import {
+  getMessageAttachmentClass,
+  getValidMessageAttachments,
+  partitionBodyAndNormalAttachments,
+} from '../util/Attachment.std.ts';
 import { isNotNil } from '../util/isNotNil.std.ts';
 import { createLogger } from '../logging/log.std.ts';
 
@@ -546,15 +550,25 @@ export function processDataMessage(
     }))
     .filter(isNotNil);
 
+  const logId = `processDataMessage(${timestamp})`;
+
   const { bodyAttachment, attachments } = partitionBodyAndNormalAttachments(
     { attachments: processedAttachments ?? [] },
-    { logId: `processDataMessage(${timestamp})` }
+    { logId }
   );
 
+  const renderableAttachments = getValidMessageAttachments(attachments);
+  if (attachments[0] && renderableAttachments.length < attachments.length) {
+    log.warn(
+      `${logId}: message leads with ${getMessageAttachmentClass(attachments)} but ` +
+        `has ${attachments.length} attachments; dropping ` +
+        `${attachments.length - renderableAttachments.length}`
+    );
+  }
   const result: ProcessedDataMessage = {
     body: message.body ?? '',
     bodyAttachment,
-    attachments,
+    attachments: renderableAttachments,
     groupV2: processGroupV2Context(message.groupV2),
     flags: message.flags ?? 0,
     expireTimer: DurationInSeconds.fromSeconds(message.expireTimer ?? 0),
@@ -625,10 +639,12 @@ export function processDataMessage(
 
   const attachmentCount = result.attachments.length;
   if (attachmentCount > ATTACHMENT_MAX) {
-    throw new Error(
+    log.warn(
       `Too many attachments: ${attachmentCount} included in one message, ` +
         `max is ${ATTACHMENT_MAX}`
     );
+
+    result.attachments = result.attachments.slice(0, ATTACHMENT_MAX);
   }
 
   return result;

ts/util/Attachment.std.ts

@@ -39,6 +39,7 @@ const {
   isString,
   omit,
   partition,
+  takeWhile,
 } = lodash;
 
 const logging = createLogger('Attachment');
@@ -325,13 +326,11 @@ export function isVideoAttachment(
   return isVideoTypeSupported(attachment.contentType);
 }
 
-export function isGIF(attachments?: ReadonlyArray<AttachmentType>): boolean {
-  if (!attachments || attachments.length !== 1) {
+export function isGIF(attachment?: AttachmentType): boolean {
+  if (!attachment) {
     return false;
   }
 
-  const [attachment] = attachments;
-
   const flag = SignalService.AttachmentPointer.Flags.GIF;
   const hasFlag =
     // oxlint-disable-next-line no-bitwise
@@ -899,6 +898,51 @@ export function partitionBodyAndNormalAttachments<
   };
 }
 
+export function getMessageAttachmentClass(
+  attachments: ReadonlyArray<AttachmentType>
+): 'file' | 'visual-media' | 'gif' | 'audio' | 'voice' | 'unknown' {
+  const first = attachments[0];
+  if (!first) {
+    return 'unknown';
+  }
+  if (isVoiceMessage(first)) {
+    return 'voice';
+  }
+  if (isGIF(first)) {
+    return 'gif';
+  }
+  if (isAudio(attachments)) {
+    return 'audio';
+  }
+  if (isImageAttachment(first) || isVideoAttachment(first)) {
+    return 'visual-media';
+  }
+  return 'file';
+}
+
+export function getValidMessageAttachments(
+  attachments: ReadonlyArray<AttachmentType>
+): ReadonlyArray<AttachmentType> {
+  if (attachments.length <= 1) {
+    return attachments;
+  }
+  const [first] = attachments;
+  if (first == null) {
+    return [];
+  }
+
+  if (getMessageAttachmentClass(attachments) === 'visual-media') {
+    return takeWhile(
+      attachments,
+      attachment =>
+        isImageAttachment(attachment) || isVideoAttachment(attachment)
+    );
+  }
+
+  // For all non-visual-media attachments, we only show the first attachment
+  return [first];
+}
+
 const MESSAGE_ATTACHMENT_TYPES_NEEDING_THUMBNAILS =
   new Set<MessageAttachmentType>(['attachment', 'sticker']);
 

ts/util/getNotificationDataForMessage.preload.ts

@@ -374,7 +374,7 @@ export function getNotificationDataForMessage(
       };
     }
 
-    if (contentType === MIME.IMAGE_GIF || Attachment.isGIF(attachments)) {
+    if (contentType === MIME.IMAGE_GIF || Attachment.isGIF(attachment)) {
       return {
         bodyRanges,
         emoji: Emoji.FERRIS_WHEEL,

ts/util/getStoryDuration.dom.ts

@@ -44,7 +44,7 @@ export async function getStoryDuration(
     return;
   }
 
-  if (isGIF([attachment]) || isVideo([attachment])) {
+  if (isGIF(attachment) || isVideo([attachment])) {
     const videoEl = document.createElement('video');
     const { url } = attachment;
 
@@ -77,7 +77,7 @@ export async function getStoryDuration(
       videoEl.load();
     }
 
-    if (isGIF([attachment])) {
+    if (isGIF(attachment)) {
       // GIFs: Loop gifs 3 times or play for 5 seconds, whichever is longer.
       return Math.min(
         Math.max(duration * 3, DEFAULT_DURATION),

ts/util/getStoryReplyText.std.ts

@@ -19,7 +19,7 @@ export function getStoryReplyText(
     return i18n('icu:message--getNotificationText--photo');
   }
 
-  if (isGIF(attachments)) {
+  if (isGIF(attachment)) {
     return i18n('icu:message--getNotificationText--gif');
   }
 

ts/util/makeQuote.preload.ts

@@ -73,7 +73,7 @@ export async function getQuoteAttachment(
 ): Promise<Array<QuotedAttachmentType>> {
   if (attachments && attachments.length) {
     const attachmentsToUse = Array.from(take(attachments, 1));
-    const isGIFQuote = isGIF(attachmentsToUse);
+    const isGIFQuote = isGIF(attachmentsToUse[0]);
 
     return Promise.all(
       map(attachmentsToUse, async attachment => {