Repair broken attachments with non-array 'data' fields

ts/messages/helpers.ts

@@ -9,7 +9,22 @@ import type {
   QuotedMessageType,
 } from '../model-types.d';
 import type { UUIDStringType } from '../types/UUID';
-import { isIncoming, isOutgoing, isStory } from '../state/selectors/message';
+
+export function isIncoming(
+  message: Pick<MessageAttributesType, 'type'>
+): boolean {
+  return message.type === 'incoming';
+}
+
+export function isOutgoing(
+  message: Pick<MessageAttributesType, 'type'>
+): boolean {
+  return message.type === 'outgoing';
+}
+
+export function isStory(message: Pick<MessageAttributesType, 'type'>): boolean {
+  return message.type === 'story';
+}
 
 export function isQuoteAMatch(
   message: MessageAttributesType | null | undefined,

ts/models/messages.ts

@@ -2511,6 +2511,7 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
                     await Attachment.migrateDataToFileSystem(downloadedAvatar, {
                       writeNewAttachmentData:
                         window.Signal.Migrations.writeNewAttachmentData,
+                      logger: log,
                     });
                   avatar = {
                     ...onDiskAttachment,

ts/sql/Client.ts

@@ -18,6 +18,7 @@ import {
   get,
   groupBy,
   isFunction,
+  isTypedArray,
   last,
   map,
   omit,
@@ -90,6 +91,7 @@ import type {
 import Server from './Server';
 import { isCorruptionError } from './errors';
 import { MINUTE } from '../util/durations';
+import { getMessageIdForLogging } from '../util/idForLogging';
 
 // We listen to a lot of events on ipc, often on the same channel. This prevents
 //   any warnings that might be sent to the console in that case.
@@ -433,12 +435,25 @@ function _cleanData(
   return cleaned;
 }
 
-function _cleanMessageData(data: MessageType): MessageType {
+export function _cleanMessageData(data: MessageType): MessageType {
   // Ensure that all messages have the received_at set properly
   if (!data.received_at) {
     assert(false, 'received_at was not set on the message');
     data.received_at = window.Signal.Util.incrementMessageCounter();
   }
+  if (data.attachments) {
+    const logId = getMessageIdForLogging(data);
+    data.attachments = data.attachments.map((attachment, index) => {
+      if (attachment.data && !isTypedArray(attachment.data)) {
+        log.warn(
+          `_cleanMessageData/${logId}: Attachment ${index} had non-array \`data\` field; deleting.`
+        );
+        return omit(attachment, ['data']);
+      }
+
+      return attachment;
+    });
+  }
   return _cleanData(omit(data, ['dataMessage']));
 }
 

ts/state/selectors/message.ts

@@ -93,6 +93,9 @@ import * as log from '../../logging/log';
 import { getConversationColorAttributes } from '../../util/getConversationColorAttributes';
 import { DAY, HOUR } from '../../util/durations';
 import { getStoryReplyText } from '../../util/getStoryReplyText';
+import { isIncoming, isOutgoing, isStory } from '../../messages/helpers';
+
+export { isIncoming, isOutgoing, isStory };
 
 const THREE_HOURS = 3 * HOUR;
 const linkify = LinkifyIt();
@@ -129,24 +132,6 @@ export type GetPropsForBubbleOptions = Readonly<{
   contactNameColorSelector: ContactNameColorSelectorType;
 }>;
 
-export function isIncoming(
-  message: Pick<MessageWithUIFieldsType, 'type'>
-): boolean {
-  return message.type === 'incoming';
-}
-
-export function isOutgoing(
-  message: Pick<MessageWithUIFieldsType, 'type'>
-): boolean {
-  return message.type === 'outgoing';
-}
-
-export function isStory(
-  message: Pick<MessageWithUIFieldsType, 'type'>
-): boolean {
-  return message.type === 'story';
-}
-
 export function hasErrors(
   message: Pick<MessageWithUIFieldsType, 'errors'>
 ): boolean {

ts/test-both/sql/cleanMessageData_test.ts

@@ -0,0 +1,51 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { assert } from 'chai';
+
+import { _cleanMessageData } from '../../sql/Client';
+import { IMAGE_GIF } from '../../types/MIME';
+
+describe('_cleanMessageData', () => {
+  it('throws if message is missing received_at', () => {
+    assert.throws(() => {
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      _cleanMessageData({} as any);
+    }, 'received_at');
+  });
+
+  it('removes `data` field in attachment if it is not a typed array', () => {
+    const data = new Uint8Array([1, 2, 3]);
+    const message = {
+      id: 'something',
+      type: 'incoming' as const,
+      sent_at: Date.now(),
+      conversationId: 'conversation-id',
+      received_at: Date.now(),
+      timestamp: Date.now(),
+      attachments: [
+        {
+          contentType: IMAGE_GIF,
+          size: 4,
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          data: 1 as any,
+        },
+        {
+          contentType: IMAGE_GIF,
+          size: 4,
+          data: {},
+        },
+        {
+          size: 4,
+          contentType: IMAGE_GIF,
+          data,
+        },
+      ],
+    };
+    const actual = _cleanMessageData(message);
+
+    assert.isUndefined(actual.attachments?.[0].data);
+    assert.isUndefined(actual.attachments?.[1].data);
+    assert.strictEqual(actual.attachments?.[2].data, data);
+  });
+});

ts/test-node/types/Attachment_test.ts

@@ -396,6 +396,7 @@ describe('Attachment', () => {
 
       const actual = await Attachment.migrateDataToFileSystem(input, {
         writeNewAttachmentData,
+        logger,
       });
       assert.deepEqual(actual, expected);
     });
@@ -417,11 +418,12 @@ describe('Attachment', () => {
 
       const actual = await Attachment.migrateDataToFileSystem(input, {
         writeNewAttachmentData,
+        logger,
       });
       assert.deepEqual(actual, expected);
     });
 
-    it('should throw error if data is not valid', async () => {
+    it('should clear `data` field if it is not a typed array', async () => {
       const input = {
         contentType: MIME.IMAGE_JPEG,
         // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -432,12 +434,12 @@ describe('Attachment', () => {
 
       const writeNewAttachmentData = async () => 'abc/abcdefgh123456789';
 
-      await assert.isRejected(
-        Attachment.migrateDataToFileSystem(input, {
-          writeNewAttachmentData,
-        }),
-        'Expected `attachment.data` to be a typed array; got: number'
-      );
+      const actual = await Attachment.migrateDataToFileSystem(input, {
+        writeNewAttachmentData,
+        logger,
+      });
+
+      assert.isUndefined(actual.data);
     });
   });
 });

ts/test-node/types/EmbeddedContact_test.ts

@@ -4,6 +4,7 @@
 import { assert } from 'chai';
 import * as sinon from 'sinon';
 
+import * as logger from '../../logging/log';
 import { IMAGE_GIF, IMAGE_PNG } from '../../types/MIME';
 import type { MessageAttributesType } from '../../model-types.d';
 import type { Avatar, Email, Phone } from '../../types/EmbeddedContact';
@@ -18,9 +19,6 @@ import { UUID } from '../../types/UUID';
 
 describe('Contact', () => {
   const NUMBER = '+12025550099';
-  const logger = {
-    error: () => undefined,
-  };
 
   const writeNewAttachmentData = sinon
     .stub()

ts/types/Attachment.ts

@@ -180,8 +180,10 @@ export async function migrateDataToFileSystem(
   attachment: AttachmentType,
   {
     writeNewAttachmentData,
+    logger,
   }: {
     writeNewAttachmentData: (data: Uint8Array) => Promise<string>;
+    logger: LoggerType;
   }
 ): Promise<AttachmentType> {
   if (!isFunction(writeNewAttachmentData)) {
@@ -195,11 +197,12 @@ export async function migrateDataToFileSystem(
     return attachment;
   }
 
+  // This attachment was already broken by a roundtrip to the database - repair it now
   if (!isTypedArray(data)) {
-    throw new TypeError(
-      'Expected `attachment.data` to be a typed array;' +
-        ` got: ${typeof attachment.data}`
+    logger.warn(
+      'migrateDataToFileSystem: Attachment had non-array `data` field; deleting.'
     );
+    return omit({ ...attachment }, ['data']);
   }
 
   const path = await writeNewAttachmentData(data);

ts/types/EmbeddedContact.ts

@@ -192,7 +192,7 @@ export function parseAndWriteAvatar(
     context: {
       message: MessageAttributesType;
       getRegionCode: () => string | undefined;
-      logger: Pick<LoggerType, 'error'>;
+      logger: LoggerType;
       writeNewAttachmentData: (data: Uint8Array) => Promise<string>;
     }
   ): Promise<EmbeddedContactType> => {

ts/types/Message2.ts

@@ -589,6 +589,7 @@ export const processNewAttachment = async (
   );
   const onDiskAttachment = await migrateDataToFileSystem(rotatedAttachment, {
     writeNewAttachmentData,
+    logger,
   });
   const finalAttachment = await captureDimensionsAndScreenshot(
     onDiskAttachment,