From 7a8f20885407cf25895aebc0ef26c438ac8b53e5 Mon Sep 17 00:00:00 2001 From: trevor-signal <131492920+trevor-signal@users.noreply.github.com> Date: Tue, 11 Nov 2025 19:11:44 -0500 Subject: [PATCH] Avoid orphaning quoted attachment thumbnails --- ts/jobs/AttachmentDownloadManager.preload.ts | 31 +- .../AttachmentDownloads.preload.ts | 51 ++- .../AttachmentDownloadManager_test.preload.ts | 124 ++++++ .../addAttachmentToMessage_test.preload.ts | 406 ++++++++++++++++++ 4 files changed, 590 insertions(+), 22 deletions(-) create mode 100644 ts/test-electron/util/addAttachmentToMessage_test.preload.ts diff --git a/ts/jobs/AttachmentDownloadManager.preload.ts b/ts/jobs/AttachmentDownloadManager.preload.ts index 48aa28ecf4..39ea590557 100644 --- a/ts/jobs/AttachmentDownloadManager.preload.ts +++ b/ts/jobs/AttachmentDownloadManager.preload.ts @@ -19,6 +19,7 @@ import { isIncrementalMacVerificationError, } from '../util/downloadAttachment.preload.js'; import { + deleteAttachmentData as doDeleteAttachmentData, deleteDownloadData as doDeleteDownloadData, processNewAttachment as doProcessNewAttachment, } from '../util/migrations.preload.js'; @@ -39,6 +40,7 @@ import { getUndownloadedAttachmentSignature, isIncremental, hasRequiredInformationForRemoteBackup, + deleteAllAttachmentFilesOnDisk, } from '../util/Attachment.std.js'; import type { ReadonlyMessageAttributesType } from '../model-types.d.ts'; import { backupsService } from '../services/backups/index.preload.js'; @@ -48,7 +50,10 @@ import { getMaximumIncomingAttachmentSizeInKb, getMaximumIncomingTextAttachmentSizeInKb, } from '../types/AttachmentSize.std.js'; -import { addAttachmentToMessage } from '../messageModifiers/AttachmentDownloads.preload.js'; +import { + addAttachmentToMessage, + AttachmentNotNeededForMessageError, +} from '../messageModifiers/AttachmentDownloads.preload.js'; import * as Errors from '../types/errors.std.js'; import { redactGenericText } from '../util/privacy.node.js'; import { @@ -491,6 +496,7 @@ export class AttachmentDownloadManager extends JobManager { if (isDownloaded(existing)) { return existing; @@ -184,13 +191,13 @@ export async function addAttachmentToMessage( return existing; } + foundPlaceForAttachment = true; return attachment; }; if (type === 'attachment') { const attachments = message.get('attachments'); - let handledAnywhere = false; let handledInEditHistory = false; const editHistory = message.get('editHistory'); @@ -207,7 +214,6 @@ export async function addAttachmentToMessage( attachments: edit.attachments.map(item => { const newItem = maybeReplaceAttachment(item); handledInEditHistory ||= item !== newItem; - handledAnywhere ||= handledInEditHistory; return newItem; }), }; @@ -220,18 +226,12 @@ export async function addAttachmentToMessage( if (attachments) { message.set({ - attachments: attachments.map(item => { - const newItem = maybeReplaceAttachment(item); - handledAnywhere ||= item !== newItem; - return newItem; - }), + attachments: attachments.map(maybeReplaceAttachment), }); } - if (!handledAnywhere) { - log.warn( - `${logPrefix}: 'attachment' type found no matching place to apply` - ); + if (!foundPlaceForAttachment) { + throw new AttachmentNotNeededForMessageError(attachment); } return; @@ -243,7 +243,7 @@ export async function addAttachmentToMessage( let handledInEditHistory = false; const editHistory = message.get('editHistory'); - if (preview && editHistory) { + if (editHistory) { const newEditHistory = editHistory.map(edit => { if (!edit.preview) { return edit; @@ -282,6 +282,10 @@ export async function addAttachmentToMessage( }); } + if (!foundPlaceForAttachment) { + throw new AttachmentNotNeededForMessageError(attachment); + } + return; } @@ -290,7 +294,6 @@ export async function addAttachmentToMessage( if (!contacts?.length) { throw new Error(`${logPrefix}: no contacts, cannot add attachment!`); } - let handled = false; const newContacts = contacts.map(contact => { if (!contact.avatar?.avatar) { @@ -301,7 +304,6 @@ export async function addAttachmentToMessage( const newAttachment = maybeReplaceAttachment(existingAttachment); if (existingAttachment !== newAttachment) { - handled = true; return { ...contact, avatar: { ...contact.avatar, avatar: newAttachment }, @@ -310,10 +312,8 @@ export async function addAttachmentToMessage( return contact; }); - if (!handled) { - throw new Error( - `${logPrefix}: Couldn't find matching contact with avatar attachment for message` - ); + if (!foundPlaceForAttachment) { + throw new AttachmentNotNeededForMessageError(attachment); } message.set({ contact: newContacts }); @@ -374,6 +374,10 @@ export async function addAttachmentToMessage( message.set({ quote: newQuote }); } + if (!foundPlaceForAttachment) { + throw new AttachmentNotNeededForMessageError(attachment); + } + return; } @@ -389,6 +393,11 @@ export async function addAttachmentToMessage( data: sticker.data ? maybeReplaceAttachment(sticker.data) : attachment, }, }); + + if (!foundPlaceForAttachment) { + throw new AttachmentNotNeededForMessageError(attachment); + } + return; } diff --git a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts index 7a118fc580..0107038ea5 100644 --- a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts +++ b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts @@ -41,6 +41,9 @@ import { type ExplodePromiseResultType, } from '../../util/explodePromise.std.js'; import { itemStorage } from '../../textsecure/Storage.preload.js'; +import { composeAttachment } from '../../test-node/util/queueAttachmentDownloads_test.preload.js'; +import { MessageCache } from '../../services/MessageCache.preload.js'; +import { AttachmentNotNeededForMessageError } from '../../messageModifiers/AttachmentDownloads.preload.js'; const { omit } = lodash; @@ -109,6 +112,7 @@ describe('AttachmentDownloadManager', () => { beforeEach(async () => { await DataWriter.removeAll(); await itemStorage.user.setAciAndDeviceId(generateAci(), 1); + MessageCache.install(); sandbox = sinon.createSandbox(); clock = sandbox.useFakeTimers(); @@ -179,6 +183,7 @@ describe('AttachmentDownloadManager', () => { timestamp: job.sentAt, received_at: job.receivedAt + 1, conversationId: 'convoId', + attachments: [job.attachment], }, { forceSave: true, @@ -625,6 +630,7 @@ describe('AttachmentDownloadManager', () => { downloadStarted.resolve(); }); }), + deleteAttachmentData: sandbox.stub(), deleteDownloadData: sandbox.stub(), processNewAttachment: sandbox.stub(), runDownloadAttachmentJobInner, @@ -741,9 +747,117 @@ describe('AttachmentDownloadManager', () => { }); }); }); +describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => { + let sandbox: sinon.SinonSandbox; + let deleteAttachmentData: sinon.SinonStub; + let deleteDownloadData: sinon.SinonStub; + let downloadAttachment: sinon.SinonStub; + let processNewAttachment: sinon.SinonStub; + + const downloadedAttachment: Awaited< + ReturnType + > = { + path: '/path/to/file', + digest: 'digest', + plaintextHash: 'plaintextHash', + localKey: 'localKey', + version: 2, + size: 128, + }; + beforeEach(async () => { + await DataWriter.removeAll(); + await itemStorage.user.setAciAndDeviceId(generateAci(), 1); + sandbox = sinon.createSandbox(); + downloadAttachment = sandbox + .stub() + .returns(Promise.resolve(downloadedAttachment)); + deleteAttachmentData = sandbox.stub(); + deleteDownloadData = sandbox.stub(); + + processNewAttachment = sandbox.stub().callsFake(attachment => attachment); + }); + + afterEach(async () => { + sandbox.restore(); + }); + + it('will delete attachment files if attachment not found on message', async () => { + const messageId = 'messageId'; + const attachment = composeAttachment(); + const abortController = new AbortController(); + + await window.MessageCache.saveMessage( + { + id: messageId, + type: 'incoming', + sent_at: Date.now(), + timestamp: Date.now(), + received_at: Date.now(), + conversationId: 'convoId', + attachments: [attachment], + }, + { + forceSave: true, + } + ); + + const job = composeJob({ + messageId: 'messageId', + receivedAt: Date.now(), + attachmentOverrides: attachment, + }); + const result = await runDownloadAttachmentJob({ + job, + isLastAttempt: false, + options: { + isForCurrentlyVisibleMessage: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100000, + maxTextAttachmentSizeInKib: 100000, + hasMediaBackups: false, + }, + dependencies: { + downloadAttachment, + deleteAttachmentData, + deleteDownloadData, + processNewAttachment, + runDownloadAttachmentJobInner: sandbox.stub().throws( + new AttachmentNotNeededForMessageError({ + contentType: MIME.IMAGE_PNG, + size: 128, + path: 'main/path', + downloadPath: '/downloadPath', + thumbnail: { + contentType: MIME.IMAGE_PNG, + size: 128, + path: 'thumbnail/path', + }, + }) + ), + }, + }); + + assert.strictEqual(result.status, 'finished'); + assert.deepStrictEqual( + deleteAttachmentData + .getCalls() + .map(call => call.args[0]) + .flat(), + ['main/path', 'thumbnail/path'] + ); + assert.deepStrictEqual( + deleteDownloadData + .getCalls() + .map(call => call.args[0]) + .flat(), + ['/downloadPath'] + ); + }); +}); describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { let sandbox: sinon.SinonSandbox; + let deleteAttachmentData: sinon.SinonStub; let deleteDownloadData: sinon.SinonStub; let downloadAttachment: sinon.SinonStub; let processNewAttachment: sinon.SinonStub; @@ -761,6 +875,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { }; beforeEach(async () => { + await DataWriter.removeAll(); + MessageCache.install(); + await itemStorage.user.setAciAndDeviceId(generateAci(), 1); sandbox = sinon.createSandbox(); downloadAttachment = sandbox .stub() @@ -791,6 +908,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -829,6 +947,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -884,6 +1003,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -921,6 +1041,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -959,6 +1080,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -995,6 +1117,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, @@ -1050,6 +1173,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxAttachmentSizeInKib: 100 * MEBIBYTE, maxTextAttachmentSizeInKib: 2 * MEBIBYTE, dependencies: { + deleteAttachmentData, deleteDownloadData, downloadAttachment, processNewAttachment, diff --git a/ts/test-electron/util/addAttachmentToMessage_test.preload.ts b/ts/test-electron/util/addAttachmentToMessage_test.preload.ts new file mode 100644 index 0000000000..5a89f2ded1 --- /dev/null +++ b/ts/test-electron/util/addAttachmentToMessage_test.preload.ts @@ -0,0 +1,406 @@ +// Copyright 2025 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import { emptyDir } from 'fs-extra'; +import { randomBytes } from 'node:crypto'; +import { readdir, writeFile } from 'node:fs/promises'; +import { v7 } from 'uuid'; + +import * as MIME from '../../types/MIME.std.js'; +import { composeAttachment } from '../../test-node/util/queueAttachmentDownloads_test.preload.js'; +import { addAttachmentToMessage } from '../../messageModifiers/AttachmentDownloads.preload.js'; +import { getMessageById } from '../../messages/getMessageById.preload.js'; +import { MessageCache } from '../../services/MessageCache.preload.js'; +import { getPath } from '../../../app/attachments.node.js'; +import { getAbsoluteAttachmentPath } from '../../util/migrations.preload.js'; +import { DataWriter } from '../../sql/Client.preload.js'; +import type { MessageAttributesType } from '../../model-types.js'; +import { itemStorage } from '../../textsecure/Storage.preload.js'; +import { generateAci } from '../../types/ServiceId.std.js'; + +describe('addAttachmentToMessage', () => { + beforeEach(async () => { + await DataWriter.removeAll(); + await itemStorage.user.setAciAndDeviceId(generateAci(), 1); + MessageCache.install(); + }); + + afterEach(async () => { + await emptyDir(getPath(window.SignalContext.config.userDataPath)); + }); + + async function saveMessage( + messageOverrides: Partial + ): Promise { + const message: MessageAttributesType = { + id: v7(), + type: 'incoming', + sent_at: Date.now(), + timestamp: Date.now(), + received_at: Date.now(), + conversationId: 'convoId', + ...messageOverrides, + }; + await window.MessageCache.saveMessage(message, { + forceSave: true, + }); + return message; + } + + async function writeAttachmentToDisk(path: string, text: string) { + await writeFile(getAbsoluteAttachmentPath(path), text); + } + + async function listAttachmentsOnDisk(): Promise> { + return readdir(getPath(window.SignalContext.config.userDataPath)); + } + it('replaces attachment on message', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + attachments: [attachment], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'attachment', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual(message?.attributes.attachments?.[0], { + ...attachment, + path: '/path/to/attachment', + }); + }); + it('throws error if matching attachment not found', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + attachments: [attachment], + }); + + await assert.isRejected( + addAttachmentToMessage( + messageId, + { ...attachment, digest: randomBytes(32).toString('base64') }, + 'logid', + { + type: 'attachment', + } + ), + 'AttachmentNotNeededForMessageError' + ); + }); + + it('throws error if attachment found but already downloaded', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + path: '/path/to/attachment', + }); + + const { id: messageId } = await saveMessage({ + attachments: [attachment], + }); + + await assert.isRejected( + addAttachmentToMessage(messageId, attachment, 'logid', { + type: 'attachment', + }), + 'AttachmentNotNeededForMessageError' + ); + }); + it('replaces preview', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + preview: [{ url: 'url', image: attachment }], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'preview', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual(message?.attributes.preview?.[0].image, { + ...attachment, + path: '/path/to/attachment', + }); + }); + it('replaces preview in edithistory', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + editHistory: [ + { + timestamp: 1, + received_at: 1, + preview: [{ url: 'url', image: attachment }], + }, + ], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'preview', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual( + message?.attributes.editHistory?.[0].preview?.[0].image, + { + ...attachment, + path: '/path/to/attachment', + } + ); + }); + it('replaces quote thumbnail', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + const { id: messageId } = await saveMessage({ + quote: { + id: null, + isViewOnce: false, + referencedMessageNotFound: false, + attachments: [{ contentType: MIME.IMAGE_PNG, thumbnail: attachment }], + }, + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'quote', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual( + message?.attributes.quote?.attachments[0].thumbnail, + { + ...attachment, + path: '/path/to/attachment', + } + ); + }); + it('replaces quote thumbnail in edit history', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + editHistory: [ + { + timestamp: 1, + received_at: 1, + quote: { + id: null, + isViewOnce: false, + referencedMessageNotFound: false, + attachments: [ + { contentType: MIME.IMAGE_PNG, thumbnail: attachment }, + ], + }, + }, + ], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'quote', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual( + message?.attributes.editHistory?.[0].quote?.attachments[0].thumbnail, + { + ...attachment, + path: '/path/to/attachment', + } + ); + }); + + it('replaces sticker', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + sticker: { + packId: 'packId', + stickerId: 1, + packKey: 'packKey', + data: attachment, + }, + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'sticker', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual(message?.attributes.sticker?.data, { + ...attachment, + path: '/path/to/attachment', + }); + }); + + it('replaces contact avatar', async () => { + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + const { id: messageId } = await saveMessage({ + contact: [ + { + avatar: { + isProfile: false, + avatar: attachment, + }, + }, + ], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: '/path/to/attachment', + }, + 'logid', + { + type: 'contact', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual(message?.attributes.contact?.[0]?.avatar?.avatar, { + ...attachment, + path: '/path/to/attachment', + }); + }); + + it('replaces body attachment and deletes file on disk', async () => { + await writeAttachmentToDisk('bodyAttachmentPath', 'attachmenttext'); + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + bodyAttachment: attachment, + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: 'bodyAttachmentPath', + }, + 'logid', + { + type: 'long-message', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual(message?.attributes.body, 'attachmenttext'); + assert.deepStrictEqual(message?.attributes.bodyAttachment, { + ...attachment, + path: 'bodyAttachmentPath', + }); + // attachment is deleted from disk after download + assert.deepEqual(await listAttachmentsOnDisk(), []); + }); + it('replaces body attachment in edit history and deletes file on disk', async () => { + await writeAttachmentToDisk('bodyAttachmentPath', 'attachmenttext'); + const attachment = composeAttachment({ + digest: randomBytes(32).toString('base64'), + }); + + const { id: messageId } = await saveMessage({ + editHistory: [ + { + timestamp: 1, + received_at: 1, + bodyAttachment: attachment, + }, + ], + }); + + await addAttachmentToMessage( + messageId, + { + ...attachment, + path: 'bodyAttachmentPath', + }, + 'logid', + { + type: 'long-message', + } + ); + + const message = await getMessageById(messageId); + assert.deepStrictEqual( + message?.attributes.editHistory?.[0].body, + 'attachmenttext' + ); + assert.deepStrictEqual( + message?.attributes.editHistory?.[0].bodyAttachment, + { + ...attachment, + path: 'bodyAttachmentPath', + } + ); + // attachment is deleted from disk after download + assert.deepEqual(await listAttachmentsOnDisk(), []); + }); +});