Check validity of link previews on import

Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com>

.github/workflows/ci.yml

@@ -194,7 +194,7 @@ jobs:
       uses: actions/checkout@v4
       with:
         repository: 'signalapp/Signal-Message-Backup-Tests'
-        ref: 'afa66e77fe2b437024ec0914cd14c02c95e7cbed'
+        ref: '05062de9656c5ed7c7e6c6a49897b42e7ad083fc'
         path: 'backup-integration-tests'
 
     - run: xvfb-run --auto-servernum npm run test-electron

ts/messages/handleDataMessage.ts

@@ -516,19 +516,12 @@ export async function handleDataMessage(
             };
           }
 
-          if (!item.image && !item.title) {
-            return null;
-          }
-          // Story link previews don't have to correspond to links in the
-          // message body.
-          if (isStory(message.attributes)) {
-            return item;
-          }
           if (
-            !urls.includes(item.url) ||
+            !LinkPreview.isValidLinkPreview(urls, item, {
-            !LinkPreview.shouldPreviewHref(item.url)
+              isStory: isStory(message.attributes),
+            })
           ) {
-            return undefined;
+            return null;
           }
 
           return item;

ts/services/backups/import.ts

@@ -21,6 +21,7 @@ import * as log from '../../logging/log';
 import { GiftBadgeStates } from '../../components/conversation/Message';
 import { StorySendMode, MY_STORY_ID } from '../../types/Stories';
 import type { AciString, ServiceIdString } from '../../types/ServiceId';
+import * as LinkPreview from '../../types/LinkPreview';
 import {
   fromAciObject,
   fromPniObject,
@@ -129,6 +130,7 @@ import { ToastType } from '../../types/Toast';
 import { isConversationAccepted } from '../../util/isConversationAccepted';
 import { saveBackupsSubscriberData } from '../../util/backupSubscriptionData';
 import { postSaveUpdates } from '../../util/cleanup';
+import type { LinkPreviewType } from '../../types/message/LinkPreviews';
 
 const MAX_CONCURRENCY = 10;
 
@@ -1429,7 +1431,10 @@ export class BackupImportStream extends Writable {
     if (item.standardMessage) {
       attributes = {
         ...attributes,
-        ...(await this.#fromStandardMessage(item.standardMessage)),
+        ...(await this.#fromStandardMessage({
+          logId,
+          data: item.standardMessage,
+        })),
       };
     } else if (item.viewOnceMessage) {
       attributes = {
@@ -1471,7 +1476,11 @@ export class BackupImportStream extends Writable {
         `${logId}: Only standard message can have revisions`
       );
 
-      const history = await this.#fromRevisions(attributes, item.revisions);
+      const history = await this.#fromRevisions({
+        mainMessage: attributes,
+        revisions: item.revisions,
+        logId,
+      });
       attributes.editHistory = history;
 
       // Update timestamps on the parent message
@@ -1737,9 +1746,13 @@ export class BackupImportStream extends Writable {
     return true;
   }
 
-  async #fromStandardMessage(
+  async #fromStandardMessage({
-    data: Backups.IStandardMessage
+    logId,
-  ): Promise<Partial<MessageAttributesType>> {
+    data,
+  }: {
+    logId: string;
+    data: Backups.IStandardMessage;
+  }): Promise<Partial<MessageAttributesType>> {
     return {
       body: data.text?.body || undefined,
       bodyRanges: this.#fromBodyRanges(data.text),
@@ -1752,18 +1765,10 @@ export class BackupImportStream extends Writable {
             .filter(isNotNil)
         : undefined,
       preview: data.linkPreview?.length
-        ? data.linkPreview.map(preview => {
+        ? this.#fromLinkPreview({
-            const { url } = preview;
+            logId,
-            strictAssert(url, 'preview must have a URL');
+            body: data.text?.body,
-            return {
+            previews: data.linkPreview,
-              url,
-              title: dropNull(preview.title),
-              description: dropNull(preview.description),
-              date: getCheckedTimestampOrUndefinedFromLong(preview.date),
-              image: preview.image
-                ? convertFilePointerToAttachment(preview.image)
-                : undefined,
-            };
           })
         : undefined,
       reactions: this.#fromReactions(data.reactions),
@@ -1771,6 +1776,45 @@ export class BackupImportStream extends Writable {
     };
   }
 
+  #fromLinkPreview({
+    logId,
+    body,
+    previews,
+  }: {
+    logId: string;
+    body: string | null | undefined;
+    previews: Array<Backups.ILinkPreview>;
+  }): Array<LinkPreviewType> {
+    const urlsInBody = LinkPreview.findLinks(body ?? '');
+    return previews
+      .map(preview => {
+        if (
+          !LinkPreview.isValidLinkPreview(urlsInBody, preview, {
+            isStory: false,
+          })
+        ) {
+          if (isNightly(window.getVersion())) {
+            throw new Error(`${logId}: dropping invalid link preview`);
+          }
+          log.warn(`${logId}: dropping invalid link preview`);
+          return;
+        }
+
+        strictAssert(preview.url, 'url must exist in valid link preview');
+
+        return {
+          url: preview.url,
+          title: dropNull(preview.title),
+          description: dropNull(preview.description),
+          date: getCheckedTimestampOrUndefinedFromLong(preview.date),
+          image: preview.image
+            ? convertFilePointerToAttachment(preview.image)
+            : undefined,
+        };
+      })
+      .filter(isNotNil);
+  }
+
   async #fromViewOnceMessage({
     attachment,
     reactions,
