From 0e386ef70587c3ea1c2371be6666abef543c6537 Mon Sep 17 00:00:00 2001 From: trevor-signal <131492920+trevor-signal@users.noreply.github.com> Date: Tue, 8 Oct 2024 17:45:00 -0400 Subject: [PATCH] Make ensureAttachmentIsReencryptable migration resilient to missing attachments --- ts/signal.ts | 12 ++++- ts/test-node/types/Message2_test.ts | 56 ++++++++++++++++++++++ ts/types/Message2.ts | 50 ++++++++++++++----- ts/util/ensureAttachmentIsReencryptable.ts | 13 +++-- ts/windows/attachments.ts | 1 + 5 files changed, 117 insertions(+), 15 deletions(-) diff --git a/ts/signal.ts b/ts/signal.ts index 109878e98c7b..8bb271c1cc69 100644 --- a/ts/signal.ts +++ b/ts/signal.ts @@ -79,6 +79,9 @@ type MigrationsModuleType = { deleteSticker: (path: string) => Promise; deleteTempFile: (path: string) => Promise; doesAttachmentExist: (path: string) => Promise; + ensureAttachmentIsReencryptable: ( + attachment: TypesAttachment.LocallySavedAttachment + ) => Promise; getAbsoluteAttachmentPath: (path: string) => string; getAbsoluteAvatarPath: (src: string) => string; getAbsoluteBadgeImageFilePath: (path: string) => string; @@ -161,6 +164,7 @@ export function initializeMigrations({ createPlaintextReader, createWriterForNew, createDoesExist, + ensureAttachmentIsReencryptable, getAvatarsPath, getDraftPath, getDownloadsPath, @@ -291,6 +295,7 @@ export function initializeMigrations({ deleteSticker, deleteTempFile, doesAttachmentExist, + ensureAttachmentIsReencryptable, getAbsoluteAttachmentPath, getAbsoluteAvatarPath, getAbsoluteBadgeImageFilePath, @@ -313,6 +318,7 @@ export function initializeMigrations({ processNewAttachment: (attachment: AttachmentType) => MessageType.processNewAttachment(attachment, { writeNewAttachmentData, + ensureAttachmentIsReencryptable, makeObjectUrl, revokeObjectUrl, getImageDimensions, @@ -341,6 +347,8 @@ export function initializeMigrations({ return MessageType.upgradeSchema(message, { deleteOnDisk, + doesAttachmentExist, + ensureAttachmentIsReencryptable, getImageDimensions, getRegionCode, makeImageThumbnail, @@ -350,7 +358,6 @@ export function initializeMigrations({ revokeObjectUrl, writeNewAttachmentData, writeNewStickerData, - logger, maxVersion, }); @@ -404,6 +411,9 @@ type AttachmentsModuleType = { name: string; }) => Promise; + ensureAttachmentIsReencryptable: ( + attachment: TypesAttachment.LocallySavedAttachment + ) => Promise; readAndDecryptDataFromDisk: (options: { absolutePath: string; keysBase64: string; diff --git a/ts/test-node/types/Message2_test.ts b/ts/test-node/types/Message2_test.ts index 517d1b71e780..d36ad96bb081 100644 --- a/ts/test-node/types/Message2_test.ts +++ b/ts/test-node/types/Message2_test.ts @@ -61,6 +61,10 @@ describe('Message', () => { width: 10, height: 20, }), + doesAttachmentExist: async () => true, + // @ts-expect-error ensureAttachmentIsReencryptable has type guards that we don't + // implement here + ensureAttachmentIsReencryptable: async attachment => attachment, getRegionCode: () => 'region-code', logger, makeImageThumbnail: async (_params: { @@ -837,4 +841,56 @@ describe('Message', () => { assert.deepEqual(result, message); }); }); + + describe('toVersion14: ensureAttachmentsAreReencryptable', () => { + it('migrates message if the file does not exist', async () => { + const message = getDefaultMessage({ + schemaVersion: 13, + schemaMigrationAttempts: 0, + attachments: [ + { + size: 128, + contentType: MIME.IMAGE_BMP, + path: 'no/file/here.png', + iv: 'iv', + digest: 'digest', + key: 'key', + }, + ], + contact: [], + }); + const result = await Message.upgradeSchema(message, { + ...getDefaultContext(), + doesAttachmentExist: async () => false, + }); + + assert.deepEqual({ ...message, schemaVersion: 14 }, result); + }); + it('if file does exist, but migration errors, does not increment version', async () => { + const message = getDefaultMessage({ + schemaVersion: 13, + schemaMigrationAttempts: 0, + attachments: [ + { + size: 128, + contentType: MIME.IMAGE_BMP, + path: 'no/file/here.png', + iv: 'iv', + digest: 'digest', + key: 'key', + }, + ], + contact: [], + }); + const result = await Message.upgradeSchema(message, { + ...getDefaultContext(), + doesAttachmentExist: async () => true, + ensureAttachmentIsReencryptable: async () => { + throw new Error("Can't reencrypt!"); + }, + }); + + assert.deepEqual(message, result); + }); + }); }); diff --git a/ts/types/Message2.ts b/ts/types/Message2.ts index 5d6137ae1cb1..34d4eb4ec21b 100644 --- a/ts/types/Message2.ts +++ b/ts/types/Message2.ts @@ -10,6 +10,8 @@ import type { AttachmentType, AttachmentWithHydratedData, LocalAttachmentV2Type, + LocallySavedAttachment, + ReencryptableAttachment, } from './Attachment'; import { captureDimensionsAndScreenshot, @@ -49,12 +51,16 @@ import { encryptLegacyAttachment } from '../util/encryptLegacyAttachment'; import { deepClone } from '../util/deepClone'; import { LONG_ATTACHMENT_LIMIT } from './Message'; import * as Bytes from '../Bytes'; -import { ensureAttachmentIsReencryptable } from '../util/ensureAttachmentIsReencryptable'; +import { redactGenericText } from '../util/privacy'; export const GROUP = 'group'; export const PRIVATE = 'private'; export type ContextType = { + doesAttachmentExist: (relativePath: string) => Promise; + ensureAttachmentIsReencryptable: ( + attachment: LocallySavedAttachment + ) => Promise; getImageDimensions: (params: { objectUrl: string; logger: LoggerType; @@ -635,17 +641,33 @@ const toVersion13 = _withSchemaVersion({ const toVersion14 = _withSchemaVersion({ schemaVersion: 14, - upgrade: _mapAllAttachments(async attachment => { - if (!isAttachmentLocallySaved(attachment)) { - return attachment; + upgrade: _mapAllAttachments( + async ( + attachment, + { logger, ensureAttachmentIsReencryptable, doesAttachmentExist } + ) => { + const logId = `Message2.toVersion14(digest=${redactGenericText(attachment.digest ?? '')})`; + + if (!isAttachmentLocallySaved(attachment)) { + return attachment; + } + + if (!(await doesAttachmentExist(attachment.path))) { + // Attachments may be missing, e.g. for quote thumbnails that reference messages + // which have been deleted + logger.info(`${logId}: File does not exist`); + return attachment; + } + + if (!attachment.digest) { + // Messages that are being upgraded prior to being sent may not have encrypted the + // attachment yet + return attachment; + } + + return ensureAttachmentIsReencryptable(attachment); } - if (!attachment.digest) { - // this attachment has not been encrypted yet; this would be expected for messages - // that are being upgraded prior to being sent - return attachment; - } - return ensureAttachmentIsReencryptable(attachment); - }), + ), }); const VERSIONS = [ @@ -677,6 +699,8 @@ export const upgradeSchema = async ( { readAttachmentData, writeNewAttachmentData, + doesAttachmentExist, + ensureAttachmentIsReencryptable, getRegionCode, makeObjectUrl, revokeObjectUrl, @@ -738,6 +762,8 @@ export const upgradeSchema = async ( writeNewAttachmentData, makeObjectUrl, revokeObjectUrl, + doesAttachmentExist, + ensureAttachmentIsReencryptable, getImageDimensions, makeImageThumbnail, makeVideoScreenshot, @@ -756,6 +782,7 @@ export const upgradeSchema = async ( export const processNewAttachment = async ( attachment: AttachmentType, { + ensureAttachmentIsReencryptable, writeNewAttachmentData, makeObjectUrl, revokeObjectUrl, @@ -773,6 +800,7 @@ export const processNewAttachment = async ( | 'makeVideoScreenshot' | 'logger' | 'deleteOnDisk' + | 'ensureAttachmentIsReencryptable' > ): Promise => { if (!isFunction(writeNewAttachmentData)) { diff --git a/ts/util/ensureAttachmentIsReencryptable.ts b/ts/util/ensureAttachmentIsReencryptable.ts index 679635656849..714653554f3e 100644 --- a/ts/util/ensureAttachmentIsReencryptable.ts +++ b/ts/util/ensureAttachmentIsReencryptable.ts @@ -4,6 +4,7 @@ import { PassThrough } from 'stream'; import { type EncryptedAttachmentV2, + ReencryptedDigestMismatchError, type ReencryptionInfo, decryptAttachmentV2ToSink, encryptAttachmentV2, @@ -20,6 +21,8 @@ import { import { strictAssert } from './assert'; import * as logging from '../logging/log'; import { fromBase64, toBase64 } from '../Bytes'; +import { redactGenericText } from './privacy'; +import { toLogFormat } from '../types/errors'; /** * Some attachments on desktop are not reencryptable to the digest we received for them. @@ -34,6 +37,7 @@ import { fromBase64, toBase64 } from '../Bytes'; export async function ensureAttachmentIsReencryptable( attachment: LocallySavedAttachment ): Promise { + const logId = `ensureAttachmentIsReencryptable(digest=${redactGenericText(attachment.digest ?? '')})`; if (isReencryptableToSameDigest(attachment)) { return attachment; } @@ -50,9 +54,12 @@ export async function ensureAttachmentIsReencryptable( isReencryptableToSameDigest: true, }; } catch (e) { - logging.info( - 'Unable to reencrypt attachment to original digest; must have had non-zero padding' - ); + if (e instanceof ReencryptedDigestMismatchError) { + logging.info( + `${logId}: Unable to reencrypt attachment to original digest; must have had non-zero padding` + ); + } + logging.error(`${logId}: error when reencrypting`, toLogFormat(e)); } } diff --git a/ts/windows/attachments.ts b/ts/windows/attachments.ts index 680d77ad8884..35e4ea84e9b2 100644 --- a/ts/windows/attachments.ts +++ b/ts/windows/attachments.ts @@ -12,6 +12,7 @@ import { writeWindowsZoneIdentifier } from '../util/windowsZoneIdentifier'; import OS from '../util/os/osMain'; import { getRelativePath, createName } from '../util/attachmentPath'; +export * from '../util/ensureAttachmentIsReencryptable'; export * from '../../app/attachments'; type FSAttrType = {