From 423a92df4d66cb245aa42f72d1f480a41ca68ff2 Mon Sep 17 00:00:00 2001 From: trevor-signal <131492920+trevor-signal@users.noreply.github.com> Date: Thu, 16 Jan 2025 11:28:53 -0500 Subject: [PATCH] Check validity of link previews on import --- .github/workflows/ci.yml | 2 +- ts/messages/handleDataMessage.ts | 15 +--- ts/services/backups/import.ts | 96 ++++++++++++++++----- ts/test-electron/backup/attachments_test.ts | 18 ++-- ts/test-electron/backup/bubble_test.ts | 61 +++++++++++++ ts/types/LinkPreview.ts | 25 ++++++ 6 files changed, 175 insertions(+), 42 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ea294858ce..e9cbdf5323c 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/ts/messages/handleDataMessage.ts b/ts/messages/handleDataMessage.ts index 3bf06931979..9b186d2f4d1 100644 --- a/ts/messages/handleDataMessage.ts +++ b/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.shouldPreviewHref(item.url) + !LinkPreview.isValidLinkPreview(urls, item, { + isStory: isStory(message.attributes), + }) ) { - return undefined; + return null; } return item; diff --git a/ts/services/backups/import.ts b/ts/services/backups/import.ts index a7378e6922b..764544cd473 100644 --- a/ts/services/backups/import.ts +++ b/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( - data: Backups.IStandardMessage - ): Promise> { + async #fromStandardMessage({ + logId, + data, + }: { + logId: string; + data: Backups.IStandardMessage; + }): Promise> { 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 => { - const { url } = preview; - strictAssert(url, 'preview must have a URL'); - return { - url, - title: dropNull(preview.title), - description: dropNull(preview.description), - date: getCheckedTimestampOrUndefinedFromLong(preview.date), - image: preview.image - ? convertFilePointerToAttachment(preview.image) - : undefined, - }; + ? this.#fromLinkPreview({ + logId, + body: data.text?.body, + previews: data.linkPreview, }) : 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; + }): Array { + 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( - mainMessage: MessageAttributesType, - revisions: ReadonlyArray - ): Promise> { + async #fromRevisions({ + mainMessage, + revisions, + logId, + }: { + mainMessage: MessageAttributesType; + revisions: ReadonlyArray; + logId: string; + }): Promise> { 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, diff --git a/ts/test-electron/backup/attachments_test.ts b/ts/test-electron/backup/attachments_test.ts index a03bd68ac3c..5be7f70ea09 100644 --- a/ts/test-electron/backup/attachments_test.ts +++ b/ts/test-electron/backup/attachments_test.ts @@ -364,17 +364,19 @@ describe('backup/attachments', () => { await asymmetricRoundtripHarness( [ composeMessage(1, { - body: 'url', - preview: [{ url: 'url', date: 1, image: attachment }], + body: 'https://signal.org', + 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', diff --git a/ts/test-electron/backup/bubble_test.ts b/ts/test-electron/backup/bubble_test.ts index 86de39c5343..b90e83f581c 100644 --- a/ts/test-electron/backup/bubble_test.ts +++ b/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; diff --git a/ts/types/LinkPreview.ts b/ts/types/LinkPreview.ts index 3b10bedc945..80f6877aea8 100644 --- a/ts/types/LinkPreview.ts +++ b/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, + 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',