@@ -1792,10 +1836,15 @@ export class BackupImportStream extends Writable {
     };
   }
 
-  async #fromRevisions(
+  async #fromRevisions({
-    mainMessage: MessageAttributesType,
+    mainMessage,
-    revisions: ReadonlyArray<Backups.IChatItem>
+    revisions,
-  ): Promise<Array<EditHistoryType>> {
+    logId,
+  }: {
+    mainMessage: MessageAttributesType;
+    revisions: ReadonlyArray<Backups.IChatItem>;
+    logId: string;
+  }): Promise<Array<EditHistoryType>> {
     const result = await Promise.all(
       revisions
         .map(async rev => {
@@ -1818,7 +1867,10 @@ export class BackupImportStream extends Writable {
           } = this.#fromDirectionDetails(rev, timestamp);
 
           return {
-            ...(await this.#fromStandardMessage(rev.standardMessage)),
+            ...(await this.#fromStandardMessage({
+              logId,
+              data: rev.standardMessage,
+            })),
             timestamp,
             received_at: incrementMessageCounter(),
             sendStateByConversationId,

ts/test-electron/backup/attachments_test.ts

@@ -364,17 +364,19 @@ describe('backup/attachments', () => {
       await asymmetricRoundtripHarness(
         [
           composeMessage(1, {
-            body: 'url',
+            body: 'https://signal.org',
-            preview: [{ url: 'url', date: 1, image: attachment }],
+            preview: [
+              { url: 'https://signal.org', date: 1, image: attachment },
+            ],
           }),
         ],
         // path & iv will not be roundtripped
         [
           composeMessage(1, {
-            body: 'url',
+            body: 'https://signal.org',
             preview: [
               {
-                url: 'url',
+                url: 'https://signal.org',
                 date: 1,
                 image: omit(attachment, NON_ROUNDTRIPPED_FIELDS),
               },
@@ -391,10 +393,10 @@ describe('backup/attachments', () => {
       await asymmetricRoundtripHarness(
         [
           composeMessage(1, {
-            body: 'url',
+            body: 'https://signal.org',
             preview: [
               {
-                url: 'url',
+                url: 'https://signal.org',
                 date: 1,
                 title: 'title',
                 description: 'description',
@@ -405,10 +407,10 @@ describe('backup/attachments', () => {
         ],
         [
           composeMessage(1, {
-            body: 'url',
+            body: 'https://signal.org',
             preview: [
               {
-                url: 'url',
+                url: 'https://signal.org',
                 date: 1,
                 title: 'title',
                 description: 'description',

ts/test-electron/backup/bubble_test.ts

@@ -605,6 +605,67 @@ describe('backup/bubble messages', () => {
       },
     ]);
   });
+  describe('link previews', async () => {
+    it('roundtrips link preview', async () => {
+      await symmetricRoundtripHarness([
+        {
+          conversationId: contactA.id,
+          id: generateGuid(),
+          type: 'incoming',
+          received_at: 3,
+          received_at_ms: 3,
+          sent_at: 3,
+          timestamp: 3,
+          sourceServiceId: CONTACT_A,
+          body: 'https://signal.org is a cool place',
+          readStatus: ReadStatus.Unread,
+          seenStatus: SeenStatus.Unseen,
+          unidentifiedDeliveryReceived: true,
+          preview: [
+            {
+              url: 'https://signal.org',
+              title: 'Signal',
+            },
+          ],
+        },
+      ]);
+    });
+    it('drops preview if URL does not exist in body', async () => {
+      const message: MessageAttributesType = {
+        conversationId: contactA.id,
+        id: generateGuid(),
+        type: 'incoming',
+        received_at: 3,
+        received_at_ms: 3,
+        sent_at: 3,
+        timestamp: 3,
+        sourceServiceId: CONTACT_A,
+        body: 'no urls here',
+        readStatus: ReadStatus.Unread,
+        seenStatus: SeenStatus.Unseen,
+        unidentifiedDeliveryReceived: true,
+      };
+      await asymmetricRoundtripHarness(
+        [
+          {
+            ...message,
+            preview: [
+              {
+                url: 'https://signal.org',
+                title: 'Signal',
+              },
+            ],
+          },
+        ],
+        [
+          {
+            ...message,
+            preview: [],
+          },
+        ]
+      );
+    });
+  });
   describe('lonely-in-group messages', async () => {
     const GROUP_ID = Bytes.toBase64(getRandomBytes(32));
     let group: ConversationModel | undefined;

ts/types/LinkPreview.ts

@@ -14,6 +14,8 @@ import {
   groupInvitesRoute,
   linkCallRoute,
 } from '../util/signalRoutes';
+import type { Backups } from '../protobuf';
+import type { LinkPreviewType } from './message/LinkPreviews';
 
 export type LinkPreviewImage = AttachmentWithHydratedData;
 
@@ -71,6 +73,29 @@ export function shouldPreviewHref(href: string): boolean {
   );
 }
 
+export function isValidLinkPreview(
+  urlsInBody: Array<string>,
+  preview: LinkPreviewType | Backups.ILinkPreview,
+  { isStory }: { isStory: boolean }
+): boolean {
+  const { url } = preview;
+  if (!url) {
+    return false;
+  }
+
+  if (!shouldPreviewHref(url)) {
+    return false;
+  }
+
+  // Story link previews don't have to correspond to links in the
+  // message body.
+  if (!urlsInBody.includes(url) && !isStory) {
+    return false;
+  }
+
+  return true;
+}
+
 const EXCLUDED_DOMAINS = [
   'debuglogs.org',
   'example',