From ed381109f5cbeadeda03a9da66840d55ddbae470 Mon Sep 17 00:00:00 2001 From: automated-signal <37887102+automated-signal@users.noreply.github.com> Date: Mon, 9 Feb 2026 11:37:27 -0600 Subject: [PATCH] Deduplicate incoming attachments on disk Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com> --- app/attachment_channel.main.ts | 14 +- app/attachments.node.ts | 10 +- app/protocol_filter.node.ts | 4 +- ts/ConversationController.preload.ts | 6 +- ts/background.preload.ts | 4 + ts/groups.preload.ts | 8 +- ts/groups/joinViaLink.preload.ts | 6 +- ts/jobs/AttachmentDownloadManager.preload.ts | 138 ++++- ts/jobs/deleteDownloadsJobQueue.preload.ts | 8 +- ts/jobs/helpers/sendPollTerminate.preload.ts | 2 +- .../AttachmentDownloads.preload.ts | 4 +- ts/messageModifiers/DeletesForMe.preload.ts | 6 - ts/models/conversations.preload.ts | 14 +- ts/services/contactSync.preload.ts | 8 +- .../expiringMessagesDeletion.preload.ts | 2 +- ts/services/writeProfile.preload.ts | 6 +- ts/sql/Client.preload.ts | 80 +-- ts/sql/Interface.std.ts | 61 ++- ts/sql/Server.node.ts | 121 ++++- .../1650-protected-attachments.std.ts | 53 ++ ts/sql/migrations/index.node.ts | 2 + ts/state/ducks/conversations.preload.ts | 2 +- ts/state/ducks/stories.preload.ts | 4 +- .../ContactsParser_test.preload.ts | 6 +- ts/test-electron/Crypto_test.preload.ts | 6 +- .../backup/filePointer_test.preload.ts | 6 +- ...cleanupOrphanedAttachments_test.preload.ts | 41 +- .../deleteMessageAttachments_test.preload.ts | 485 +++++++++++++---- .../normalizedAttachments_test.preload.ts | 240 +++++---- .../AttachmentDownloadManager_test.preload.ts | 499 ++++++++++++++++-- ...ment_download_backup_stats_test.preload.ts | 4 +- ts/test-electron/sql/sendLog_test.preload.ts | 4 +- .../addAttachmentToMessage_test.preload.ts | 10 +- ts/test-helpers/attachments.node.ts | 18 + ts/test-node/sql/migration_1650_test.node.ts | 139 +++++ ts/test-node/types/Message2_test.preload.ts | 4 +- ts/textsecure/ContactsParser.preload.ts | 4 +- ts/types/Conversation.node.ts | 2 +- ts/types/Message2.preload.ts | 99 ++-- ts/util/Attachment.std.ts | 27 - ts/util/basePaths.preload.ts | 4 +- ts/util/callDisposition.preload.ts | 2 +- ts/util/captureAudioDuration.dom.ts | 4 + ts/util/cleanup.preload.ts | 12 +- ts/util/deleteForEveryone.preload.ts | 2 +- ts/util/deleteForMe.preload.ts | 31 +- .../encryptConversationAttachments.preload.ts | 8 +- ...ndDeleteOnboardingStoryIfExists.preload.ts | 2 +- ts/util/messageFilePaths.std.ts | 29 +- ts/util/migrations.preload.ts | 28 +- ts/util/modifyTargetMessage.preload.ts | 8 +- 51 files changed, 1727 insertions(+), 560 deletions(-) create mode 100644 ts/sql/migrations/1650-protected-attachments.std.ts create mode 100644 ts/test-helpers/attachments.node.ts create mode 100644 ts/test-node/sql/migration_1650_test.node.ts diff --git a/app/attachment_channel.main.ts b/app/attachment_channel.main.ts index 4e7196f0fb..cf35ff6296 100644 --- a/app/attachment_channel.main.ts +++ b/app/attachment_channel.main.ts @@ -48,7 +48,7 @@ import { sleep } from '../ts/util/sleep.std.js'; import { toWebStream } from '../ts/util/toWebStream.node.js'; import { createLogger } from '../ts/logging/log.std.js'; import { - deleteAll as deleteAllAttachments, + deleteAllAttachments, deleteAllBadges, deleteAllDownloads, deleteAllDraftAttachments, @@ -62,7 +62,7 @@ import { getAvatarsPath, getDownloadsPath, getDraftPath, - getPath, + getAttachmentsPath, getStickersPath, getTempPath, } from './attachments.node.js'; @@ -450,6 +450,14 @@ function deleteOrphanedAttachments({ await sql.sqlRead('finishGetKnownMessageAttachments', cursor); } } + + const protectedAttachments = await sql.sqlRead( + 'getAllProtectedAttachmentPaths' + ); + for (const protectedAttachment of protectedAttachments) { + orphanedAttachments.delete(protectedAttachment); + } + log.info( `cleanupOrphanedAttachments: ${totalAttachmentsFound} attachments and \ ${totalDownloadsFound} downloads found on disk` @@ -514,7 +522,7 @@ export function initialize({ } initialized = true; - attachmentsDir = getPath(configDir); + attachmentsDir = getAttachmentsPath(configDir); stickersDir = getStickersPath(configDir); tempDir = getTempPath(configDir); draftDir = getDraftPath(configDir); diff --git a/app/attachments.node.ts b/app/attachments.node.ts index 2be51fd998..0eef95273d 100644 --- a/app/attachments.node.ts +++ b/app/attachments.node.ts @@ -26,7 +26,7 @@ const { map, isString } = lodash; const log = createLogger('attachments'); -const PATH = 'attachments.noindex'; +const ATTACHMENTS_PATH = 'attachments.noindex'; const AVATAR_PATH = 'avatars.noindex'; const BADGES_PATH = 'badges.noindex'; const STICKER_PATH = 'stickers.noindex'; @@ -69,7 +69,7 @@ export const getBadgesPath = createPathGetter(BADGES_PATH); export const getDraftPath = createPathGetter(DRAFT_PATH); export const getDownloadsPath = createPathGetter(DOWNLOADS_PATH); export const getMegaphonesPath = createPathGetter(MEGAPHONES_PATH); -export const getPath = createPathGetter(PATH); +export const getAttachmentsPath = createPathGetter(ATTACHMENTS_PATH); export const getStickersPath = createPathGetter(STICKER_PATH); export const getTempPath = createPathGetter(TEMP_PATH); export const getUpdateCachePath = createPathGetter(UPDATE_CACHE_PATH); @@ -111,7 +111,7 @@ async function getAllFiles(dir: string): Promise> { export const getAllAttachments = ( userDataPath: string ): Promise> => { - return getAllFiles(getPath(userDataPath)); + return getAllFiles(getAttachmentsPath(userDataPath)); }; export const getAllDownloads = ( @@ -186,14 +186,14 @@ export const deleteStaleDownloads = async ( await deleteAllDownloads({ userDataPath, downloads: stale }); }; -export const deleteAll = async ({ +export const deleteAllAttachments = async ({ userDataPath, attachments, }: { userDataPath: string; attachments: ReadonlyArray; }): Promise => { - const deleteFromDisk = createDeleter(getPath(userDataPath)); + const deleteFromDisk = createDeleter(getAttachmentsPath(userDataPath)); await pMap(attachments, deleteFromDisk, { concurrency: FS_CONCURRENCY }); diff --git a/app/protocol_filter.node.ts b/app/protocol_filter.node.ts index 483d1c4e40..9bcf6bd4a3 100644 --- a/app/protocol_filter.node.ts +++ b/app/protocol_filter.node.ts @@ -10,7 +10,7 @@ import { getBadgesPath, getDraftPath, getDownloadsPath, - getPath, + getAttachmentsPath, getStickersPath, getTempPath, getUpdateCachePath, @@ -66,7 +66,7 @@ function _createFileHandler({ getBadgesPath(userDataPath), getDraftPath(userDataPath), getDownloadsPath(userDataPath), - getPath(userDataPath), + getAttachmentsPath(userDataPath), getStickersPath(userDataPath), getTempPath(userDataPath), getUpdateCachePath(userDataPath), diff --git a/ts/ConversationController.preload.ts b/ts/ConversationController.preload.ts index c6043c38aa..5ad4ba7aba 100644 --- a/ts/ConversationController.preload.ts +++ b/ts/ConversationController.preload.ts @@ -20,7 +20,7 @@ import { } from './util/whatTypeOfConversation.dom.js'; import { doesAttachmentExist, - deleteAttachmentData, + maybeDeleteAttachmentFile, } from './util/migrations.preload.js'; import { isServiceIdString, @@ -1640,14 +1640,14 @@ export class ConversationController { drop( (async () => { if (avatarPath && (await doesAttachmentExist(avatarPath))) { - await deleteAttachmentData(avatarPath); + await maybeDeleteAttachmentFile(avatarPath); } if ( profileAvatarPath && (await doesAttachmentExist(profileAvatarPath)) ) { - await deleteAttachmentData(profileAvatarPath); + await maybeDeleteAttachmentFile(profileAvatarPath); } })() ); diff --git a/ts/background.preload.ts b/ts/background.preload.ts index 408d08b5b7..def7f35382 100644 --- a/ts/background.preload.ts +++ b/ts/background.preload.ts @@ -1025,6 +1025,10 @@ export async function startApp(): Promise { setAppLoadingScreenMessage(i18n('icu:optimizingApplication'), i18n); + // These paths are protected while they are referenced in memory but not in + // message_attachments, so we can safely clear them at app start + await DataWriter.resetProtectedAttachmentPaths(); + if (newVersion || itemStorage.get('needOrphanedAttachmentCheck')) { await itemStorage.remove('needOrphanedAttachmentCheck'); await DataWriter.cleanupOrphanedAttachments(); diff --git a/ts/groups.preload.ts b/ts/groups.preload.ts index 6db53cd304..be9a1d4cdb 100644 --- a/ts/groups.preload.ts +++ b/ts/groups.preload.ts @@ -27,7 +27,7 @@ import { dropNull } from './util/dropNull.std.js'; import { writeNewAttachmentData, readAttachmentData, - deleteAttachmentData, + maybeDeleteAttachmentFile, } from './util/migrations.preload.js'; import type { ConversationAttributesType, @@ -5749,7 +5749,7 @@ export async function applyNewAvatar( // Avatar has been dropped if (!newAvatarUrl && attributes.avatar) { if (attributes.avatar.path) { - await deleteAttachmentData(attributes.avatar.path); + await maybeDeleteAttachmentFile(attributes.avatar.path); } result.avatar = undefined; } @@ -5796,7 +5796,7 @@ export async function applyNewAvatar( } if (attributes.avatar?.path) { - await deleteAttachmentData(attributes.avatar.path); + await maybeDeleteAttachmentFile(attributes.avatar.path); } const local = await writeNewAttachmentData(data); result.avatar = { @@ -5811,7 +5811,7 @@ export async function applyNewAvatar( Errors.toLogFormat(error) ); if (result.avatar && result.avatar.path) { - await deleteAttachmentData(result.avatar.path); + await maybeDeleteAttachmentFile(result.avatar.path); } result.avatar = undefined; } diff --git a/ts/groups/joinViaLink.preload.ts b/ts/groups/joinViaLink.preload.ts index 7d8d5a28cf..d8223626b6 100644 --- a/ts/groups/joinViaLink.preload.ts +++ b/ts/groups/joinViaLink.preload.ts @@ -27,7 +27,7 @@ import { } from '../groups.preload.js'; import { createGroupV2JoinModal } from '../state/roots/createGroupV2JoinModal.dom.js'; import { explodePromise } from '../util/explodePromise.std.js'; -import { deleteAttachmentData } from '../util/migrations.preload.js'; +import { maybeDeleteAttachmentFile } from '../util/migrations.preload.js'; import { isAccessControlEnabled } from './util.std.js'; import { isGroupV1 } from '../util/whatTypeOfConversation.dom.js'; import { longRunningTaskWrapper } from '../util/longRunningTaskWrapper.dom.js'; @@ -224,7 +224,7 @@ export async function joinViaLink(value: string): Promise { localAvatar?.loadingState === LoadingState.Loaded && localAvatar.value?.path ) { - await deleteAttachmentData(localAvatar.value.path); + await maybeDeleteAttachmentFile(localAvatar.value.path); } resolve(); } catch (error) { @@ -428,7 +428,7 @@ export async function joinViaLink(value: string): Promise { // Dialog has been dismissed; we'll delete the unneeeded avatar if (!groupV2InfoRoot) { - await deleteAttachmentData(attributes.avatar.path); + await maybeDeleteAttachmentFile(attributes.avatar.path); return; } } else { diff --git a/ts/jobs/AttachmentDownloadManager.preload.ts b/ts/jobs/AttachmentDownloadManager.preload.ts index 712a90fa78..5749409899 100644 --- a/ts/jobs/AttachmentDownloadManager.preload.ts +++ b/ts/jobs/AttachmentDownloadManager.preload.ts @@ -19,9 +19,10 @@ import { isIncrementalMacVerificationError, } from '../util/downloadAttachment.preload.js'; import { - deleteAttachmentData as doDeleteAttachmentData, - deleteDownloadData as doDeleteDownloadData, - processNewAttachment as doProcessNewAttachment, + maybeDeleteAttachmentFile, + deleteDownloadFile, + doesAttachmentExist, + processNewAttachment, } from '../util/migrations.preload.js'; import { DataReader, DataWriter } from '../sql/Client.preload.js'; import { getValue } from '../RemoteConfig.dom.js'; @@ -40,7 +41,6 @@ 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'; @@ -62,7 +62,11 @@ import { type JobManagerJobResultType, type JobManagerJobType, } from './JobManager.std.js'; -import { IMAGE_WEBP } from '../types/MIME.std.js'; +import { + IMAGE_WEBP, + type MIMEType, + stringToMIMEType, +} from '../types/MIME.std.js'; import { AttachmentDownloadSource } from '../sql/Interface.std.js'; import { drop } from '../util/drop.std.js'; import { type ReencryptedAttachmentV2 } from '../AttachmentCrypto.node.js'; @@ -87,6 +91,8 @@ import { JobCancelReason } from './types.std.js'; import { isAbortError } from '../util/isAbortError.std.js'; import { itemStorage } from '../textsecure/Storage.preload.js'; import { calculateExpirationTimestamp } from '../util/expirationTimer.std.js'; +import { cleanupAttachmentFiles } from '../types/Message2.preload.js'; +import type { WithRequiredProperties } from '../types/Util.std.js'; const { noop, omit, throttle } = lodash; @@ -497,10 +503,11 @@ export class AttachmentDownloadManager extends JobManager = { + ...downloadedAttachment, + ...(await getExistingAttachmentDataForReuse({ + downloadedAttachment, + contentType: attachment.contentType, + logId, + dependencies, + })), + }; + const upgradedAttachment = await dependencies.processNewAttachment( { ...omit(attachment, ['error', 'pending']), - ...downloadedAttachment, + ...attachmentDataToUse, }, attachmentType ); @@ -885,7 +900,7 @@ export async function runDownloadAttachmentJobInner({ const shouldDeleteDownload = downloadPath && !isShowingLightbox(); if (downloadPath) { if (shouldDeleteDownload) { - await dependencies.deleteDownloadData(downloadPath); + await dependencies.deleteDownloadFile(downloadPath); } else { deleteDownloadsJobQueue.pause(); await deleteDownloadsJobQueue.add({ @@ -1040,3 +1055,92 @@ function _markAttachmentAsTransientlyErrored( ): AttachmentType { return { ...attachment, pending: false, error: true }; } + +type AttachmentDataToBeReused = WithRequiredProperties< + Pick< + AttachmentType, + 'path' | 'localKey' | 'version' | 'thumbnail' | 'screenshot' + >, + 'path' | 'localKey' | 'version' +>; +async function getExistingAttachmentDataForReuse({ + downloadedAttachment, + contentType, + logId, + dependencies, +}: { + downloadedAttachment: ReencryptedAttachmentV2; + contentType: MIMEType; + logId: string; + dependencies: Pick; +}): Promise { + const existingAttachmentData = + await DataWriter.getAndProtectExistingAttachmentPath({ + plaintextHash: downloadedAttachment.plaintextHash, + version: downloadedAttachment.version, + contentType, + }); + + if (!existingAttachmentData) { + return null; + } + + strictAssert(existingAttachmentData.path, 'path must exist for reuse'); + strictAssert( + existingAttachmentData.version === downloadedAttachment.version, + 'version mismatch' + ); + strictAssert(existingAttachmentData.localKey, 'localKey must exist'); + + if (!(await doesAttachmentExist(existingAttachmentData.path))) { + log.warn( + `${logId}: Existing attachment no longer exists, using newly downloaded one` + ); + return null; + } + + log.info(`${logId}: Reusing existing attachment`); + + await dependencies.maybeDeleteAttachmentFile(downloadedAttachment.path); + + const dataToReuse: AttachmentDataToBeReused = { + path: existingAttachmentData.path, + localKey: existingAttachmentData.localKey, + version: existingAttachmentData.version, + }; + const { thumbnailPath, thumbnailSize, thumbnailContentType } = + existingAttachmentData; + + if ( + thumbnailPath && + thumbnailSize && + thumbnailContentType && + (await doesAttachmentExist(thumbnailPath)) + ) { + dataToReuse.thumbnail = { + path: thumbnailPath, + localKey: existingAttachmentData.thumbnailLocalKey ?? undefined, + version: existingAttachmentData.thumbnailVersion ?? undefined, + size: thumbnailSize, + contentType: stringToMIMEType(thumbnailContentType), + }; + } + + const { screenshotPath, screenshotSize, screenshotContentType } = + existingAttachmentData; + if ( + screenshotPath && + screenshotSize && + screenshotContentType && + (await doesAttachmentExist(screenshotPath)) + ) { + dataToReuse.screenshot = { + path: screenshotPath, + localKey: existingAttachmentData.screenshotLocalKey ?? undefined, + version: existingAttachmentData.screenshotVersion ?? undefined, + size: screenshotSize, + contentType: stringToMIMEType(screenshotContentType), + }; + } + return dataToReuse; +} diff --git a/ts/jobs/deleteDownloadsJobQueue.preload.ts b/ts/jobs/deleteDownloadsJobQueue.preload.ts index 6b30a255d0..6790d23871 100644 --- a/ts/jobs/deleteDownloadsJobQueue.preload.ts +++ b/ts/jobs/deleteDownloadsJobQueue.preload.ts @@ -7,7 +7,7 @@ import lodash from 'lodash'; import { JobQueue } from './JobQueue.std.js'; import { jobQueueDatabaseStore } from './JobQueueDatabaseStore.preload.js'; import { parseUnknown } from '../util/schemas.std.js'; -import { deleteDownloadData } from '../util/migrations.preload.js'; +import { deleteDownloadFile } from '../util/migrations.preload.js'; import { DataReader } from '../sql/Client.preload.js'; import type { JOB_STATUS } from './JobQueue.std.js'; @@ -63,7 +63,7 @@ export class DeleteDownloadsJobQueue extends JobQueue { const message = await DataReader.getMessageById(messageId); if (!message) { log?.warn('Message not found; attempting to delete download path.'); - await deleteDownloadData(downloadPath); + await deleteDownloadFile(downloadPath); return undefined; } @@ -86,7 +86,7 @@ export class DeleteDownloadsJobQueue extends JobQueue { log?.warn( 'Target attachment not found; attempting to delete download path.' ); - await deleteDownloadData(downloadPath); + await deleteDownloadFile(downloadPath); return undefined; } @@ -97,7 +97,7 @@ export class DeleteDownloadsJobQueue extends JobQueue { throw new Error('Attachment still downloading'); } - await deleteDownloadData(downloadPath); + await deleteDownloadFile(downloadPath); const updatedMessage = { ...message, diff --git a/ts/jobs/helpers/sendPollTerminate.preload.ts b/ts/jobs/helpers/sendPollTerminate.preload.ts index 9111c18431..eb6ff2d524 100644 --- a/ts/jobs/helpers/sendPollTerminate.preload.ts +++ b/ts/jobs/helpers/sendPollTerminate.preload.ts @@ -199,7 +199,7 @@ async function markTerminateFailed( if (notificationMessage) { log.info('markTerminateFailed: Deleting poll-terminate notification'); - await DataWriter.removeMessage(notificationMessage.id, { + await DataWriter.removeMessageById(notificationMessage.id, { cleanupMessages, }); } diff --git a/ts/messageModifiers/AttachmentDownloads.preload.ts b/ts/messageModifiers/AttachmentDownloads.preload.ts index 0d9a1002aa..45c1c08b08 100644 --- a/ts/messageModifiers/AttachmentDownloads.preload.ts +++ b/ts/messageModifiers/AttachmentDownloads.preload.ts @@ -12,7 +12,7 @@ import { } from '../util/Attachment.std.js'; import { loadAttachmentData, - deleteAttachmentData, + maybeDeleteAttachmentFile, } from '../util/migrations.preload.js'; import { getMessageById } from '../messages/getMessageById.preload.js'; import { trimMessageWhitespace } from '../types/BodyRange.std.js'; @@ -170,7 +170,7 @@ export async function addAttachmentToMessage( }); } finally { if (attachment.path) { - await deleteAttachmentData(attachment.path); + await maybeDeleteAttachmentFile(attachment.path); } if (!handledAnywhere) { // eslint-disable-next-line no-unsafe-finally diff --git a/ts/messageModifiers/DeletesForMe.preload.ts b/ts/messageModifiers/DeletesForMe.preload.ts index 4d5a9ee135..81d1d9e8c1 100644 --- a/ts/messageModifiers/DeletesForMe.preload.ts +++ b/ts/messageModifiers/DeletesForMe.preload.ts @@ -11,10 +11,6 @@ import type { ConversationIdentifier, AddressableMessage, } from '../textsecure/messageReceiverEvents.std.js'; -import { - deleteAttachmentData, - deleteDownloadData, -} from '../util/migrations.preload.js'; import { deleteAttachmentFromMessage, deleteMessage, @@ -115,8 +111,6 @@ export async function onDelete(item: DeleteForMeAttributesType): Promise { item.message, item.deleteAttachmentData, { - deleteAttachmentOnDisk: deleteAttachmentData, - deleteDownloadOnDisk: deleteDownloadData, logId, } ); diff --git a/ts/models/conversations.preload.ts b/ts/models/conversations.preload.ts index 65ad5a0258..39059e8489 100644 --- a/ts/models/conversations.preload.ts +++ b/ts/models/conversations.preload.ts @@ -21,7 +21,7 @@ import { DataReader, DataWriter } from '../sql/Client.preload.js'; import { getConversation } from '../util/getConversation.preload.js'; import { copyAttachmentIntoTempDirectory, - deleteAttachmentData, + maybeDeleteAttachmentFile, doesAttachmentExist, getAbsoluteAttachmentPath, getAbsoluteTempPath, @@ -3691,7 +3691,7 @@ export class ConversationModel { const message = window.MessageCache.getById(notificationId); if (message) { - await DataWriter.removeMessage(message.id, { + await DataWriter.removeMessageById(message.id, { cleanupMessages, }); } @@ -3734,7 +3734,7 @@ export class ConversationModel { const message = window.MessageCache.getById(notificationId); if (message) { - await DataWriter.removeMessage(message.id, { + await DataWriter.removeMessageById(message.id, { cleanupMessages, }); } @@ -4214,7 +4214,7 @@ export class ConversationModel { if (preview && preview.length && !isForwarding) { attachments.forEach(attachment => { if (attachment.path) { - void deleteAttachmentData(attachment.path); + drop(maybeDeleteAttachmentFile(attachment.path)); } }); } @@ -4238,7 +4238,7 @@ export class ConversationModel { const downscaledAttachment = await downscaleOutgoingAttachment(attachment); if (downscaledAttachment !== attachment && attachment.path) { - drop(deleteAttachmentData(attachment.path)); + drop(maybeDeleteAttachmentFile(attachment.path)); } return downscaledAttachment; }) @@ -5156,7 +5156,7 @@ export class ConversationModel { { data: decrypted, writeNewAttachmentData, - deleteAttachmentData, + deleteAttachmentData: maybeDeleteAttachmentFile, doesAttachmentExist, } ); @@ -5689,7 +5689,7 @@ export class ConversationModel { ); return getAbsoluteTempPath(tempPath); } finally { - await deleteAttachmentData(plaintextPath); + await maybeDeleteAttachmentFile(plaintextPath); } } diff --git a/ts/services/contactSync.preload.ts b/ts/services/contactSync.preload.ts index 8112106159..d236a575d1 100644 --- a/ts/services/contactSync.preload.ts +++ b/ts/services/contactSync.preload.ts @@ -22,7 +22,7 @@ import type { ConversationModel } from '../models/conversations.preload.js'; import { validateConversation } from '../util/validateConversation.dom.js'; import { writeNewAttachmentData, - deleteAttachmentData, + maybeDeleteAttachmentFile, doesAttachmentExist, } from '../util/migrations.preload.js'; import { @@ -77,7 +77,7 @@ async function updateConversationFromContactSync( { newAvatar: avatar, writeNewAttachmentData, - deleteAttachmentData, + deleteAttachmentData: maybeDeleteAttachmentFile, doesAttachmentExist, } ); @@ -85,7 +85,7 @@ async function updateConversationFromContactSync( } else { const { attributes } = conversation; if (attributes.avatar && attributes.avatar.path) { - await deleteAttachmentData(attributes.avatar.path); + await maybeDeleteAttachmentFile(attributes.avatar.path); } conversation.set({ avatar: null }); } @@ -143,7 +143,7 @@ async function downloadAndParseContactAttachment( }); } finally { if (downloaded?.path) { - await deleteAttachmentData(downloaded.path); + await maybeDeleteAttachmentFile(downloaded.path); } } } diff --git a/ts/services/expiringMessagesDeletion.preload.ts b/ts/services/expiringMessagesDeletion.preload.ts index ff86370a61..f28f27f909 100644 --- a/ts/services/expiringMessagesDeletion.preload.ts +++ b/ts/services/expiringMessagesDeletion.preload.ts @@ -45,7 +45,7 @@ class ExpiringMessagesDeletionService { inMemoryMessages.push(message); }); - await DataWriter.removeMessages(messageIds, { + await DataWriter.removeMessagesById(messageIds, { cleanupMessages, }); diff --git a/ts/services/writeProfile.preload.ts b/ts/services/writeProfile.preload.ts index d660719272..dc67ccb29f 100644 --- a/ts/services/writeProfile.preload.ts +++ b/ts/services/writeProfile.preload.ts @@ -13,7 +13,7 @@ import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue.preload.js'; import { strictAssert } from '../util/assert.std.js'; import { writeNewAttachmentData, - deleteAttachmentData, + maybeDeleteAttachmentFile, } from '../util/migrations.preload.js'; import { isWhitespace } from '../util/whitespaceStringUtil.std.js'; import { imagePathToBytes } from '../util/imagePathToBytes.dom.js'; @@ -117,7 +117,7 @@ export async function writeProfile( log.info('removing old avatar and saving the new one'); const [local] = await Promise.all([ writeNewAttachmentData(newAvatar), - rawAvatarPath ? deleteAttachmentData(rawAvatarPath) : undefined, + rawAvatarPath ? maybeDeleteAttachmentFile(rawAvatarPath) : undefined, ]); maybeProfileAvatarUpdate = { profileAvatar: { hash, ...local }, @@ -128,7 +128,7 @@ export async function writeProfile( } else if (rawAvatarPath) { log.info('removing avatar'); await Promise.all([ - deleteAttachmentData(rawAvatarPath), + maybeDeleteAttachmentFile(rawAvatarPath), itemStorage.put('avatarUrl', undefined), ]); diff --git a/ts/sql/Client.preload.ts b/ts/sql/Client.preload.ts index ce71232f82..cea27385d6 100644 --- a/ts/sql/Client.preload.ts +++ b/ts/sql/Client.preload.ts @@ -17,7 +17,7 @@ import { deleteExternalFiles } from '../types/Conversation.node.js'; import { createBatcher } from '../util/batcher.std.js'; import { assertDev, softAssert } from '../util/assert.std.js'; import { mapObjectWithSpec } from '../util/mapObjectWithSpec.std.js'; -import { deleteAttachmentData } from '../util/migrations.preload.js'; +import { maybeDeleteAttachmentFile } from '../util/migrations.preload.js'; import { cleanDataForIpc } from './cleanDataForIpc.std.js'; import createTaskWithTimeout from '../textsecure/TaskWithTimeout.std.js'; import { isValidUuid, isValidUuidV7 } from '../util/isValidUuid.std.js'; @@ -66,6 +66,7 @@ import type { StoredKyberPreKeyType, ClientOnlyReadableInterface, ClientOnlyWritableInterface, + RemoveMessageOptions, } from './Interface.std.js'; import { AttachmentDownloadSource } from './Interface.std.js'; import type { MessageAttributesType } from '../model-types.d.ts'; @@ -132,8 +133,8 @@ const clientOnlyWritable: ClientOnlyWritableInterface = { updateConversation, removeConversation, - removeMessage, - removeMessages, + removeMessageById, + removeMessagesById, saveMessage, saveMessages, @@ -560,7 +561,7 @@ async function removeConversation(id: string): Promise { if (existing) { await writableChannel.removeConversation(id); await deleteExternalFiles(existing, { - deleteAttachmentData, + deleteAttachmentData: maybeDeleteAttachmentFile, }); } } @@ -691,67 +692,30 @@ async function saveMessagesIndividually( return result; } -async function removeMessage( - id: string, - options: { - cleanupMessages: ( - messages: ReadonlyArray, - options: { fromSync?: boolean } - ) => Promise; - fromSync?: boolean; - } -): Promise { - const message = await readableChannel.getMessageById(id); - - // Note: It's important to have a fully database-hydrated model to delete here because - // it needs to delete all associated on-disk files along with the database delete. - if (message) { - await writableChannel.removeMessage(id); - await options.cleanupMessages([message], { - fromSync: options.fromSync, - }); - } +async function removeMessageById( + messageId: string, + options: RemoveMessageOptions +) { + return removeMessagesById([messageId], options); } -export async function deleteAndCleanup( - messages: Array, - logId: string, - options: { - fromSync?: boolean; - cleanupMessages: ( - messages: ReadonlyArray, - options: { fromSync?: boolean } - ) => Promise; - } -): Promise { - const ids = messages.map(message => message.id); - - log.info(`deleteAndCleanup/${logId}: Deleting ${ids.length} messages...`); - await writableChannel.removeMessages(ids); - - log.info(`deleteAndCleanup/${logId}: Cleanup for ${ids.length} messages...`); - await options.cleanupMessages(messages, { - fromSync: Boolean(options.fromSync), - }); - - log.info(`deleteAndCleanup/${logId}: Complete`); -} - -async function removeMessages( +async function removeMessagesById( messageIds: ReadonlyArray, - options: { - fromSync?: boolean; - cleanupMessages: ( - messages: ReadonlyArray, - options: { fromSync?: boolean } - ) => Promise; - } + options: RemoveMessageOptions ): Promise { const messages = await readableChannel.getMessagesById(messageIds); + return removeMessages(messages, options); +} + +export async function removeMessages( + messages: ReadonlyArray, + options: RemoveMessageOptions +): Promise { + const messageIds = messages.map(msg => msg.id); + await writableChannel.removeMessages(messageIds); await options.cleanupMessages(messages, { fromSync: Boolean(options.fromSync), }); - await writableChannel.removeMessages(messageIds); } async function getNewerMessagesByConversation( @@ -832,7 +796,7 @@ async function removeMessagesInConversation( } // eslint-disable-next-line no-await-in-loop - await deleteAndCleanup(messages, logId, { fromSync, cleanupMessages }); + await removeMessages(messages, { fromSync, cleanupMessages }); } while (messages.length > 0); } diff --git a/ts/sql/Interface.std.ts b/ts/sql/Interface.std.ts index 643bfb29d4..760ecb9f18 100644 --- a/ts/sql/Interface.std.ts +++ b/ts/sql/Interface.std.ts @@ -78,6 +78,7 @@ import type { RemoteMegaphoneType, } from '../types/Megaphone.std.js'; import { sqlFragment, sqlId, sqlJoin } from './util.std.js'; +import type { MIMEType } from '../types/MIME.std.js'; export type ReadableDB = Database & { __readable_db: never }; export type WritableDB = ReadableDB & { __writable_db: never }; @@ -803,6 +804,30 @@ strictAssert( 'attachment_columns must match DB fields type' ); +export type ExistingAttachmentData = Pick< + MessageAttachmentDBType, + | 'version' + | 'path' + | 'localKey' + | 'thumbnailPath' + | 'thumbnailLocalKey' + | 'thumbnailVersion' + | 'thumbnailContentType' + | 'thumbnailSize' + | 'screenshotPath' + | 'screenshotLocalKey' + | 'screenshotVersion' + | 'screenshotContentType' + | 'screenshotSize' +>; + +export type RemoveMessageOptions = { + cleanupMessages: ( + messages: ReadonlyArray, + options: { fromSync?: boolean } + ) => Promise; + fromSync?: boolean; +}; type ReadableInterface = { close: () => void; @@ -963,6 +988,7 @@ type ReadableInterface = { conversationId: string, limit?: number ) => Array; + getAllProtectedAttachmentPaths: () => Array; getUnprocessedCount: () => number; @@ -1049,6 +1075,8 @@ type ReadableInterface = { messageIds: Array ) => Array; + isAttachmentSafeToDelete: (path: string) => boolean; + getMessageCountBySchemaVersion: () => MessageCountBySchemaVersionType; getMessageSampleForSchemaVersion: ( version: number @@ -1409,6 +1437,18 @@ type WritableInterface = { beforeTimestamp: number ) => ReadonlyArray; + getAndProtectExistingAttachmentPath: ({ + plaintextHash, + version, + contentType, + }: { + plaintextHash: string; + version: number; + contentType: MIMEType; + }) => ExistingAttachmentData | undefined; + _protectAttachmentPathFromDeletion: (path: string) => void; + resetProtectedAttachmentPaths: () => void; + removeAll: () => void; removeAllConfiguration: () => void; eraseStorageServiceState: () => void; @@ -1647,25 +1687,10 @@ export type ClientOnlyWritableInterface = ClientInterfaceWrap<{ postSaveUpdates: () => Promise; } ) => { failedIndices: Array }; - removeMessage: ( - id: string, - options: { - fromSync?: boolean; - cleanupMessages: ( - messages: ReadonlyArray, - options: { fromSync?: boolean | undefined } - ) => Promise; - } - ) => void; - removeMessages: ( + removeMessageById: (id: string, options: RemoveMessageOptions) => void; + removeMessagesById: ( ids: ReadonlyArray, - options: { - fromSync?: boolean; - cleanupMessages: ( - messages: ReadonlyArray, - options: { fromSync?: boolean | undefined } - ) => Promise; - } + options: RemoveMessageOptions ) => void; createOrUpdateIdentityKey: (data: IdentityKeyType) => void; diff --git a/ts/sql/Server.node.ts b/ts/sql/Server.node.ts index 800ee77218..673c56cde3 100644 --- a/ts/sql/Server.node.ts +++ b/ts/sql/Server.node.ts @@ -197,6 +197,7 @@ import type { BackupAttachmentDownloadProgress, GetMessagesBetweenOptions, MaybeStaleCallHistory, + ExistingAttachmentData, } from './Interface.std.js'; import { AttachmentDownloadSource, @@ -299,8 +300,9 @@ import type { } from '../types/Colors.std.js'; import { sqlLogger } from './sqlLogger.node.js'; import { permissiveMessageAttachmentSchema } from './server/messageAttachments.std.js'; -import { getFilePathsOwnedByMessage } from '../util/messageFilePaths.std.js'; +import { getFilePathsReferencedByMessage } from '../util/messageFilePaths.std.js'; import { createMessagesOnInsertTrigger } from './migrations/1500-search-polls.std.js'; +import { isValidPlaintextHash } from '../types/Crypto.std.js'; const { forEach, @@ -563,6 +565,8 @@ export const DataReader: ServerReadableInterface = { getAttachmentReferencesForMessages, getMessageCountBySchemaVersion, getMessageSampleForSchemaVersion, + isAttachmentSafeToDelete, + getAllProtectedAttachmentPaths, // Server-only getKnownMessageAttachments, @@ -703,6 +707,10 @@ export const DataWriter: ServerWritableInterface = { removeAllBackupAttachmentDownloadJobs, resetBackupAttachmentDownloadStats, + getAndProtectExistingAttachmentPath, + _protectAttachmentPathFromDeletion, + resetProtectedAttachmentPaths, + getNextAttachmentBackupJobs, saveAttachmentBackupJob, markAllAttachmentBackupJobsInactive, @@ -2918,6 +2926,114 @@ function saveMessageAttachment({ } } +function getAndProtectExistingAttachmentPath( + db: WritableDB, + { + plaintextHash, + version, + contentType, + }: { plaintextHash: string; version: number; contentType: string } +): ExistingAttachmentData | undefined { + if (!isValidPlaintextHash(plaintextHash)) { + logger.error('getAndProtectExistingAttachmentPath: Invalid plaintextHash'); + return; + } + if (version < 2) { + logger.error( + 'getAndProtectExistingAttachmentPath: Invalid version', + version + ); + return; + } + + const [query, params] = sql` + SELECT + path, + version, + localKey, + thumbnailPath, + thumbnailLocalKey, + thumbnailVersion, + thumbnailContentType, + thumbnailSize, + screenshotPath, + screenshotLocalKey, + screenshotVersion, + screenshotContentType, + screenshotSize + FROM message_attachments + WHERE + plaintextHash = ${plaintextHash} AND + path IS NOT NULL AND + version = ${version} AND + contentType = ${contentType} + LIMIT 1; + `; + + const existingData = db.prepare(query).get(params); + + if (!existingData) { + return undefined; + } + + const [protectQuery, protectParams] = sql` + WITH existingMessageAttachmentPaths(path) AS ( + VALUES + (${existingData.path}), + (${existingData.thumbnailPath}), + (${existingData.screenshotPath}) + ) + INSERT OR REPLACE INTO attachments_protected_from_deletion(path) + SELECT path + FROM existingMessageAttachmentPaths + WHERE path IS NOT NULL; + `; + db.prepare(protectQuery).run(protectParams); + + return existingData; +} + +function _protectAttachmentPathFromDeletion( + db: WritableDB, + path: string +): void { + const [protectQuery, protectParams] = sql` + INSERT OR REPLACE INTO attachments_protected_from_deletion(path) + VALUES (${path}); + `; + db.prepare(protectQuery).run(protectParams); +} + +function resetProtectedAttachmentPaths(db: WritableDB): void { + db.prepare('DELETE FROM attachments_protected_from_deletion').run(); +} + +function getAllProtectedAttachmentPaths(db: ReadableDB): Array { + return db + .prepare('SELECT path FROM attachments_protected_from_deletion', { + pluck: true, + }) + .all(); +} + +function isAttachmentSafeToDelete(db: ReadableDB, path: string): boolean { + const [query, params] = sql` + SELECT EXISTS ( + SELECT 1 FROM attachments_protected_from_deletion + WHERE path = ${path} + UNION ALL + SELECT 1 FROM message_attachments + WHERE + path = ${path} OR + thumbnailPath = ${path} OR + screenshotPath = ${path} OR + backupThumbnailPath = ${path} + ); + `; + + return db.prepare(query, { pluck: true }).get(params) === 0; +} + function _testOnlyRemoveMessageAttachments( db: WritableDB, timestamp: number @@ -8382,6 +8498,7 @@ function removeAll(db: WritableDB): void { DELETE FROM attachment_downloads; DELETE FROM attachment_backup_jobs; DELETE FROM attachment_downloads_backup_stats; + DELETE FROM attachments_protected_from_deletion; DELETE FROM backup_cdn_object_metadata; DELETE FROM badgeImageFiles; DELETE FROM badges; @@ -8703,7 +8820,7 @@ function getKnownMessageAttachments( const { messages, cursor: newCursor } = pageMessages(db, innerCursor); for (const message of messages) { const { externalAttachments, externalDownloads } = - getFilePathsOwnedByMessage(message); + getFilePathsReferencedByMessage(message); externalAttachments.forEach(file => attachments.add(file)); externalDownloads.forEach(file => downloads.add(file)); } diff --git a/ts/sql/migrations/1650-protected-attachments.std.ts b/ts/sql/migrations/1650-protected-attachments.std.ts new file mode 100644 index 0000000000..e64d550410 --- /dev/null +++ b/ts/sql/migrations/1650-protected-attachments.std.ts @@ -0,0 +1,53 @@ +// Copyright 2026 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only +import type { WritableDB } from '../Interface.std.js'; + +export default function updateToSchemaVersion1650(db: WritableDB): void { + db.exec(` + CREATE TABLE attachments_protected_from_deletion ( + path TEXT NOT NULL, + UNIQUE (path) + ) STRICT; + `); + + db.exec(` + CREATE INDEX message_attachments_plaintextHash ON message_attachments (plaintextHash); + `); + db.exec(` + CREATE INDEX message_attachments_path ON message_attachments (path); + `); + db.exec(` + CREATE INDEX message_attachments_thumbnailPath ON message_attachments (thumbnailPath); + `); + db.exec(` + CREATE INDEX message_attachments_screenshotPath ON message_attachments (screenshotPath); + `); + db.exec(` + CREATE INDEX message_attachments_backupThumbnailPath ON message_attachments (backupThumbnailPath); + `); + + db.exec(` + CREATE TRIGGER stop_protecting_attachments_after_update + AFTER UPDATE OF path, thumbnailPath, screenshotPath, backupThumbnailPath + ON message_attachments + WHEN + OLD.path IS NOT NEW.path OR + OLD.thumbnailPath IS NOT NEW.thumbnailPath OR + OLD.screenshotPath IS NOT NEW.screenshotPath OR + OLD.backupThumbnailPath IS NOT NEW.backupThumbnailPath + BEGIN + DELETE FROM attachments_protected_from_deletion + WHERE path IN (NEW.path, NEW.thumbnailPath, NEW.screenshotPath, NEW.backupThumbnailPath); + END; + `); + + db.exec(` + CREATE TRIGGER stop_protecting_attachments_after_insert + AFTER INSERT + ON message_attachments + BEGIN + DELETE FROM attachments_protected_from_deletion + WHERE path IN (NEW.path, NEW.thumbnailPath, NEW.screenshotPath, NEW.backupThumbnailPath); + END; + `); +} diff --git a/ts/sql/migrations/index.node.ts b/ts/sql/migrations/index.node.ts index 1cfd857521..89fbe72b41 100644 --- a/ts/sql/migrations/index.node.ts +++ b/ts/sql/migrations/index.node.ts @@ -141,6 +141,7 @@ import updateToSchemaVersion1610 from './1610-has-contacts.std.js'; import updateToSchemaVersion1620 from './1620-sort-bigger-media.std.js'; import updateToSchemaVersion1630 from './1630-message-pin-message-data.std.js'; import updateToSchemaVersion1640 from './1640-key-transparency.std.js'; +import updateToSchemaVersion1650 from './1650-protected-attachments.std.js'; import { DataWriter } from '../Server.node.js'; @@ -1642,6 +1643,7 @@ export const SCHEMA_VERSIONS: ReadonlyArray = [ { version: 1620, update: updateToSchemaVersion1620 }, { version: 1630, update: updateToSchemaVersion1630 }, { version: 1640, update: updateToSchemaVersion1640 }, + { version: 1650, update: updateToSchemaVersion1650 }, ]; export class DBVersionFromFutureError extends Error { diff --git a/ts/state/ducks/conversations.preload.ts b/ts/state/ducks/conversations.preload.ts index 323f654ece..179cfca434 100644 --- a/ts/state/ducks/conversations.preload.ts +++ b/ts/state/ducks/conversations.preload.ts @@ -1982,7 +1982,7 @@ function deleteMessages({ } } - await DataWriter.removeMessages(messageIds, { + await DataWriter.removeMessagesById(messageIds, { cleanupMessages, }); diff --git a/ts/state/ducks/stories.preload.ts b/ts/state/ducks/stories.preload.ts index d87251ee26..265559302e 100644 --- a/ts/state/ducks/stories.preload.ts +++ b/ts/state/ducks/stories.preload.ts @@ -304,7 +304,7 @@ function deleteGroupStoryReply( messageId: string ): ThunkAction { return async dispatch => { - await DataWriter.removeMessage(messageId, { cleanupMessages }); + await DataWriter.removeMessageById(messageId, { cleanupMessages }); dispatch({ type: STORY_REPLY_DELETED, payload: messageId, @@ -1425,7 +1425,7 @@ function removeAllContactStories( log.info(`${logId}: removing ${messages.length} stories`); - await DataWriter.removeMessages(messageIds, { cleanupMessages }); + await DataWriter.removeMessagesById(messageIds, { cleanupMessages }); dispatch({ type: 'NOOP', diff --git a/ts/test-electron/ContactsParser_test.preload.ts b/ts/test-electron/ContactsParser_test.preload.ts index 318504ca42..baf05f53b6 100644 --- a/ts/test-electron/ContactsParser_test.preload.ts +++ b/ts/test-electron/ContactsParser_test.preload.ts @@ -21,7 +21,7 @@ import * as Bytes from '../Bytes.std.js'; import * as Errors from '../types/errors.std.js'; import { getAbsoluteAttachmentPath, - deleteAttachmentData, + maybeDeleteAttachmentFile, readAttachmentData, } from '../util/migrations.preload.js'; import { APPLICATION_OCTET_STREAM } from '../types/MIME.std.js'; @@ -83,7 +83,7 @@ describe('ContactsParser', () => { await Promise.all(contacts.map(contact => verifyContact(contact))); } finally { if (path) { - await deleteAttachmentData(path); + await maybeDeleteAttachmentFile(path); } } }); @@ -236,7 +236,7 @@ async function verifyContact( strictAssert(contact.avatar?.path, 'Avatar needs path'); const avatarBytes = await readAttachmentData(contact.avatar); - await deleteAttachmentData(contact.avatar.path); + await maybeDeleteAttachmentFile(contact.avatar.path); for (let j = 0; j < 255; j += 1) { assert.strictEqual(avatarBytes[j], j); diff --git a/ts/test-electron/Crypto_test.preload.ts b/ts/test-electron/Crypto_test.preload.ts index bcb59b1fdd..9276f2f5a9 100644 --- a/ts/test-electron/Crypto_test.preload.ts +++ b/ts/test-electron/Crypto_test.preload.ts @@ -57,7 +57,7 @@ import { getAesCbcCiphertextSize, getAttachmentCiphertextSize, } from '../util/AttachmentCrypto.std.js'; -import { getPath } from '../windows/main/attachments.preload.js'; +import { getAttachmentsPath } from '../windows/main/attachments.preload.js'; import { MediaTier } from '../types/AttachmentDownload.std.js'; import { deriveAccessKeyFromProfileKey } from '../util/zkgroup.node.js'; @@ -645,7 +645,9 @@ describe('Crypto', () => { describe('decryptAttachmentV2ToSink', () => { afterEach(async () => { - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); }); it('throws if digest is wrong', async () => { diff --git a/ts/test-electron/backup/filePointer_test.preload.ts b/ts/test-electron/backup/filePointer_test.preload.ts index 4f33f74f28..14fe421e24 100644 --- a/ts/test-electron/backup/filePointer_test.preload.ts +++ b/ts/test-electron/backup/filePointer_test.preload.ts @@ -24,7 +24,7 @@ import { strictAssert } from '../../util/assert.std.js'; import { isValidAttachmentKey } from '../../types/Crypto.std.js'; import { itemStorage } from '../../textsecure/Storage.preload.js'; import { getAbsoluteAttachmentPath } from '../../util/migrations.preload.js'; -import { getPath } from '../../../app/attachments.node.js'; +import { getAttachmentsPath } from '../../../app/attachments.node.js'; import { sha256 } from '../../Crypto.node.js'; describe('convertFilePointerToAttachment', () => { @@ -232,7 +232,9 @@ describe('getFilePointerForAttachment', () => { afterEach(async () => { sandbox.restore(); - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); }); it('if missing key, generates a new one and removes transit info & digest', async () => { diff --git a/ts/test-electron/cleanupOrphanedAttachments_test.preload.ts b/ts/test-electron/cleanupOrphanedAttachments_test.preload.ts index 2b7f049e18..7d0255b487 100644 --- a/ts/test-electron/cleanupOrphanedAttachments_test.preload.ts +++ b/ts/test-electron/cleanupOrphanedAttachments_test.preload.ts @@ -18,7 +18,7 @@ import { import { getDownloadsPath, getDraftPath, - getPath, + getAttachmentsPath, } from '../windows/main/attachments.preload.js'; import { generateAci } from '../types/ServiceId.std.js'; @@ -70,13 +70,17 @@ describe('cleanupOrphanedAttachments', () => { attachmentIndex = 0; downloadIndex = 0; - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); await emptyDir(getDownloadsPath(window.SignalContext.config.userDataPath)); await emptyDir(getDraftPath(window.SignalContext.config.userDataPath)); }); afterEach(async () => { - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); await emptyDir(getDownloadsPath(window.SignalContext.config.userDataPath)); await emptyDir(getDraftPath(window.SignalContext.config.userDataPath)); }); @@ -259,6 +263,37 @@ describe('cleanupOrphanedAttachments', () => { ); }); + it('does not delete any protected attachment paths', async () => { + await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE + 5, 'attachment'); + await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE + 5, 'download'); + + await DataWriter.saveMessage(composeMessageWithAllAttachments(), { + ourAci: generateAci(), + forceSave: true, + postSaveUpdates: () => Promise.resolve(), + }); + + await DataWriter._protectAttachmentPathFromDeletion( + `attachment${attachmentIndex + 1}` + ); + await DataWriter.cleanupOrphanedAttachments({ _block: true }); + + assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); + + const attachmentFiles = listFiles('attachment'); + + assert.strictEqual( + attachmentFiles.length, + NUM_ATTACHMENT_FILES_IN_MESSAGE + 1 + ); + assert.sameDeepMembers( + attachmentFiles, + new Array(attachmentIndex + 1) + .fill(null) + .map((_, idx) => `attachment${idx + 1}`) + ); + }); + it('works with non-normalized message attachments', async () => { await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE + 5, 'attachment'); await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE + 5, 'download'); diff --git a/ts/test-electron/deleteMessageAttachments_test.preload.ts b/ts/test-electron/deleteMessageAttachments_test.preload.ts index 582836461e..592cd6ec99 100644 --- a/ts/test-electron/deleteMessageAttachments_test.preload.ts +++ b/ts/test-electron/deleteMessageAttachments_test.preload.ts @@ -10,22 +10,30 @@ import { dirname } from 'node:path'; import { missingCaseError } from '../util/missingCaseError.std.js'; import { getDownloadsPath, - getPath, + getAttachmentsPath, } from '../windows/main/attachments.preload.js'; import { IMAGE_JPEG, LONG_MESSAGE } from '../types/MIME.std.js'; import type { MessageAttributesType } from '../model-types.d.ts'; import type { AttachmentType } from '../types/Attachment.std.js'; -import { deleteAllAttachmentFilesOnDisk } from '../util/Attachment.std.js'; import { getAbsoluteAttachmentPath, getAbsoluteDownloadsPath, getAbsoluteDraftPath, - deleteAttachmentData, - deleteDownloadData, - deleteExternalMessageFiles, + maybeDeleteAttachmentFile, } from '../util/migrations.preload.js'; import { strictAssert } from '../util/assert.std.js'; +import { + cleanupAllMessageAttachmentFiles, + cleanupAttachmentFiles, +} from '../types/Message2.preload.js'; +import { DataReader, DataWriter } from '../sql/Client.preload.js'; +import { generateAci } from '../types/ServiceId.std.js'; +import { + testAttachmentLocalKey, + testPlaintextHash, +} from '../test-helpers/attachments.node.js'; +import { cleanupMessages } from '../util/cleanup.preload.js'; const { emptyDir, ensureFile } = fsExtra; @@ -56,7 +64,7 @@ async function writeFiles( num: number, type: 'attachment' | 'download' | 'draft' ) { - for (let i = 1; i <= num; i += 1) { + for (let i = 0; i < num; i += 1) { // eslint-disable-next-line no-await-in-loop await writeFile(`${type}${i}`, type); } @@ -69,48 +77,68 @@ function listFiles(type: 'attachment' | 'download' | 'draft'): Array { let attachmentIndex = 0; let downloadIndex = 0; -describe('Attachment deletion', () => { +describe('deleteMessageAttachments', () => { beforeEach(async () => { attachmentIndex = 0; downloadIndex = 0; - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await DataWriter.removeAll(); + await window.ConversationController.reset(); + await window.ConversationController.load(); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); await emptyDir(getDownloadsPath(window.SignalContext.config.userDataPath)); }); afterEach(async () => { - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await DataWriter.removeAll(); + await window.ConversationController.reset(); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); await emptyDir(getDownloadsPath(window.SignalContext.config.userDataPath)); }); function getAttachmentFilePath() { + const path = `attachment${attachmentIndex}`; attachmentIndex += 1; - return `attachment${attachmentIndex}`; + return path; } function getDownloadFilePath() { + const path = `download${downloadIndex}`; downloadIndex += 1; - return `download${downloadIndex}`; + return path; } function composeAttachment(): AttachmentType { return { contentType: IMAGE_JPEG, size: 128, + version: 2, path: getAttachmentFilePath(), + localKey: testAttachmentLocalKey(), downloadPath: getDownloadFilePath(), + plaintextHash: testPlaintextHash(), thumbnail: { contentType: IMAGE_JPEG, size: 128, + version: 2, path: getAttachmentFilePath(), + localKey: testAttachmentLocalKey(), }, screenshot: { contentType: IMAGE_JPEG, size: 128, + version: 2, path: getAttachmentFilePath(), + localKey: testAttachmentLocalKey(), }, thumbnailFromBackup: { contentType: IMAGE_JPEG, size: 128, + version: 2, path: getAttachmentFilePath(), + localKey: testAttachmentLocalKey(), }, }; } @@ -124,58 +152,23 @@ describe('Attachment deletion', () => { }; } - it('deleteAllAttachmentFilesOnDisk deletes all paths referenced', async () => { - await writeFiles(5, 'attachment'); - await writeFiles(3, 'download'); - - await deleteAllAttachmentFilesOnDisk({ - deleteAttachmentOnDisk: deleteAttachmentData, - deleteDownloadOnDisk: deleteDownloadData, - })(composeAttachment()); - - assert.strictEqual(attachmentIndex, 4); - assert.sameDeepMembers(listFiles('attachment'), ['attachment5']); - assert.sameDeepMembers(listFiles('download'), ['download2', 'download3']); - }); - - it('deleteAllAttachmentFilesOnDisk does not delete files for copied attachments', async () => { - await writeFiles(5, 'attachment'); - await writeFiles(5, 'download'); - - const attachment = composeAttachment(); - attachment.copied = true; - - await deleteAllAttachmentFilesOnDisk({ - deleteAttachmentOnDisk: deleteAttachmentData, - deleteDownloadOnDisk: deleteDownloadData, - })(attachment); - - assert.sameDeepMembers(listFiles('attachment'), [ - 'attachment1', - 'attachment2', - 'attachment3', - 'attachment4', - 'attachment5', - ]); - assert.sameDeepMembers(listFiles('download'), [ - 'download1', - 'download2', - 'download3', - 'download4', - 'download5', - ]); - }); - // Update these if more paths are added to composeMessageWithAllAttachments - const NUM_ATTACHMENT_FILES_IN_MESSAGE = 42; - const NUM_DOWNLOAD_FILES_IN_MESSAGE = 12; - function composeMessageWithAllAttachments(): MessageAttributesType { - const message: MessageAttributesType = { + function composeMessage(): MessageAttributesType { + return { id: generateUuid(), type: 'outgoing', sent_at: Date.now(), timestamp: Date.now(), received_at: Date.now(), conversationId: generateUuid(), + }; + } + + // Update these if more paths are added to composeMessageWithAllAttachments + const NUM_ATTACHMENT_FILES_IN_MESSAGE = 42; + const NUM_DOWNLOAD_FILES_IN_MESSAGE = 12; + function composeMessageWithAllAttachments(): MessageAttributesType { + const message: MessageAttributesType = { + ...composeMessage(), bodyAttachment: composeBodyAttachment(), attachments: [composeAttachment(), composeAttachment()], contact: [ @@ -247,58 +240,352 @@ describe('Attachment deletion', () => { return message; } - it('deleteExternalMessageFiles deletes all message attachments, including editHistory', async () => { - await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE + 3, 'attachment'); - await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE + 3, 'download'); - const message = composeMessageWithAllAttachments(); + describe('isSafeToDeleteAttachment', () => { + beforeEach(async () => { + await writeFiles(5, 'attachment'); + }); - await deleteExternalMessageFiles(message); + it('is safe to delete if no references', async () => { + assert.isTrue(await DataReader.isAttachmentSafeToDelete('attachment0')); + }); + it('is not safe to delete if a message references it', async () => { + const attachment1: AttachmentType = { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment0', + version: 2, + thumbnail: { size: 1, contentType: IMAGE_JPEG, path: 'attachment1' }, + screenshot: { size: 1, contentType: IMAGE_JPEG, path: 'attachment2' }, + thumbnailFromBackup: { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment3', + }, + }; - assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); - assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); + await DataWriter.saveMessage( + { ...composeMessage(), attachments: [attachment1] }, + { + ourAci: generateAci(), + forceSave: true, + postSaveUpdates: () => Promise.resolve(), + } + ); - assert.sameDeepMembers(listFiles('attachment'), [ - 'attachment43', - 'attachment44', - 'attachment45', - ]); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment0')); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment1')); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment2')); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment3')); + assert.isTrue(await DataReader.isAttachmentSafeToDelete('attachment4')); - assert.sameDeepMembers(listFiles('download'), [ - 'download13', - 'download14', - 'download15', - ]); + assert.deepStrictEqual(await maybeDeleteAttachmentFile('attachment0'), { + wasDeleted: false, + }); + assert.deepStrictEqual(await maybeDeleteAttachmentFile('attachment1'), { + wasDeleted: false, + }); + assert.deepStrictEqual(await maybeDeleteAttachmentFile('attachment2'), { + wasDeleted: false, + }); + assert.deepStrictEqual(await maybeDeleteAttachmentFile('attachment3'), { + wasDeleted: false, + }); + assert.deepStrictEqual(await maybeDeleteAttachmentFile('attachment4'), { + wasDeleted: true, + }); + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment0', + 'attachment1', + 'attachment2', + 'attachment3', + ]); + }); + + it('is not safe to delete if the file is protected, even if no references', async () => { + const attachment = { + size: 1, + version: 2, + contentType: IMAGE_JPEG, + plaintextHash: testPlaintextHash(), + path: 'attachment0', + thumbnail: { size: 1, contentType: IMAGE_JPEG, path: 'attachment1' }, + screenshot: { size: 1, contentType: IMAGE_JPEG, path: 'attachment2' }, + // thumbnailFromBackup is not protected + thumbnailFromBackup: { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment3', + }, + } as const; + const message = { ...composeMessage(), attachments: [attachment] }; + + await DataWriter.saveMessage(message, { + ourAci: generateAci(), + forceSave: true, + postSaveUpdates: () => Promise.resolve(), + }); + + await DataWriter.getAndProtectExistingAttachmentPath({ + plaintextHash: attachment.plaintextHash, + version: 2, + contentType: attachment.contentType, + }); + + await DataWriter.removeMessageById(message.id, { + cleanupMessages: () => Promise.resolve(), + }); + + assert.isUndefined(await DataReader.getMessageById(message.id)); + + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment0')); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment1')); + assert.isFalse(await DataReader.isAttachmentSafeToDelete('attachment2')); + assert.isTrue(await DataReader.isAttachmentSafeToDelete('attachment3')); + assert.isTrue(await DataReader.isAttachmentSafeToDelete('attachment4')); + }); }); - it('deleteExternalMessageFiles does not delete copied quote attachments', async () => { - await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE + 3, 'attachment'); - await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE + 3, 'download'); - const message = composeMessageWithAllAttachments(); + describe('cleanupAllMessageAttachmentFiles', () => { + it('deletes all referenced files, including those in editHistory', async () => { + await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE + 3, 'attachment'); + await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE + 3, 'download'); + const message = composeMessageWithAllAttachments(); - const quotedThumbnail = message.quote?.attachments[0].thumbnail; - strictAssert(quotedThumbnail, 'thumbnail exists'); - quotedThumbnail.copied = true; + await cleanupAllMessageAttachmentFiles(message); - await deleteExternalMessageFiles(message); + assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); + assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); - assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); - assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment42', + 'attachment43', + 'attachment44', + ]); - assert.sameDeepMembers(listFiles('attachment'), [ - quotedThumbnail.path, - quotedThumbnail.thumbnail?.path, - quotedThumbnail.thumbnailFromBackup?.path, - quotedThumbnail.screenshot?.path, - 'attachment43', - 'attachment44', - 'attachment45', - ]); + assert.sameDeepMembers(listFiles('download'), [ + 'download12', + 'download13', + 'download14', + ]); + }); - assert.sameDeepMembers(listFiles('download'), [ - quotedThumbnail.downloadPath, - 'download13', - 'download14', - 'download15', - ]); + it('does not delete any attachment file if message is still saved, but does cleanup downloads', async () => { + await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE, 'attachment'); + await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE, 'download'); + const message = composeMessageWithAllAttachments(); + assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); + assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); + await DataWriter.saveMessage(message, { + forceSave: true, + ourAci: generateAci(), + postSaveUpdates: () => Promise.resolve(), + }); + + await cleanupAllMessageAttachmentFiles(message); + + assert.strictEqual( + listFiles('attachment').length, + NUM_ATTACHMENT_FILES_IN_MESSAGE + ); + assert.strictEqual(listFiles('download').length, 0); + }); + + it('does not delete an attachment file if referenced by another message', async () => { + await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE, 'attachment'); + await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE, 'download'); + const message1 = composeMessageWithAllAttachments(); + const duplicatedAttachment: AttachmentType = message1.attachments?.[0]; + const message2: MessageAttributesType = { + ...composeMessage(), + attachments: [duplicatedAttachment], + }; + assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); + assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); + + await DataWriter.saveMessage(message2, { + forceSave: true, + ourAci: generateAci(), + postSaveUpdates: () => Promise.resolve(), + }); + + await cleanupAllMessageAttachmentFiles(message1); + + assert.sameDeepMembers(listFiles('attachment'), [ + duplicatedAttachment.path, + duplicatedAttachment.thumbnail?.path, + duplicatedAttachment.screenshot?.path, + duplicatedAttachment.thumbnailFromBackup?.path, + ]); + assert.strictEqual(listFiles('download').length, 0); + }); + + it('does not delete an attachment path if protected', async () => { + await writeFiles(NUM_ATTACHMENT_FILES_IN_MESSAGE, 'attachment'); + await writeFiles(NUM_DOWNLOAD_FILES_IN_MESSAGE, 'download'); + const message1 = composeMessageWithAllAttachments(); + const attachment1: AttachmentType = message1.attachments?.[0]; + const message2 = { ...composeMessage() }; + + assert.strictEqual(attachmentIndex, NUM_ATTACHMENT_FILES_IN_MESSAGE); + assert.strictEqual(downloadIndex, NUM_DOWNLOAD_FILES_IN_MESSAGE); + + strictAssert(attachment1.plaintextHash, 'plaintextHash exists'); + strictAssert(attachment1.version, 'version exists'); + await DataWriter.saveMessage(message1, { + forceSave: true, + ourAci: generateAci(), + postSaveUpdates: () => Promise.resolve(), + }); + + // protect existing attachment paths + const existingAttachment = + await DataWriter.getAndProtectExistingAttachmentPath({ + plaintextHash: attachment1.plaintextHash, + version: attachment1.version, + contentType: attachment1.contentType, + }); + + assert.strictEqual(existingAttachment?.path, attachment1.path); + assert.strictEqual(existingAttachment?.localKey, attachment1.localKey); + + // delete existing message (e.g. before the new message using the attachment has + // been saved) + await DataWriter.removeMessageById(message1.id, { cleanupMessages }); + + await cleanupAllMessageAttachmentFiles(message1); + + assert.sameDeepMembers(listFiles('attachment'), [ + attachment1.path, + attachment1.thumbnail?.path, + attachment1.screenshot?.path, + ]); + assert.strictEqual(listFiles('download').length, 0); + await DataWriter.removeMessageById(message2.id, { cleanupMessages }); + await cleanupAllMessageAttachmentFiles(message2); + }); + }); + + describe('cleanupAttachmentFiles', () => { + beforeEach(async () => { + await writeFiles(5, 'attachment'); + await writeFiles(5, 'download'); + }); + + it('cleans up attachment files', async () => { + const attachment: AttachmentType = { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment0', + version: 2, + downloadPath: 'download0', + thumbnail: { size: 1, contentType: IMAGE_JPEG, path: 'attachment1' }, + screenshot: { size: 1, contentType: IMAGE_JPEG, path: 'attachment2' }, + thumbnailFromBackup: { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment3', + }, + }; + await cleanupAttachmentFiles(attachment); + assert.sameDeepMembers(listFiles('attachment'), ['attachment4']); + assert.sameDeepMembers(listFiles('download'), [ + 'download1', + 'download2', + 'download3', + 'download4', + ]); + }); + it('does not delete files if referenced', async () => { + const attachment: AttachmentType = { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment0', + version: 2, + downloadPath: 'download0', + thumbnail: { size: 1, contentType: IMAGE_JPEG, path: 'attachment1' }, + screenshot: { size: 1, contentType: IMAGE_JPEG, path: 'attachment2' }, + thumbnailFromBackup: { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment3', + }, + }; + + await DataWriter.saveMessage( + { ...composeMessage(), attachments: [attachment] }, + { + ourAci: generateAci(), + forceSave: true, + postSaveUpdates: () => Promise.resolve(), + } + ); + await cleanupAttachmentFiles(attachment); + // Only downloadPath gets cleaned up + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment0', + 'attachment1', + 'attachment2', + 'attachment3', + 'attachment4', + ]); + assert.sameDeepMembers(listFiles('download'), [ + 'download1', + 'download2', + 'download3', + 'download4', + ]); + }); + + it('does not delete files if protected', async () => { + const attachment: AttachmentType = { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment0', + version: 2, + downloadPath: 'download0', + thumbnail: { size: 1, contentType: IMAGE_JPEG, path: 'attachment1' }, + screenshot: { size: 1, contentType: IMAGE_JPEG, path: 'attachment2' }, + thumbnailFromBackup: { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment3', + }, + }; + + await DataWriter._protectAttachmentPathFromDeletion('attachment0'); + await cleanupAttachmentFiles(attachment); + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment0', + 'attachment4', + ]); + }); + it('does not delete copied quote thumbnails', async () => { + const attachment: AttachmentType = { + size: 1, + contentType: IMAGE_JPEG, + path: 'attachment0', + version: 2, + copied: true, + }; + + await cleanupAttachmentFiles(attachment); + // not cleaned up + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment0', + 'attachment1', + 'attachment2', + 'attachment3', + 'attachment4', + ]); + + // sanity check: if not copied, gets cleaned up + await cleanupAttachmentFiles({ ...attachment, copied: false }); + assert.sameDeepMembers(listFiles('attachment'), [ + 'attachment1', + 'attachment2', + 'attachment3', + 'attachment4', + ]); + }); }); }); diff --git a/ts/test-electron/normalizedAttachments_test.preload.ts b/ts/test-electron/normalizedAttachments_test.preload.ts index 9c973ef7b6..7c64627e25 100644 --- a/ts/test-electron/normalizedAttachments_test.preload.ts +++ b/ts/test-electron/normalizedAttachments_test.preload.ts @@ -25,12 +25,15 @@ import { SeenStatus } from '../MessageSeenStatus.std.js'; import { DataWriter, DataReader } from '../sql/Client.preload.js'; import { strictAssert } from '../util/assert.std.js'; import { HOUR, MINUTE } from '../util/durations/index.std.js'; +import { + testAttachmentDigest, + testAttachmentKey, + testAttachmentLocalKey, + testPlaintextHash, +} from '../test-helpers/attachments.node.js'; const CONTACT_A = generateAci(); const contactAConversationId = generateGuid(); -function getBase64(str: string): string { - return Bytes.toBase64(Bytes.fromString(str)); -} function composeThumbnail( index: number, @@ -82,20 +85,20 @@ function composeAttachment( // Make sure you add a column to the `message_attachments` table and update // MESSAGE_ATTACHMENT_COLUMNS. ): Required> { - const label = `${key ?? 'attachment'}${index}`; + const label = key ?? `attachment${index}`; const attachment = { cdnKey: `cdnKey${label}`, cdnNumber: 3, - key: getBase64(`key${label}`), - digest: getBase64(`digest${label}`), + key: testAttachmentKey(), + digest: testAttachmentDigest(), duration: 123, size: 100, downloadPath: 'downloadPath', contentType: IMAGE_JPEG, - path: `path/to/file${label}`, + path: `path/to/file.${label}`, pending: false, - localKey: 'localKey', - plaintextHash: `plaintextHash${label}`, + localKey: testAttachmentLocalKey(), + plaintextHash: testPlaintextHash(), uploadTimestamp: index, clientUuid: generateGuid(), width: 100, @@ -164,9 +167,144 @@ function composeMessage( }; } +function composeMessageWithEveryAttachment() { + const attachment1 = composeAttachment('attachment1'); + const attachment2 = composeAttachment('attachment2'); + const previewAttachment = composeAttachment('preview'); + const quoteAttachment = composeAttachment('quote'); + const contactAttachment = composeAttachment('contact'); + const stickerAttachment = composeAttachment('sticker'); + const bodyAttachment = composeAttachment('body', { + contentType: LONG_MESSAGE, + }); + + const now = Date.now(); + return composeMessage(now, { + attachments: [attachment1, attachment2], + bodyAttachment, + preview: [ + { + title: 'preview', + description: 'description', + domain: 'domain', + url: 'https://signal.org', + isStickerPack: false, + isCallLink: false, + image: previewAttachment, + date: now, + }, + ], + quote: { + id: now, + referencedMessageNotFound: true, + isViewOnce: false, + messageId: 'quotedMessageId', + attachments: [ + { + contentType: IMAGE_JPEG, + thumbnail: quoteAttachment, + }, + ], + }, + contact: [ + { + name: { + givenName: 'Alice', + familyName: 'User', + }, + avatar: { + isProfile: true, + avatar: contactAttachment, + }, + }, + ], + sticker: { + packId: 'stickerPackId', + stickerId: 123, + packKey: 'abcdefg', + data: stickerAttachment, + }, + editMessageReceivedAt: now + HOUR + 42, + editMessageTimestamp: now + HOUR, + editHistory: [ + { + timestamp: now + HOUR, + received_at: now + HOUR + 42, + attachments: [ + composeAttachment('attachment.edit1.1'), + composeAttachment('attachment.edit1.2'), + ], + bodyAttachment: composeAttachment('body.edit1', { + contentType: LONG_MESSAGE, + }), + preview: [ + { + title: 'preview', + description: 'description', + domain: 'domain', + url: 'https://signal.org', + isStickerPack: false, + isCallLink: true, + image: composeAttachment('preview.edit1'), + date: now, + }, + ], + quote: { + id: now, + referencedMessageNotFound: true, + isViewOnce: false, + messageId: 'quotedMessageId', + attachments: [ + { + contentType: IMAGE_JPEG, + thumbnail: composeAttachment('quote.edit1'), + }, + ], + }, + }, + { + timestamp: now + MINUTE, + received_at: now + MINUTE + 42, + attachments: [ + composeAttachment('attachment.edit2.1'), + composeAttachment('attachment.edit2.2'), + ], + bodyAttachment: composeAttachment('body.edit2', { + contentType: LONG_MESSAGE, + }), + preview: [ + { + title: 'preview', + description: 'description', + domain: 'domain', + url: 'https://signal.org', + isStickerPack: false, + isCallLink: true, + image: composeAttachment('preview.edit2'), + date: now, + }, + ], + quote: { + id: now, + referencedMessageNotFound: true, + isViewOnce: false, + messageId: 'quotedMessageId', + attachments: [ + { + contentType: IMAGE_JPEG, + thumbnail: composeAttachment('quote.edit2'), + }, + ], + }, + }, + ], + }); +} + describe('normalizes attachment references', () => { beforeEach(async () => { await DataWriter.removeAll(); + index = 0; }); it('saves message with undownloaded attachments', async () => { @@ -236,89 +374,7 @@ describe('normalizes attachment references', () => { }); it('saves and re-hydrates messages with normal, body, preview, quote, contact, and sticker attachments', async () => { - const attachment1 = composeAttachment('first'); - const attachment2 = composeAttachment('second'); - const previewAttachment1 = composeAttachment('preview1'); - const previewAttachment2 = composeAttachment('preview2'); - const quoteAttachment1 = composeAttachment('quote1'); - const quoteAttachment2 = composeAttachment('quote2'); - const contactAttachment1 = composeAttachment('contact1'); - const contactAttachment2 = composeAttachment('contact2'); - const stickerAttachment = composeAttachment('sticker'); - const bodyAttachment = composeAttachment('body', { - contentType: LONG_MESSAGE, - }); - - const message = composeMessage(Date.now(), { - attachments: [attachment1, attachment2], - bodyAttachment, - preview: [ - { - title: 'preview', - description: 'description', - domain: 'domain', - url: 'https://signal.org', - isStickerPack: false, - isCallLink: false, - image: previewAttachment1, - date: Date.now(), - }, - { - title: 'preview2', - description: 'description2', - domain: 'domain2', - url: 'https://signal2.org', - isStickerPack: true, - isCallLink: false, - image: previewAttachment2, - date: Date.now(), - }, - ], - quote: { - id: Date.now(), - referencedMessageNotFound: true, - isViewOnce: false, - messageId: 'quotedMessageId', - attachments: [ - { - contentType: IMAGE_JPEG, - thumbnail: quoteAttachment1, - }, - { - contentType: IMAGE_PNG, - thumbnail: quoteAttachment2, - }, - ], - }, - contact: [ - { - name: { - givenName: 'Alice', - familyName: 'User', - }, - avatar: { - isProfile: true, - avatar: contactAttachment1, - }, - }, - { - name: { - givenName: 'Bob', - familyName: 'User', - }, - avatar: { - isProfile: false, - avatar: contactAttachment2, - }, - }, - ], - sticker: { - packId: 'stickerPackId', - stickerId: 123, - packKey: 'abcdefg', - data: stickerAttachment, - }, - }); + const message = composeMessageWithEveryAttachment(); await DataWriter.saveMessage(message, { forceSave: true, diff --git a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts index 2ea0a3f2a8..1cb969665c 100644 --- a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts +++ b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts @@ -5,8 +5,10 @@ /* eslint-disable @typescript-eslint/no-floating-promises */ import * as sinon from 'sinon'; import { assert } from 'chai'; -import lodash from 'lodash'; -import type { StatsFs } from 'node:fs'; +import lodash, { pick } from 'lodash'; +import { type StatsFs } from 'node:fs'; +import { v7 } from 'uuid'; +import { emptyDir, ensureFile } from 'fs-extra'; import * as MIME from '../../types/MIME.std.js'; import { @@ -33,8 +35,7 @@ import { generateAttachmentKeys } from '../../AttachmentCrypto.node.js'; import { getAttachmentCiphertextSize } from '../../util/AttachmentCrypto.std.js'; import { MEBIBYTE } from '../../types/AttachmentSize.std.js'; import { generateAci } from '../../types/ServiceId.std.js'; -import { toBase64, toHex } from '../../Bytes.std.js'; -import { getRandomBytes } from '../../Crypto.node.js'; +import { toBase64 } from '../../Bytes.std.js'; import { JobCancelReason } from '../../jobs/types.std.js'; import { explodePromise, @@ -44,6 +45,15 @@ 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'; +import { + testAttachmentDigest, + testAttachmentKey, + testAttachmentLocalKey, + testPlaintextHash, +} from '../../test-helpers/attachments.node.js'; +import type { MessageAttributesType } from '../../model-types.js'; +import { getAttachmentsPath } from '../../../app/attachments.node.js'; +import { getAbsoluteAttachmentPath } from '../../util/migrations.preload.js'; const { omit } = lodash; @@ -57,7 +67,7 @@ function composeJob({ jobOverrides?: Partial; }): AttachmentDownloadJobType { const digest = `digestFor${messageId}`; - const plaintextHash = toHex(getRandomBytes(32)); + const plaintextHash = testPlaintextHash(); const size = 128; const contentType = MIME.IMAGE_PNG; return { @@ -83,7 +93,7 @@ function composeJob({ size, digest, plaintextHash, - key: toBase64(generateAttachmentKeys()), + key: testAttachmentKey(), ...attachmentOverrides, }, ...jobOverrides, @@ -630,9 +640,10 @@ describe('AttachmentDownloadManager', () => { downloadStarted.resolve(); }); }), - deleteAttachmentData: sandbox.stub(), - deleteDownloadData: sandbox.stub(), + cleanupAttachmentFiles: sandbox.stub(), + deleteDownloadFile: sandbox.stub(), processNewAttachment: sandbox.stub(), + maybeDeleteAttachmentFile: sandbox.stub(), runDownloadAttachmentJobInner, }, }) @@ -749,8 +760,9 @@ describe('AttachmentDownloadManager', () => { }); describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => { let sandbox: sinon.SinonSandbox; - let deleteAttachmentData: sinon.SinonStub; - let deleteDownloadData: sinon.SinonStub; + let cleanupAttachmentFiles: sinon.SinonStub; + let maybeDeleteAttachmentFile: sinon.SinonStub; + let deleteDownloadFile: sinon.SinonStub; let downloadAttachment: sinon.SinonStub; let processNewAttachment: sinon.SinonStub; @@ -771,8 +783,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => { downloadAttachment = sandbox .stub() .returns(Promise.resolve(downloadedAttachment)); - deleteAttachmentData = sandbox.stub(); - deleteDownloadData = sandbox.stub(); + cleanupAttachmentFiles = sandbox.stub(); + maybeDeleteAttachmentFile = sandbox.stub(); + deleteDownloadFile = sandbox.stub(); processNewAttachment = sandbox.stub().callsFake(attachment => attachment); }); @@ -818,8 +831,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => { }, dependencies: { downloadAttachment, - deleteAttachmentData, - deleteDownloadData, + maybeDeleteAttachmentFile, + cleanupAttachmentFiles, + deleteDownloadFile, processNewAttachment, runDownloadAttachmentJobInner: sandbox.stub().throws( new AttachmentNotNeededForMessageError({ @@ -838,28 +852,31 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => { }); 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'] - ); + assert.strictEqual(cleanupAttachmentFiles.callCount, 1); + assert.deepStrictEqual(cleanupAttachmentFiles.getCall(0).args[0], { + contentType: MIME.IMAGE_PNG, + size: 128, + path: 'main/path', + downloadPath: '/downloadPath', + thumbnail: { + contentType: MIME.IMAGE_PNG, + size: 128, + path: 'thumbnail/path', + }, + }); }); }); describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { let sandbox: sinon.SinonSandbox; - let deleteAttachmentData: sinon.SinonStub; - let deleteDownloadData: sinon.SinonStub; - let downloadAttachment: sinon.SinonStub; + let cleanupAttachmentFiles: sinon.SinonStub; + let maybeDeleteAttachmentFile: sinon.SinonStub; + let deleteDownloadFile: sinon.SinonStub; + let downloadAttachment: sinon.SinonStub< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + any, + ReturnType + >; let processNewAttachment: sinon.SinonStub; const abortController = new AbortController(); @@ -882,7 +899,8 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { downloadAttachment = sandbox .stub() .returns(Promise.resolve(downloadedAttachment)); - + cleanupAttachmentFiles = sandbox.stub(); + maybeDeleteAttachmentFile = sandbox.stub(); processNewAttachment = sandbox.stub().callsFake(attachment => attachment); }); @@ -909,8 +927,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -949,8 +968,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1006,8 +1026,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1045,8 +1066,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1085,8 +1107,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1123,8 +1146,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1180,8 +1204,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { maxTextAttachmentSizeInKib: 2 * MEBIBYTE, messageExpiresAt: null, dependencies: { - deleteAttachmentData, - deleteDownloadData, + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, downloadAttachment, processNewAttachment, }, @@ -1198,4 +1223,386 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { ); }); }); + describe('deduplicates attachment if one exists on disk', () => { + const existingAttachment = { + size: 128, + contentType: MIME.VIDEO_MP4, + version: 2, + plaintextHash: testPlaintextHash(), + path: 'existingPath', + localKey: testAttachmentLocalKey(), + thumbnail: { + path: 'existingThumbnailPath', + version: 2, + localKey: testAttachmentLocalKey(), + contentType: MIME.IMAGE_BMP, + size: 256, + }, + screenshot: { + path: 'existingScreenshotPath', + version: 2, + localKey: testAttachmentLocalKey(), + contentType: MIME.IMAGE_JPEG, + size: 512, + }, + thumbnailFromBackup: { + path: 'shouldbeignored', + contentType: MIME.IMAGE_JPEG, + size: 1024, + }, + } as const satisfies AttachmentType; + + function composeMessage(): MessageAttributesType { + return { + id: v7(), + type: 'incoming', + sent_at: Date.now(), + timestamp: Date.now(), + received_at: Date.now(), + conversationId: v7(), + }; + } + const existingMessageWithDownloadedAttachment = { + ...composeMessage(), + attachments: [existingAttachment], + }; + + const undownloadedAttachment = { + cdnKey: 'cdnKey', + cdnNumber: 3, + version: 2, + key: testAttachmentKey(), + size: 128, + digest: testAttachmentDigest(), + plaintextHash: undefined, + contentType: MIME.VIDEO_MP4, + fileName: 'new filename', + } as const; + + const newMessage = { + ...composeMessage(), + attachments: [undownloadedAttachment], + }; + + async function writeAttachmentFile(path: string) { + await ensureFile(getAbsoluteAttachmentPath(path)); + } + beforeEach(async () => { + await DataWriter.saveMessages( + [existingMessageWithDownloadedAttachment, newMessage], + { + forceSave: true, + ourAci: generateAci(), + postSaveUpdates: async () => Promise.resolve(), + } + ); + }); + afterEach(async () => { + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); + }); + + it('reuses existing attachment based on plaintextHash, version, and contentType', async () => { + await writeAttachmentFile('existingPath'); + await writeAttachmentFile('existingThumbnailPath'); + await writeAttachmentFile('existingScreenshotPath'); + + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version, + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: undownloadedAttachment, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment, + }, + }); + + // Cleans up newly downloaded path + assert.equal(maybeDeleteAttachmentFile.callCount, 1); + assert.isTrue( + maybeDeleteAttachmentFile.calledWith('newlyDownloadedPath') + ); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + const propsThatShouldBeTransferred = [ + 'path', + 'localKey', + 'version', + 'thumbnail.path', + 'thumbnail.localKey', + 'thumbnail.size', + 'thumbnail.version', + 'thumbnail.contentType', + 'screenshot.path', + 'screenshot.localKey', + 'screenshot.size', + 'screenshot.version', + 'screenshot.contentType', + ]; + + assert.strictEqual(attachment?.path, existingAttachment.path); + assert.deepStrictEqual( + pick(attachment, propsThatShouldBeTransferred), + pick(existingAttachment, propsThatShouldBeTransferred) + ); + }); + + it('does not reuse files if contentType differs', async () => { + await writeAttachmentFile('existingPath'); + await writeAttachmentFile('existingThumbnailPath'); + await writeAttachmentFile('existingScreenshotPath'); + + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version, + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: { + ...undownloadedAttachment, + contentType: MIME.VIDEO_QUICKTIME, + }, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment, + }, + }); + + assert.equal(maybeDeleteAttachmentFile.callCount, 0); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + + assert.strictEqual(attachment.contentType, MIME.VIDEO_QUICKTIME); + assert.strictEqual(attachment.path, 'newlyDownloadedPath'); + }); + + it('does not reuse derived files if version differs', async () => { + await writeAttachmentFile('existingPath'); + await writeAttachmentFile('existingThumbnailPath'); + await writeAttachmentFile('existingScreenshotPath'); + + // @ts-expect-error new attachment version + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version + 1, + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: undownloadedAttachment, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment, + }, + }); + + assert.equal(maybeDeleteAttachmentFile.callCount, 0); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + + assert.strictEqual(attachment.path, 'newlyDownloadedPath'); + }); + + it('does not reuse attachment if it does not exist on disk', async () => { + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version, + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: undownloadedAttachment, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment, + }, + }); + + // Cleans up newly downloaded path + assert.equal(maybeDeleteAttachmentFile.callCount, 0); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + assert.strictEqual(attachment?.path, 'newlyDownloadedPath'); + }); + + it('does not reuse thumbnail if it does not exist on disk', async () => { + await writeAttachmentFile('existingPath'); + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version, + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: undownloadedAttachment, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment: sandbox.stub().callsFake(attachment => ({ + ...attachment, + thumbnail: { path: 'newThumbnailPath' }, + })), + }, + }); + + // Cleans up newly downloaded path + assert.equal(maybeDeleteAttachmentFile.callCount, 1); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + assert.strictEqual(attachment?.path, 'existingPath'); + assert.strictEqual(attachment?.thumbnail?.path, 'newThumbnailPath'); + }); + + it('does not reuse attachment if version is not the same as requested version', async () => { + await writeAttachmentFile('existingPath'); + await writeAttachmentFile('existingThumbnailPath'); + await writeAttachmentFile('existingScreenshotPath'); + + // @ts-expect-error version is wrong + downloadAttachment.callsFake(async ({ attachment }) => { + return { + path: 'newlyDownloadedPath', + plaintextHash: existingAttachment.plaintextHash, + version: existingAttachment.version + 1, // Newer version! + localKey: testAttachmentLocalKey(), + size: existingAttachment.size, + digest: attachment.digest, + }; + }); + const job = composeJob({ + messageId: newMessage.id, + receivedAt: newMessage.received_at, + attachmentOverrides: undownloadedAttachment, + }); + + await runDownloadAttachmentJobInner({ + job, + isForCurrentlyVisibleMessage: false, + hasMediaBackups: false, + abortSignal: abortController.signal, + maxAttachmentSizeInKib: 100 * MEBIBYTE, + maxTextAttachmentSizeInKib: 2 * MEBIBYTE, + messageExpiresAt: null, + dependencies: { + cleanupAttachmentFiles, + maybeDeleteAttachmentFile, + deleteDownloadFile, + downloadAttachment, + processNewAttachment, + }, + }); + + // Cleans up newly downloaded path + assert.equal(maybeDeleteAttachmentFile.callCount, 0); + + const updatedMessage = window.MessageCache.getById(newMessage.id); + const attachment = updatedMessage?.attributes.attachments?.[0]; + assert.strictEqual(attachment?.path, 'newlyDownloadedPath'); + }); + }); }); diff --git a/ts/test-electron/sql/attachment_download_backup_stats_test.preload.ts b/ts/test-electron/sql/attachment_download_backup_stats_test.preload.ts index e8faee93d6..b487fc4a60 100644 --- a/ts/test-electron/sql/attachment_download_backup_stats_test.preload.ts +++ b/ts/test-electron/sql/attachment_download_backup_stats_test.preload.ts @@ -138,7 +138,7 @@ describe('sql/AttachmentDownloadBackupStats', () => { { totalBytes: 128, completedBytes: 0 } ); - await DataWriter.removeMessage('message0', { + await DataWriter.removeMessageById('message0', { cleanupMessages, }); assert.deepStrictEqual( @@ -194,7 +194,7 @@ describe('sql/AttachmentDownloadBackupStats', () => { ); assert.strictEqual(savedJob?.source, AttachmentDownloadSource.STANDARD); - await DataWriter.removeMessage('message0', { + await DataWriter.removeMessageById('message0', { cleanupMessages, }); assert.deepStrictEqual( diff --git a/ts/test-electron/sql/sendLog_test.preload.ts b/ts/test-electron/sql/sendLog_test.preload.ts index 2565f9b349..650ed8432b 100644 --- a/ts/test-electron/sql/sendLog_test.preload.ts +++ b/ts/test-electron/sql/sendLog_test.preload.ts @@ -25,7 +25,7 @@ const { insertProtoRecipients, insertSentProto, removeAllSentProtos, - removeMessage, + removeMessageById, saveMessage, } = DataWriter; @@ -155,7 +155,7 @@ describe('sql/sendLog', () => { assert.strictEqual(actual.timestamp, proto.timestamp); - await removeMessage(id, { cleanupMessages }); + await removeMessageById(id, { cleanupMessages }); assert.lengthOf(await getAllSentProtos(), 0); }); diff --git a/ts/test-electron/util/addAttachmentToMessage_test.preload.ts b/ts/test-electron/util/addAttachmentToMessage_test.preload.ts index 5a89f2ded1..e7eff9aea3 100644 --- a/ts/test-electron/util/addAttachmentToMessage_test.preload.ts +++ b/ts/test-electron/util/addAttachmentToMessage_test.preload.ts @@ -12,7 +12,7 @@ import { composeAttachment } from '../../test-node/util/queueAttachmentDownloads 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 { getAttachmentsPath } 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'; @@ -27,7 +27,9 @@ describe('addAttachmentToMessage', () => { }); afterEach(async () => { - await emptyDir(getPath(window.SignalContext.config.userDataPath)); + await emptyDir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); }); async function saveMessage( @@ -53,7 +55,9 @@ describe('addAttachmentToMessage', () => { } async function listAttachmentsOnDisk(): Promise> { - return readdir(getPath(window.SignalContext.config.userDataPath)); + return readdir( + getAttachmentsPath(window.SignalContext.config.userDataPath) + ); } it('replaces attachment on message', async () => { const attachment = composeAttachment({ diff --git a/ts/test-helpers/attachments.node.ts b/ts/test-helpers/attachments.node.ts new file mode 100644 index 0000000000..bda1e07e08 --- /dev/null +++ b/ts/test-helpers/attachments.node.ts @@ -0,0 +1,18 @@ +// Copyright 2026 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import * as Bytes from '../Bytes.std.js'; +import { getRandomBytes } from '../Crypto.node.js'; + +export function testPlaintextHash(): string { + return Bytes.toHex(getRandomBytes(32)); +} +export function testAttachmentKey(): string { + return Bytes.toBase64(getRandomBytes(64)); +} +export function testAttachmentLocalKey(): string { + return Bytes.toBase64(getRandomBytes(32)); +} +export function testAttachmentDigest(): string { + return Bytes.toBase64(getRandomBytes(32)); +} diff --git a/ts/test-node/sql/migration_1650_test.node.ts b/ts/test-node/sql/migration_1650_test.node.ts new file mode 100644 index 0000000000..c286895f9f --- /dev/null +++ b/ts/test-node/sql/migration_1650_test.node.ts @@ -0,0 +1,139 @@ +// Copyright 2026 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; + +import type { WritableDB } from '../../sql/Interface.std.js'; +import { + createDB, + explain, + getTableData, + insertData, + updateToVersion, +} from './helpers.node.js'; +import { sql } from '../../sql/util.std.js'; + +describe('SQL/updateToSchemaVersion1650', () => { + let db: WritableDB; + + beforeEach(() => { + db = createDB(); + updateToVersion(db, 1650); + }); + + afterEach(() => { + db.close(); + }); + + it('uses indexes for isAttachmentSafeToDelete', () => { + const details = explain( + db, + sql`SELECT EXISTS ( + SELECT 1 FROM attachments_protected_from_deletion + WHERE path = 'thepath' + UNION ALL + SELECT 1 FROM message_attachments + WHERE + path = 'thepath' OR + thumbnailPath = 'thepath' OR + screenshotPath = 'thepath' + );` + ); + + assert.isTrue( + details.includes( + 'USING COVERING INDEX sqlite_autoindex_attachments_protected_from_deletion' + ) + ); + assert.isTrue(details.includes('MULTI-INDEX OR')); + assert.isTrue( + details.includes('USING INDEX message_attachments_path (path=?)') + ); + assert.isTrue( + details.includes( + 'USING INDEX message_attachments_thumbnailPath (thumbnailPath=?)' + ) + ); + assert.isTrue( + details.includes( + 'USING INDEX message_attachments_screenshotPath (screenshotPath=?)' + ) + ); + }); + + it('removes protected path when attachment is inserted or updated', () => { + insertData(db, 'messages', [ + { + id: 'messageId', + conversationId: 'convoId', + }, + ]); + + // Protect some paths + insertData(db, 'attachments_protected_from_deletion', [ + { + path: 'protected1', + }, + { + path: 'protected2', + }, + { + path: 'protected3', + }, + { + path: 'protected4', + }, + { + path: 'protected5', + }, + { + path: 'protected6', + }, + { + path: 'protected7', + }, + ]); + + // Insert an attachment that uses a protected =path + insertData(db, 'message_attachments', [ + { + messageId: 'messageId', + editHistoryIndex: -1, + orderInMessage: 0, + attachmentType: 'attachment', + receivedAt: 42, + sentAt: 42, + size: 128, + contentType: 'image/png', + conversationId: 'convoId', + path: 'protected1', + thumbnailPath: 'protected2', + screenshotPath: 'protected3', + }, + ]); + + assert.deepStrictEqual( + getTableData(db, 'attachments_protected_from_deletion'), + [ + { path: 'protected4' }, + { path: 'protected5' }, + { path: 'protected6' }, + { path: 'protected7' }, + ] + ); + + // Updates also remove protection + db.prepare( + `UPDATE message_attachments SET + path='protected4', + screenshotPath='protected5', + thumbnailPath='protected6'; + ` + ).run(); + + assert.deepStrictEqual( + getTableData(db, 'attachments_protected_from_deletion'), + [{ path: 'protected7' }] + ); + }); +}); diff --git a/ts/test-node/types/Message2_test.preload.ts b/ts/test-node/types/Message2_test.preload.ts index b1b39dd2e4..2afeeabab9 100644 --- a/ts/test-node/types/Message2_test.preload.ts +++ b/ts/test-node/types/Message2_test.preload.ts @@ -97,7 +97,9 @@ describe('Message', () => { localKey: '123', plaintextHash: 'hash', }), - deleteAttachmentOnDisk: async (_path: string) => undefined, + maybeDeleteAttachmentFile: async (_path: string) => ({ + wasDeleted: true, + }), ...props, }; } diff --git a/ts/textsecure/ContactsParser.preload.ts b/ts/textsecure/ContactsParser.preload.ts index af0a9931d6..6de483fcab 100644 --- a/ts/textsecure/ContactsParser.preload.ts +++ b/ts/textsecure/ContactsParser.preload.ts @@ -10,7 +10,7 @@ import { DurationInSeconds } from '../util/durations/index.std.js'; import { getAbsoluteAttachmentPath, writeNewAttachmentData, - deleteAttachmentData, + maybeDeleteAttachmentFile, } from '../util/migrations.preload.js'; import type { ContactAvatarType } from '../types/Avatar.std.js'; import type { AttachmentType } from '../types/Attachment.std.js'; @@ -173,7 +173,7 @@ export class ParseContactsTransform extends Transform { this.contacts.push(prepared); } else { // eslint-disable-next-line no-await-in-loop - await deleteAttachmentData(local.path); + await maybeDeleteAttachmentFile(local.path); } this.activeContact = undefined; diff --git a/ts/types/Conversation.node.ts b/ts/types/Conversation.node.ts index 10813489d7..db82ee421d 100644 --- a/ts/types/Conversation.node.ts +++ b/ts/types/Conversation.node.ts @@ -12,7 +12,7 @@ const log = createLogger('Conversation'); export type BuildAvatarUpdaterOptions = Readonly<{ data?: Uint8Array; newAvatar?: ContactAvatarType; - deleteAttachmentData: (path: string) => Promise; + deleteAttachmentData: (path: string) => Promise<{ wasDeleted: boolean }>; doesAttachmentExist: (path: string) => Promise; writeNewAttachmentData: (data: Uint8Array) => Promise; }>; diff --git a/ts/types/Message2.preload.ts b/ts/types/Message2.preload.ts index 46e99fb633..6508add436 100644 --- a/ts/types/Message2.preload.ts +++ b/ts/types/Message2.preload.ts @@ -56,7 +56,14 @@ import { deepClone } from '../util/deepClone.std.js'; import * as Bytes from '../Bytes.std.js'; import { isBodyTooLong } from '../util/longAttachment.std.js'; import type { MessageAttachmentType } from './AttachmentDownload.std.js'; -import { getFilePathsOwnedByMessage } from '../util/messageFilePaths.std.js'; +import { + getFilePathsReferencedByAttachment, + getFilePathsReferencedByMessage, +} from '../util/messageFilePaths.std.js'; +import { + deleteDownloadFile, + maybeDeleteAttachmentFile, +} from '../util/migrations.preload.js'; const { isFunction, isObject, identity } = lodash; @@ -96,7 +103,7 @@ export type ContextType = { ) => Promise; writeNewAttachmentData: (data: Uint8Array) => Promise; writeNewStickerData: (data: Uint8Array) => Promise; - deleteAttachmentOnDisk: (path: string) => Promise; + maybeDeleteAttachmentFile: (path: string) => Promise<{ wasDeleted: boolean }>; }; // Schema version history @@ -710,21 +717,7 @@ export const VERSION_NEEDED_FOR_DISPLAY = 9; // UpgradeStep export const upgradeSchema = async ( rawMessage: MessageAttributesType, - { - readAttachmentData, - writeNewAttachmentData, - doesAttachmentExist, - getRegionCode, - makeObjectUrl, - revokeObjectUrl, - getImageDimensions, - makeImageThumbnail, - makeVideoScreenshot, - writeNewStickerData, - deleteAttachmentOnDisk, - logger, - maxVersion = CURRENT_SCHEMA_VERSION, - }: ContextType, + context: ContextType, upgradeOptions: { versions: ReadonlyArray< ( @@ -734,6 +727,7 @@ export const upgradeSchema = async ( >; } = { versions: VERSIONS } ): Promise => { + const { logger, maxVersion = CURRENT_SCHEMA_VERSION } = context; const { versions } = upgradeOptions; let message = rawMessage; const startingVersion = message.schemaVersion ?? 0; @@ -747,20 +741,7 @@ export const upgradeSchema = async ( // We really do want this intra-loop await because this is a chained async action, // each step dependent on the previous // eslint-disable-next-line no-await-in-loop - message = await currentVersion(message, { - readAttachmentData, - writeNewAttachmentData, - makeObjectUrl, - revokeObjectUrl, - doesAttachmentExist, - getImageDimensions, - makeImageThumbnail, - makeVideoScreenshot, - logger, - getRegionCode, - writeNewStickerData, - deleteAttachmentOnDisk, - }); + message = await currentVersion(message, context); } catch (e) { // Throw the error if we were unable to upgrade the message at all if (message.schemaVersion === startingVersion) { @@ -1055,41 +1036,31 @@ export const loadStickerData = ( }; }; -export const deleteAllExternalFiles = ({ - deleteAttachmentOnDisk, - deleteDownloadOnDisk, -}: { - deleteAttachmentOnDisk: (path: string) => Promise; - deleteDownloadOnDisk: (path: string) => Promise; -}): ((message: MessageAttributesType) => Promise) => { - if (!isFunction(deleteAttachmentOnDisk)) { - throw new TypeError( - 'deleteAllExternalFiles: deleteAttachmentOnDisk must be a function' - ); - } - - if (!isFunction(deleteDownloadOnDisk)) { - throw new TypeError( - 'deleteAllExternalFiles: deleteDownloadOnDisk must be a function' - ); - } - return async (message: MessageAttributesType) => { - const { externalAttachments, externalDownloads } = - getFilePathsOwnedByMessage(message); - - await Promise.all( - [...externalAttachments].map(attachmentPath => - deleteAttachmentOnDisk(attachmentPath) - ) - ); - await Promise.all( - [...externalDownloads].map(downloadPath => - deleteDownloadOnDisk(downloadPath) - ) - ); - }; +export const cleanupAllMessageAttachmentFiles = async ( + message: MessageAttributesType +): Promise => { + const { externalAttachments, externalDownloads } = + getFilePathsReferencedByMessage(message); + await Promise.all( + [...externalAttachments].map(attachmentPath => + maybeDeleteAttachmentFile(attachmentPath) + ) + ); + await Promise.all( + [...externalDownloads].map(downloadPath => deleteDownloadFile(downloadPath)) + ); }; +export async function cleanupAttachmentFiles( + attachment: AttachmentType +): Promise { + const result = getFilePathsReferencedByAttachment(attachment); + await Promise.all( + [...result.externalAttachments].map(maybeDeleteAttachmentFile) + ); + await Promise.all([...result.externalDownloads].map(deleteDownloadFile)); +} + export async function migrateBodyAttachmentToDisk( message: MessageAttributesType, { logger, writeNewAttachmentData }: ContextType diff --git a/ts/util/Attachment.std.ts b/ts/util/Attachment.std.ts index 44b7382990..5c7d63a6c3 100644 --- a/ts/util/Attachment.std.ts +++ b/ts/util/Attachment.std.ts @@ -33,7 +33,6 @@ import { } from '../types/Crypto.std.js'; import { missingCaseError } from './missingCaseError.std.js'; import type { MessageAttachmentType } from '../types/AttachmentDownload.std.js'; -import { getFilePathsOwnedByAttachment } from './messageFilePaths.std.js'; const { isNumber, @@ -203,32 +202,6 @@ export function loadData( }; } -export function deleteAllAttachmentFilesOnDisk({ - deleteAttachmentOnDisk, - deleteDownloadOnDisk, -}: { - deleteAttachmentOnDisk: (path: string) => Promise; - deleteDownloadOnDisk: (path: string) => Promise; -}): (attachment?: AttachmentType) => Promise { - if (!isFunction(deleteAttachmentOnDisk)) { - throw new TypeError( - 'deleteAttachmentOnDisk: deleteAttachmentOnDisk must be a function' - ); - } - - return async (attachment?: AttachmentType): Promise => { - if (!isValid(attachment)) { - throw new TypeError('deleteData: attachment is not valid'); - } - - const result = getFilePathsOwnedByAttachment(attachment); - await Promise.all( - [...result.externalAttachments].map(deleteAttachmentOnDisk) - ); - await Promise.all([...result.externalDownloads].map(deleteDownloadOnDisk)); - }; -} - // UI-focused functions export function getExtensionForDisplay({ diff --git a/ts/util/basePaths.preload.ts b/ts/util/basePaths.preload.ts index 5c734fe67b..1bc0e657a6 100644 --- a/ts/util/basePaths.preload.ts +++ b/ts/util/basePaths.preload.ts @@ -2,7 +2,7 @@ // SPDX-License-Identifier: AGPL-3.0-only import { - getPath, + getAttachmentsPath, getDraftPath, getStickersPath, getTempPath, @@ -14,7 +14,7 @@ import { const userDataPath = window.SignalContext.getPath('userData'); -export const ATTACHMENTS_PATH = getPath(userDataPath); +export const ATTACHMENTS_PATH = getAttachmentsPath(userDataPath); export const DRAFT_PATH = getDraftPath(userDataPath); export const STICKERS_PATH = getStickersPath(userDataPath); export const TEMP_PATH = getTempPath(userDataPath); diff --git a/ts/util/callDisposition.preload.ts b/ts/util/callDisposition.preload.ts index 275e423b43..ff74b7d8fd 100644 --- a/ts/util/callDisposition.preload.ts +++ b/ts/util/callDisposition.preload.ts @@ -1198,7 +1198,7 @@ async function saveCallHistory({ if (isDeleted) { if (prevMessage != null) { - await DataWriter.removeMessage(prevMessage.id, { + await DataWriter.removeMessageById(prevMessage.id, { fromSync: true, cleanupMessages, }); diff --git a/ts/util/captureAudioDuration.dom.ts b/ts/util/captureAudioDuration.dom.ts index 0c056ff946..8d49376ebe 100644 --- a/ts/util/captureAudioDuration.dom.ts +++ b/ts/util/captureAudioDuration.dom.ts @@ -14,6 +14,10 @@ export async function captureAudioDuration( logger: LoggerType; } ): Promise { + if (attachment.duration) { + return attachment; + } + const audio = new window.Audio(); audio.muted = true; audio.src = getLocalAttachmentUrl(attachment); diff --git a/ts/util/cleanup.preload.ts b/ts/util/cleanup.preload.ts index ff26fc5ada..662f1e9632 100644 --- a/ts/util/cleanup.preload.ts +++ b/ts/util/cleanup.preload.ts @@ -26,12 +26,12 @@ import { getMessageIdForLogging } from './idForLogging.preload.js'; import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue.preload.js'; import { MINUTE } from './durations/index.std.js'; import { drop } from './drop.std.js'; -import { deleteExternalMessageFiles } from './migrations.preload.js'; import { hydrateStoryContext } from './hydrateStoryContext.preload.js'; import { update as updateExpiringMessagesService } from '../services/expiringMessagesDeletion.preload.js'; import { tapToViewMessagesDeletionService } from '../services/tapToViewMessagesDeletionService.preload.js'; import { throttledUpdateBackupMediaDownloadProgress } from './updateBackupMediaDownloadProgress.preload.js'; import { messageAttrsToPreserveAfterErase } from '../types/Message.std.js'; +import { cleanupAllMessageAttachmentFiles } from '../types/Message2.preload.js'; const log = createLogger('cleanup'); @@ -59,7 +59,7 @@ export async function eraseMessageContents( // a viewed (or outgoing) View-Once message is deleted for everyone. try { - await deleteMessageData(message.attributes); + await cleanupFilesAndReferencesToMessage(message.attributes); } catch (error) { log.error( `Error erasing data for message ${getMessageIdForLogging(message.attributes)}:`, @@ -118,7 +118,7 @@ export async function cleanupMessages( drop( unloadedQueue.addAll( messages.map((message: MessageAttributesType) => async () => { - await deleteMessageData(message); + await cleanupFilesAndReferencesToMessage(message); }) ) ); @@ -184,7 +184,7 @@ async function cleanupStoryReplies( if (isGroupConversation) { // Delete all group replies - await DataWriter.removeMessages( + await DataWriter.removeMessagesById( replies.map(reply => reply.id), { cleanupMessages } ); @@ -208,10 +208,10 @@ async function cleanupStoryReplies( }); } -export async function deleteMessageData( +export async function cleanupFilesAndReferencesToMessage( message: MessageAttributesType ): Promise { - await deleteExternalMessageFiles(message); + await cleanupAllMessageAttachmentFiles(message); if (isStory(message)) { await cleanupStoryReplies(message); diff --git a/ts/util/deleteForEveryone.preload.ts b/ts/util/deleteForEveryone.preload.ts index 4278331a24..ff66393f10 100644 --- a/ts/util/deleteForEveryone.preload.ts +++ b/ts/util/deleteForEveryone.preload.ts @@ -102,7 +102,7 @@ export async function handleDeleteForEveryone( if (shouldPersist) { // We delete the message first, before re-saving it -- this causes any foreign key // ON DELETE CASCADE and messages_on_delete triggers to run, which is important - await DataWriter.removeMessage(message.attributes.id, { + await DataWriter.removeMessageById(message.attributes.id, { cleanupMessages: async () => { // We don't actually want to remove this message up from in-memory caches }, diff --git a/ts/util/deleteForMe.preload.ts b/ts/util/deleteForMe.preload.ts index a864718524..4c0a2aa83d 100644 --- a/ts/util/deleteForMe.preload.ts +++ b/ts/util/deleteForMe.preload.ts @@ -4,12 +4,7 @@ import lodash from 'lodash'; import { createLogger } from '../logging/log.std.js'; -import { - DataReader, - DataWriter, - deleteAndCleanup, -} from '../sql/Client.preload.js'; -import { deleteAllAttachmentFilesOnDisk } from './Attachment.std.js'; +import { DataReader, DataWriter } from '../sql/Client.preload.js'; import type { MessageAttributesType } from '../model-types.d.ts'; import type { ConversationModel } from '../models/conversations.preload.js'; @@ -22,6 +17,7 @@ import { getMessageQueryFromTarget, } from './syncIdentifiers.preload.js'; import { itemStorage } from '../textsecure/Storage.preload.js'; +import { cleanupAttachmentFiles } from '../types/Message2.preload.js'; const { last, sortBy } = lodash; @@ -45,15 +41,15 @@ export async function deleteMessage( } const message = window.MessageCache.register(new MessageModel(found)); - await applyDeleteMessage(message.attributes, logId); + await applyDeleteMessage(message.attributes); return true; } + export async function applyDeleteMessage( - message: MessageAttributesType, - logId: string + message: MessageAttributesType ): Promise { - await deleteAndCleanup([message], logId, { + await DataWriter.removeMessageById(message.id, { fromSync: true, cleanupMessages, }); @@ -68,12 +64,8 @@ export async function deleteAttachmentFromMessage( fallbackPlaintextHash?: string; }, { - deleteAttachmentOnDisk, - deleteDownloadOnDisk, logId, }: { - deleteAttachmentOnDisk: (path: string) => Promise; - deleteDownloadOnDisk: (path: string) => Promise; logId: string; } ): Promise { @@ -90,8 +82,6 @@ export async function deleteAttachmentFromMessage( const message = window.MessageCache.register(new MessageModel(found)); return applyDeleteAttachmentFromMessage(message, deleteAttachmentData, { - deleteAttachmentOnDisk, - deleteDownloadOnDisk, logId, shouldSave: true, }); @@ -109,13 +99,9 @@ export async function applyDeleteAttachmentFromMessage( fallbackPlaintextHash?: string; }, { - deleteAttachmentOnDisk, - deleteDownloadOnDisk, shouldSave, logId, }: { - deleteAttachmentOnDisk: (path: string) => Promise; - deleteDownloadOnDisk: (path: string) => Promise; shouldSave: boolean; logId: string; } @@ -153,10 +139,7 @@ export async function applyDeleteAttachmentFromMessage( if (shouldSave) { await saveMessage(message.attributes, { ourAci, postSaveUpdates }); } - await deleteAllAttachmentFilesOnDisk({ - deleteAttachmentOnDisk, - deleteDownloadOnDisk, - })(attachment); + await cleanupAttachmentFiles(attachment); return true; } diff --git a/ts/util/encryptConversationAttachments.preload.ts b/ts/util/encryptConversationAttachments.preload.ts index 4bf6df1e39..ca0d763b53 100644 --- a/ts/util/encryptConversationAttachments.preload.ts +++ b/ts/util/encryptConversationAttachments.preload.ts @@ -10,7 +10,7 @@ import { encryptLegacyAttachment } from './encryptLegacyAttachment.preload.js'; import { AttachmentDisposition } from './getLocalAttachmentUrl.std.js'; import { isNotNil } from './isNotNil.std.js'; import { - deleteAttachmentData, + maybeDeleteAttachmentFile, deleteAvatar, deleteDraftFile, readAttachmentData, @@ -28,7 +28,7 @@ const log = createLogger('encryptConversationAttachments'); const CONCURRENCY = 32; -type CleanupType = Array<() => Promise>; +type CleanupType = Array<() => Promise>; export async function encryptConversationAttachments(): Promise { const all = await DataReader.getAllConversations(); @@ -100,7 +100,7 @@ async function encryptOne(attributes: ConversationAttributesType): Promise< ); if (result.profileAvatar !== attributes.profileAvatar) { const { path } = attributes.profileAvatar; - cleanup.push(() => deleteAttachmentData(path)); + cleanup.push(() => maybeDeleteAttachmentFile(path)); } } @@ -113,7 +113,7 @@ async function encryptOne(attributes: ConversationAttributesType): Promise< }); if (result.avatar !== attributes.avatar) { const { path } = attributes.avatar; - cleanup.push(() => deleteAttachmentData(path)); + cleanup.push(() => maybeDeleteAttachmentFile(path)); } } diff --git a/ts/util/findAndDeleteOnboardingStoryIfExists.preload.ts b/ts/util/findAndDeleteOnboardingStoryIfExists.preload.ts index b3de08bc74..47956ab686 100644 --- a/ts/util/findAndDeleteOnboardingStoryIfExists.preload.ts +++ b/ts/util/findAndDeleteOnboardingStoryIfExists.preload.ts @@ -49,7 +49,7 @@ export async function findAndDeleteOnboardingStoryIfExists(): Promise { log.info('removing onboarding stories'); - await DataWriter.removeMessages(existingOnboardingStoryMessageIds, { + await DataWriter.removeMessagesById(existingOnboardingStoryMessageIds, { cleanupMessages, }); diff --git a/ts/util/messageFilePaths.std.ts b/ts/util/messageFilePaths.std.ts index 0118ffe0fc..606ea35493 100644 --- a/ts/util/messageFilePaths.std.ts +++ b/ts/util/messageFilePaths.std.ts @@ -4,14 +4,19 @@ import type { MessageAttributesType } from '../model-types.d.ts'; import type { AttachmentType } from '../types/Attachment.std.js'; -export function getFilePathsOwnedByAttachment(attachment: AttachmentType): { +export function getFilePathsReferencedByAttachment( + attachment: AttachmentType +): { externalAttachments: Set; externalDownloads: Set; } { const externalAttachments = new Set(); const externalDownloads = new Set(); - // Copied attachments weakly reference their paths and do not 'own' them + // Copied attachments weakly reference their paths -- there is a chance that + // non-normalized attachments may still exist on messages.json, in which case + // `isAttachmentSafeToDelete` would incorrectly mark a copied thumbnail as safe to + // delete if (attachment.copied) { return { externalAttachments, externalDownloads }; } @@ -50,8 +55,8 @@ function getFilePathsForVersionOfMessage( } { const externalAttachments = new Set(); const externalDownloads = new Set(); - function addFilePathsOwnedByAttachment(attachment: AttachmentType) { - const result = getFilePathsOwnedByAttachment(attachment); + function addFilePathsReferencedByAttachment(attachment: AttachmentType) { + const result = getFilePathsReferencedByAttachment(attachment); result.externalAttachments.forEach(path => externalAttachments.add(path)); result.externalDownloads.forEach(path => externalDownloads.add(path)); } @@ -59,16 +64,16 @@ function getFilePathsForVersionOfMessage( const { attachments, bodyAttachment, contact, quote, preview, sticker } = rootOrEditHistoryMessage; - attachments?.forEach(addFilePathsOwnedByAttachment); + attachments?.forEach(addFilePathsReferencedByAttachment); if (bodyAttachment) { - addFilePathsOwnedByAttachment(bodyAttachment); + addFilePathsReferencedByAttachment(bodyAttachment); } if (quote?.attachments) { quote.attachments.forEach(attachment => { if (attachment.thumbnail) { - addFilePathsOwnedByAttachment(attachment.thumbnail); + addFilePathsReferencedByAttachment(attachment.thumbnail); } }); } @@ -76,7 +81,7 @@ function getFilePathsForVersionOfMessage( if (contact) { contact.forEach(item => { if (item.avatar?.avatar) { - addFilePathsOwnedByAttachment(item.avatar.avatar); + addFilePathsReferencedByAttachment(item.avatar.avatar); } }); } @@ -84,18 +89,20 @@ function getFilePathsForVersionOfMessage( if (preview) { preview.forEach(item => { if (item.image) { - addFilePathsOwnedByAttachment(item.image); + addFilePathsReferencedByAttachment(item.image); } }); } if (sticker?.data) { - addFilePathsOwnedByAttachment(sticker.data); + addFilePathsReferencedByAttachment(sticker.data); } return { externalAttachments, externalDownloads }; } -export function getFilePathsOwnedByMessage(message: MessageAttributesType): { +export function getFilePathsReferencedByMessage( + message: MessageAttributesType +): { externalAttachments: Array; externalDownloads: Array; } { diff --git a/ts/util/migrations.preload.ts b/ts/util/migrations.preload.ts index 12edda55b8..dd28e6479a 100644 --- a/ts/util/migrations.preload.ts +++ b/ts/util/migrations.preload.ts @@ -28,7 +28,6 @@ import { loadStickerData as doLoadStickerData, processNewAttachment as doProcessNewAttachment, processNewSticker as doProcessNewSticker, - deleteAllExternalFiles, createAttachmentLoader, upgradeSchema, } from '../types/Message2.preload.js'; @@ -51,6 +50,7 @@ import { DOWNLOADS_PATH, MEGAPHONES_PATH, } from './basePaths.preload.js'; +import { DataReader } from '../sql/Client.preload.js'; const logger = createLogger('migrations'); @@ -112,7 +112,23 @@ export const loadQuoteData = doLoadQuoteData(loadAttachmentData); export const loadStickerData = doLoadStickerData(loadAttachmentData); export const getAbsoluteAttachmentPath = createAbsolutePathGetter(ATTACHMENTS_PATH); -export const deleteAttachmentData = createDeleter(ATTACHMENTS_PATH); + +// eslint-disable-next-line camelcase +const __DANGEROUS__deleteAttachmentFile = createDeleter(ATTACHMENTS_PATH); +export const maybeDeleteAttachmentFile = async ( + relativePath: string +): Promise<{ wasDeleted: boolean }> => { + const isSafeToDelete = + await DataReader.isAttachmentSafeToDelete(relativePath); + + if (!isSafeToDelete) { + return { wasDeleted: false }; + } + + await __DANGEROUS__deleteAttachmentFile(relativePath); + return { wasDeleted: true }; +}; + export const writeNewAttachmentData = createEncryptedWriterForNew(ATTACHMENTS_PATH); export const doesAttachmentExist = createDoesExist(ATTACHMENTS_PATH); @@ -157,17 +173,13 @@ export const readDraftData = createEncryptedReader(DRAFT_PATH); export const getAbsoluteDownloadsPath = createAbsolutePathGetter(DOWNLOADS_PATH); -export const deleteDownloadData = createDeleter(DOWNLOADS_PATH); +export const deleteDownloadFile = createDeleter(DOWNLOADS_PATH); export const readAvatarData = createEncryptedReader(AVATARS_PATH); export const getAbsoluteAvatarPath = createAbsolutePathGetter(AVATARS_PATH); export const writeNewAvatarData = createEncryptedWriterForNew(AVATARS_PATH); export const deleteAvatar = createDeleter(AVATARS_PATH); -export const deleteExternalMessageFiles = deleteAllExternalFiles({ - deleteAttachmentOnDisk: deleteAttachmentData, - deleteDownloadOnDisk: deleteDownloadData, -}); export const loadMessage = createAttachmentLoader(loadAttachmentData); export const processNewAttachment = ( @@ -207,7 +219,7 @@ export const upgradeMessageSchema = ( const { maxVersion } = options; return upgradeSchema(message, { - deleteAttachmentOnDisk: deleteAttachmentData, + maybeDeleteAttachmentFile, doesAttachmentExist, getImageDimensions, getRegionCode: () => itemStorage.get('regionCode'), diff --git a/ts/util/modifyTargetMessage.preload.ts b/ts/util/modifyTargetMessage.preload.ts index 2ed8e63071..f96753b1e0 100644 --- a/ts/util/modifyTargetMessage.preload.ts +++ b/ts/util/modifyTargetMessage.preload.ts @@ -33,10 +33,6 @@ import { getSourceServiceId } from '../messages/sources.preload.js'; import { missingCaseError } from './missingCaseError.std.js'; import { reduce } from './iterables.std.js'; import { strictAssert } from './assert.std.js'; -import { - deleteAttachmentData, - deleteDownloadData, -} from './migrations.preload.js'; import { applyDeleteAttachmentFromMessage, applyDeleteMessage, @@ -87,7 +83,7 @@ export async function modifyTargetMessage( if (isFullDelete) { if (!isFirstRun) { - await applyDeleteMessage(message.attributes, logId); + await applyDeleteMessage(message.attributes); } return ModifyTargetMessageResult.Deleted; @@ -111,8 +107,6 @@ export async function modifyTargetMessage( { logId, shouldSave: false, - deleteAttachmentOnDisk: deleteAttachmentData, - deleteDownloadOnDisk: deleteDownloadData, } ); if (result) {